Tuple support for DeclarationNameCompletionProvider (name suggestion) by MaStr11 · Pull Request #22673 · dotnet/roslyn

MaStr11 · 2017-10-12T12:29:32Z

(note from jcouv: Merge, do not squash this PR, as it includes merge commits from master)

Customer scenario

Suggested names support for tuples.

Bugs this fixes:

#22342 (Kind of)

Workarounds, if any

No. Feature improvement.

Risk

Name suggestion might be triggered in inappropriate circumstances.

Performance impact

Editor performance and memory critical.

Is this a regression from a previous update?

No. Feature improvement.

Root cause analysis:

Tuple support was missing.

How was the bug found?

Found by investigating #22342. #22342 reports a (very) special case but more general cases were missing too.

Test documentation updated?

No

Remarks

It seems DeclarationNameCompletionProvider does not handle tuples at all. In the following example none of the occurrence of name, action and accessViolationException is suggested):

static void Main(string[] args)
{
    (Array array, Action action) Test((Array array, AccessViolationException accessViolationException) tuple) => default;
    (var array, var action) = Test((null, null));
}

#22342 suggested that there is support for tuples but there isn't (Just typing (Array suggest array because it triggers IsPossibleVariableOrLocalMethodDeclaration.).

This PR adds support for the cases above (except (var array, var action)).

TupleElementDefinition1 to TupleElementDefinition7 test the new tuple support. The remaining two tests fail because

TupleElementTypeInference I couldn't get the typeInference to work and
TupleElementInGenericTypeArgument The expression tree generated by the code of Suggested names not provided for tuple elements in generic type arguments #22342 System.Func<(System.Action $$ has nothing to do with tuples at all (combination of LessThan and Parenthesized expression). It's questionable if this really should be supported.

I hardly understand what my call to IsLastTokenOfType does. It gives the desired result but reviewers should take a close look whether that call is appropriate.

…' in CSharpCompletionCommandHandlerTests.

…port

MaStr11 · 2018-01-17T17:07:12Z

Could I get some feedback please?

sharwell · 2018-03-14T13:47:47Z

...Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.DeclarationInfo.cs

                var typeInferenceService = document.GetLanguageService<ITypeInferenceService>();

-                if (IsParameterDeclaration(token, semanticModel, position, cancellationToken, out var result)
+                if (IsTupleElementDeclaration(token, semanticModel, position, cancellationToken, out var result)


💭 Why is IsTupleElementDeclaration up here, but IsTupleExpressionDeclaration called much lower in the list?

IsTupleElementDeclaration needs to come before IsParameterDeclaration. Otherwise constructs as
void M((System.Array $$ or (System.Array $$ are treated by IsParameterDeclaration and the completion list is empty. This is covered by TupleElementDefinition6 and TupleElementDefinition7.

Otherwise constructs as void M((System.Array $$ [is] treated by IsParameterDeclaration

That itself seems like a bug. IsParameterDeclaration doesn't seem like it should be true here...

IsParameterDeclaration doesn't seem like it should be true here

This is the syntax tree of void M((System.Array $$))

So the token is part of a TupleType that itself is a parameter . So @CyrusNajmabadi is right as this is a type declaration taking place in a parameter. This can be solved the way I did (check for tuple declarations before parameter declarations) or by special casing TupleType in IsParameterDeclaration. I'm fine with both ways.

sharwell · 2018-03-14T16:53:01Z

...Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.DeclarationInfo.cs

+
+            private static bool IsTupleExpressionDeclaration(SyntaxToken token, SemanticModel semanticModel, int position, CancellationToken cancellationToken, out NameDeclarationInfo result)
+            {
+                if (token.GetAncestor<TupleExpressionSyntax>() != null)


❓ Why is this GetAncestor? I'm concerned that in a nested expression scenario this could provide incorrect results. Can you describe the scenarios that IsTupleExpressionDeclaration applies is meant to handle?

The TupleExpressionSyntax is a few steps above the token:

(System.Array array, System.Action $$ IdentifierNameSyntax IdentifierName Action ↳ MemberAccessExpressionSyntax SimpleMemberAccessExpression System.Action ↳ ArgumentSyntax Argument System.Action ↳ TupleExpressionSyntax TupleExpression (System.Array array, System.Action

The call IsFollowingTypeOrComma does some further checks of the relationship between the token and the node so nesting shouldn't be a problem.

I also added some tests in b0a7390 and 07f6e63.

… tuples. Renamed tests for clarity.

…ts and support for suggestion in the middle of a tuple.

MaStr11 · 2018-03-25T18:17:28Z

@sharwell Is the IDE-team interested in this or should I close it?

CyrusNajmabadi · 2018-03-26T07:40:27Z

...Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.DeclarationInfo.cs

+                switch (argumentSyntax.Expression?.Kind())
+                {
+                    case SyntaxKind.DeclarationExpression:
+                        return (argumentSyntax.Expression as DeclarationExpressionSyntax).Type;


just do a normal cast, you did the 'case' check above.

CyrusNajmabadi · 2018-03-26T07:41:09Z

...Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.DeclarationInfo.cs

+                    case SyntaxKind.DeclarationExpression:
+                        return (argumentSyntax.Expression as DeclarationExpressionSyntax).Type;
+                    default:
+                        return argumentSyntax.Expression;


This seems odd... you asked for the type syntax, but you're returning just hte expression? I can envision why you want this. But this line def needs a comment explaining why this is ok.

…ent and added comments.

Neme12 · 2018-03-26T11:04:05Z

...Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.DeclarationInfo.cs

+            {
+                switch (argumentSyntax.Expression?.Kind())
+                {
+                    case SyntaxKind.DeclarationExpression:


pattern matching?

jaredpar · 2018-04-19T17:00:43Z

We apologize, but we are closing this PR due to code drift. We regret letting this PR go so long without attention, but at this point the code base has changed too much for us to revisit older PRs. Going forward, we will manage PRs better under an SLA currently under draft in issue #26266 – we encourage you to add comments to that draft as you see fit. Although that SLA is still in draft form, we nevertheless want to move forward with the identification of older PRs to give contributors as much early notice as possible to review and update them.

If you are interested in pursuing this PR, please reset it against the head of master and we will reopen it. Thank you!

MaStr11 · 2018-04-27T10:58:27Z

@jaredpar Please reopen this PR and assign a "buddy". It was me waiting for feedback not the other way around.

jcouv · 2018-05-25T03:52:47Z

...Features/CSharpTest/Completion/CompletionProviders/DeclarationNameCompletionProviderTests.cs

+        }
+
+        [WorkItem(22342, "https://github.com/dotnet/roslyn/issues/22342")]
+        [Fact(Skip = "Not yet supported"), Trait(Traits.Feature, Traits.Features.Completion)]


Please unskip the test, capture the current behavior and add a link to an issue for tracking the remaining gap.

jcouv · 2018-05-25T03:59:37Z

...Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.DeclarationInfo.cs

+                SyntaxToken token, SemanticModel semanticModel, int position,
+                CancellationToken cancellationToken, out NameDeclarationInfo result)
+            {
+                if (token.GetAncestor(node => node.IsKind(SyntaxKind.TupleExpression)) != null)


This ancestor check feels too loose. Imagine an invocation inside a tuple expression, and the caret is after the comma in the argument list of the invocation.
Can we just check the parent or grandparent instead?

I tried to come up with a test to make this fail but I couldn't. Can you have a look in commit 3ebfe28? We somehow need to get a type declaration inside that call to make the completion fail. The method examines code like the following and I don't see how there can be a method invocation:

class Test { void Do() { (System.Array array, System.Action $$ } }

See also my previous discussion with @sharwell #22673 (comment)

jcouv · 2018-05-25T04:00:09Z

...Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.DeclarationInfo.cs

+            }
+
+            private static bool IsTupleExpressionDeclaration(
+                SyntaxToken token, SemanticModel semanticModel, int position,


The position argument seems unused.

There are a bunch of methods with about the same signature:

if (IsTupleElementDeclaration(token, semanticModel, position, cancellationToken, out var result) || IsParameterDeclaration(token, semanticModel, position, cancellationToken, out result) || IsTypeParameterDeclaration(token, semanticModel, position, cancellationToken, out result) || IsVariableDeclaration(token, semanticModel, position, cancellationToken, out result) || IsIncompleteMemberDeclaration(token, semanticModel, position, cancellationToken, out result) || IsFieldDeclaration(token, semanticModel, position, cancellationToken, out result) || IsMethodDeclaration(token, semanticModel, position, cancellationToken, out result) || IsPropertyDeclaration(token, semanticModel, position, cancellationToken, out result) || IsPossibleOutVariableDeclaration(token, semanticModel, position, typeInferenceService, cancellationToken, out result) || IsTupleExpressionDeclaration(token, semanticModel, position, cancellationToken, out result) || IsPossibleVariableOrLocalMethodDeclaration(token, semanticModel, position, cancellationToken, out result))

I would like to keep the signature as is just for symmetry.

Saw that. Fine to leave as-is :-)

jcouv · 2018-05-25T04:04:14Z

...Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.DeclarationInfo.cs

+                        // We also assume that this name could be resolved to a type.
+                        return argumentSyntax.Expression;
+                }
+            }


Not directly related to this PR: FYI I added a note to the test plan for recursive patterns to test declaration name completion. For instance: e is (System.Array $$.
#25935

jcouv · 2018-05-25T04:07:01Z

...Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.DeclarationInfo.cs

                return default;
            }

+            private static bool IsTupleElementDeclaration(


Nit: Consider renaming this method to IsTupleTypeElement and the following one IsTupleLiteralElement.

Done cf1a973.

jcouv · 2018-05-25T04:16:07Z

...Features/CSharpTest/Completion/CompletionProviders/DeclarationNameCompletionProviderTests.cs

+
+        [WorkItem(22342, "https://github.com/dotnet/roslyn/issues/22342")]
+        [Fact, Trait(Traits.Feature, Traits.Features.Completion)]
+        public async void TupleExpressionDeclaration1()


The test name is confusing. This is a tuple type, not a tuple expression.
I think the same applies for some other tests.

use async Task for all tests

BTW, the existing tests are already fixed if you merge latest master. Please remove async void from your tests too.

@jcouv
You are right. The reason for this is that the code under test is parsed as a tuple expression.

I haven't figured out how to find better names for such the tests.

@Neme12 Done (e752f3c). Thanks for also pointing out the already fixed tests in master.

jcouv · 2018-05-25T04:18:45Z

...Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.DeclarationInfo.cs

+                return result.Type != null;
+            }
+
+            private static bool IsTupleExpressionDeclaration(


It would be good to add a comment to clarify that this is for handling incomplete code that parsed like a tuple literal.
But the purpose is not to help typing names in legit tuple literals (that's not possible since they don't have any TypeSyntax).

Done cf1a973.

jcouv

Done with review pass (iteration 7).

MaStr11 · 2018-05-25T07:35:55Z

...Features/CSharpTest/Completion/CompletionProviders/DeclarationNameCompletionProviderTests.cs

+        }
+
+        [WorkItem(22342, "https://github.com/dotnet/roslyn/issues/22342")]
+        [Fact(Skip = "Not yet supported"), Trait(Traits.Feature, Traits.Features.Completion)]


If we unskip the previous test, should we unskip this one too?

…port

jcouv · 2018-05-25T16:44:26Z

...Features/CSharpTest/Completion/CompletionProviders/DeclarationNameCompletionProviderTests.cs

Muscle memory is strong, my German friend ;-)
"ist"

jcouv

LGTM Thanks

sharwell · 2018-05-25T18:56:44Z

@jinujoseph for approval to merge

jinujoseph · 2018-05-25T19:07:44Z

Approved to merge for 15.8.Preview3

Initial support for tuples.

cdb06bc

dnfclas added the cla-already-signed label Oct 12, 2017

MaStr11 changed the title ~~Initial support for tuples.~~ WIP Tuple support for DeclarationNameCompletionProvider (name suggestion) Oct 12, 2017

Changed failing 'TestParenthesizedDeconstructionDeclarationWithSymbol…

765d4ff

…' in CSharpCompletionCommandHandlerTests.

jcouv added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Oct 24, 2017

Merge branch 'master' into DeclarationNameCompletionProvider_TupleSup…

9d4f3cb

…port

MaStr11 requested a review from a team as a code owner December 12, 2017 13:42

MaStr11 changed the title ~~WIP Tuple support for DeclarationNameCompletionProvider (name suggestion)~~ Tuple support for DeclarationNameCompletionProvider (name suggestion) Dec 12, 2017

sharwell suggested changes Mar 14, 2018

View reviewed changes

MaStr11 added 3 commits March 20, 2018 13:07

Check that token is last token of parent node. Added tests for nested…

b0a7390

… tuples. Renamed tests for clarity.

Switch from IsLastTokenOfType to IsFollowingTypeOrComma and added tes…

07f6e63

…ts and support for suggestion in the middle of a tuple.

Code formatting.

9ee59fd

CyrusNajmabadi reviewed Mar 26, 2018

View reviewed changes

Renamed GetTypeSyntaxOfArgument to GetNodeDenotingTheTypeOfTupleArgum…

82e6943

…ent and added comments.

Neme12 reviewed Mar 26, 2018

View reviewed changes

Neme12 mentioned this pull request Apr 8, 2018

Suggested variable names inside using, for & foreach #26026

Merged

jcouv added this to the 15.8 milestone Apr 9, 2018

jaredpar closed this Apr 19, 2018

jaredpar added the Resolution-Expired The request is closed due to inactivity under our contribution review policy. label Apr 19, 2018

sharwell reopened this Apr 27, 2018

sharwell self-assigned this Apr 27, 2018

sharwell removed the Resolution-Expired The request is closed due to inactivity under our contribution review policy. label Apr 27, 2018

jcouv added the Area-IDE label May 3, 2018

jinujoseph assigned jcouv May 24, 2018

jcouv reviewed May 25, 2018

View reviewed changes

MaStr11 mentioned this pull request May 25, 2018

Completion provider: Completion based on element type name is missing if type needs to be inferred (var) #27138

Closed

MaStr11 commented May 25, 2018

View reviewed changes

MaStr11 added 4 commits May 25, 2018 10:13

Unskip test and add test for invocation inside tuple expression.

3ebfe28

Renamed methods and added code comment.

cf1a973

Merge branch 'master' into DeclarationNameCompletionProvider_TupleSup…

349847d

…port

Changed async void to async Task in tests.

e752f3c

jcouv reviewed May 25, 2018

View reviewed changes

jcouv approved these changes May 25, 2018

View reviewed changes

sharwell approved these changes May 25, 2018

View reviewed changes

jinujoseph added the Approved to merge label May 25, 2018

sharwell merged commit 016111c into dotnet:master May 25, 2018

Conversation

MaStr11 commented Oct 12, 2017 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaStr11 commented Jan 17, 2018

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!

sharwell Mar 14, 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!

MaStr11 Mar 20, 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!

MaStr11 commented Mar 25, 2018

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!

jaredpar commented Apr 19, 2018

Uh oh!

MaStr11 commented Apr 27, 2018

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!

MaStr11 May 25, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaStr11 May 25, 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!

jcouv left a comment

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

MaStr11 commented Oct 12, 2017 •

edited by jcouv

Loading

sharwell Mar 14, 2018 •

edited

Loading

MaStr11 Mar 20, 2018 •

edited

Loading

MaStr11 May 25, 2018 •

edited

Loading

MaStr11 May 25, 2018 •

edited

Loading