Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Sep 29, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 52 additions & 29 deletions src/Compilers/CSharp/Portable/Binder/Binder_Attributes.cs
Original file line number Diff line number Diff line change
Expand Up @@ -825,49 +825,39 @@ private TypedConstant GetDefaultValueArgument(ParameterSymbol parameter, Attribu
{
Debug.Assert(argsConsumedCount <= argumentsCount);

int paramArrayArgCount = argumentsCount - argsConsumedCount;
if (paramArrayArgCount == 0)
{
return new TypedConstant(parameter.Type, ImmutableArray<TypedConstant>.Empty);
}

int argIndex = -1;
// If there's a named argument, we'll use that
if (!constructorArgumentNamesOpt.IsDefault)
{
argIndex = constructorArgumentNamesOpt.IndexOf(parameter.Name);
int argIndex = constructorArgumentNamesOpt.IndexOf(parameter.Name);
if (TryGetNormalParamValue(parameter, constructorArgsArray, argIndex, conversions, out var namedValue))
Copy link
Member

@cston cston Sep 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does TryGetNormalParamValue handle argIndex = -1? #Resolved

Copy link
Member Author

@jcouv jcouv Sep 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
return namedValue;
}

// 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());
Copy link
Member

@cston cston Sep 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

// If there's exactly one argument and it's an array, we'll use that
if (argIndex < 0 && paramArrayArgCount == 1 && constructorArgsArray[argsConsumedCount].Kind == TypedConstantKind.Array)
int paramArrayArgCount = argumentsCount - argsConsumedCount;

// If there are zero arguments left
if (paramArrayArgCount == 0)
{
argIndex = argsConsumedCount;
return new TypedConstant(parameter.Type, ImmutableArray<TypedConstant>.Empty);
}
Copy link
Member

@cston cston Sep 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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


if (argIndex >= 0)
// If there's exactly one argument left and it's an array, we'll try that
if (paramArrayArgCount == 1 &&
TryGetNormalParamValue(parameter, constructorArgsArray,argsConsumedCount, conversions, out var lastValue))
{
TypeSymbol argumentType = (TypeSymbol)constructorArgsArray[argIndex].Type;

// Easy out (i.e. don't both classifying conversion).
if (argumentType == parameter.Type)
{
return constructorArgsArray[argIndex];
}

HashSet<DiagnosticInfo> useSiteDiagnostics = null; // ignoring, since already bound argument and parameter
Conversion conversion = conversions.ClassifyBuiltInConversion(argumentType, parameter.Type, ref useSiteDiagnostics);

// NOTE: Won't always succeed, even though we've performed overload resolution.
// For example, passing int[] to params object[] actually treats the int[] as an element of the object[].
if (conversion.IsValid && conversion.Kind == ConversionKind.ImplicitReference)
{
return constructorArgsArray[argIndex];
}
return lastValue;
}

Debug.Assert(!constructorArgsArray.IsDefault);
Debug.Assert(argsConsumedCount <= constructorArgsArray.Length);

// Take the trailing arguments as an array for expanded form
var values = new TypedConstant[paramArrayArgCount];

for (int i = 0; i < paramArrayArgCount; i++)
Expand All @@ -878,6 +868,39 @@ private TypedConstant GetDefaultValueArgument(ParameterSymbol parameter, Attribu
return new TypedConstant(parameter.Type, values.AsImmutableOrNull());
}

private static bool TryGetNormalParamValue(ParameterSymbol parameter, ImmutableArray<TypedConstant> constructorArgsArray,
int argIndex, Conversions conversions, out TypedConstant result)
{
TypedConstant argument = constructorArgsArray[argIndex];
if (argument.Kind != TypedConstantKind.Array)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

result = default;
return false;
}

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

@cston cston Sep 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't bother? #Resolved

if (argumentType == parameter.Type)
{
result = argument;
return true;
}

HashSet<DiagnosticInfo> useSiteDiagnostics = null; // ignoring, since already bound argument and parameter
Conversion conversion = conversions.ClassifyBuiltInConversion(argumentType, parameter.Type, ref useSiteDiagnostics);

// NOTE: Won't always succeed, even though we've performed overload resolution.
// For example, passing int[] to params object[] actually treats the int[] as an element of the object[].
if (conversion.IsValid && conversion.Kind == ConversionKind.ImplicitReference)
{
result = argument;
return true;
}

result = default;
return false;
}

#endregion

#region AttributeExpressionVisitor
Expand Down
97 changes: 64 additions & 33 deletions src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -99,39 +99,86 @@ sealed class MarkAttribute : Attribute
{
public MarkAttribute(bool a, params object[] b)
{
A = a;
B = b;
}
public bool A { get; }
public object[] B { get; }
public object[] B { get; }
}

[Mark(b: new object[] { ""Hello"", ""World"" }, a: true)]
Copy link
Member

@cston cston Sep 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@jcouv jcouv Sep 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

static class Program
{
private static void Test(bool a, params object[] b)
=> PrintOutArgsInfo(b);

public static void Main()
{
Console.Write(""Method call: "");
Test(b: new object[] { ""Hello"", ""World"" }, a: true);
var attr = typeof(Program).GetCustomAttribute<MarkAttribute>();
Console.Write(attr.B[0]);
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

}
}", options: TestOptions.DebugExe);
source.VerifyDiagnostics();

CompileAndVerify(source, expectedOutput: @"Hello");
}

[Fact]
[WorkItem(20741, "https://github.com/dotnet/roslyn/issues/20741")]
public void TestNamedArgumentOnObjectParamsArgument2()
{
var source = CreateCompilationWithMscorlib46(@"
using System;
using System.Reflection;

sealed class MarkAttribute : Attribute
{
public MarkAttribute(bool a, params object[] b)
{
B = b;
}
public object[] B { get; }
}

[Mark(b: ""Hello"", a: true)]
static class Program
{
public static void Main()
{
var attr = typeof(Program).GetCustomAttribute<MarkAttribute>();
Console.Write(""Attribute constructor call: "");
PrintOutArgsInfo(attr.B);
Console.Write(attr.B[0]);
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

}
}", options: TestOptions.DebugExe);
source.VerifyDiagnostics();

CompileAndVerify(source, expectedOutput: @"Hello");
}

[Fact]
[WorkItem(20741, "https://github.com/dotnet/roslyn/issues/20741")]
public void TestNamedArgumentOnObjectParamsArgument3()
{
var source = CreateCompilationWithMscorlib46(@"
using System;
using System.Reflection;

sealed class MarkAttribute : Attribute
{
public MarkAttribute(bool a, params object[] b)
{
B = b;
}
public object[] B { get; }
}

private static void PrintOutArgsInfo(object[] args)
[Mark(true, new object[] { ""Hello"" }, new object[] { ""World"" })]
static class Program
{
public static void Main()
{
Console.WriteLine($""{args[0]}"");
var attr = typeof(Program).GetCustomAttribute<MarkAttribute>();
var worldArray = (object[])attr.B[1];
Console.Write(worldArray[0]);
}
}", options: TestOptions.DebugExe);
source.VerifyDiagnostics();

CompileAndVerify(source, expectedOutput:
@"Method call: Hello
Attribute constructor call: Hello");
CompileAndVerify(source, expectedOutput: @"World");
}

[Fact]
Expand All @@ -146,39 +193,23 @@ sealed class MarkAttribute : Attribute
{
public MarkAttribute(int a, int b)
{
A = a;
B = b;
}
public int A { get; }
public int B { get; }
}

[Mark(b: 42, a: 1)]
static class Program
{
private static void Test(int a, int b)
=> PrintOutArgsInfo(b);

public static void Main()
{
Console.Write(""Method call: "");
Test(b: 42, a: 1);

var attr = typeof(Program).GetCustomAttribute<MarkAttribute>();
Console.Write(""Attribute constructor call: "");
PrintOutArgsInfo(attr.B);
}

private static void PrintOutArgsInfo(int value)
{
Console.WriteLine($""Value={value}"");
Console.Write(attr.B);
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Consider verifying the value of a as well. #Closed

}
}", options: TestOptions.DebugExe);
source.VerifyDiagnostics();

CompileAndVerify(source, expectedOutput:
@"Method call: Value=42
Attribute constructor call: Value=42");
CompileAndVerify(source, expectedOutput: "42");
}

[WorkItem(984896, "http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/984896")]
Expand Down