Skip to content

Conversation

@333fred
Copy link
Member

@333fred 333fred commented Aug 21, 2025

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.

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.
@333fred 333fred requested a review from a team as a code owner August 21, 2025 23:38

// 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
Copy link
Member Author

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.

Copy link
Member Author

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.

@333fred
Copy link
Member Author

333fred commented Aug 21, 2025

@dotnet/roslyn-compiler for review.

@333fred 333fred force-pushed the fix-property-access branch from 7406282 to 7a95ae3 Compare August 22, 2025 16:39
receiver = new BoundBadExpression(receiver.Syntax, LookupResultKind.NotAValue, ImmutableArray<Symbol?>.Empty, ImmutableArray.Create(receiver), CreateErrorType());
}
else
else if (!receiverIsCheckedForRValue)
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider asserting that

@333fred 333fred force-pushed the fix-property-access branch from 7a95ae3 to 65e88a8 Compare August 22, 2025 17:47
@333fred
Copy link
Member Author

333fred commented Aug 22, 2025

@dotnet/roslyn-compiler for a second review

1 similar comment
@333fred
Copy link
Member Author

333fred commented Aug 25, 2025

@dotnet/roslyn-compiler for a second review

Copy link
Member

@jcouv jcouv left a 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)

@jcouv jcouv self-assigned this Aug 25, 2025
@333fred 333fred enabled auto-merge (squash) August 26, 2025 19:00
@333fred 333fred disabled auto-merge August 29, 2025 00:18
@333fred 333fred merged commit 5bde237 into dotnet:main Aug 29, 2025
24 checks passed
@333fred 333fred deleted the fix-property-access branch August 29, 2025 00:18
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Aug 29, 2025
@akhera99 akhera99 modified the milestones: Next, 18.0 P1, 18.0 P2 Sep 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Assert in WasPropertyBackingFieldAccessChecked

4 participants