Push task-ifying extract-method return type to the code gen stage. #76670
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| @@ -89,7 +89,7 @@ protected override IMethodSymbol GenerateMethodDefinition( | |||
| attributes: [], | |||
| accessibility: Accessibility.Private, | |||
| modifiers: CreateMethodModifiers(), | |||
| returnType: AnalyzerResult.ReturnType, | |||
| returnType: this.GetFinalReturnType(), | |||
There was a problem hiding this comment.
only when we're actually generating the final method declaration do we get teh type, potentially wrapped with 'Task'. Everything else sees the 'core' type and can ask questions of that.
| @@ -209,7 +209,7 @@ private bool ShouldPutUnsafeModifier() | |||
| private DeclarationModifiers CreateMethodModifiers() | |||
| { | |||
| var isUnsafe = ShouldPutUnsafeModifier(); | |||
| var isAsync = this.SelectionResult.CreateAsyncMethod(); | |||
| var isAsync = this.SelectionResult.ContainsAwaitExpression(); | |||
There was a problem hiding this comment.
renamed for clarity
| { | ||
| Name: nameof(Task.ConfigureAwait), | ||
| Parameters: [{ Type.SpecialType: SpecialType.System_Boolean }], | ||
| })) |
There was a problem hiding this comment.
note: i removed the check that the final type we're extracting actually has a .ConfigureAwait(bool) method on it. We're only adding .ConfigureAwait(false) because we saw it in the user's original code. So we can trust they know what pattern they're following and that we should follow it as well. This also makes things work if the ConfigureAwait is an extension method.
| @@ -68,27 +66,6 @@ protected override SyntaxNode GetNodeForDataFlowAnalysis() | |||
| : node; | |||
| } | |||
|
|
|||
| protected override bool UnderAnonymousOrLocalMethod(SyntaxToken token, SyntaxToken firstToken, SyntaxToken lastToken) | |||
There was a problem hiding this comment.
these are no longer needed as they were hacks on how we previously determined certain syntactic facts about the code being extracted.
| // change return type to be wrapped in Task | ||
| var shouldPutAsyncModifier = SelectionResult.CreateAsyncMethod(); | ||
| if (shouldPutAsyncModifier) | ||
| return WrapReturnTypeInTask(returnType); |
There was a problem hiding this comment.
we no longer do the wrapping step here. we hold off on that until we need the final type for the method decl to have.
| /// </summary> | ||
| /// <param name="predicate"></param> | ||
| /// <returns></returns> | ||
| private bool CheckNodesInSelection(Func<ISyntaxFacts, SyntaxNode, bool> predicate) |
There was a problem hiding this comment.
this is the right wayu to write the loop. descend the nodes. do not descend into localfunctions/lambdas, and check only those that intersect the span of interest. Both ContainsAwaitExpression and ContainsConfigureAwait can now jusst defer to this helper.
| { | ||
| get | ||
| { | ||
| return ReturnType.SpecialType != SpecialType.System_Void && !AwaitTaskReturn; |
There was a problem hiding this comment.
note: i genuinely don't know what this expression meant before. especially the AwaitTaskReturn part. This is much clearer now. There is a core return type (prior to any task wrapping) and it can be checked. To know if there is a return type or not, it can be checked against System.Void.
CyrusNajmabadi
changed the title
| continue; | ||
|
|
||
| if (IsConfigureAwaitFalse(node) && !UnderAnonymousOrLocalMethod(node.GetFirstToken(), firstToken, lastToken)) | ||
| if (current.Span.OverlapsWith(span) && predicate(syntaxFacts, current)) |
There was a problem hiding this comment.
hrm... i could potentially remove it. iw as being paranoid. let me see about that in followup.
There was a problem hiding this comment.
ok. this can be removed. will do in followup pr.
The goal here is to have the internal model for EM store the true type of the value returned by the extract method, as a distinct concept from what the final return-type might be (which might be wrapped in something like 'Task').
This benefits the work being down in another branch to extract methods with complex flow control, but allowing manipulation of the original return type, a new return value for flow control purposes, and then any wrapping that needs to happen around that.