Implement FAR for GetAwaiter methods #28230
Conversation
| @@ -231,6 +231,18 @@ public ForEachSymbols GetForEachSymbols(SemanticModel semanticModel, SyntaxNode | |||
| } | |||
| } | |||
|
|
|||
| public IMethodSymbol GetAwaitExpressionMethod(SemanticModel semanticModel, SyntaxNode node) | |||
There was a problem hiding this comment.
as weird as it sounds, i think this should be: GetGetAwaiterMethod, or if hat sounds awful: GetInvokedGetAwaiterMethod.
There was a problem hiding this comment.
Is AwaitExpressionInfo shared across languages? If so, this could also be: GetAwaitExpressionInfo.
There was a problem hiding this comment.
Unfortunately, there are two AwaitExpressionInfo types (one for VB and one for C#).
There was a problem hiding this comment.
Ughhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhh
can we have a CommonAwaitExpressionInfo? (seriously...)?
There was a problem hiding this comment.
Both AwaitExpressionInfo types are structs :-(
So if we want a common experience for callers, we'd have to use some interface.
There was a problem hiding this comment.
Or a Common-Struct. we do that for some other APIs.
| { | ||
| if (node is AwaitExpressionSyntax awaitExpression) | ||
| { | ||
| var builder = ArrayBuilder<IMethodSymbol>.GetInstance(); |
| public bool IsAwaitExpression(SyntaxNode node) | ||
| { | ||
| return node.IsKind(SyntaxKind.AwaitExpression); | ||
| } |
| var info = await SyntaxTreeIndex.GetIndexAsync(d, c).ConfigureAwait(false); | ||
| return info.ContainsAwait; | ||
| }, cancellationToken); | ||
| } |
There was a problem hiding this comment.
note: we're starting to have a bunch of duplication with the above methods. consider having a single helper here that takes a Func<SyntaxTreeIndex, bool> and does all the common work.
| } | ||
|
|
||
| return locations.ToImmutableAndFree(); | ||
| } |
There was a problem hiding this comment.
these methods feel like there's a lot of duplication (in the overall logic, even though individual bits seem different). I'm on the fence if we should extract out a common helper...
There was a problem hiding this comment.
Thanks. I think the factorings you suggested are worth it, as I'll probably add support for Dispose in using and Add in collection initializers next.
| End If | ||
|
|
||
| Return Nothing | ||
| End Function |
There was a problem hiding this comment.
ok. it feels lke we don't need this method. We already have IsAwaitExpression. So all we need to do is call model.GetAwaitExpressionInfo() if we ahve an await expression. The rest of the code can all be shared by the caller.
There was a problem hiding this comment.
I left this method as-is, due to the problem with AwaitExpressionInfo.
|
Thanks @heejaechang and @CyrusNajmabadi for the review. @jinujoseph for ask-mode approval for 15.8. Thanks |
|
Approve to merge for 15.8.Preview5 |
…atures/compiler * dotnet/features/compiler: (137 commits) Actually fix tabs. Added back breaking changes doc. Addressed PR feedback. Track enclosing symbol in data flow analysis, which might be a field or property. (dotnet#28252) Change text from 'Spell check' to 'Fix typo'. Implement FAR for GetAwaiter methods (dotnet#28230) Fix typo Fix typo Fix import. Address PR feedback and fix failing test. Lower expressions of in arguments to correct temp locals (dotnet#28181) Move to 2.0.7 runtime Produce errors on ref-assignment to non-ref parameters Fix spelling. Preserve left-to-right evaluation order for dynamic compound addition and subtraction. inline. Provide a spellchecking suggestion when someone mispells a constructor. Typo (dotnet#28177) PR Comments Fix regression in bitness of Interactive Window host (dotnet#28006) ...
The parts of the change:
awaitexpressions into the syntax tree index (and update theSerializationFormatfor the index),AbstractReferenceFinderandOrdinaryMethodReferenceFinderinspectawaitexpressions when looking for references to aGetAwaitermethod,CSharpRenameConflictLanguageServiceproduces a conflict if you try to rename aGetAwaitermethod that is used in anawaitexpression.Customer scenario
Invoked FindAllReferences on a
GetAwaitermethod. You'd expect theawaitexpressions that implicitly involve that method to be identified as references.Bugs this fixes
Fixes #25714
Workarounds, if any
N/A
Risk
Performance impact
Low. This is following a well-established template (see similar change for FAR to handle
Deconstructmethods).Is this a regression from a previous update?
No
How was the bug found?
Reported by customer