dotnet / dotnet/roslyn

EvaluationContext.CompileExpressions method in CSharp.ExpressionCompiler has unclear purpose and should probably be removed from production code

Open
#80,482 2 comments 0 reactions 0 assignees View on GitHub
Area-Compilers Concept-Design Debt Interactive-Debugging
Dominant language
C#
Stars
20.7k
Forks
4.3k
PR merge metrics
PR metrics pending

Description

```
///
/// Compile a collection of expressions at the same location. If all expressions
/// compile successfully, a single assembly is returned along with the method
/// tokens for the expression evaluation methods. If there are errors compiling
/// any expression, null is returned along with the collection of error messages
/// for all expressions.
///
///
/// Errors are returned as a single collection rather than grouped by expression
/// since some errors (such as those detected during emit) are not easily
/// attributed to a particular expression.
///
internal byte[]? CompileExpressions(
```

The method was added in #17721. Right now it is not used in production code (as far as I can tell, never was used there), it is used only in tests, in `src\ExpressionEvaluator\CSharp\Test\ExpressionCompiler\CompileExpressionsTests.cs`. It looks like all tests, but one were added in the same PR that added the method. The one test added later compiles a single expression.

It is not clear what was the intent for the method. Since it resides in production code, its behavior should be tested, which increases our test matrix. For example, if we are changing some of the methods used by `CompileExpressions` to take a new input in order to behave properly in certain scenarios, we should test that `CompileExpressions` provides correct input, etc. However, if the method is meant to be a test helper and is not meant to be used in production code, then testing it would not be a smart way to spend dev resources.

We should consider moving `CompileExpressions` out of the production code and to location where other EE test helpers reside. It might be simpler and better to remove it completely. Currently only 12 tests are using it and 6 of them pass a single expression. It could be fairly simple to switch the tests to use `CompileExpression` API instead.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.