Mark lambda conversions as side-affecting#25917
Conversation
The scenario listed in dotnet#22700 demonstrates the problem. The direct cause of the crash is that local lowering produces two instances of the lambda and lamda conversions in the resulting bound nodes. This crashes the closure conversion pass since the same lambda cannot appear in two places in the tree. However, even if that were allowed, this would violate the language specification. Placing a lambda inside an indexer which is nested in a compound assignment creates two calls which contain the delegate produced by the lambda expression. If the expression kind is marked as side-affecting it will be spilled into a temporary variable. However, the local lowering doesn't distinguish between whether an expression is side-affecting and whether or not it is referentially transparent. In this case, both matter. Since the delegate produced by the conversion can be compared by reference to any other delegate produced by the same lambda, the code must be referentially transparent -- it must produce the same result when evaluated repeatedly. The fix is small: mark lambda conversions as side-affecting. There are certain cases, like the creation of temporaries for out-of-order calls with named parameters, where the lambda conversion is only required to be non-side-affecting instead of referentially transparent, but it does not currently seem worth the implementation effort to split up the code path. Fixes dotnet#22700
|
cc @jaredpar for ask-mode approval |
| break; | ||
|
|
||
| case ConversionKind.AnonymousFunction: | ||
| case ConversionKind.MethodGroup: |
There was a problem hiding this comment.
ConversionKind.MethodGroup [](start = 37, length = 26)
Let's investigate whether this change is going to have any impact on scenarios involving Linq expression trees #Closed
There was a problem hiding this comment.
I've added a similar test using an expression lambda. Please let me know if there's something else you were thinking of.
|
Done with review pass (iteration 1) #Closed |
| Console.WriteLine(object.ReferenceEquals(_f1, _f2)); | ||
| } | ||
|
|
||
| int this[Expression<Func<int>> f] { |
There was a problem hiding this comment.
Expression<Func> [](start = 13, length = 21)
If this test is in response to my comment about expression trees, this isn't the scenario. The scenario is when the expression that uses lambda as an argument is converted to an expression tree, not just the lambda itself #Closed
There was a problem hiding this comment.
We now put lambda or a delegate in a temp in some cases, that might affect our ability to create an expression tree.
In reply to: 179239269 [](ancestors = 179239269)
There was a problem hiding this comment.
Ah! Thanks I will try to write such a test.
There was a problem hiding this comment.
Note that your change affects more scenarios, not just compound assignments.
In reply to: 179240865 [](ancestors = 179240865)
|
@AlekseyTs I found a case where this code path could be hit using the changed baseline from the PDB tests. It doesn't appear to produce any change in the expression tree. The only other two places this change affects, compound assignments and named parameter re-ordering, can't be explicitly specified in an expression tree. |
|
The root cause of the crash is that we have code that relies on identity of bound nodes. |
Even though that is true, I believe there would be a behavior problem as we would be creating two delegates instead of one.
In general, we should not be relying on identity of bound nodes because it is rather common to reuse the same nodes in multiple places in the same tree. |
|
|
||
| return; | ||
|
|
||
| bool IsLambdaConversion(BoundExpression expr) |
There was a problem hiding this comment.
IsLambdaConversion [](start = 17, length = 18)
I think our convention is to start local function names with a lower-case letter. #Closed
|
test windows_debug_unit64_prtest please |
|
test windows_release_unit64_prtest please |
|
approved |
Customer scenario
The scenario listed in #22700 demonstrates the problem. The direct cause
of the crash is that local lowering produces two instances of the lambda and
lamda conversions in the resulting bound nodes. This crashes the closure
conversion pass since the same lambda cannot appear in two places in the tree.
However, even if that were allowed, this would violate the language
specification.
Placing a lambda inside an indexer which is nested in a compound
assignment creates two calls which contain the delegate produced by the
lambda expression. Since the delegate produced by the conversion can be
compared by-reference to any other delegate produced by the same lambda,
the code must be referentially transparent -- it must produce the same result
when evaluated repeatedly. This could be achieved by spilling the evaluation
result into a temporary. However, the local lowering only spills if the expression
is side-affecting (which lambda conversions are not), when what it actually
needs is for the expression to be referentially transparent.
The fix is small: mark lambda conversions as side-affecting. There are
certain cases, like the creation of temporaries for out-of-order calls
with named parameters, where the lambda conversion is only required to
be non-side-affecting instead of referentially transparent, but it does
not currently seem worth the implementation effort to split up the code
path.
Bugs this fixes
#22700
Workarounds, if any
Manually spill the conversion into a local variable.
Risk
Very small, focused change. Introduces a temp, so this is more conservative than alternatives.
Performance impact
None. No significant amount of new code.
Is this a regression from a previous update?
No.
Root cause analysis
Very rare scenario with very little use in the real world.
How was the bug found?
Customer reported