Completion: Suggest types after keywords indicating a local function #54493

MaStr11 · 2021-06-30T12:51:44Z

…n context.

MaStr11 · 2021-06-30T12:55:59Z

...SharedUtilitiesAndExtensions/Workspace/CSharp/Extensions/ContextQuery/CSharpSyntaxContext.cs

+                SyntaxKind.ExternKeyword,
+                SyntaxKind.StaticKeyword,
+                SyntaxKind.AsyncKeyword,
+                SyntaxKind.UnsafeKeyword,


This list is derived from all calls to IsLocalFunctionDeclarationContext (ExternKeywordRecommender, StaticKeywordRecommender, AsyncKeywordRecommender and UnsafeKeywordRecommender). The spec only mentions async and unsafe, but dates back to C# 7.0.

MaStr11 · 2021-06-30T13:08:37Z

src/Workspaces/CSharp/Portable/Recommendations/CSharpRecommendationServiceRunner.cs

@@ -78,7 +78,7 @@ private ImmutableArray<ISymbol> GetSymbolsForCurrentContext()
                // is a type-only context, we'll show all symbols anyway.
                return GetSymbolsForExpressionOrStatementContext();
            }
-            else if (_context.IsTypeContext || _context.IsNamespaceContext)
+            else if (_context.IsTypeContext || _context.IsNamespaceContext || _context.IsLocalFunctionDeclarationContext)


@CyrusNajmabadi This is probably naive or just plain wrong. Do you have any objections? If not, I will add some more tests.

I'm not sure if IsTypeContext should be true here instead of introducing the new property. Also consider revisiting other callers to IsTypeContext and check what the desired behavior is for these callers.

For example, generate type codefix is (most likely) also affected by IsTypeContext returning false.

roslyn/src/Features/Core/Portable/GenerateType/AbstractGenerateTypeService.State.cs

Line 135 in acf3e27

if (!semanticFacts.IsTypeContext(semanticModel, NameOrMemberAccessExpression.SpanStart, cancellationToken) &&

class C { Test NotLocal() { return null; } // Generate class 'Test' is offered here void M() { static Test Local(){ return null; } // But not offered here. } }

I gave this a try in 842a1f6 and removed the new property in eb1b5be. This results in one test failure, discussed below.

I tracked down every reference of IsTypeContext (at least I tried) and added tests for each occurrence.

Youssef1313 · 2021-06-30T14:31:15Z

src/EditorFeatures/CSharpTest/Completion/CompletionProviders/SymbolCompletionProviderTests.cs

+    }
+}
+";
+            await VerifyItemExistsAsync(markup, "String");


Is string offered too? It has a separate keyword recommender IIRC (there are other keywords too).

It wasn't offered. I fixed this and added tests.

…LocalFunctionKeywords

…ntext

MaStr11 · 2021-07-19T10:57:32Z

@CyrusNajmabadi This simple approach expands syntaxTree.IsTypeContext, to also be true in syntaxTree.IsLocalFunctionDeclarationContext. The only test that fails with this addition is the delegate keyword recommender (test failure: delegate is suggested after static):

roslyn/src/EditorFeatures/CSharpTest2/Recommendations/DelegateKeywordRecommenderTests.cs

Lines 289 to 291 in 6453b4f

    
           public async Task TestAfterStatic() 
        
           { 
        
               await VerifyAbsenceAsync(SourceCodeKind.Regular, @"static $$");

The root cause is here in ValidTypeContext:

roslyn/src/Features/CSharp/Portable/Completion/KeywordRecommenders/DelegateKeywordRecommender.cs

Lines 31 to 47 in 6453b4f

    
           protected override bool IsValidContext(int position, CSharpSyntaxContext context, CancellationToken cancellationToken) 
        
           { 
        
               return 
        
                   context.IsGlobalStatementContext || 
        
                   ValidTypeContext(context) || 
        
                   IsAfterAsyncKeywordInExpressionContext(context, cancellationToken) || 
        
                   context.IsTypeDeclarationContext( 
        
                       validModifiers: s_validModifiers, 
        
                       validTypeDeclarations: SyntaxKindSet.ClassInterfaceStructRecordTypeDeclarations, 
        
                       canBePartial: false, 
        
                       cancellationToken: cancellationToken); 
        
               static bool ValidTypeContext(CSharpSyntaxContext context) 
        
                   => (context.IsNonAttributeExpressionContext || context.IsTypeContext) 
        
                      && !context.IsConstantExpressionContext 
        
                      && !context.LeftToken.IsTopLevelOfUsingAliasDirective(); 
        
           }

I'm not too confident about the delegate keyword, but to me, it feels like it should not be suggested in a type context. For instance, it is suggested in method parameters (void M($$), which I think is wrong. @CyrusNajmabadi It probably should read context.IsTypeDeclarationContext in line 44?

@CyrusNajmabadi What do you think about the approach of expanding syntaxTree.IsTypeContext? What should we do about the DelegateKeywordRecommender.

CyrusNajmabadi · 2021-07-19T17:29:23Z

delegate is now legal in a type-context due to function pointers (which look like delegate*<int, void>). So i would update thigns accordingly based on that. If you still have any remaining issues/questions, lmk at that point. Thanks!

…SyntaxContext" This reverts commit eb1b5be.

MaStr11 · 2021-07-20T19:28:54Z

@CyrusNajmabadi Ready for review

CyrusNajmabadi · 2021-07-20T20:05:43Z

src/EditorFeatures/CSharpTest2/Recommendations/DelegateKeywordRecommenderTests.cs

@@ -288,7 +288,7 @@ public async Task TestNotAfterSealed()
        [Fact, Trait(Traits.Feature, Traits.Features.KeywordRecommending)]
        public async Task TestAfterStatic()
        {
-            await VerifyAbsenceAsync(SourceCodeKind.Regular, @"static $$");
+            await VerifyKeywordAsync(SourceCodeKind.Regular, @"static $$");
            await VerifyKeywordAsync(SourceCodeKind.Script, @"static $$");


can we have a test for this after static inside a method as well?

Done in b581ce2 (with a fix in 8439d03)

CyrusNajmabadi · 2021-07-20T20:06:34Z

src/EditorFeatures/CSharpTest2/Recommendations/UShortKeywordRecommenderTests.cs

+        public async Task TestAfterStaticLocalFunction()
+        {
+            await VerifyKeywordAsync(AddInsideMethod(@"
+static $$"));


so, you had all the tests after 'static' in a method, can you do the same for after 'async'? as well? or make all the tests theories and test static async async static and static async. Thanks!

I used a ClassData theory added in b46098a and applied in f686b67.

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

...haredUtilitiesAndExtensions/Workspace/CSharp/Extensions/ContextQuery/SyntaxTreeExtensions.cs

CyrusNajmabadi

LGTM with requested test additions.

…xtensions/ContextQuery/SyntaxTreeExtensions.cs Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com>

…nder

…legate recommender tests

MaStr11 · 2021-07-21T09:59:36Z

@CyrusNajmabadi Feedback is addressed and tests are green (except one unrelated integration test failure). I added the new file TheoryDataKeywordsIndicatingLocalFunction.cs for the theory data ([ClassData(typeof(...)]) for reuse in all the recommender tests. I hope that's fine (using [MemberData] and a property on the tests base class would be an alternative).

roslyn/src/EditorFeatures/CSharpTest2/Recommendations/TheoryDataKeywordsIndicatingLocalFunction.cs

Lines 11 to 16 in 8439d03

    
           internal class TheoryDataKeywordsIndicatingLocalFunction : IEnumerable<object[]> 
        
           { 
        
               public IEnumerator<object[]> GetEnumerator() 
        
               { 
        
                   yield return new object[] { "extern" }; 
        
                   yield return new object[] { "static extern" };

...haredUtilitiesAndExtensions/Workspace/CSharp/Extensions/ContextQuery/SyntaxTreeExtensions.cs

…xtensions/ContextQuery/SyntaxTreeExtensions.cs

CyrusNajmabadi · 2021-07-21T19:48:13Z

Thanks!

azure-pipelines · 2021-07-28T17:46:27Z

Azure Pipelines successfully started running 3 pipeline(s).

CyrusNajmabadi · 2021-07-28T20:01:15Z

/azp run

azure-pipelines · 2021-07-28T20:01:40Z

Azure Pipelines successfully started running 3 pipeline(s).

WIP: Add initial support for completion after static in local functio…

46208a9

…n context.

MaStr11 requested a review from a team as a code owner June 30, 2021 12:51

dotnet-issue-labeler bot added the Area-IDE label Jun 30, 2021

MaStr11 commented Jun 30, 2021

View reviewed changes

Youssef1313 reviewed Jun 30, 2021

View reviewed changes

jinujoseph added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jul 5, 2021

jinujoseph assigned CyrusNajmabadi Jul 5, 2021

MaStr11 added 3 commits July 19, 2021 08:29

Merge remote-tracking branch 'upstream/main' into TypeCompletionAfter…

fa8d961

…LocalFunctionKeywords

Move "IsLocalFunctionDeclarationContext" to IsTypeContext

842a1f6

Remove unused "IsLocalFunctionDeclarationContext" from CSharpSyntaxCo…

eb1b5be

…ntext

MaStr11 added 13 commits July 20, 2021 10:34

Fix "delegate recommended after static" test

d29cdd5

Add string keywordrecommender test (fails)

fa0655d

Revert "Remove unused "IsLocalFunctionDeclarationContext" from CSharp…

77a285c

…SyntaxContext" This reverts commit eb1b5be.

Fix StringKeywordRecommender

8f18ffc

Unify duplicate modifier set for IsLocalFunctionDeclarationContext

47d2b23

Add tests for all SpecialTypeRecommenders

7c3f349

Fix SpecialTypeRecommender

cd1c4ad

Fix misplaced test

f03ba1c

Add test for local function modifier combinations

801f3b8

Add NotAfterAsyncLocalFunctionWithTwoAsyncs symbol test

2ceba93

Add test in NativeIntegerKeywordRecommenderTests

95b99a4

Add test for TypeImporterCompletionProvider

4bcac1a

Add test for generate type

ae59210

CyrusNajmabadi reviewed Jul 20, 2021

View reviewed changes

src/Features/CSharp/Portable/Completion/KeywordRecommenders/IntKeywordRecommender.cs Show resolved Hide resolved

CyrusNajmabadi reviewed Jul 20, 2021

View reviewed changes

...haredUtilitiesAndExtensions/Workspace/CSharp/Extensions/ContextQuery/SyntaxTreeExtensions.cs Outdated Show resolved Hide resolved

CyrusNajmabadi approved these changes Jul 20, 2021

View reviewed changes

MaStr11 and others added 6 commits July 21, 2021 08:44

Update src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/E…

17c137f

…xtensions/ContextQuery/SyntaxTreeExtensions.cs Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com>

Add test "Not after return type"

a246e03

Add TheoryDataKeywordsIndicatingLocalFunction

b46098a

Add TestAfterKeywordIndicatingLocalFunction in DelegateKeywordRecomme…

b581ce2

…nder

Use Theory for SpecialTypesKeywordsRecommenders Tests

f686b67

Fix copy/paste error in TestAfterKeywordIndicatingLocalFunction in de…

8439d03

…legate recommender tests

CyrusNajmabadi reviewed Jul 21, 2021

View reviewed changes

...haredUtilitiesAndExtensions/Workspace/CSharp/Extensions/ContextQuery/SyntaxTreeExtensions.cs Outdated Show resolved Hide resolved

Update src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/E…

2dd919e

…xtensions/ContextQuery/SyntaxTreeExtensions.cs

CyrusNajmabadi approved these changes Jul 21, 2021

View reviewed changes

CyrusNajmabadi enabled auto-merge July 28, 2021 17:46

CyrusNajmabadi merged commit e36e194 into dotnet:main Jul 28, 2021

ghost added this to the Next milestone Jul 28, 2021

MaStr11 deleted the TypeCompletionAfterLocalFunctionKeywords branch July 29, 2021 07:20

jaredpar mentioned this pull request Aug 17, 2021

Azure Agent timeout #48030

Closed

dibarbet modified the milestones: Next, 17.0.P4 Aug 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Completion: Suggest types after keywords indicating a local function #54493

Completion: Suggest types after keywords indicating a local function #54493

MaStr11 commented Jun 30, 2021

MaStr11 Jun 30, 2021

MaStr11 Jun 30, 2021

Youssef1313 Jun 30, 2021 •

edited

Loading

MaStr11 Jul 19, 2021

MaStr11 Jul 20, 2021

Youssef1313 Jun 30, 2021

MaStr11 Jul 20, 2021

MaStr11 commented Jul 19, 2021

CyrusNajmabadi commented Jul 19, 2021

MaStr11 commented Jul 20, 2021

CyrusNajmabadi Jul 20, 2021

MaStr11 Jul 21, 2021

CyrusNajmabadi Jul 20, 2021

MaStr11 Jul 21, 2021

CyrusNajmabadi left a comment

MaStr11 commented Jul 21, 2021

CyrusNajmabadi commented Jul 21, 2021

azure-pipelines bot commented Jul 28, 2021

CyrusNajmabadi commented Jul 28, 2021

azure-pipelines bot commented Jul 28, 2021

Completion: Suggest types after keywords indicating a local function #54493

Completion: Suggest types after keywords indicating a local function #54493

Conversation

MaStr11 commented Jun 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Youssef1313 Jun 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaStr11 commented Jul 19, 2021

CyrusNajmabadi commented Jul 19, 2021

MaStr11 commented Jul 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

MaStr11 commented Jul 21, 2021

CyrusNajmabadi commented Jul 21, 2021

azure-pipelines bot commented Jul 28, 2021

CyrusNajmabadi commented Jul 28, 2021

azure-pipelines bot commented Jul 28, 2021

Youssef1313 Jun 30, 2021 •

edited

Loading