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 13 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
131 changes: 55 additions & 76 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 All @@ -610,9 +611,6 @@ private ImmutableArray<TypedConstant> GetRewrittenAttributeConstructorArguments(
Debug.Assert(!hasNamedCtorArguments ||
constructorArgumentNamesOpt.Length == argumentsCount);

// index of the first named constructor argument
int firstNamedArgIndex = -1;

ImmutableArray<ParameterSymbol> parameters = attributeConstructor.Parameters;
int parameterCount = parameters.Length;

Expand Down Expand Up @@ -650,28 +648,20 @@ private ImmutableArray<TypedConstant> GetRewrittenAttributeConstructorArguments(
}
else
{
// named constructor argument

// Store the index of the first named constructor argument
if (firstNamedArgIndex == -1)
{
firstNamedArgIndex = argsConsumedCount;
}

// Current parameter must either have a matching named argument or a default value
// For the former case, argsConsumedCount must be incremented to note that we have
// 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);
reorderedArgument = getMatchingNamedOrOptionalConstructorArgument(out matchingArgumentIndex, constructorArgsArray,
parameter, 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, argumentsCount, diagnostics);
sourceIndices = sourceIndices ?? CreateSourceIndicesArray(i, parameterCount);
}

Expand All @@ -697,6 +687,36 @@ private ImmutableArray<TypedConstant> GetRewrittenAttributeConstructorArguments(

constructorArgumentsSourceIndices = sourceIndices != null ? sourceIndices.AsImmutableOrNull() : default(ImmutableArray<int>);
return reorderedArguments.AsImmutableOrNull();

// Local functions
TypedConstant getMatchingNamedOrOptionalConstructorArgument(
out int matchingArgumentIndex,
ImmutableArray<TypedConstant> constructorArgsArray,
ParameterSymbol parameter,
int argumentsCount,
ref int argsConsumedCount,
AttributeSyntax syntax,
ImmutableArray<int> argumentsToParams,
BindingDiagnosticBag diagnostics)
{
matchingArgumentIndex = argumentsToParams.IsDefaultOrEmpty
? parameter.Ordinal
: argumentsToParams.IndexOf(parameter.Ordinal);

if (matchingArgumentIndex > -1)
{
// found a matching named argument
Debug.Assert(matchingArgumentIndex < argumentsCount);

// increment argsConsumedCount
argsConsumedCount++;
return constructorArgsArray[matchingArgumentIndex];
}
else
{
return GetDefaultValueArgument(parameter, syntax, argumentsToParams, argumentsCount, diagnostics);
}
}
}

private static int[] CreateSourceIndicesArray(int paramIndex, int parameterCount)
Expand All @@ -718,64 +738,7 @@ private static int[] CreateSourceIndicesArray(int paramIndex, int parameterCount
return sourceIndices;
}

private TypedConstant GetMatchingNamedOrOptionalConstructorArgument(
out int matchingArgumentIndex,
ImmutableArray<TypedConstant> constructorArgsArray,
ImmutableArray<string> constructorArgumentNamesOpt,
ParameterSymbol parameter,
int startIndex,
int argumentsCount,
ref int argsConsumedCount,
AttributeSyntax syntax,
BindingDiagnosticBag diagnostics)
{
int index = GetMatchingNamedConstructorArgumentIndex(parameter.Name, constructorArgumentNamesOpt, startIndex, argumentsCount);

if (index < argumentsCount)
{
// found a matching named argument
Debug.Assert(index >= startIndex);

// increment argsConsumedCount
argsConsumedCount++;
matchingArgumentIndex = index;
return constructorArgsArray[index];
}
else
{
matchingArgumentIndex = -1;
return GetDefaultValueArgument(parameter, syntax, diagnostics);
}
}

private static int GetMatchingNamedConstructorArgumentIndex(string parameterName, ImmutableArray<string> argumentNamesOpt, int startIndex, int argumentsCount)
{
RoslynDebug.Assert(parameterName != null);
Debug.Assert(startIndex >= 0 && startIndex < argumentsCount);

if (parameterName.IsEmpty() || !argumentNamesOpt.Any())
{
return argumentsCount;
}

// get the matching named (constructor) argument
int argIndex = startIndex;
while (argIndex < argumentsCount)
{
var name = argumentNamesOpt[argIndex];

if (string.Equals(name, parameterName, StringComparison.Ordinal))
{
break;
}

argIndex++;
}

return argIndex;
}

private TypedConstant GetDefaultValueArgument(ParameterSymbol parameter, AttributeSyntax syntax, BindingDiagnosticBag diagnostics)
private TypedConstant GetDefaultValueArgument(ParameterSymbol parameter, AttributeSyntax syntax, ImmutableArray<int> argumentsToParams, int argumentsCount, BindingDiagnosticBag diagnostics)
{
var parameterType = parameter.Type;
ConstantValue? defaultConstantValue = parameter.IsOptional ? parameter.ExplicitDefaultConstantValue : ConstantValue.NotAvailable;
Expand Down Expand Up @@ -803,22 +766,31 @@ private TypedConstant GetDefaultValueArgument(ParameterSymbol parameter, Attribu
else
{
// Boxing or identity conversion:
parameterType = Compilation.GetSpecialType(SpecialType.System_Int32);
parameterType = GetSpecialType(SpecialType.System_Int32, diagnostics, syntax);
defaultValue = line;
}
}
else if (!IsEarlyAttributeBinder && parameter.IsCallerFilePath)
{
parameterType = Compilation.GetSpecialType(SpecialType.System_String);
parameterType = GetSpecialType(SpecialType.System_String, diagnostics, syntax);
kind = TypedConstantKind.Primitive;
defaultValue = syntax.SyntaxTree.GetDisplayPath(syntax.Name.Span, Compilation.Options.SourceReferenceResolver);
}
else if (!IsEarlyAttributeBinder && parameter.IsCallerMemberName && (object)((ContextualAttributeBinder)this).AttributedMember != null)
{
parameterType = Compilation.GetSpecialType(SpecialType.System_String);
parameterType = GetSpecialType(SpecialType.System_String, diagnostics, syntax);
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 < argumentsCount)
{
Debug.Assert(argumentsCount <= syntax.ArgumentList.Arguments.Count);
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 = GetSpecialType(SpecialType.System_String, diagnostics, syntax);
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 +846,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
Loading