Allow independent methods to have the same parameter name by sharwell · Pull Request #45501 · dotnet/roslyn

sharwell · 2020-06-27T22:31:18Z

miqm · 2020-06-28T08:59:17Z

@sharwell What in case of local methods? In C#8 we can hide existing parameter, but in netcore2.1 suggested name can breake up build.

Here's a testCase:

    public class Context { }
    public class C
    {
        void Goo(Context context) { 
        
            void InnerGoo(Context c$$)
        }        
    }

sharwell · 2020-06-28T16:09:59Z

@miqm I believe that's an existing bug, so I wasn't trying to fix it. Is there a specific case that worked in 16.6 but isn't working anymore?

miqm · 2020-06-28T16:53:35Z

@sharwell - I think I didn't make myself clear enough. The full test cases, that IMHO should work, are as follows:

{
var markup = @"
public class DbContext { }
public class C
{
    void Goo(DbContext context) {
        void InnerGoo(DbContext $$) { }
    }
}
";
await VerifyItemExistsAsync(markup, "dbContext", glyph: (int) Glyph.Parameter);
await VerifyItemExistsAsync(markup, "db", glyph: (int) Glyph.Parameter);
await VerifyItemExistsAsync(markup, "context1", glyph: (int)Glyph.Parameter);
}

{
var markup = @"
public class DbContext { }
public class C
{
    void Goo() {
        DbContext context;
        void InnerGoo(DbContext $$) { }
    }
}
";
await VerifyItemExistsAsync(markup, "dbContext", glyph: (int) Glyph.Parameter);
await VerifyItemExistsAsync(markup, "db", glyph: (int) Glyph.Parameter);
await VerifyItemExistsAsync(markup, "context1", glyph: (int)Glyph.Parameter);
}

{
var markup = @"
public class DbContext { }
public class C
{
    DbContext dbContext;
    void Goo(DbContext context) {
        void InnerGoo(DbContext $$) { }
    }
}
";
await VerifyItemExistsAsync(markup, "dbContext", glyph: (int) Glyph.Parameter);
await VerifyItemExistsAsync(markup, "db", glyph: (int) Glyph.Parameter);
await VerifyItemExistsAsync(markup, "context1", glyph: (int)Glyph.Parameter);
}

I verified those test cases on 16.7-preview2 and they all work. I'm not sure if they will work with your code. Stopping search on ParameterList syntax will take into account only items on current parameter list (inner method's list) and will not take into account the outer method block.

miqm

Instead adding additional case I'd revert to what it was before but use a new method with old IsExecutableBlock method content.

miqm · 2020-06-28T17:02:40Z

...paces/SharedUtilitiesAndExtensions/Compiler/CSharp/Services/SyntaxFacts/CSharpSyntaxFacts.cs

        public SyntaxNode GetParameterList(SyntaxNode node)
            => node.GetParameterList();

+        public bool IsParameterList(SyntaxNode node)


public bool IsParameterScope(SyntaxNode node) => node.IsKind(SyntaxKind.Block, SyntaxKind.SwitchSection);

miqm · 2020-06-28T17:03:19Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Services/SyntaxFacts/ISyntaxFacts.cs

        SyntaxToken? GetNameOfParameter(SyntaxNode node);
        SyntaxNode GetDefaultOfParameter(SyntaxNode node);
        SyntaxNode GetParameterList(SyntaxNode node);
+        bool IsParameterList(SyntaxNode node);


bool IsParameterScope(SyntaxNode node);

miqm · 2020-06-28T17:05:24Z

...edUtilitiesAndExtensions/Compiler/VisualBasic/Services/SyntaxFacts/VisualBasicSyntaxFacts.vb

            Return node.GetParameterList()
        End Function

+        Public Function IsParameterList(node As SyntaxNode) As Boolean Implements ISyntaxFacts.IsParameterList


Public Function IsParameterScope(node As SyntaxNode) As Boolean Implements ISyntaxFacts.IsParameterScope Return node.IsKind(SyntaxKind.Block, SyntaxKind.SwitchSection) End Function

miqm · 2020-06-28T17:06:34Z

...nsions/Workspace/Core/LanguageServices/SemanticsFactsService/AbstractSemanticFactsService.cs

        {
            var container = containerOpt ?? location.AncestorsAndSelf().FirstOrDefault(
-                a => SyntaxFacts.IsExecutableBlock(a) || SyntaxFacts.IsMethodBody(a));
+                a => SyntaxFacts.IsExecutableBlock(a) || SyntaxFacts.IsParameterList(a) || SyntaxFacts.IsMethodBody(a));


a => SyntaxFacts.IsParameterScope(a) || SyntaxFacts.IsMethodBody(a));

IsParameterScope isn't something SyntaxFacts should know about. SyntaxFacts is just a way to structurally ask what type of node something is, in a way that is agnostic to VB/C#. i.e. if both VB/C# have an InvocationExpressionSyntax, then SyntaxFacts can be used with IsInvocationExpression to ask that question in a uniform way, without worrying about lang.

It's specifically for syntactic similarity questions, not for semantic questions (like scopes). Thanks!

@CyrusNajmabadi Ok, understood. Although I still think this particular change will lead to check only current ParameterList and will not take into account i.e. method body for local method parameters. Perhaps it could be added some sort of container filter which can be used by Declaration Name provider to take into account only MethodBody (if there is one)?

miqm · 2020-06-29T08:28:37Z

@miqm I believe that's an existing bug, so I wasn't trying to fix it. Is there a specific case that worked in 16.6 but isn't working anymore?

@sharwell This is screenshot from 16.6.2:

As you can see, context1 name is suggested here correctly - context is used in the method parameters list. In this solution I think "context" will be suggested, as the lookup for container will stop on first Parameters list - in this case - on the parameters list for the local method.

sharwell · 2020-06-30T00:06:00Z

@miqm I added all your tests locally and they worked without other changes. See my new commit!

miqm

Oh, apparently inner method's parameterList is not ParameterList in SyntaxFacts understanding. Happy to approve :)

Allow independent methods to have the same parameter name

7126092

Fixes dotnet#45492

sharwell requested a review from a team as a code owner June 27, 2020 22:31

sharwell added the Area-IDE label Jun 27, 2020

miqm suggested changes Jun 28, 2020

View reviewed changes

CyrusNajmabadi approved these changes Jun 28, 2020

View reviewed changes

sharwell changed the base branch from master to release/dev16.7 June 29, 2020 23:46

jinujoseph added this to the 16.7.P4 milestone Jun 29, 2020

jinujoseph added the Approved to merge label Jun 29, 2020

Additional name suggestion tests

90e2929

miqm approved these changes Jun 30, 2020

View reviewed changes

sharwell merged commit ca3c46b into dotnet:release/dev16.7 Jun 30, 2020

sharwell deleted the param-name branch June 30, 2020 05:57

Conversation

sharwell commented Jun 27, 2020

Uh oh!

miqm commented Jun 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sharwell commented Jun 28, 2020

Uh oh!

miqm commented Jun 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miqm left a comment

Choose a reason for hiding this comment

Uh oh!

miqm Jun 28, 2020

Choose a reason for hiding this comment

Uh oh!

miqm Jun 28, 2020

Choose a reason for hiding this comment

Uh oh!

miqm Jun 28, 2020

Choose a reason for hiding this comment

Uh oh!

miqm Jun 28, 2020

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Jun 28, 2020

Choose a reason for hiding this comment

Uh oh!

miqm Jun 28, 2020 • 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!

miqm commented Jun 29, 2020

Uh oh!

sharwell commented Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miqm left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

miqm commented Jun 28, 2020 •

edited

Loading

miqm commented Jun 28, 2020 •

edited

Loading

miqm Jun 28, 2020 •

edited

Loading

sharwell commented Jun 30, 2020 •

edited

Loading