EvaluationContext.CompileExpressions method in CSharp.ExpressionCompiler has unclear purpose and should probably be removed from production code
- 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
Assessment
This issue has not been assessed yet.