Assert duplicate named arguments produce correct diagnostics by OmarTawfik · Pull Request #25730 · dotnet/roslyn

OmarTawfik · 2018-03-26T20:07:59Z

Customer scenario

Getting IOperations trees on call expressions that have duplicate explicitly named arguments, will cause a Debug.Assert() to throw in the compiler. Example:

    public class C
    {
        void M()
        {
            string.Format(format: "", format: "");
        }
    }

Bugs this fixes

Fixes #20050

Workarounds, if any

None.

Risk

Low. One line fix.

Is this a regression from a previous update?

No.

Root cause analysis

It is a case where we didn't count for invalid arguments when constructing the IOperations tree.

How was the bug found?

Reported on GitHub.

jcouv

LGTM

cston · 2018-03-26T20:11:24Z

Does this test hit MakeArgumentsInEvaluationOrder?

No. The original assert mentioned in the comment was refactored out. I'll add an IOperations test as well per Julien's comment to cover more ground.

jcouv · 2018-03-26T20:13:09Z

On second thought, the stack in the bug report shows the problem happens in GetOperationsToAnalyze. Can you test such IOperation scenarios? #Resolved

Probably need to test IOperation scenarios too

OmarTawfik · 2018-03-26T20:38:25Z

I was actually able to repro a failure through tests (VS didn't). Will fix this PR and tag people shortly. #Closed

OmarTawfik · 2018-03-26T23:30:05Z

@jcouv @cston after more inspection, we have two options:

Add the invalid (duplicate) argument to the list of arguments when constructing the tree.
Ignore it completely and depend on diagnostics informing the user that there is a duplicate argument.

I chose the first solution, and added tests to make sure that the node we add is marked as IsInvalid. #Closed

AlekseyTs · 2018-03-26T23:42:30Z

IArgumentOperation (ArgumentKind.Explicit, Matching Parameter: format) (OperationKind.Argument, Type: null, IsInvalid) (Syntax: 'format: """"') [](start = 6, length = 143)

IArgumentOperation (ArgumentKind.Explicit, Matching Parameter: format) (OperationKind.Argument, Type: null, IsInvalid) (Syntax: 'format: """"') [](start = 6, length = 143)

I think this is wrong, we are not supposed to create IInvocationOperation when there are issues with mapping arguments to parameters. The IInvocationOperation can be created only for successful overload resolution case. #Closed

AlekseyTs · 2018-03-26T23:43:32Z

CorrectOrder [](start = 75, length = 12)

CorrectOrder [](start = 75, length = 12)

I am not sure what "CorrectOrder" mean in this scenario #Closed

Arguments are not ordered. I'll fix it to be more explicit.

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

AlekseyTs · 2018-03-26T23:45:25Z

Debug.Assert(!processedParameters.Contains(p)); [](start = 16, length = 47)

I think this assert should be put back. It catches real problems. #Resolved

The method should not be called in scenarios outlined in the issue.

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

AlekseyTs

The proposed fix violates an invariant that IInvocationOperation is created only when overload resolution succeeds.

AlekseyTs · 2018-03-26T23:47:51Z

Done with review pass (iteration 1) #Closed

OmarTawfik · 2018-03-27T05:19:54Z

The proposed fix violates an invariant that IInvocationOperation is created only when overload resolution succeeds.

The method should not be called in scenarios outlined in the issue.

@AlekseyTs the compiler indeed generates LazyInvalidOperation for overload resolution failures. Check the first few lines of CSharpOperationFactory.CreateBoundCallOperation(). But in case of duplicate named parameters, the compiler produces diagnostics (as expected), but considers it a Viable overload, which continues to generate an IInvocationOperation.

For example, this is a true overload resolution failure:

void N(int a, int b)
{
}

void M()
{
    /*<bind>*/N(a: 1, a: 2)/*</bind>*/;
}

We produce:

// file.cs(10,27): error CS1740: Named argument 'a' cannot be specified multiple times
// file.cs(10,19): error CS7036: There is no argument given that corresponds to the required formal parameter 'b' of 'C.N(int, int)'

And the overload resolution failure leads to this IOperations tree:

IInvalidOperation (OperationKind.Invalid, Type: System.Void, IsInvalid) (Syntax: 'N(a: 1, a: 2)')
  Children(2):
      ILiteralOperation (OperationKind.Literal, Type: System.Int32, Constant: 1) (Syntax: '1')
      ILiteralOperation (OperationKind.Literal, Type: System.Int32, Constant: 2) (Syntax: '2')

Are you suggesting that additional duplicate arguments should fail overload resolution? #Closed

AlekseyTs · 2018-03-27T16:49:13Z

Are you suggesting that additional duplicate arguments should fail overload resolution?

I am not suggesting this, I am pretty sure overload resolution fails in this case. #Closed

AlekseyTs · 2018-03-27T16:51:36Z

@OmarTawfik If you need more help with this issue, let's take this offline #Closed

OmarTawfik · 2018-03-30T22:19:42Z

Discussed offline. Overload resolution actually succeeds with such arguments. I'll change it and update the PR. I'll tag the reviewers again when ready. #Closed

OmarTawfik · 2018-04-02T21:29:41Z

@cston @jcouv @AlekseyTs comments addressed. #Closed

AlekseyTs · 2018-04-02T22:22:11Z

        NoCorrespondingParameter,
        FirstInvalid = NoCorrespondingParameter,
        NoCorrespondingNamedParameter,
+        DuplicateNamedArguments,


DuplicateNamedArguments [](start = 8, length = 23)

Should we use singular as the other members, i.e. "DuplicateNamedArgument"? #Closed

I chose the plural form as you cannot have one duplicate argument :)

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

I chose the plural form as you cannot have one duplicate argument

Of course I can, the second one is a duplicate, the first one is not.

In reply to: 178707798 [](ancestors = 178707798,178667114)

Alright. I'll fix.

In reply to: 178710288 [](ancestors = 178710288,178707798,178667114)

jaredpar · 2018-04-02T22:29:30Z

            Debug.Assert(badArguments.Length != 0);
            return new MemberAnalysisResult(
-                MemberResolutionKind.BadArguments,
+                MemberResolutionKind.BadArgumentConversion,


MemberResolutionKind.BadArgumentConversion, [](start = 15, length = 44)

Why was this change necessary? #Resolved

Never mind. I see why now.

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

AlekseyTs · 2018-04-02T22:54:48Z

            switch (argumentAnalysis.Kind)

Should we handle new kind in this switch? #Closed

Refers to: src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs:2918 in 738230c. [](commit_id = 738230c, deletion_comment = False)

AlekseyTs · 2018-04-02T22:55:14Z

        switch (normalResult.Kind)

Should we explicitly handle new kind in this switch? #Closed

Refers to: src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs:776 in 738230c. [](commit_id = 738230c, deletion_comment = False)

AlekseyTs · 2018-04-02T23:12:57Z

+        private void ReportDuplicateNamedArguments(MemberResolutionResult<TMember> result, DiagnosticBag diagnostics, AnalyzedArguments arguments)
+        {
+            Debug.Assert(result.Result.BadArgumentsOpt.Length == 1);
+            var name = arguments.Names[result.Result.BadArgumentsOpt[0]];


var [](start = 12, length = 3)

Please spell out the type #Closed

AlekseyTs · 2018-04-02T23:21:42Z

+        [Fact]
+        [CompilerTrait(CompilerFeature.IOperation)]
+        [WorkItem(20050, "https://github.com/dotnet/roslyn/issues/20050")]
+        public void BuildsArgumentsOperationsForDuplicateExplicitArguments_Repro()


BuildsArgumentsOperationsForDuplicateExplicitArguments_Repro [](start = 20, length = 60)

Please add similar tests for properties (indexers). #Closed

AlekseyTs · 2018-04-02T23:23:18Z

            Assert.Equal("M(x: 1, x: 2)", invocation.ToString());
-            Assert.Equal("void C.M(params System.Int32[] x)", model.GetSymbolInfo(invocation).Symbol.ToTestDisplayString());
+
+            var symbolInfo = model.GetSymbolInfo(invocation);


symbolInfo [](start = 16, length = 10)

Please add Assert.Null for symbolInfo.Symbol. #Closed

AlekseyTs · 2018-04-02T23:26:36Z

                // (8,38): error CS1740: Named argument 'x' cannot be specified multiple times
                //         System.Console.Write(c[x: 1, x: 2, 3]);
-                Diagnostic(ErrorCode.ERR_DuplicateNamedArgument, "x").WithArguments("x").WithLocation(8, 38),
-                // (8,32): error CS1739: The best overload for 'this' does not have a parameter named 'x'


// (8,32): error CS1739: The best overload for 'this' does not have a parameter named 'x' [](start = 16, length = 89)

It feels like this error is more useful than the duplicate name error. #Closed

AlekseyTs · 2018-04-02T23:30:44Z

-            // (1) Is there any named argument used out-of-position and followed by unnamed arguments?
-            // (2) Is there any argument without a corresponding parameter?
-            // (3) Was there any named argument that specified a parameter that was already
+            // (1) Is there any named argument that were specified twice?


Is there any named argument that were specified twice? [](start = 19, length = 54)

It feels like we shouldn't start with this check. Consider adding it last. Usually, adding new checks last is a better approach, especially for condition that was not considered an error before. #Closed

AlekseyTs · 2018-04-02T23:32:40Z

Done with review pass (iteration 1) #Closed

AlekseyTs · 2018-04-02T23:35:20Z

-            const int noCorrespondingParameterPriority = 3;
-            const int badNonTrailingNamedArgument = 4;
+            var supportedInPriorityOrder = new MemberResolutionResult<TMember>[6]; // from highest to lowest priority
+            const int duplicateNamedArguments = 0;


const int duplicateNamedArguments = 0; [](start = 12, length = 38)

Here it is probably appropriate to treat the new failure as highest priority because before it wasn't considered a failure. #Closed

jaredpar · 2018-04-02T23:49:24Z

+            const int nameUsedForPositionalPriority = 2;
+            const int noCorrespondingNamedParameterPriority = 3;
+            const int noCorrespondingParameterPriority = 4;
+            const int badNonTrailingNamedArgument = 5;


const int badNonTrailingNamedArgument = 5; [](start = 11, length = 43)

Curious: why do we use constants here to establish priority vs. giving the memwber of MemberResolutionKind an explicit numeric value and using that as priority? #Resolved

Curious: why do we use constants here to establish priority vs. giving the memwber of MemberResolutionKind an explicit numeric value and using that as priority?

There are way more MemberResolutionKinds than priorities. This is the only place where we prioritize things. And we avoid spooky action at a distance. :-) #Resolved

jaredpar · 2018-04-02T23:50:28Z

+                return null;
+            }
+
+            var alreadyDefined = PooledHashSet<string>.GetInstance();


GetInstance(); [](start = 55, length = 14)

Should we pass in arguments.Names.Count as the initial capacity here? #Resolved

ObjectPool does not consider initial capacity :( Maybe we can revise that design later.

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

AlekseyTs · 2018-04-03T17:16:37Z

+        [Fact]
+        [CompilerTrait(CompilerFeature.IOperation)]
+        [WorkItem(20050, "https://github.com/dotnet/roslyn/issues/20050")]
+        public void BuildsArgumentsOperationsForDuplicateExplicitArguments_CorrectArgumentsOrder_Lambdas()


Lambdas [](start = 97, length = 7)

I do not see lambdas in this tests scenario. There are delegates though. #Closed

AlekseyTs

LGTM (iteration 2), modulo a small issue around names for tests.

OmarTawfik · 2018-04-03T22:18:15Z

@jaredpar @cston @jcouv comments addressed.

AlekseyTs

LGTM (iteration 3)

OmarTawfik · 2018-04-04T21:03:59Z

@jaredpar @cston @jcouv comments addressed. can I get another review?

jaredpar · 2018-04-05T03:13:24Z

approved

OmarTawfik added Area-Compilers Test Test failures in roslyn-CI labels Mar 26, 2018

OmarTawfik added this to the 15.7 milestone Mar 26, 2018

OmarTawfik self-assigned this Mar 26, 2018

OmarTawfik requested a review from a team as a code owner March 26, 2018 20:08

jcouv previously approved these changes Mar 26, 2018

View reviewed changes

cston reviewed Mar 26, 2018

View reviewed changes

AlekseyTs reviewed Mar 26, 2018

View reviewed changes

AlekseyTs requested changes Mar 26, 2018

View reviewed changes

Overload Resolution now fails with the correct error

738230c

AlekseyTs reviewed Apr 2, 2018

View reviewed changes

jaredpar reviewed Apr 2, 2018

View reviewed changes

AlekseyTs reviewed Apr 2, 2018

View reviewed changes

jaredpar reviewed Apr 2, 2018

View reviewed changes

PR Comments

9e66653

AlekseyTs reviewed Apr 3, 2018

View reviewed changes

AlekseyTs approved these changes Apr 3, 2018

View reviewed changes

Clean up

7669a21

AlekseyTs approved these changes Apr 3, 2018

View reviewed changes

jaredpar approved these changes Apr 5, 2018

View reviewed changes

jaredpar added the Approved to merge label Apr 5, 2018

OmarTawfik merged commit 8887bc1 into dotnet:dev15.7.x Apr 5, 2018

OmarTawfik deleted the fix-200500-parameter-assert branch April 5, 2018 03:21

Conversation

OmarTawfik commented Mar 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

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

Uh oh!

jcouv commented Mar 26, 2018 • edited by OmarTawfik Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OmarTawfik commented Mar 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OmarTawfik commented Mar 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Mar 26, 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!

AlekseyTs Mar 26, 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!

AlekseyTs Mar 26, 2018 • edited by OmarTawfik 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!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Mar 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OmarTawfik commented Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OmarTawfik commented Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OmarTawfik commented Apr 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Apr 2, 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!

jaredpar Apr 2, 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!

OmarTawfik commented Mar 26, 2018 •

edited

Loading

jcouv commented Mar 26, 2018 •

edited by OmarTawfik

Loading

OmarTawfik commented Mar 26, 2018 •

edited

Loading

OmarTawfik commented Mar 26, 2018 •

edited

Loading

AlekseyTs Mar 26, 2018 •

edited

Loading

AlekseyTs Mar 26, 2018 •

edited

Loading

AlekseyTs Mar 26, 2018 •

edited by OmarTawfik

Loading

AlekseyTs commented Mar 26, 2018 •

edited

Loading

OmarTawfik commented Mar 27, 2018 •

edited

Loading

AlekseyTs commented Mar 27, 2018 •

edited

Loading

AlekseyTs commented Mar 27, 2018 •

edited

Loading

OmarTawfik commented Mar 30, 2018 •

edited

Loading

OmarTawfik commented Apr 2, 2018 •

edited

Loading

AlekseyTs Apr 2, 2018 •

edited

Loading

jaredpar Apr 2, 2018 •

edited

Loading

AlekseyTs commented Apr 2, 2018 •

edited

Loading

AlekseyTs commented Apr 2, 2018 •

edited

Loading

AlekseyTs Apr 2, 2018 •

edited

Loading

AlekseyTs Apr 2, 2018 •

edited

Loading

AlekseyTs Apr 2, 2018 •

edited

Loading

AlekseyTs Apr 2, 2018 •

edited

Loading

AlekseyTs Apr 2, 2018 •

edited

Loading

AlekseyTs commented Apr 2, 2018 •

edited

Loading

AlekseyTs Apr 2, 2018 •

edited

Loading

jaredpar Apr 2, 2018 •

edited by OmarTawfik

Loading

AlekseyTs Apr 3, 2018 •

edited by OmarTawfik

Loading

jaredpar Apr 2, 2018 •

edited by OmarTawfik

Loading

AlekseyTs Apr 3, 2018 •

edited

Loading