Implement FAR for GetAwaiter methods by jcouv · Pull Request #28230 · dotnet/roslyn

jcouv · 2018-06-30T19:15:23Z

The parts of the change:

store information about which documents have await expressions into the syntax tree index (and update the SerializationFormat for the index),
AbstractReferenceFinder and OrdinaryMethodReferenceFinder inspect await expressions when looking for references to a GetAwaiter method,
CSharpRenameConflictLanguageService produces a conflict if you try to rename a GetAwaiter method that is used in an await expression.

Customer scenario

Invoked FindAllReferences on a GetAwaiter method. You'd expect the await expressions 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 Deconstruct methods).

Is this a regression from a previous update?

No

How was the bug found?

Reported by customer

CyrusNajmabadi · 2018-06-30T19:42:41Z

src/Workspaces/CSharp/Portable/LanguageServices/CSharpSemanticFactsService.cs

            }
        }

+        public IMethodSymbol GetAwaitExpressionMethod(SemanticModel semanticModel, SyntaxNode node)


as weird as it sounds, i think this should be: GetGetAwaiterMethod, or if hat sounds awful: GetInvokedGetAwaiterMethod.

Is AwaitExpressionInfo shared across languages? If so, this could also be: GetAwaitExpressionInfo.

Unfortunately, there are two AwaitExpressionInfo types (one for VB and one for C#).

Ughhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhh

can we have a CommonAwaitExpressionInfo? (seriously...)?

Both AwaitExpressionInfo types are structs :-(
So if we want a common experience for callers, we'd have to use some interface.

Or a Common-Struct. we do that for some other APIs.

CyrusNajmabadi · 2018-06-30T19:43:16Z

src/Workspaces/CSharp/Portable/LanguageServices/CSharpSemanticFactsService.cs

+        {
+            if (node is AwaitExpressionSyntax awaitExpression)
+            {
+                var builder = ArrayBuilder<IMethodSymbol>.GetInstance();


builder unneeded.

CyrusNajmabadi · 2018-06-30T19:43:34Z

src/Workspaces/CSharp/Portable/LanguageServices/CSharpSyntaxFactsService.cs

+        public bool IsAwaitExpression(SyntaxNode node)
+        {
+            return node.IsKind(SyntaxKind.AwaitExpression);
+        }


CyrusNajmabadi · 2018-06-30T19:44:41Z

src/Workspaces/Core/Portable/FindSymbols/FindReferences/Finders/AbstractReferenceFinder.cs

+                var info = await SyntaxTreeIndex.GetIndexAsync(d, c).ConfigureAwait(false);
+                return info.ContainsAwait;
+            }, cancellationToken);
+        }


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.

CyrusNajmabadi · 2018-06-30T19:46:42Z

src/Workspaces/Core/Portable/FindSymbols/FindReferences/Finders/AbstractReferenceFinder.cs

+            }
+
+            return locations.ToImmutableAndFree();
+        }


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...

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.

CyrusNajmabadi · 2018-06-30T19:48:49Z

src/Workspaces/VisualBasic/Portable/LanguageServices/VisualBasicSemanticFactsService.vb

+            End If
+
+            Return Nothing
+        End Function


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.

I left this method as-is, due to the problem with AwaitExpressionInfo.

CyrusNajmabadi

Nice! I would def like a better story around AwaitExpressionInfo. However, given the current compiler API, this is the best we can do currently :)

jcouv · 2018-07-02T22:51:54Z

Thanks @heejaechang and @CyrusNajmabadi for the review.

@jinujoseph for ask-mode approval for 15.8. Thanks

jinujoseph · 2018-07-03T07:24:56Z

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) ...

Implement FAR for GetAwaiter methods

3d57dcc

jcouv added the Area-IDE label Jun 30, 2018

jcouv added this to the 15.8 milestone Jun 30, 2018

jcouv requested a review from a team as a code owner June 30, 2018 19:15

This was referenced Jun 30, 2018

FindAllReferences should handle Add methods used in collection initializers #28229

Closed

FindAllReferences should handle Dispose referenced in using and foreach statements #28228

Closed

CyrusNajmabadi reviewed Jun 30, 2018

View reviewed changes

Address PR feedback from Cyrus

e5e3888

CyrusNajmabadi approved these changes Jun 30, 2018

View reviewed changes

jinujoseph assigned heejaechang Jul 2, 2018

heejaechang approved these changes Jul 2, 2018

View reviewed changes

jinujoseph added the Approved to merge label Jul 3, 2018

jcouv merged commit 2ccd656 into dotnet:master Jul 3, 2018

jcouv deleted the far-getawaiter branch July 3, 2018 07:39

jcouv mentioned this pull request Jul 5, 2018

Track if a file contains a tuple expression or tuple type in our index. #28287

Merged

jcouv mentioned this pull request Dec 6, 2023

FAR on Create method of a collection builder doesn't find all the usages #71133

Closed

Conversation

jcouv commented Jun 30, 2018

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

How was the bug found?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Jul 2, 2018

Uh oh!

jinujoseph commented Jul 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jcouv Jul 1, 2018 •

edited

Loading