Fix serialization of attribute data to account for named params argument #22231

jcouv · 2017-09-20T20:57:30Z

Customer scenario

Define attribute with MarkAttribute(bool a, params object[] b) and use it with [Mark(b: new object[] { "Hello", "World" }, a: true)].
It will be incorrectly emitted as [Mark(true, new object[] { true })].

Bugs this fixes:
Fixes #20741

Workarounds, if any
Use attribute without naming arguments.

Risk
Performance impact
Low. This only affects the serialization of attribute data for attributes that have a params parameter.

Is this a regression from a previous update?
No. This code was largely unchanged during the whole git history.

Root cause analysis:
The matching of arguments to parameters during overload resolution is fine. But when it comes time to prepare attribute data, we have logic to put arguments in the proper order (GetRewrittenAttributeConstructorArguments). It's implementation didn't handle named params arguments properly.

How was the bug found?
Reported by customer

Test documentation updated?
Not applicable

jcouv · 2017-09-20T22:54:40Z

@dotnet/roslyn-compiler for review. Thanks

cston · 2017-09-21T15:21:15Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests.cs

+    public object[] B { get;  }
+}
+
+[Mark(b: new object[] { ""Hello"", ""World"" }, a: true)]


Consider testing [Mark(b: "Hello", a: true)] as well. #Resolved

Good catch. That is broken. I'm investigating. #Resolved

cston · 2017-09-21T15:24:40Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests.cs

+    public static void Main()
+    {
+        Console.Write(""Method call: "");
+        Test(b: new object[] { ""Hello"", ""World"" }, a: true);


Is this Test necessary? #Resolved

cston · 2017-09-21T15:30:41Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests.cs

+        [WorkItem(20741, "https://github.com/dotnet/roslyn/issues/20741")]
+        public void TestNamedArgumentOnStringParamsArgument()
+        {
+            var source = CreateCompilationWithMscorlib46(@"


Minor point: Can use CreateStandardCompilation in these tests. #Resolved

I needed 46 for those reflection APIs.

cston · 2017-09-21T16:46:37Z

src/Compilers/CSharp/Portable/Binder/Binder_Attributes.cs

+            }
+
+            TypeSymbol argumentType = (TypeSymbol)argument.Type;
+            // Easy out (i.e. don't both classifying conversion).


don't bother? #Resolved

cston · 2017-09-21T16:51:20Z

src/Compilers/CSharp/Portable/Binder/Binder_Attributes.cs

+
+                // A named argument for a params parameter must be trailing, so expanded params must have a single value
+                var singleValue = new TypedConstant[1] { constructorArgsArray[argIndex] };
+                return new TypedConstant(parameter.Type, singleValue.AsImmutableOrNull());


Avoid the extra array with var singleValue = ImmutableArray.Create(constructorArgsArray[argIndex])); #Resolved

cston · 2017-09-21T17:00:47Z

src/Compilers/CSharp/Portable/Binder/Binder_Attributes.cs

+            if (!constructorArgumentNamesOpt.IsDefault)
+            {
+                int argIndex = constructorArgumentNamesOpt.IndexOf(parameter.Name);
+                if (TryGetNormalParamValue(parameter, constructorArgsArray, argIndex, conversions, out var namedValue))


Does TryGetNormalParamValue handle argIndex = -1? #Resolved

There's indeed a problem there. Thanks
Fixed in new commit #Resolved

cston · 2017-09-22T17:19:02Z

src/Compilers/CSharp/Portable/Binder/Binder_Attributes.cs

+        {
+            TypedConstant argument = constructorArgsArray[argIndex];
+            if (argument.Kind != TypedConstantKind.Array)
+            {


Do we hit this case with Mark(a: true, b: null)? (We should generate a null array.)

cston · 2017-09-22T17:21:42Z

src/Compilers/CSharp/Portable/Binder/Binder_Attributes.cs

            int paramArrayArgCount = argumentsCount - argsConsumedCount;
+
+            // If there are zero arguments left
            if (paramArrayArgCount == 0)
            {
                return new TypedConstant(parameter.Type, ImmutableArray<TypedConstant>.Empty);
            }


Minor point: consider handling the zero arguments left case first. #Resolved

No, I think it must be after the named arguments case.

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

AlekseyTs · 2017-09-22T20:46:21Z

src/Compilers/CSharp/Portable/Binder/Binder_Attributes.cs

+                        return namedValue;
+                    }
+
+                    // A named argument for a params parameter must be trailing, so expanded params must have a single value


// A named argument for a params parameter must be trailing, so expanded params must have a single value [](start = 20, length = 104)

Named argument doesn't have to be trailing, so this comment is somewhat confusing. Consider rephrasing to something like: "Using an expanded form, the argument is the only element of the array." #Closed

AlekseyTs · 2017-09-22T21:25:52Z

src/Compilers/CSharp/Portable/Binder/Binder_Attributes.cs

@@ -578,7 +578,7 @@ private TypeSymbol BindNamedAttributeArgumentType(AttributeArgumentSyntax namedA

                if (parameter.IsParams && parameter.Type.IsSZArray() && i + 1 == parameterCount)
                {
-                    reorderedArgument = GetParamArrayArgument(parameter, constructorArgsArray, argumentsCount, argsConsumedCount, this.Conversions);
+                    reorderedArgument = GetParamArrayArgument(parameter, constructorArgsArray, constructorArgumentNamesOpt, argumentsCount, argsConsumedCount, this.Conversions);
                    sourceIndices = sourceIndices ?? CreateSourceIndicesArray(i, parameterCount);


sourceIndices = sourceIndices ?? CreateSourceIndicesArray(i, parameterCount); [](start = 20, length = 77)

Should sourceIndices be adjusted differently for a named case? #Closed

I'm looking into this. Thanks #Closed

AlekseyTs · 2017-09-22T22:07:24Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests.cs

+    public static void Main()
+    {
+        var attr = typeof(Program).GetCustomAttribute<MarkAttribute>();
+        Console.Write(attr.B[0]);


Console.Write(attr.B[0]); [](start = 8, length = 25)

Consider verifying the length of the array and the value of a as well. #Closed

AlekseyTs · 2017-09-22T22:07:43Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests.cs

+    public static void Main()
+    {
+        var attr = typeof(Program).GetCustomAttribute<MarkAttribute>();
+        Console.Write(attr.B[0]);


Console.Write(attr.B[0]); [](start = 8, length = 25)

Consider verifying the length of the array and the value of a as well. #Closed

AlekseyTs · 2017-09-22T22:09:15Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests.cs

+    public static void Main()
+    {
+        var attr = typeof(Program).GetCustomAttribute<MarkAttribute>();
+        Console.Write(attr.B);


Console.Write(attr.B); [](start = 8, length = 22)

Consider verifying the value of a as well. #Closed

AlekseyTs · 2017-09-22T22:11:08Z

Done with review pass (iteration 4). #Closed

AlekseyTs · 2017-09-25T23:08:01Z

src/Compilers/CSharp/Portable/Binder/Binder_Attributes.cs

-                    sourceIndices = sourceIndices ?? CreateSourceIndicesArray(i, parameterCount);
+                    reorderedArgument = GetParamArrayArgument(parameter, constructorArgsArray, constructorArgumentNamesOpt, argumentsCount,
+                        argsConsumedCount, this.Conversions, out bool foundNamed);
+                    if (!foundNamed)


if (!foundNamed) [](start = 20, length = 16)

It doesn't feel right that we don't allocate the sourceIndices for the case when the argument is named, but the form is not expanded.

AlekseyTs · 2017-09-25T23:10:01Z

Done with review pass (iteration 6). It looks like the issue about sourceIndices is not critical and can be addressed separately.

jcouv · 2017-09-27T16:55:02Z

Filed #22372 for the pre-existing problem of params values getting a -1 source index.

@AlekseyTs I think this should be good to go.

AlekseyTs

LGTM

jcouv · 2017-09-28T17:14:30Z

test windows_debug_unit32_prtest please

jcouv · 2017-09-28T17:31:02Z

windows_debug_unit32_prtest failed with Microsoft.CodeAnalysis.UnitTests.WorkspaceServices.SQLitePersistentStorageTests.PersistentService_Document_WriteReadSameInstance [FAIL] (details)

@dotnet/roslyn-infrastructure Is this a known issue?

jcouv · 2017-09-28T20:51:47Z

test windows_debug_unit32_prtest please

jcouv · 2017-09-29T00:07:25Z

@dotnet-bot test windows_debug_unit32_prtest please

* dotnet/master: Report binding error for typed LINQ query on a type expression (dotnet#22412) clean up more refactoring Fixed tests fix test after rebase one more test CR feedback Allow taking unmanaged/pinned addresses of readonly variables. Fix typo in INotifyCompletion API in doc (dotnet#22417) Fix serialization of attribute data to account for named params argument (dotnet#22231) Create tests to cover test plans dotnet#19216 and dotnet#20127 (dotnet#22284) Checked uses of RefKind.RefReadOnly String rename Check for null before registering incremental analyzer in work coordinator. clean up Create tests to cover test plans dotnet#19216 and dotnet#20127

Fix serialization of attribute data to account for named params argument

d19eca9

jcouv added the Area-Compilers label Sep 20, 2017

jcouv added this to the 15.5 milestone Sep 20, 2017

jcouv self-assigned this Sep 20, 2017

dnfclas added the cla-already-signed label Sep 20, 2017

Add test for VB

c5941a9

cston reviewed Sep 21, 2017

View reviewed changes

Fix handling of expanded form with a named params argument

3612926

cston reviewed Sep 21, 2017

View reviewed changes

Fix crash with argIndex=-1

6163640

cston reviewed Sep 22, 2017

View reviewed changes

AlekseyTs reviewed Sep 22, 2017

View reviewed changes

jcouv added 2 commits September 22, 2017 17:38

Add test suggestions

af667ae

Fix source indices

cefce71

cston approved these changes Sep 23, 2017

View reviewed changes

jcouv added this to Requires BCL work in Compiler: Julien's umbrellas Sep 25, 2017

jcouv moved this from Requires BCL work to Bad PRs in Compiler: Julien's umbrellas Sep 25, 2017

AlekseyTs reviewed Sep 25, 2017

View reviewed changes

jcouv mentioned this pull request Sep 27, 2017

Attribute serialization doesn't track source indices for params #22372

Open

AlekseyTs approved these changes Sep 27, 2017

View reviewed changes

jcouv merged commit 999e20e into dotnet:master Sep 29, 2017

jcouv deleted the named-params branch September 29, 2017 02:09

jcouv removed this from Bad PRs and bugs in Compiler: Julien's umbrellas Jan 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix serialization of attribute data to account for named params argument #22231

Fix serialization of attribute data to account for named params argument #22231

jcouv commented Sep 20, 2017

jcouv commented Sep 20, 2017

cston Sep 21, 2017 •

edited by jcouv

Loading

jcouv Sep 21, 2017 •

edited

Loading

cston Sep 21, 2017 •

edited by jcouv

Loading

cston Sep 21, 2017 •

edited by jcouv

Loading

jcouv Sep 21, 2017

cston Sep 21, 2017 •

edited by jcouv

Loading

cston Sep 21, 2017 •

edited by jcouv

Loading

cston Sep 21, 2017 •

edited by jcouv

Loading

jcouv Sep 21, 2017 •

edited

Loading

cston Sep 22, 2017

cston Sep 22, 2017 •

edited by jcouv

Loading

jcouv Sep 22, 2017

AlekseyTs Sep 22, 2017 •

edited

Loading

AlekseyTs Sep 22, 2017 •

edited

Loading

jcouv Sep 23, 2017 •

edited

Loading

AlekseyTs Sep 22, 2017 •

edited

Loading

AlekseyTs Sep 22, 2017 •

edited

Loading

AlekseyTs Sep 22, 2017 •

edited

Loading

AlekseyTs commented Sep 22, 2017 •

edited

Loading

AlekseyTs Sep 25, 2017

AlekseyTs commented Sep 25, 2017

jcouv commented Sep 27, 2017

AlekseyTs left a comment

jcouv commented Sep 28, 2017

jcouv commented Sep 28, 2017 •

edited

Loading

jcouv commented Sep 28, 2017

jcouv commented Sep 29, 2017

Fix serialization of attribute data to account for named params argument #22231

Fix serialization of attribute data to account for named params argument #22231

Conversation

jcouv commented Sep 20, 2017

jcouv commented Sep 20, 2017

cston Sep 21, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

cston Sep 21, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Sep 21, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Sep 21, 2017

Choose a reason for hiding this comment

cston Sep 21, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Sep 21, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Sep 21, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

cston Sep 22, 2017

Choose a reason for hiding this comment

cston Sep 22, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Sep 22, 2017

Choose a reason for hiding this comment

AlekseyTs Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

jcouv Sep 23, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Sep 22, 2017 • edited Loading

AlekseyTs Sep 25, 2017

Choose a reason for hiding this comment

AlekseyTs commented Sep 25, 2017

jcouv commented Sep 27, 2017

AlekseyTs left a comment

Choose a reason for hiding this comment

jcouv commented Sep 28, 2017

jcouv commented Sep 28, 2017 • edited Loading

jcouv commented Sep 28, 2017

jcouv commented Sep 29, 2017

cston Sep 21, 2017 •

edited by jcouv

Loading

jcouv Sep 21, 2017 •

edited

Loading

cston Sep 21, 2017 •

edited by jcouv

Loading

cston Sep 21, 2017 •

edited by jcouv

Loading

cston Sep 21, 2017 •

edited by jcouv

Loading

cston Sep 21, 2017 •

edited by jcouv

Loading

cston Sep 21, 2017 •

edited by jcouv

Loading

jcouv Sep 21, 2017 •

edited

Loading

cston Sep 22, 2017 •

edited by jcouv

Loading

AlekseyTs Sep 22, 2017 •

edited

Loading

AlekseyTs Sep 22, 2017 •

edited

Loading

jcouv Sep 23, 2017 •

edited

Loading

AlekseyTs Sep 22, 2017 •

edited

Loading

AlekseyTs Sep 22, 2017 •

edited

Loading

AlekseyTs Sep 22, 2017 •

edited

Loading

AlekseyTs commented Sep 22, 2017 •

edited

Loading

jcouv commented Sep 28, 2017 •

edited

Loading