Avoid binding lambdas unnecessarily in some error scenarios #59733
Conversation
cston
force-pushed
the
BindForErrorRecovery
branch
from
8f56558 to
72b4a11
Compare
cston
force-pushed
the
BindForErrorRecovery
branch
from
72b4a11 to
f2ab18e
Compare
| var data = new LambdaBindingData(); | ||
| comp.TestOnlyCompilationData = data; | ||
| var diagnostics = comp.GetDiagnostics(); | ||
| Assert.Equal((84, 126), (data.LambdaBindingCount, data.UnboundLambdaStateCount)); |
| @@ -1663,14 +1668,17 @@ private ImmutableArray<BoundExpression> BuildArgumentsForErrorRecovery(AnalyzedA | |||
| { | |||
| // bind the argument against each applicable parameter | |||
| var unboundArgument = (UnboundLambda)argument; | |||
| foreach (var parameterList in parameterListList) | |||
| if (!unboundArgument.HasBoundForErrorRecovery) | |||
| var data = new LambdaBindingData(); | ||
| comp.TestOnlyCompilationData = data; | ||
| var diagnostics = comp.GetDiagnostics(); | ||
| Assert.Equal((84, 126), (data.LambdaBindingCount, data.UnboundLambdaStateCount)); |
| @@ -987,5 +987,231 @@ static void M(object o) | |||
| var containingTypes = exprs.SelectAsArray(e => model.GetSymbolInfo(e).Symbol.ContainingSymbol).ToTestDisplayStrings(); | |||
| Assert.Equal(new[] { "A", "B", "B", "A", "B", "B" }, containingTypes); | |||
| } | |||
|
|
|||
| [Fact] | |||
| public void NestedLambdasAndOverloads_NoTypes_NoErrors() | |||
There was a problem hiding this comment.
I think we should verify the IOperation trees for these tests to make sure that we know what semantic information will be availble to the IDE in these cases.
| @@ -1756,14 +1772,18 @@ private ImmutableArray<BoundExpression> BuildArgumentsForErrorRecovery(AnalyzedA | |||
| { | |||
| // bind the argument against each applicable parameter | |||
| var unboundArgument = (UnboundLambda)argument; | |||
| foreach (var parameterList in parameterListList) | |||
| if (NestedLambdaExpressionDepth(unboundArgument.Syntax) <= MaxLambdaExpressionDepthForErrorRecovery && | |||
There was a problem hiding this comment.
NestedLambdaExpressionDepth(unboundArgument.Syntax) <= MaxLambdaExpressionDepthForErrorRecovery
This condition looks questionable to me. Perhaps, the lambda is in the third level of nesting. Why is that important on its own? We might not be rebinding any of the enclosing lambdas for error recovery, doing simple plain body binding, and recovering in this case will not cause any noticeable overhead.
The issue says: "Reduce the overhead of binding lambdas when there are nested lambda expressions with multiple overloads. Specifically, consider avoiding binding nested lambdas unnecessarily for overloads that do not match." It doesn't look like any of the conditions, except the nesting, are checked.
|
Done with review pass (commit 5) |
Inspired by dotnet#59733.
* Bind fully-typed lambda only once for error recovery * Assert number of lambda bindings Inspired by #59733. * Test only with one lambda and one statement * Copy a comment
|
@cston What is the status on this PR please? |
Fixes #63896.