-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Extensions: apply same nullability checks on receiver as for classic extension methods #81871
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
e03370c to
8fbdbbe
Compare
8fbdbbe to
75cfef6
Compare
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.DebugVerifier.cs
Outdated
Show resolved
Hide resolved
|
|
||
| void setAnalyzedNullabilityAndUpdateSymbol(BoundAssignmentOperator node, BoundObjectInitializerMember objectInitializer, Symbol? updatedSymbol) | ||
| { | ||
| // https://github.com/dotnet/roslyn/issues/35040: Should likely be setting _resultType in VisitObjectCreationInitializer |
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 this issue still valid? It seems like since we could be calling this from a continuation, the _resultType may have changed since then. Perhaps we should close that issue out and remove the reference to it. #Resolved
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 think the issue is still valid. The type from initial binding (stored in the bound node) wouldn't have the nullable-updated information.
RikkiGibson
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.
Done review pass
|
@dotnet/roslyn-compiler for second review. Thanks |
Reverted changes related to setters
| if (isExtensionBlockMember) | ||
| { | ||
| Debug.Assert(symbol is not null); | ||
| refKindsOpt = AdjustArgumentRefKindsIfNeeded(refKindsOpt, isExtensionBlockMember, symbol, objectInitializer.Arguments.Length); |
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.
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 5)
Fixes #81851 (conversion on extension receiver should be subject to same rules as conversion on
thisparameter). This is just a matter of passing the right flag indicating that the first argument is an extension receiver.For readability, I switched usages of
Actiondelegates to use named delegates.I also switched usages of tuples to use named types (this made it easier to add an extra field).
Relates to #76130