Analyze flow in receiver of extension method group in delegate creation #60093
Conversation
jcouv
force-pushed
the
definite-assignment
branch
from
eca246b to
c159d78
Compare
| { | ||
| VisitLocalFunctionUse(localFunc, node.Syntax, isCall: false); | ||
| } | ||
| else if (node.MethodOpt is { } method && methodGroup.ReceiverOpt is { } receiver && !ignoreReceiver(receiver, method)) | ||
| { | ||
| EnterRegionIfNeeded(methodGroup); | ||
| VisitRvalue(methodGroup.ReceiverOpt); |
There was a problem hiding this comment.
I would use the local receiver here #Closed
| static bool ignoreReceiver(BoundExpression receiver, MethodSymbol method) | ||
| { | ||
| // ignore the implicit `this` receiver on a static method | ||
| return method.IsStatic && receiver is { Kind: BoundKind.ThisReference, WasCompilerGenerated: true }; |
There was a problem hiding this comment.
Perhaps we should add a comment here:
we may need to visit the receiver / can't ignore static methods at all because of extension methods.
| static bool ignoreReceiver(BoundExpression receiver, MethodSymbol method) | ||
| { | ||
| // ignore the implicit `this` receiver on a static method | ||
| return method.IsStatic && receiver is { Kind: BoundKind.ThisReference, WasCompilerGenerated: true }; |
There was a problem hiding this comment.
I think we can have this as a receiver for a legitimate extension method reference. We have a special property IsExtensionMethod clearly identifying extension methods. Consider using it instead and, if it is not set, ignoring receivers for a static method. #Closed
There was a problem hiding this comment.
There was a problem hiding this comment.
That might be too broad. AnalyzeDataFlow should still see the invalid receiver as being read for extract method to work as intended.
There was a problem hiding this comment.
AnalyzeDataFlow should still see the invalid receiver as being read for extract method to work as intended.
I am not sure if I agree with that. Clearly, the receiver for static methods (excluding extension scenario) is not going to be evaluated, so I think we have a flexibility in terms of how flow analysis should work in the error scenario.
The current implementation is incorrect in my opinion. Primarily because the PR description provides no clarity about what is our philosophy around visiting and not visiting receivers. Also, it is not clear to me why do we want to handle receivers differently by comparison to BoundCall. Perhaps the behavior is the same, but it is not obvious from the code.
There was a problem hiding this comment.
In general, I do not see us actually making sure that region analysis will work for any misplaced expression. For example, there are BoundLocalDeclaration.ArgumentsOpt and it looks like flow analysis never visits them. Therefore, it is not clear to me why we should visit erroneous (misplaced) receiver. BTW, the issue that we are trying to fix is about failure for a valid scenario.
|
Done with review pass (commit 3) |
|
@dotnet/roslyn-compiler for second review. Thanks |
ghost
added this to the 
| @@ -1533,8 +1533,8 @@ public override BoundNode VisitDelegateCreationExpression(BoundDelegateCreationE | |||
|
|
|||
| static bool ignoreReceiver(BoundExpression receiver, MethodSymbol method) | |||
| { | |||
| // ignore the implicit `this` receiver on a static method | |||
| return method.IsStatic && receiver is { Kind: BoundKind.ThisReference, WasCompilerGenerated: true }; | |||
| // static methods that aren't extensions get an implicit `this` receiver that should be ignored | |||
There was a problem hiding this comment.
Do we have a test for an extension method called in non-reduced form? It may not be very meaningful since I think this would either have no receiver or have a type expression as a receiver, but thought I would check anyway.
There was a problem hiding this comment.
the static method itself doesn't get an implicit receiver - but the methodgroup...
Fixes #59738 again
This is PR #59874, but with an extra commit to ignore the implicit
thisreceiver on a static method.Adjusting the initial binding to remove that receiver seems desirable but riskier, so I'm sticking with a local fix. See #56395 for a similar situation.