-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Ensure that CheckValue isn't called twice on lambda query receivers #80010
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
When creating query expressions, most of the time the receiver the query is not checked for being a valid RValue by calling code. However, in one case, it is; this can then cause an assert that we are checking for field-backed property access multiple times. Fixes dotnet#80008.
|
|
||
| // We transform the expression from "expr" to "expr.Cast<castTypeOpt>()". | ||
| boundExpression = lambdaBodyBinder.MakeQueryInvocation(expression, boundExpression, "Cast", castTypeSyntax, castType, diagnostics | ||
| boundExpression = lambdaBodyBinder.MakeQueryInvocation(expression, boundExpression, receiverIsCheckedForRValue: true, "Cast", castTypeSyntax, castType, diagnostics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the bug: lambdaBodyBinder.BindValue above calls CheckValue on its result. In every other MakeQueryInvocation callsite, we are not passing something that has already been checked; this is the only location where we are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm, the added tests in this file also asserted before this change.
|
@dotnet/roslyn-compiler for review. |
7406282 to
7a95ae3
Compare
| receiver = new BoundBadExpression(receiver.Syntax, LookupResultKind.NotAValue, ImmutableArray<Symbol?>.Empty, ImmutableArray.Create(receiver), CreateErrorType()); | ||
| } | ||
| else | ||
| else if (!receiverIsCheckedForRValue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any risk that we will not call updateUltimateReceiver for this expression now and have the wrong receiver in the bound tree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. The body here is not a BoundQueryClause.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider asserting that
7a95ae3 to
65e88a8
Compare
|
@dotnet/roslyn-compiler for a second review |
1 similar comment
|
@dotnet/roslyn-compiler for a second review |
jcouv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM Thanks (commit 2)
When creating query expressions, most of the time the receiver the query is not checked for being a valid RValue by calling code. However, in one case, it is; this can then cause an assert that we are checking for field-backed property access multiple times. Fixes #80008.