Test/adjust keyword recommendations for Global Using directives #52059

AlekseyTs · 2021-03-22T21:22:36Z

Relates to #51307 (test plan for global usings)

AlekseyTs · 2021-03-23T15:18:50Z

@CyrusNajmabadi, @dotnet/roslyn-compiler Please review.

CyrusNajmabadi · 2021-03-23T17:23:29Z

src/EditorFeatures/CSharpTest2/Recommendations/GlobalKeywordRecommenderTests.cs

+        {
+            await VerifyKeywordAsync(
+@"using Goo;
+$$");


this technically isn't legal right? global usings need to come before normal usings? Or do we allow them to come after?

Note: even if this is not legal, i'm fine with the keyword recommender offering this. #Resolved

this technically isn't legal right? global usings need to come before normal usings? Or do we allow them to come after?

Note: even if this is not legal, i'm fine with the keyword recommender offering this.

You are correct. It is not legal, but I think it is better to offer it and report an error, than to not offer it.

In reply to: 599780626 [](ancestors = 599780626)

but I think it is better to offer it and report an error, than to not offer it.

Agreed. I just wanted to make sure my mental model was correct. In the impl it would be good to have a comment stating that in the place where we make the decisino just to make the intent clear. Up to you. #Resolved

In the impl it would be good to have a comment stating that in the place where we make the decisino just to make the intent clear. Up to you.

There isn't really a logict to specifically allow that. We simply do not make any effort to disallow.

In reply to: 599801190 [](ancestors = 599801190)

CyrusNajmabadi · 2021-03-23T17:24:16Z

src/EditorFeatures/CSharpTest2/Recommendations/GlobalKeywordRecommenderTests.cs

+            await VerifyAbsenceAsync(
+@"global using Goo;
+global $$
+using Bar;");


can you add a test about adding it before a namespace, class, top level statement, and global/normal attributes? #Resolved

CyrusNajmabadi · 2021-03-23T17:25:45Z

src/Features/CSharp/Portable/Completion/KeywordRecommenders/GlobalKeywordRecommender.cs

@@ -36,6 +36,7 @@ protected override bool IsValidContext(int position, CSharpSyntaxContext context
            return
                context.IsStatementContext ||
                context.IsGlobalStatementContext ||
+                UsingKeywordRecommender.IsUsingDirectiveContext(context, forGlobalKeyword: true, cancellationToken) ||


i don't love this in that it seems to go against the general 'context' pattern we have. but i'm not opposed to this, and i'm willing to make the change to unify this with the existing pattern if you're ok with that @AlekseyTs #Resolved

and i'm willing to make the change to unify this with the existing pattern if you're ok with that

Are you talking about unifing this in a follow-up PR, or in contect of this PR. I am not sure it is clear to me what you are offering.

In reply to: 599782529 [](ancestors = 599782529)

So, my preference is that we have a new boolean property on the context object (Called something like IsUsingDirectiveContext and IsGlobalUsingContext). And then this recommender would just check that.

I'm also saying that i'm not expecting that in this PR. I'm also saying, if you'd rather i make that change, i'm happy to do that as well in a followup PR. I don't expect you to have to make that change. Thanks! #Resolved

So, my preference is that we have a new boolean property on the context object (Called something like IsUsingDirectiveContext and IsGlobalUsingContext). And then this recommender would just check that.

I'm also saying that i'm not expecting that in this PR. I'm also saying, if you'd rather i make that change, i'm happy to do that as well in a followup PR. I don't expect you to have to make that change. Thanks!

Thanks for clarifying. I'll let you follow up on this, especially that IsUsingDirectiveContext isn't a property on the context today and I am not sure if it is worth making it a property and precomputing its value (it looks like some properties on the context are precomputed) for scenarios when it is not even needed.

In reply to: 599802941 [](ancestors = 599802941)

sounds good to me #Resolved

CyrusNajmabadi · 2021-03-23T17:26:19Z

src/Features/CSharp/Portable/Completion/KeywordRecommenders/UsingKeywordRecommender.cs

-                }
-
-                return true;
+                return isValidContextAtTheRoot(context, originalToken, cancellationToken);


IDE naming rule for local functions is to PascalCAse them :) we think of them more as methods than as locals. #Resolved

CyrusNajmabadi · 2021-03-23T17:28:20Z

src/Features/CSharp/Portable/Completion/KeywordRecommenders/UsingKeywordRecommender.cs

+                    (previousToken.Parent.IsKind(SyntaxKind.ExternAliasDirective) ||
+                    previousToken.Parent.IsKind(SyntaxKind.UsingDirective)))


Suggested change

(previousToken.Parent.IsKind(SyntaxKind.ExternAliasDirective) ||

previousToken.Parent.IsKind(SyntaxKind.UsingDirective)))

previousToken.Parent.IsKind(SyntaxKind.ExternAliasDirective, SyntaxKind.UsingDirective))

``` #Resolved

CyrusNajmabadi · 2021-03-23T17:45:25Z

src/Features/CSharp/Portable/Completion/KeywordRecommenders/UsingKeywordRecommender.cs

+                {
+                    if (token.Kind() == SyntaxKind.GlobalKeyword)
+                    {
+                        if (!token.Parent.IsKind(SyntaxKind.UsingDirective) || token.TrailingTrivia.Any(SyntaxKind.EndOfLineTrivia))


interesting. can you comment what teh EOL case is for? #Resolved

and, by 'comment' i mean: leave a comment in the code. not in the PR. thanks! #Resolved

CyrusNajmabadi · 2021-03-23T17:47:25Z

...aredUtilitiesAndExtensions/Workspace/CSharp/Extensions/ContextQuery/SyntaxTokenExtensions.cs

@@ -13,11 +13,12 @@ namespace Microsoft.CodeAnalysis.CSharp.Extensions.ContextQuery
 {
    internal static partial class SyntaxTokenExtensions
    {
-        public static bool IsUsingOrExternKeyword(this SyntaxToken token)
+        public static bool IsStartOfUsingOrExternDirective(this SyntaxToken token)


hrmm.. this isn't quite accurate now. prior it was really just askign about the keywords. now it strongly implies that it's at the start of the directive. but that's not really true for the checks below anymore. for exampple using var x or public extern void Foo() would both return true here, but wouldn't really be the start of a using/extern directive. Consider keeping the method the same, and possibly adding the global check to the caller? #Resolved

CyrusNajmabadi · 2021-03-23T17:48:08Z

Love the tests. A couple of nits around the impl. I'm happy to take this up if you'd prefer that. Or, if you're comfortable just making teh changes, that's great too. Thanks! #Resolved

jcouv

Compiler changes LGTM Thanks (iteration 3)

AlekseyTs · 2021-03-23T21:50:12Z

@CyrusNajmabadi I think I addressed feedback.

AlekseyTs · 2021-03-23T21:50:53Z

@dotnet/roslyn-compiler Please review need a second sign-off for small changes in compiler.

cston

Compiler changes LGTM.

CyrusNajmabadi · 2021-03-23T23:06:25Z

src/Features/CSharp/Portable/Completion/KeywordRecommenders/UsingKeywordRecommender.cs

+                    else if (token.Kind() == SyntaxKind.IdentifierToken && SyntaxFacts.GetContextualKeywordKind((string)token.Value!) == SyntaxKind.GlobalKeyword)
+                    {
+                        return true;
+                    }


so there's a slight corner here that you could fixup if you wanted. it looks like with this that if you have:

extern alias Foo;

tehn we won't offer here:

$$ extern alias Foo;

but if you have:

extern alias Foo; $$ extern alias Bar;

taht we will offer.

this is likely so minor as to be irrelevant though :) #Resolved

so there's a slight corner here that you could fixup if you wanted. it looks like with this that if you have:

I'd say that this is a preexisting condition and the current behavior around offering using keyword (when global) isn't present. I'll open an issue to decide if we want that addressed.

In reply to: 600021445 [](ancestors = 600021445)

CyrusNajmabadi

IDE side lgtm. thanks for all the tests

Test/adjust keyword recommendations for Global Using directives

8e9e1ad

AlekseyTs added Area-Compilers PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Mar 22, 2021

AlekseyTs added 2 commits March 22, 2021 17:33

fixup

f011a4f

fixup

4a3b33b

AlekseyTs removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Mar 23, 2021

AlekseyTs marked this pull request as ready for review March 23, 2021 15:17

AlekseyTs requested review from a team as code owners March 23, 2021 15:18

AlekseyTs requested a review from CyrusNajmabadi March 23, 2021 15:18

jcouv added the Area-IDE label Mar 23, 2021

jcouv added this to the C# 10 milestone Mar 23, 2021

CyrusNajmabadi reviewed Mar 23, 2021

View reviewed changes

jcouv approved these changes Mar 23, 2021

View reviewed changes

jcouv self-assigned this Mar 23, 2021

PR feedback

91425ab

cston approved these changes Mar 23, 2021

View reviewed changes

CyrusNajmabadi reviewed Mar 23, 2021

View reviewed changes

CyrusNajmabadi approved these changes Mar 23, 2021

View reviewed changes

AlekseyTs merged commit bef5415 into dotnet:features/GlobalUsingDirective Mar 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test/adjust keyword recommendations for Global Using directives #52059

Test/adjust keyword recommendations for Global Using directives #52059

AlekseyTs commented Mar 22, 2021 •

edited by jcouv

Loading

AlekseyTs commented Mar 23, 2021

CyrusNajmabadi Mar 23, 2021 •

edited by AlekseyTs

Loading

AlekseyTs Mar 23, 2021

CyrusNajmabadi Mar 23, 2021 •

edited by AlekseyTs

Loading

AlekseyTs Mar 23, 2021

CyrusNajmabadi Mar 23, 2021 •

edited by AlekseyTs

Loading

CyrusNajmabadi Mar 23, 2021 •

edited by AlekseyTs

Loading

AlekseyTs Mar 23, 2021

CyrusNajmabadi Mar 23, 2021 •

edited by AlekseyTs

Loading

AlekseyTs Mar 23, 2021 •

edited

Loading

CyrusNajmabadi Mar 23, 2021 •

edited by AlekseyTs

Loading

CyrusNajmabadi Mar 23, 2021 •

edited by AlekseyTs

Loading

CyrusNajmabadi Mar 23, 2021 •

edited by AlekseyTs

Loading

CyrusNajmabadi Mar 23, 2021 •

edited by AlekseyTs

Loading

CyrusNajmabadi Mar 23, 2021 •

edited by AlekseyTs

Loading

CyrusNajmabadi Mar 23, 2021 •

edited by AlekseyTs

Loading

CyrusNajmabadi commented Mar 23, 2021 •

edited by AlekseyTs

Loading

jcouv left a comment

AlekseyTs commented Mar 23, 2021

AlekseyTs commented Mar 23, 2021

cston left a comment

CyrusNajmabadi Mar 23, 2021 •

edited by AlekseyTs

Loading

AlekseyTs Mar 23, 2021

CyrusNajmabadi left a comment

		(previousToken.Parent.IsKind(SyntaxKind.ExternAliasDirective) \|\|
		previousToken.Parent.IsKind(SyntaxKind.UsingDirective)))

Test/adjust keyword recommendations for Global Using directives #52059

Test/adjust keyword recommendations for Global Using directives #52059

Conversation

AlekseyTs commented Mar 22, 2021 • edited by jcouv Loading

AlekseyTs commented Mar 23, 2021

CyrusNajmabadi Mar 23, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Mar 23, 2021

Choose a reason for hiding this comment

CyrusNajmabadi Mar 23, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Mar 23, 2021

Choose a reason for hiding this comment

CyrusNajmabadi Mar 23, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

CyrusNajmabadi Mar 23, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Mar 23, 2021

Choose a reason for hiding this comment

CyrusNajmabadi Mar 23, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi Mar 23, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

CyrusNajmabadi Mar 23, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

CyrusNajmabadi Mar 23, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

CyrusNajmabadi Mar 23, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

CyrusNajmabadi Mar 23, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

CyrusNajmabadi Mar 23, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

CyrusNajmabadi commented Mar 23, 2021 • edited by AlekseyTs Loading

jcouv left a comment

Choose a reason for hiding this comment

AlekseyTs commented Mar 23, 2021

AlekseyTs commented Mar 23, 2021

cston left a comment

Choose a reason for hiding this comment

CyrusNajmabadi Mar 23, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Mar 23, 2021

Choose a reason for hiding this comment

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

AlekseyTs commented Mar 22, 2021 •

edited by jcouv

Loading

CyrusNajmabadi Mar 23, 2021 •

edited by AlekseyTs

Loading

CyrusNajmabadi Mar 23, 2021 •

edited by AlekseyTs

Loading

CyrusNajmabadi Mar 23, 2021 •

edited by AlekseyTs

Loading

CyrusNajmabadi Mar 23, 2021 •

edited by AlekseyTs

Loading

CyrusNajmabadi Mar 23, 2021 •

edited by AlekseyTs

Loading

AlekseyTs Mar 23, 2021 •

edited

Loading

CyrusNajmabadi Mar 23, 2021 •

edited by AlekseyTs

Loading

CyrusNajmabadi Mar 23, 2021 •

edited by AlekseyTs

Loading

CyrusNajmabadi Mar 23, 2021 •

edited by AlekseyTs

Loading

CyrusNajmabadi Mar 23, 2021 •

edited by AlekseyTs

Loading

CyrusNajmabadi Mar 23, 2021 •

edited by AlekseyTs

Loading

CyrusNajmabadi Mar 23, 2021 •

edited by AlekseyTs

Loading

CyrusNajmabadi commented Mar 23, 2021 •

edited by AlekseyTs

Loading

CyrusNajmabadi Mar 23, 2021 •

edited by AlekseyTs

Loading