Fix bug when caret is at the end of an argument #19489

alrz · 2017-05-14T14:52:51Z

Fixes #19175

sharwell

Can you add some tests for VB omitted arguments syntax:

MsgBox("Test message", , "Title bar text")

sharwell

❗️ I'm going to request one more test, just to make sure. Make M return an Integer.

Input:

M(1, M(1, [||], 3))

Expected behavior: no code fix provided

sharwell · 2017-05-14T18:13:56Z

...table/CodeRefactorings/UseNamedArguments/AbstractUseNamedArgumentsCodeRefactoringProvider.cs

@@ -34,7 +34,13 @@ protected abstract class Analyzer<TBaseArgumentSyntax, TArgumentSyntax, TArgumen
                    return;
                }

-                var argument = root.FindNode(context.Span).FirstAncestorOrSelf<TBaseArgumentSyntax>() as TArgumentSyntax;
+                var token = root.FindToken(context.Span.Start);
+                if (token.ValueText == ")" || token.ValueText == ",")


💭 Personally I would prefer to use the token's kind instead of text (which may require an abstract method implemented by the derived types), but if @CyrusNajmabadi doesn't have a problem with the current approach then I won't block on this.

this should definitely be checking if it is .Kind() == SyntaxKind.CloseParen and SyntaxKind.Comma.

It should also only move back if the caret is literally at the .Span.Start of the token.

Because this is in abstrcat code, you'll need to provide customized logic for VB/C# through the derived types.

sharwell · 2017-05-14T18:16:57Z

src/EditorFeatures/VisualBasicTest/CodeActions/UseNamedArguments/UseNamedArgumentsTests.vb

+
+        <WorkItem(19175, "https://github.com/dotnet/roslyn/issues/19175")>
+        <Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseNamedArguments)>
+        Public Async Function TestCaretPositionAtTheEnd4() As Task


💡 Maybe file a issue for the future showing how this refactoring operation could support missing arguments by omitting them during the conversion to named arguments. The output for this example would be:

M(arg1:=1, arg3:=3)

That was in the initial implementation (#15687) but superseded by Cyrus changes, supposedly that's not the desirable behavior. IMO it makes sense because the refactoring says "add argument name" and it merely would do that, otherwise we're changing the code more than it's expected.

See TestOmittedArguments2 for an example of a use case that we do offer the fix in presence of omitted args.

The changes by Cyrus affecting this behavior appear to have been an oversight. I went through every comment and commit message of the issue and both PRs, and it appears that the change slipped through as opposed to being deliberate. I believe it would be helpful to include the refactoring operation for the following basic reason: if the user believes that the use of a named argument will improve readability for the argument 1, then this is the minimal change required to provide that readability improvement. Also, it will not be clear to users why this feature works for some provided arguments, but not for others.

My concern with such an approach would be that it could subtly change overload resolution if you now removed an argument. if there was a two-arg method, with matching names, you'd now be calling that, instead of the original member.

alrz · 2017-05-14T20:17:42Z

...table/CodeRefactorings/UseNamedArguments/AbstractUseNamedArgumentsCodeRefactoringProvider.cs

+                if (IsCloseParenOrComma(token))
+                {
+                    if (token.Span.Start != context.Span.Start ||
+                        IsCloseParenOrComma(token = token.GetPreviousToken()))


This certainly smells.

please don't write it this way :)

You mean the inline assignment or duplicated check for comma? I could inline the whole FirstAncestorOrSelf and bail when we reach an argument list instead. thoughts?

the inline assignment. I would simply start with:

if (token.Span.Start == context.Span.Start && IsCloseParenOrComma(token) && token.GetPreviousToken().Span.End == context.Span.Start) { token = token.GetPreviousToken(); } // rest of your logic.

sharwell

I'll let you work with @CyrusNajmabadi on the implementation details, but the tests appear to cover the cases I was thinking of.

CyrusNajmabadi · 2017-05-15T18:57:34Z

...table/CodeRefactorings/UseNamedArguments/AbstractUseNamedArgumentsCodeRefactoringProvider.cs

-                        IsCloseParenOrComma(token = token.GetPreviousToken()))
+                    token = token.GetPreviousToken();
+                    if (token.Span.End != context.Span.Start || 
+                        IsCloseParenOrComma(token))


i don't think you need this second IsCloseParenOrComma

TestCaretPositionAtTheEnd5 requires it. though it's VB only.

@CyrusNajmabadi It's a sneaky edge case. The missing argument led to the previous token being ,, which is not part of the argument list but not the argument itself. This caused the code fix to search upwards and find the outer invocation and try to add the named argument there. It would be a rare but really weird experience when it occurred.

I think the walk up should stop immediately at the first parameterlist. that would make more sense to me.

yeah I did suggest that earlier.. so I need to fork FirstAncestorOrSelf method somewhere else since there is no helper for that case, I guess.

@alrz I think the following would work:

FirstAncestorOrSelf<SyntaxNode>(node => node.IsKind(SyntaxKind.A, SyntaxKind.B))

@sharwell it "filters" the result, it doesn't cause the walk up to stop.

alrz · 2017-05-15T23:00:01Z

src/Workspaces/Core/Portable/Shared/Extensions/SyntaxNodeExtensions.cs

+
+        private static SyntaxNode GetParent(this SyntaxNode node)
+        {
+            return (node as IStructuredTriviaSyntax)?.ParentTrivia.Token.Parent ?? node.Parent;


I think this is a behavioral change. should use a pattern instead.

sharwell · 2017-05-16T16:46:44Z

src/Workspaces/Core/Portable/Shared/Extensions/SyntaxNodeExtensions.cs

+        {
+            for (var current = node; current != null; current = current.GetParent())
+            {
+                if (predicate(current))


❗️ This needs to come after the other check

sharwell

I would prefer e18581ad3ad280cd528307e1c27e8a2d76bba0fc be a separate pull request.

alrz · 2017-05-16T19:38:50Z

I would prefer e18581a be a separate pull request.

The method I added depends on GetParent method added in the clean up.

CyrusNajmabadi · 2017-05-17T19:37:04Z

Tagging @MattGertz

Customer scenario

Our 'add named argument' feature doesn't work if the user is at the end of an argument. This can be annoying and confusing.

Bugs this fixes:

Fixes #19175

Workarounds, if any

None.

Risk

Low.

Performance impact

Low.

Is this a regression from a previous update?

No.

Root cause analysis:

We didn't validate the feature at all possible positions of the caret.

How was the bug found?

Bug report.

MattGertz · 2017-05-17T19:54:23Z

Approved pending sharwell signoff. Thank you!

sharwell · 2017-05-17T21:34:15Z

Thanks @alrz for the PR, and @CyrusNajmabadi for getting the approval through!

Fix bug when caret is at the end of an argument

05fc405

dnfclas added the cla-already-signed label May 14, 2017

sharwell added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label May 14, 2017

sharwell requested changes May 14, 2017

View reviewed changes

Add test for omitted arguments

20079ae

sharwell requested changes May 14, 2017

View reviewed changes

alrz force-pushed the bug-19175 branch from c0ea576 to eda4481 Compare May 14, 2017 20:12

alrz commented May 14, 2017

View reviewed changes

sharwell approved these changes May 14, 2017

View reviewed changes

CyrusNajmabadi reviewed May 15, 2017

View reviewed changes

alrz commented May 15, 2017

View reviewed changes

sharwell reviewed May 16, 2017

View reviewed changes

sharwell requested changes May 16, 2017

View reviewed changes

Clean up SyntaxNodeExtensions

c0e5a04

alrz force-pushed the bug-19175 branch from e4f85b6 to 5ba6831 Compare May 16, 2017 19:35

alrz force-pushed the bug-19175 branch from 5ba6831 to 54131ae Compare May 16, 2017 19:39

Do not leak out of the argument list when looking for an argument

f907560

alrz force-pushed the bug-19175 branch from 54131ae to f907560 Compare May 16, 2017 19:42

CyrusNajmabadi approved these changes May 17, 2017

View reviewed changes

MattGertz added the Approved to merge label May 17, 2017

sharwell approved these changes May 17, 2017

View reviewed changes

sharwell added this to the 15.3 milestone May 17, 2017

sharwell merged commit f9f2b17 into dotnet:master May 17, 2017

alrz deleted the bug-19175 branch May 17, 2017 21:37

sharwell self-assigned this May 25, 2017

sharwell removed the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Aug 31, 2017

sharwell added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Aug 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug when caret is at the end of an argument #19489

Fix bug when caret is at the end of an argument #19489

alrz commented May 14, 2017

sharwell left a comment

sharwell left a comment •

edited

Loading

sharwell May 14, 2017

CyrusNajmabadi May 14, 2017

CyrusNajmabadi May 14, 2017

CyrusNajmabadi May 14, 2017

sharwell May 14, 2017

alrz May 14, 2017 •

edited

Loading

alrz May 14, 2017

sharwell May 14, 2017 •

edited

Loading

CyrusNajmabadi May 14, 2017

alrz May 14, 2017

CyrusNajmabadi May 14, 2017

alrz May 14, 2017

CyrusNajmabadi May 14, 2017

sharwell left a comment

CyrusNajmabadi May 15, 2017

alrz May 15, 2017

sharwell May 15, 2017

CyrusNajmabadi May 15, 2017

alrz May 15, 2017

sharwell May 15, 2017

alrz May 15, 2017

alrz May 15, 2017

sharwell May 16, 2017

sharwell left a comment

alrz commented May 16, 2017

CyrusNajmabadi commented May 17, 2017

MattGertz commented May 17, 2017

sharwell commented May 17, 2017

Fix bug when caret is at the end of an argument #19489

Fix bug when caret is at the end of an argument #19489

Conversation

alrz commented May 14, 2017

sharwell left a comment

Choose a reason for hiding this comment

sharwell left a comment • 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

Choose a reason for hiding this comment

alrz May 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell May 14, 2017 • 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

Choose a reason for hiding this comment

sharwell left a 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

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

Choose a reason for hiding this comment

sharwell left a comment

Choose a reason for hiding this comment

alrz commented May 16, 2017

CyrusNajmabadi commented May 17, 2017

MattGertz commented May 17, 2017

sharwell commented May 17, 2017

sharwell left a comment •

edited

Loading

alrz May 14, 2017 •

edited

Loading

sharwell May 14, 2017 •

edited

Loading