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

InterpolatedStringHandlerArgumentAttribute Decoding #53932

2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5607,7 +5607,7 @@ private BoundExpression BindTypeParameterCreationExpression(SyntaxNode node, Typ
/// of this method (i.e. not populating a pre-existing <see cref="OverloadResolutionResult{MethodSymbol}"/>).
/// Presently, rationalizing this behavior is not worthwhile.
/// </remarks>
private bool TryPerformConstructorOverloadResolution(
internal bool TryPerformConstructorOverloadResolution(
NamedTypeSymbol typeContainingConstructors,
AnalyzedArguments analyzedArguments,
string errorName,
Expand Down
7 changes: 7 additions & 0 deletions src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml
Original file line number Diff line number Diff line change
Expand Up @@ -2020,6 +2020,13 @@

<Node Name="BoundInterpolatedStringHandlerPlaceholder" Base="BoundValuePlaceholderBase"/>

<!-- Type expression placeholder for the arguments to the constructor call for an interpolated string handler
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 9, 2021

Choose a reason for hiding this comment

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

Type expression

"Type expression" sounds a lot like BoundTypeExpression, but I believe this node doesn't represent a type reference. Because of that the comment feels confusing to me. I suggest saying "An expression" or "A typed expression" instead. #Closed

conversion. We intentionally use a placeholder for overload resolution here to ensure that no conversion
from expression can occur. -->
<Node Name="BoundInterpolatedStringArgumentPlaceholder" Base="BoundValuePlaceholderBase">
<Field Name="Type" Type="TypeSymbol" Override="true" Null="disallow" />
</Node>

<Node Name="BoundStringInsert" Base="BoundExpression">
<Field Name="Type" Type="TypeSymbol?" Override="true" Null="always"/>
<Field Name="Value" Type="BoundExpression" Null="disallow"/>
Expand Down
28 changes: 28 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -6656,4 +6656,32 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_GlobalUsingOutOfOrder" xml:space="preserve">
<value>A global using directive must precede all non-global using directives.</value>
</data>
<data name="ERR_NullInvalidInterpolatedStringHandlerArgumentName" xml:space="preserve">
<value>null is not a valid parameter name. To get access to the receiver of an instance method, use the empty string as the parameter name.</value>
</data>
<data name="ERR_NotInstanceInvalidInterpolatedStringHandlerArgumentName" xml:space="preserve">
<value>'{0}' is not an instance method, the receiver cannot be an interpolated string handler argument.</value>
</data>
<data name="ERR_InvalidInterpolatedStringHandlerArgumentName" xml:space="preserve">
<value>'{0}' is not a valid parameter name from '{1}'.</value>
</data>
<data name="ERR_TypeIsNotAnInterpolatedStringHandlerType" xml:space="preserve">
<value>'{0}' is not an interpolated string handler type.</value>
</data>
<data name="ERR_InterpolatedStringHandlerIncorrectNumberOfConstructorArguments" xml:space="preserve">
<value>'{0}' does not have a constructor that takes {1} arguments.</value>
</data>
<data name="WRN_ParameterOccursAfterInterpolatedStringHandlerParameter" xml:space="preserve">
<value>Parameter {0} occurs after {1} in the parameter list, but is used as an argument for interpolated string handler conversions. This will require the caller to reorder parameters with named arguments at the call site. Consider putting the interpolated string handler parameter after all arguments involved.</value>
</data>
<data name="WRN_ParameterOccursAfterInterpolatedStringHandlerParameter_Title" xml:space="preserve">
<value>Parameter to interpolated string handler conversion occurs after handler parameter</value>
</data>
<data name="ERR_CannotUseSelfAsInterpolatedStringHandlerArgument" xml:space="preserve">
<value>Interpolated string handler arguments cannot refer to themselves.</value>
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 9, 2021

Choose a reason for hiding this comment

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

Is this error about referring to the parameter in the attribute applied to the same parameter? Perhaps it would be clearer to say something like: "cannot be used to create a handler for this parameter"? #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 tried to make it a bit more clear. Let me know if you think it still needs work.

</data>
<data name="ERR_CouldNotFindApplicableInterpolatedStringHandlerConstructor" xml:space="preserve">
<value>Could not find a constructor on '{0}' that matches the parameters specified in the InterpolatedStringHandlerArgumentAttribute.</value>
<remarks>InterpolatedStringHandlerArgumentAttribute is a type name and should not be translated.</remarks>
</data>
</root>
8 changes: 8 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1942,6 +1942,14 @@ internal enum ErrorCode
ERR_InterpolatedStringHandlerMethodReturnMalformed = 9001,
ERR_InterpolatedStringHandlerMethodReturnInconsistent = 9002,
ERR_InterpolatedStringHandlerInvalidCreateMethod = 9003,
ERR_NullInvalidInterpolatedStringHandlerArgumentName = 9004,
ERR_NotInstanceInvalidInterpolatedStringHandlerArgumentName = 9005,
ERR_InvalidInterpolatedStringHandlerArgumentName = 9006,
ERR_TypeIsNotAnInterpolatedStringHandlerType = 9007,
ERR_InterpolatedStringHandlerIncorrectNumberOfConstructorArguments = 9008,
WRN_ParameterOccursAfterInterpolatedStringHandlerParameter = 9009,
ERR_CannotUseSelfAsInterpolatedStringHandlerArgument = 9010,
ERR_CouldNotFindApplicableInterpolatedStringHandlerConstructor = 9011,

#endregion diagnostics introduced for C# 10.0

Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs
Original file line number Diff line number Diff line change
Expand Up @@ -472,6 +472,7 @@ internal static int GetWarningLevel(ErrorCode code)
case ErrorCode.WRN_AnalyzerReferencesFramework:
case ErrorCode.WRN_UnreadRecordParameter:
case ErrorCode.WRN_DoNotCompareFunctionPointers:
case ErrorCode.WRN_ParameterOccursAfterInterpolatedStringHandlerParameter:
return 1;
default:
return 0;
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -158,5 +158,21 @@ public void AddNotNullIfParameterNotNull(string parameterName)
_notNullIfParameterNotNull = _notNullIfParameterNotNull.Add(parameterName);
SetDataStored();
}

private ImmutableArray<int> _interpolatedStringHandlerArguments = ImmutableArray<int>.Empty;
public ImmutableArray<int> InterpolatedStringHandlerArguments
{
get
{
VerifySealed(expected: true);
return _interpolatedStringHandlerArguments;
}
set
{
VerifySealed(expected: false);
_interpolatedStringHandlerArguments = value;
SetDataStored();
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -78,5 +78,6 @@ internal int MethodHashCode()
internal override bool IsCallerMemberName => false;
internal override FlowAnalysisAnnotations FlowAnalysisAnnotations => FlowAnalysisAnnotations.None;
internal override ImmutableHashSet<string> NotNullIfParameterNotNull => ImmutableHashSet<string>.Empty;
internal override ImmutableArray<int> InterpolatedStringHandlerArgumentIndexes => ImmutableArray<int>.Empty;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
using System.Linq;
using System.Reflection;
using System.Reflection.Metadata;
using System.Runtime.InteropServices;
Expand Down Expand Up @@ -140,6 +141,7 @@ public bool TryGetFlowAnalysisAnnotations(out FlowAnalysisAnnotations value)
private ImmutableArray<CSharpAttributeData> _lazyCustomAttributes;
private ConstantValue _lazyDefaultValue = ConstantValue.Unset;
private ThreeState _lazyIsParams;
private ImmutableArray<int> _lazyInterpolatedStringHandlerAttributeIndexes;

/// <summary>
/// Attributes filtered out from m_lazyCustomAttributes, ParamArray, etc.
Expand Down Expand Up @@ -702,6 +704,82 @@ private static FlowAnalysisAnnotations DecodeFlowAnalysisAttributes(PEModule mod
return annotations;
}

#nullable enable
internal override ImmutableArray<int> InterpolatedStringHandlerArgumentIndexes
{
get
{
ImmutableArray<int> indexes = _lazyInterpolatedStringHandlerAttributeIndexes;
if (indexes.IsDefault)
{
indexes = DecodeInterpolatedStringHandlerArgumentAttribute();
Debug.Assert(!indexes.IsDefault);
var initialized = ImmutableInterlocked.InterlockedInitialize(ref _lazyInterpolatedStringHandlerAttributeIndexes, indexes);
Debug.Assert(initialized || indexes.SequenceEqual(_lazyInterpolatedStringHandlerAttributeIndexes));
}

return indexes;
}
}

private ImmutableArray<int> DecodeInterpolatedStringHandlerArgumentAttribute()
{
// If we encounter an invalid attribute, we ignore the contents and just use the empty list.

if (Type is not NamedTypeSymbol { IsInterpolatedStringHandlerType: true })
{
return ImmutableArray<int>.Empty;
}

var paramNames = _moduleSymbol.Module.GetInterpolatedStringHandlerArgumentAttributeValues(_handle);
Debug.Assert(!paramNames.IsDefault);

if (paramNames.IsEmpty)
{
return ImmutableArray<int>.Empty;
}

var builder = ArrayBuilder<int>.GetInstance(paramNames.Length);
var parameters = ContainingSymbol switch
{
MethodSymbol { Parameters: var p } => p,
PropertySymbol { Parameters: var p } => p,
_ => throw ExceptionUtilities.UnexpectedValue(ContainingSymbol)
};
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 9, 2021

Choose a reason for hiding this comment

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

It looks like we already have internal static ImmutableArray<ParameterSymbol> GetParameters(this Symbol member) helper that can be used here. #Closed

Copy link
Contributor

@AlekseyTs AlekseyTs Jun 9, 2021

Choose a reason for hiding this comment

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

BTW, are we going to crash on an event accessor parameter here?


foreach (var name in paramNames)
{
switch (name)
{
case null:
case { Length: 0 } when ContainingSymbol.IsStatic:
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 9, 2021

Choose a reason for hiding this comment

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

when ContainingSymbol.IsStatic:

I guess I am missing what static-ness of a container has to do with validity of the data? Is there a relevant spec? #Closed

Copy link
Contributor

@AlekseyTs AlekseyTs Jun 9, 2021

Choose a reason for hiding this comment

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

Perhaps the Length check confused me as I thought we are dealing with an array. Consider comparing with an empty string instead, for clarity. Below as well.

// Invalid data, bail
builder.Free();
return ImmutableArray<int>.Empty;
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 9, 2021

Choose a reason for hiding this comment

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

return ImmutableArray.Empty;

As I mentioned elsewhere, I think we should not silently ignore malformed applications of the attribute at the call site of the method. #Closed


case { Length: 0 }:
builder.Add(-1);
break;

default:
var param = parameters.FirstOrDefault(static (p, name) => string.Equals(p.Name, name, StringComparison.Ordinal), name);
Copy link
Contributor

@chsienki chsienki Jun 8, 2021

Choose a reason for hiding this comment

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

Given that this is the only place we're using FirstOrDefault, is it worth just putting the implementation here, avoiding the lambda, and probably isnt any more lines of code as you don't need the below checks? #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.

I don't think so. I prefer it the way I wrote it.

if (param is not null)
{
builder.Add(param.Ordinal);
break;
}
else
{
builder.Free();
return ImmutableArray<int>.Empty;
}
}
}

return builder.ToImmutableAndFree();
}
#nullable disable

internal override ImmutableHashSet<string> NotNullIfParameterNotNull
{
get
Expand Down
7 changes: 7 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/ParameterSymbol.cs
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,13 @@ internal sealed override ObsoleteAttributeData ObsoleteAttributeData

internal abstract ImmutableHashSet<string> NotNullIfParameterNotNull { get; }

/// <summary>
/// Indexes of the parameters that will be passed to the constructor of the interpolated string handler type
/// when an interpolated string handler conversion occurs. An index of -1 means the "this" parameter. These
/// indexes are ordered in the order to be passed to the constructor.
/// </summary>
internal abstract ImmutableArray<int> InterpolatedStringHandlerArgumentIndexes { get; }

protected sealed override int HighestPriorityUseSiteError
{
get
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@ public SignatureOnlyParameterSymbol(

internal override ModuleSymbol ContainingModule { get { throw ExceptionUtilities.Unreachable; } }

internal override ImmutableArray<int> InterpolatedStringHandlerArgumentIndexes => ImmutableArray<int>.Empty;

#endregion Not used by MethodSignatureComparer

public override bool Equals(Symbol obj, TypeCompareKind compareKind)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ internal sealed override ObsoleteAttributeData ObsoleteAttributeData
}
}

internal override void DecodeWellKnownAttribute(ref DecodeWellKnownAttributeArguments<AttributeSyntax, CSharpAttributeData, AttributeLocation> arguments)
internal override void DecodeWellKnownAttribute(ref DecodeWellKnownAttributeArguments<AttributeSyntax, CSharpAttributeData, AttributeLocation> arguments, Binder binder)
{
Debug.Assert((object)arguments.AttributeSyntaxOpt != null);
var diagnostics = (BindingDiagnosticBag)arguments.Diagnostics;
Expand Down
Loading