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.

Sign up for GitHub

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

Jump to bottom

Handle CallerArgumentExpression in attributes #53535

Merged
merged 18 commits into from
Jun 1, 2021
Merged

Handle CallerArgumentExpression in attributes #53535

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
2b111de
Handle CallerArgumentExpression in attributes
Youssef1313 May 20, 2021
91d65f2
Add test
Youssef1313 May 20, 2021
c5c8b12
Feedback
Youssef1313 May 20, 2021
2a6ceb7
Address feedback
Youssef1313 May 21, 2021
a434aa5
Check lang version, add failing test :/
Youssef1313 May 21, 2021
446bfd1
Add test and fix using new approach
Youssef1313 May 21, 2021
d945e3c
Fix test
Youssef1313 May 21, 2021
0e19e35
Simplify code using argumentsToParams
Youssef1313 May 24, 2021
0bce268
Fix bug
Youssef1313 May 24, 2021
413c81a
Test coverage (still needs more)
Youssef1313 May 25, 2021
68c8078
Use Binder.GetSpecialType instead of Compilation.GetSpecialType
Youssef1313 May 25, 2021
146b609
More tests
Youssef1313 May 25, 2021
6131eb4
Fix :)
Youssef1313 May 27, 2021
c05caed
Feedback
Youssef1313 May 27, 2021
4b3d333
Explicitly set language version
Youssef1313 May 28, 2021
a8ba7d1
Fix
Youssef1313 May 28, 2021
d103a43
Feedback
Youssef1313 May 28, 2021
2194bc9
Address feedback
Youssef1313 May 28, 2021
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
27 changes: 22 additions & 5 deletions src/Compilers/CSharp/Portable/Binder/Binder_Attributes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ private CSharpAttributeData GetAttribute(BoundAttribute boundAttribute, BindingD
else
{
constructorArguments = GetRewrittenAttributeConstructorArguments(out constructorArgumentsSourceIndices, attributeConstructor,
constructorArgsArray, boundAttribute.ConstructorArgumentNamesOpt, (AttributeSyntax)boundAttribute.Syntax, diagnostics, ref hasErrors);
constructorArgsArray, boundAttribute.ConstructorArgumentNamesOpt, (AttributeSyntax)boundAttribute.Syntax, boundAttribute.ConstructorArgumentsToParamsOpt, diagnostics, ref hasErrors);
Copy link
Contributor

Choose a reason for hiding this comment

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

ConstructorArgumentNamesOpt

It feels like we should be able to completely get rid of this field in favor of using ConstructorArgumentsToParamsOpt. However, it is probably better to follow up on that separately.

}

CompoundUseSiteInfo<AssemblySymbol> useSiteInfo = GetNewCompoundUseSiteInfo(diagnostics);
Expand Down Expand Up @@ -593,6 +593,7 @@ private ImmutableArray<TypedConstant> GetRewrittenAttributeConstructorArguments(
ImmutableArray<TypedConstant> constructorArgsArray,
ImmutableArray<string> constructorArgumentNamesOpt,
AttributeSyntax syntax,
ImmutableArray<int> argumentsToParams,
BindingDiagnosticBag diagnostics,
ref bool hasErrors)
{
Expand Down Expand Up @@ -663,15 +664,15 @@ private ImmutableArray<TypedConstant> GetRewrittenAttributeConstructorArguments(
// consumed a named argument. For the latter case, argsConsumedCount stays same.
int matchingArgumentIndex;
reorderedArgument = GetMatchingNamedOrOptionalConstructorArgument(out matchingArgumentIndex, constructorArgsArray,
constructorArgumentNamesOpt, parameter, firstNamedArgIndex, argumentsCount, ref argsConsumedCount, syntax, diagnostics);
constructorArgumentNamesOpt, parameter, firstNamedArgIndex, argumentsCount, ref argsConsumedCount, syntax, argumentsToParams, diagnostics);

sourceIndices = sourceIndices ?? CreateSourceIndicesArray(i, parameterCount);
sourceIndices[i] = matchingArgumentIndex;
}
}
else
{
reorderedArgument = GetDefaultValueArgument(parameter, syntax, diagnostics);
reorderedArgument = GetDefaultValueArgument(parameter, syntax, argumentsToParams, diagnostics);
sourceIndices = sourceIndices ?? CreateSourceIndicesArray(i, parameterCount);
}

Expand Down Expand Up @@ -727,6 +728,7 @@ private TypedConstant GetMatchingNamedOrOptionalConstructorArgument(
int argumentsCount,
ref int argsConsumedCount,
AttributeSyntax syntax,
ImmutableArray<int> argumentsToParams,
BindingDiagnosticBag diagnostics)
{
int index = GetMatchingNamedConstructorArgumentIndex(parameter.Name, constructorArgumentNamesOpt, startIndex, argumentsCount);
Copy link
Contributor

@AlekseyTs AlekseyTs May 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

GetMatchingNamedConstructorArgumentIndex

Do we still need this helper and the constructorArgumentNamesOpt if we now have argumentsToParams map? #Closed

Expand All @@ -744,7 +746,7 @@ private TypedConstant GetMatchingNamedOrOptionalConstructorArgument(
else
{
matchingArgumentIndex = -1;
return GetDefaultValueArgument(parameter, syntax, diagnostics);
return GetDefaultValueArgument(parameter, syntax, argumentsToParams, diagnostics);
}
}

Expand Down Expand Up @@ -775,7 +777,7 @@ private static int GetMatchingNamedConstructorArgumentIndex(string parameterName
return argIndex;
}

private TypedConstant GetDefaultValueArgument(ParameterSymbol parameter, AttributeSyntax syntax, BindingDiagnosticBag diagnostics)
private TypedConstant GetDefaultValueArgument(ParameterSymbol parameter, AttributeSyntax syntax, ImmutableArray<int> argumentsToParams, BindingDiagnosticBag diagnostics)
{
var parameterType = parameter.Type;
ConstantValue? defaultConstantValue = parameter.IsOptional ? parameter.ExplicitDefaultConstantValue : ConstantValue.NotAvailable;
Expand Down Expand Up @@ -819,6 +821,14 @@ private TypedConstant GetDefaultValueArgument(ParameterSymbol parameter, Attribu
kind = TypedConstantKind.Primitive;
defaultValue = ((ContextualAttributeBinder)this).AttributedMember.GetMemberCallerName();
}
else if (!IsEarlyAttributeBinder && syntax.ArgumentList is not null &&
getCallerArgumentArgumentIndex(parameter, argumentsToParams) is int argumentIndex && argumentIndex > -1 && argumentIndex < syntax.ArgumentList.Arguments.Count)
Copy link
Contributor

@AlekseyTs AlekseyTs May 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

syntax.ArgumentList.Arguments.Count

This looks suspicious. Isn't this going to include property/field initializers? Do we have tests for scenarios like that? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

this going to include property/field initializers

Indeed, but it's probably that getCallerArgumentArgumentIndex won't return the index of a property/field initializer. But I'll give a deeper look and add more tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

@AlekseyTs Fixed. Thanks!

{
CheckFeatureAvailability(syntax.ArgumentList, MessageID.IDS_FeatureCallerArgumentExpression, diagnostics);
Copy link
Contributor

Choose a reason for hiding this comment

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

CheckFeatureAvailability

It doesn't look like we are testing effect of this call. In fact, I also don't see an existing test covering the same scenario for a regular invocation (looked only in the modified test file).

parameterType = Compilation.GetSpecialType(SpecialType.System_String);
Copy link
Contributor

@AlekseyTs AlekseyTs May 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

parameterType = Compilation.GetSpecialType(SpecialType.System_String);

Is there a guarantee that the parameter type is a string? If so, why not pull it from the ParameterSymbol and assert? If there is no guarantee, we should have a test covering the scenario. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

I followed the same pattern used by other caller member info above. But I'll look into that.

Copy link
Member Author

Choose a reason for hiding this comment

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

@AlekseyTs It's not guaranteed to be a string. It can be an object, and in such case if we pulled it from the ParameterSymbol, emit will fail with Binary format of the specified custom attribute was invalid.
I'll add a test covering this.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not guaranteed to be a string. It can be an object

In this case, I think we shouldn't be asking for the type this way. Binder should have a helper that is going to report an error if the requested type is missing or otherwise bad. I think we should use that helper here instead. Other similar places in this function should probably be adjusted in the same fashion as well.

Copy link
Member Author

@Youssef1313 Youssef1313 May 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@AlekseyTs The diagnostic is reported elsewhere. This is not done in binder so that the error is still produced even if there is no usage of the method having the argument expression parameter (i.e, the diagnostic is produced at the declaration, not the call site).

Copy link
Contributor

Choose a reason for hiding this comment

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

The diagnostic is reported elsewhere. This is not done in binder so that the error is still produced even if there is no usage of the method having the argument expression parameter (i.e, the diagnostic is produced at the declaration, not the call site).

I think we are talking about different diagnostics. I am talking about "bad" string type. It might not be part of the signature. The error is not about missing conversion.

Copy link
Member Author

Choose a reason for hiding this comment

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

@AlekseyTs I'm not sure I understand this (sorry for that). Does this suggest to use the following helper?

roslyn/src/Compilers/CSharp/Portable/Binder/Binder_Symbols.cs

Line 1468 in f6ba232

internal static NamedTypeSymbol GetSpecialType(CSharpCompilation compilation, SpecialType typeId, SyntaxNode node, BindingDiagnosticBag diagnostics)

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this suggest to use the following helper?

roslyn/src/Compilers/CSharp/Portable/Binder/Binder_Symbols.cs

Line 1468 in f6ba232

internal static NamedTypeSymbol GetSpecialType(CSharpCompilation compilation, SpecialType typeId, SyntaxNode node, BindingDiagnosticBag diagnostics)

Yes. Binder.GetSpecialType helper that takes diagnostics

kind = TypedConstantKind.Primitive;
defaultValue = syntax.ArgumentList.Arguments[argumentIndex].Expression.ToString();
}
else if (defaultConstantValue == ConstantValue.NotAvailable)
{
// There is no constant value given for the parameter in source/metadata.
Expand Down Expand Up @@ -874,6 +884,13 @@ private TypedConstant GetDefaultValueArgument(ParameterSymbol parameter, Attribu
{
return new TypedConstant(parameterType, kind, defaultValue);
}

static int getCallerArgumentArgumentIndex(ParameterSymbol parameter, ImmutableArray<int> argumentsToParams)
{
return argumentsToParams.IsDefault
? parameter.CallerArgumentExpressionParameterIndex
: argumentsToParams.IndexOf(parameter.CallerArgumentExpressionParameterIndex);
Copy link
Contributor

@AlekseyTs AlekseyTs May 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

parameter.CallerArgumentExpressionParameterIndex

Suppose parameter.CallerArgumentExpressionParameterIndex is -1 and there is an argument that doesn't map to any parameter (a name doesn't match or an extra argument is provided). What is going to happen then? We probably should add test(s) like that. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

@AlekseyTs I'm not sure what exactly we want to test here. Extra argument or unmatched named argument would produce an error elsewhere, so I assume such a test shouldn't be affected by the logic here? Please correct me. I'm certainly open to add more tests, but I'm not getting which scenario I should try to hit in the test.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what exactly we want to test here. Extra argument or unmatched named argument would produce an error elsewhere, so I assume such a test shouldn't be affected by the logic here? Please correct me. I'm certainly open to add more tests, but I'm not getting which scenario I should try to hit in the test.

I'll try to explain why I am asking for more testing here. The logic here as it stands right now feels fragile to me. In particular, if parameter.CallerArgumentExpressionParameterIndex is -1, we want the enclosing local function to return -1. However, we are unconditionally (without checking the value of parameter.CallerArgumentExpressionParameterIndex) trying to look it up in the array. It is quite possible that this code is unreachable with -1s in the array, therefore, the lookup would always return -1 if we are looking for -1. This, however, depends on things that we don't control here locally. Therfore I am asking for tests that would demonstrate the "unreachability" of the code under the specified conditions. I made some suggestions, but these are just gueses at this point and likely there are other scenarios to try out. My ultimate goal is to confirm that this local function never returns something other than -1 when it is called with parameter.CallerArgumentExpressionParameterIndex -1. If I was writing the code, I would take an alternative approach. I would explicitly check if parameter.CallerArgumentExpressionParameterIndex is -1 before doing the lookup. This would make the implementation more robust and independent from what other components do.

Copy link
Member Author

Choose a reason for hiding this comment

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

@AlekseyTs Thanks a ton! Yes I wrote the code under the assumption that the array won't contain -1.
I explicitly checked for CallerArgumentExpressionParameterIndex == -1 in the last commit. Does that do job?

}
}

private static TypedConstant GetParamArrayArgument(ParameterSymbol parameter, ImmutableArray<TypedConstant> constructorArgsArray,
Expand Down
214 changes: 214 additions & 0 deletions src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_CallerInfoAttributes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,220 @@ public static void M(
value");
}

[ConditionalFact(typeof(CoreClrOnly))]
public void TestArgumentExpressionInAttributeConstructor()
{
string source = @"
using System;
using System.Reflection;
using System.Runtime.CompilerServices;

public class MyAttribute : Attribute
{
public MyAttribute(string s, [CallerArgumentExpression(""s"")] string x = """") => Console.WriteLine($""'{s}', '{x}'"");
}

[My(""Hello"")]
Copy link
Contributor

@AlekseyTs AlekseyTs May 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Consider also testing what do we get from GetAttributes API on the calss. #Closed

public class Program
{
static void Main()
{
typeof(Program).GetCustomAttribute(typeof(MyAttribute));
}
}
";

var compilation = CreateCompilation(source, targetFramework: TargetFramework.NetCoreApp, options: TestOptions.ReleaseExe, parseOptions: TestOptions.RegularPreview);
compilation.VerifyDiagnostics();
CompileAndVerify(compilation, expectedOutput: "'Hello', '\"Hello\"'");
var namedType = compilation.GetTypeByMetadataName("Program").GetPublicSymbol();
var attributeArguments = namedType.GetAttributes().Single().ConstructorArguments;
Assert.Equal(2, attributeArguments.Length);
Assert.Equal("Hello", attributeArguments[0].Value);
Assert.Equal("\"Hello\"", attributeArguments[1].Value);
}

[ConditionalFact(typeof(CoreClrOnly))]
public void TestArgumentExpressionInAttributeConstructor_NamedArgument()
{
string source = @"
using System;
using System.Reflection;
using System.Runtime.CompilerServices;

public class MyAttribute : Attribute
{
public MyAttribute(string s, [CallerArgumentExpression(""s"")] string x = """") => Console.WriteLine($""'{s}', '{x}'"");
}

[My(s:""Hello"")]
public class Program
{
static void Main()
{
typeof(Program).GetCustomAttribute(typeof(MyAttribute));
}
}
";

var compilation = CreateCompilation(source, targetFramework: TargetFramework.NetCoreApp, options: TestOptions.ReleaseExe, parseOptions: TestOptions.RegularPreview);
compilation.VerifyDiagnostics();
CompileAndVerify(compilation, expectedOutput: "'Hello', '\"Hello\"'");
var namedType = compilation.GetTypeByMetadataName("Program").GetPublicSymbol();
var attributeArguments = namedType.GetAttributes().Single().ConstructorArguments;
Assert.Equal(2, attributeArguments.Length);
Assert.Equal("Hello", attributeArguments[0].Value);
Assert.Equal("\"Hello\"", attributeArguments[1].Value);
}

[ConditionalFact(typeof(CoreClrOnly))]
public void TestArgumentExpressionInAttributeConstructor_NamedArgumentsSameOrder()
{
string source = @"
using System;
using System.Reflection;
using System.Runtime.CompilerServices;

public class MyAttribute : Attribute
{
public MyAttribute(string s, string s2, [CallerArgumentExpression(""s"")] string x = """") => Console.WriteLine($""'{s}', '{s2}', '{x}'"");
}

[My(s:""Hello"", s2:""World"")]
public class Program
{
static void Main()
{
typeof(Program).GetCustomAttribute(typeof(MyAttribute));
}
}
";

var compilation = CreateCompilation(source, targetFramework: TargetFramework.NetCoreApp, options: TestOptions.ReleaseExe, parseOptions: TestOptions.RegularPreview);
compilation.VerifyDiagnostics();
CompileAndVerify(compilation, expectedOutput: "'Hello', 'World', '\"Hello\"'");
var namedType = compilation.GetTypeByMetadataName("Program").GetPublicSymbol();
var attributeArguments = namedType.GetAttributes().Single().ConstructorArguments;
Assert.Equal(3, attributeArguments.Length);
Assert.Equal("Hello", attributeArguments[0].Value);
Assert.Equal("World", attributeArguments[1].Value);
Assert.Equal("\"Hello\"", attributeArguments[2].Value);
}

[ConditionalFact(typeof(CoreClrOnly))]
public void TestArgumentExpressionInAttributeConstructor_NamedArgumentsOutOfOrder()
{
string source = @"
using System;
using System.Reflection;
using System.Runtime.CompilerServices;

public class MyAttribute : Attribute
{
public MyAttribute(string s, string s2, [CallerArgumentExpression(""s"")] string x = """") => Console.WriteLine($""'{s}', '{s2}', '{x}'"");
}

[My(s2:""World"", s:""Hello"")]
public class Program
{
static void Main()
{
typeof(Program).GetCustomAttribute(typeof(MyAttribute));
}
}
";

var compilation = CreateCompilation(source, targetFramework: TargetFramework.NetCoreApp, options: TestOptions.ReleaseExe, parseOptions: TestOptions.RegularPreview);
compilation.VerifyDiagnostics();
CompileAndVerify(compilation, expectedOutput: "'Hello', 'World', '\"Hello\"'");
var namedType = compilation.GetTypeByMetadataName("Program").GetPublicSymbol();
var attributeArguments = namedType.GetAttributes().Single().ConstructorArguments;
Assert.Equal(3, attributeArguments.Length);
Assert.Equal("Hello", attributeArguments[0].Value);
Assert.Equal("World", attributeArguments[1].Value);
Assert.Equal("\"Hello\"", attributeArguments[2].Value);
}

[ConditionalFact(typeof(CoreClrOnly))]
public void TestArgumentExpressionInAttributeConstructor_Complex()
{
string source = @"
using System;
using System.Runtime.CompilerServices;

[AttributeUsage(AttributeTargets.Class, AllowMultiple = true)]
public class MyAttribute : Attribute
{
public MyAttribute([CallerArgumentExpression(""param2"")] string param1 = ""param1_default"", [CallerArgumentExpression(""param1"")] string param2 = ""param2_default"") => Console.WriteLine($""param1: {param1}, param2: {param2}"");
}

[My]
[My()]
[My(""param1_value"")]
[My(param1: ""param1_value"")]
[My(param2: ""param2_value"")]
[My(param1: ""param1_value"", param2: ""param2_value"")]
[My(param2: ""param2_value"", param1: ""param1_value"")]
public class Program
{
static void Main()
{
typeof(Program).GetCustomAttributes(typeof(MyAttribute), false);
}
}
";

var compilation = CreateCompilation(source, targetFramework: TargetFramework.NetCoreApp, options: TestOptions.ReleaseExe, parseOptions: TestOptions.RegularPreview);
compilation.VerifyDiagnostics();
CompileAndVerify(compilation, expectedOutput:
@"param1: param1_default, param2: param2_default
param1: param1_default, param2: param2_default
param1: param1_value, param2: ""param1_value""
param1: param1_value, param2: ""param1_value""
param1: ""param2_value"", param2: param2_value
param1: param1_value, param2: param2_value
param1: param1_value, param2: param2_value");
}

[ConditionalFact(typeof(CoreClrOnly))]
public void TestArgumentExpressionInAttributeConstructor_NamedAndOptionalParameters()
{
string source = @"
using System;
using System.Reflection;
using System.Runtime.CompilerServices;

class MyAttribute : Attribute
{
public MyAttribute(int a = 1, int b = 2, int c = 3, [CallerArgumentExpression(""a"")] string expr_a = """", [CallerArgumentExpression(""b"")] string expr_b = """", [CallerArgumentExpression(""c"")] string expr_c = """")
{
Console.WriteLine($""'{a}', '{b}', '{c}', '{expr_a}', '{expr_b}', '{expr_c}'"");
A = a;
B = b;
C = c;
}

public int X;
public int A;
public int B;
public int C;
}

[My(0+0, c:1+1, X=2+2)]
class Program
{
static void Main()
{
typeof(Program).GetCustomAttribute(typeof(MyAttribute));
_ = new MyAttribute(0+0, c: 1+1);
}
}";
var compilation = CreateCompilation(source, targetFramework: TargetFramework.NetCoreApp, options: TestOptions.ReleaseExe, parseOptions: TestOptions.RegularPreview);
compilation.VerifyDiagnostics();
CompileAndVerify(compilation, expectedOutput: @"'0', '2', '2', '0+0', '', '1+1'
'0', '2', '2', '0+0', '', '1+1'");
}

[Fact]
public void TestCallerInfoAttributesWithSaneDefaultValues()
{
Expand Down