-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Fix nullable warning for implicit extension member access in collection initializers #81103
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
base: main
Are you sure you want to change the base?
Conversation
|
Regarding:
This assessment looks incorrect to me. The representation of old and new extension invocations is different, and deliberately so. We most likely shouldn't make any actual changes to the representation of the call in order to fix the nullable analysis. Instead, we need to adjust the code in NullableWalker which is visiting the call, in order to properly understand it. Look at existing places which call 'IsExtensionBlockMember' for examples. |
…mber access Add methods Co-authored-by: RikkiGibson <[email protected]>
…cit extension member access Co-authored-by: RikkiGibson <[email protected]>
| } | ||
|
|
||
| [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/78828")] | ||
| public void Nullability_CollectionInitializer_WithImplicitExtensionMemberAccess() |
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.
@copilot it looks like the scenario covered by this test is covered just as well by Nullability_CollectionInitializer_WithImplicitExtensionMemberAccess_Nullable. Please delete this test.
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.
Deleted in commit 93c622a.
…ree representation Per @RikkiGibson's feedback, the fix now synthesizes the receiver as a temporary argument for nullable analysis within NullableWalker, rather than modifying the representation. This matches the pattern used in ReInferMethodAndVisitArguments for BoundCall nodes. Co-authored-by: RikkiGibson <[email protected]>
You're correct. I've revised the fix in commit 87bc18e to synthesize the receiver as a temporary argument for nullable analysis within NullableWalker, following the same pattern as ReInferMethodAndVisitArguments. The fix now works with argument results without modifying the BoundTree representation. |
Deleted Nullability_CollectionInitializer_WithImplicitExtensionMemberAccess as its scenario is fully covered by Nullability_CollectionInitializer_WithImplicitExtensionMemberAccess_Nullable. Co-authored-by: RikkiGibson <[email protected]>
|
@copilot It would be preferable to address the issue by making it so VisitObjectCreationExpressionBase calls ReInferMethodAndVisitArguments, rather than reimplementing logic of ReInferMethodAndVisitArguments. |
Per @RikkiGibson's feedback, refactored VisitCollectionElementInitializer to call ReInferMethodAndVisitArguments for new extension methods when not delaying completion, eliminating duplication of its logic. Co-authored-by: RikkiGibson <[email protected]>
Refactored in commit 9101ac6 to call ReInferMethodAndVisitArguments for new extension methods when not delaying completion, avoiding logic duplication. |
|
@RikkiGibson would you say having the copilot agent doing the implementation here was a net benefit? given that you had to read and correct basically everything it did? (i'm just skimming this from the surface) |
Fix nullable warning in object initializer member using extension Add method
Problem
When using implicit extension member access methods (the new
extensionfeature) in object initializer collection Add calls, unexpected nullable warnings are reported. The warning incorrectly references the implicit receiver parameter instead of the actual user-provided parameter.Root Cause
For implicit extension member access methods in collection initializers:
ImplicitReceiverOpt, not in the Arguments arraySolution
Modified
VisitCollectionElementInitializerin NullableWalker to:IsExtensionBlockMember())ReInferMethodAndVisitArgumentsfor new extension methods when not delaying completion (reusing existing logic)addMethodAsMemberOfContainingTypeandsetUpdatedSymbolAsContinuationto handle this caseThe fix leverages existing
ReInferMethodAndVisitArgumentslogic to avoid duplication and ensure consistent handling of new extension methods.Testing
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.