-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Fix WasPropertyBackingFieldAccessChecked assertion with query continuations #81793
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
Co-authored-by: 333fred <[email protected]>
…y set Co-authored-by: 333fred <[email protected]>
|
@copilot The change in Binder.ValueChecks is incorrect. The assert should remain as is, it is what is showing us the problem exists. You need to find and fix the actual cause. |
…roupBy call Co-authored-by: 333fred <[email protected]>
You're right - I've reverted the Binder.ValueChecks.cs change and fixed the actual root cause. In Binder_Query.cs line 279, the optimized GroupBy invocation was passing Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@dotnet/roslyn-compiler for review |
| // The optimized form. We store the unoptimized form for analysis | ||
| unoptimizedForm = result; | ||
| result = MakeQueryInvocation(state.selectOrGroup, e, receiverIsCheckedForRValue: false, "GroupBy", lambdaLeft, diagnostics | ||
| result = MakeQueryInvocation(state.selectOrGroup, e, receiverIsCheckedForRValue: true, "GroupBy", lambdaLeft, 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 receiver (e) is checked on line 263. I've re-audited the rest of the call sites of MakeQueryInvocation and didn't see any others that need to be updated.
|
Done with review pass (commit 6) #Closed |
…CheckValue` are retained.
Should this Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs:7767 in ea83009. [](commit_id = ea83009, deletion_comment = False) |
|
Done with review pass (commit 7) #Closed |
Co-authored-by: 333fred <[email protected]>
…Invocation always calls CheckValue Per @AlekseyTs's suggestion: - Modified CheckValue to create a new node under DEBUG instead of mutating the original - Removed conditional logic in MakeQueryInvocation to always call CheckValue unconditionally - This allows the same node to be passed to CheckValue multiple times safely Co-authored-by: 333fred <[email protected]>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@AlekseyTs ready for another review |
|
@333fred It looks like there are still errors in XML doc comments #Closed |
| get { return this.PropertySymbol; } | ||
| } | ||
|
|
||
| public BoundPropertyAccess Clone() |
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.
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.
I viewed this as effectively an Update method, which is why I put it in this file. If you prefer, I can move it out.
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.
I viewed this as effectively an Update method, which is why I put it in this file.
The reason for this conclusion isn't that obvious to me. For example, why Constructors.cs isn't the right place? I just want to make sure that the right rational is applied consistently.
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.
I can see the argument for viewing this as a constructor, but perhaps for the most clarity I'll move it to its own file.
|
Done with review pass (commit 13) |
AlekseyTs
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 (commit 14). Still having questions about the new API placement
|
@dotnet/roslyn-compiler for a second review. |
|
Should there be a link to the issue being fixed in the PR description? |
Debug builds fail with "should not be set twice" assertion when compiling LINQ queries containing
group by intowith property accesses on anonymous types.Root Cause
In
Binder_Query.cs, when processing agroup byclause with the optimized form (when the group expression is the range variable identifier), the code was callingMakeQueryInvocationtwice with the same receiver. This caused the receiver's property accesses to be checked twice, triggering the "should not be set twice" assertion.Changes
Binder.ValueChecks.cs: Modified
CheckValueto create a newBoundPropertyAccessnode under DEBUG instead of mutating the original node'sWasPropertyBackingFieldAccessCheckedflag. To ensure a new node is always created (sinceUpdate()returns the same instance if nothing changed), the implementation togglesInitialBindingReceiverIsSubjectToCloningand then restores it, forcing two Update calls. This allows the same node to be passed toCheckValuemultiple times safely.Binder_Query.cs: Removed the conditional logic around
CheckValuecall inMakeQueryInvocation. Now always callsCheckValueunconditionally since it's safe to call multiple times on the same node.Binder_Expressions.cs: Updated
crefdocumentation to match the current signature ofMakeQueryInvocation.QueryTests.cs: Added test case for the specific scenario.
Example
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.