Skip to content

Commit

Permalink
parameter attribute order
Browse files Browse the repository at this point in the history
  • Loading branch information
@RikkiGibson
RikkiGibson committed May 14, 2024
1 parent 677ecf0 commit b02a3f4
Show file tree
Hide file tree
Showing 6 changed files with 56 additions and 40 deletions.
19 changes: 13 additions & 6 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceComplexParameterSymbol.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -458,10 +458,15 @@ AttributeLocation IAttributeTargetSymbol.AllowedAttributeLocations
/// <summary>
/// Symbol to copy bound attributes from, or null if the attributes are not shared among multiple source parameter symbols.
/// </summary>
/// <remarks>
/// This is inconsistent with analogous 'BoundAttributesSource' on other symbols.
/// Usually the definition part is the source, but for parameters the implementation part is the source.
/// This affects the location of diagnostics among other things.
/// </remarks>
private SourceParameterSymbol? BoundAttributesSource
=> PartialDefinitionPart;
=> PartialImplementationPart;

private SourceParameterSymbol? PartialImplementationPart
protected SourceParameterSymbol? PartialImplementationPart
{
get
{
Expand All @@ -481,7 +486,7 @@ private SourceParameterSymbol? PartialImplementationPart
}
}

private SourceParameterSymbol? PartialDefinitionPart
protected SourceParameterSymbol? PartialDefinitionPart
{
get
{
Expand All @@ -501,7 +506,7 @@ private SourceParameterSymbol? PartialDefinitionPart
}
}

internal sealed override SyntaxList<AttributeListSyntax> AttributeDeclarationList
internal override SyntaxList<AttributeListSyntax> AttributeDeclarationList
{
get
{
Expand All @@ -515,11 +520,13 @@ internal sealed override SyntaxList<AttributeListSyntax> AttributeDeclarationLis
/// </summary>
internal virtual OneOrMany<SyntaxList<AttributeListSyntax>> GetAttributeDeclarations()
{
if (PartialImplementationPart is { } implementationPart)
// TODO2: should this be non-virtual and subtypes override 'AttributeDeclarationList' instead?
Debug.Assert(PartialImplementationPart is null);
if (PartialDefinitionPart is { } definitionPart)
{
return OneOrMany.Create(
AttributeDeclarationList,
implementationPart.AttributeDeclarationList);
definitionPart.AttributeDeclarationList);
}
else
{
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,7 @@ protected sealed override SourceMemberMethodSymbol BoundAttributesSource

internal sealed override OneOrMany<SyntaxList<AttributeListSyntax>> GetAttributeDeclarations()
{
Debug.Assert(PartialDefinitionPart is null);
if ((object)this.SourcePartialImplementation != null)
{
return OneOrMany.Create(ImmutableArray.Create(AttributeDeclarationSyntaxList, this.SourcePartialImplementation.AttributeDeclarationSyntaxList));
Expand Down
3 changes: 2 additions & 1 deletion src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertyAccessorSymbol.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -631,6 +631,7 @@ public sealed override ImmutableArray<MethodSymbol> ExplicitInterfaceImplementat

internal sealed override OneOrMany<SyntaxList<AttributeListSyntax>> GetAttributeDeclarations()
{
Debug.Assert(PartialDefinitionPart is null);
if (PartialImplementationPart is { } implementation)
{
return OneOrMany.Create(AttributeDeclarationList, ((SourcePropertyAccessorSymbol)implementation).AttributeDeclarationList);
Expand All @@ -639,7 +640,7 @@ internal sealed override OneOrMany<SyntaxList<AttributeListSyntax>> GetAttribute
return OneOrMany.Create(AttributeDeclarationList);
}

private SyntaxList<AttributeListSyntax> AttributeDeclarationList
internal SyntaxList<AttributeListSyntax> AttributeDeclarationList
{
get
{
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbol.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ public override OneOrMany<SyntaxList<AttributeListSyntax>> AttributeDeclarationL
{
get
{
Debug.Assert(PartialDefinitionPart is null);
if (PartialImplementationPart is { } implementationPart)
{
return OneOrMany.Create(
Expand Down
10 changes: 6 additions & 4 deletions src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedAccessorValueParameterSymbol.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,13 @@ protected override IAttributeTargetSymbol AttributeOwner
get { return (SourceMemberMethodSymbol)this.ContainingSymbol; }
}

internal override OneOrMany<SyntaxList<AttributeListSyntax>> GetAttributeDeclarations()
internal sealed override SyntaxList<AttributeListSyntax> AttributeDeclarationList
{
// Bind the attributes on the accessor's attribute syntax list with "param" target specifier.
var accessor = (SourceMemberMethodSymbol)this.ContainingSymbol;
return accessor.GetAttributeDeclarations();
get
{
var accessor = (SourcePropertyAccessorSymbol)this.ContainingSymbol;
return accessor.AttributeDeclarationList;
}
}

internal override void AddSynthesizedAttributes(PEModuleBuilder moduleBuilder, ref ArrayBuilder<SynthesizedAttributeData> attributes)
Expand Down
62 changes: 33 additions & 29 deletions src/Compilers/CSharp/Test/Symbol/Symbols/PartialPropertiesTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3197,6 +3197,8 @@ public partial int P
[Fact]
public void Attributes_SetValueParam()
{
// Just as with parameter attributes on partial methods,
// the implementation part attributes are emitted before the definition part attributes.
var source = """
#pragma warning disable CS9113 // Primary constructor parameter is unread

Expand Down Expand Up @@ -3231,8 +3233,8 @@ public partial int P
var accessor = comp.GetMember<MethodSymbol>("C.set_P");
AssertEx.Equal([], accessor.GetAttributes().SelectAsArray(a => a.ToString()));
AssertEx.Equal([], accessor.PartialImplementationPart.GetAttributes().SelectAsArray(a => a.ToString()));
AssertEx.Equal(["A(1)", "B(1)", "A(2)", "B(2)"], accessor.Parameters.Single().GetAttributes().SelectAsArray(a => a.ToString()));
AssertEx.Equal(["A(1)", "B(1)", "A(2)", "B(2)"], accessor.PartialImplementationPart!.Parameters.Single().GetAttributes().SelectAsArray(a => a.ToString()));
AssertEx.Equal(["A(2)", "B(2)", "A(1)", "B(1)"], accessor.Parameters.Single().GetAttributes().SelectAsArray(a => a.ToString()));
AssertEx.Equal(["A(2)", "B(2)", "A(1)", "B(1)"], accessor.PartialImplementationPart!.Parameters.Single().GetAttributes().SelectAsArray(a => a.ToString()));
}

[Fact]
Expand Down Expand Up @@ -3269,6 +3271,8 @@ partial class C
[Fact]
public void Attributes_IndexerParameter()
{
// Unlike other symbol kinds, for parameters, the implementation part attributes are emitted first, then the definition part attributes.
// This is consistent with partial methods.
var source = """
#pragma warning disable CS9113 // Primary constructor parameter is unread

Expand Down Expand Up @@ -3302,7 +3306,7 @@ partial class C

void verify(ParameterSymbol param)
{
AssertEx.Equal(["A(1)", "B(1)", "A(2)", "B(2)"], param.GetAttributes().SelectAsArray(a => a.ToString()));
AssertEx.Equal(["A(2)", "B(2)", "A(1)", "B(1)"], param.GetAttributes().SelectAsArray(a => a.ToString()));
}
}

Expand All @@ -3317,27 +3321,27 @@ class Attr : Attribute { }
partial class C
{
[Attr]
public partial int P { [Attr] get; [Attr] [param: Attr] set; }
public partial int P { [Attr] get; [Attr] [param: Attr] set; } // 1 (param)

[Attr]
public partial int P { [Attr] get => 1; [Attr] [param: Attr] set { } }
[Attr] // 2
public partial int P { [Attr] get => 1; [Attr] [param: Attr] set { } } // 3, 4 (accessors)
}
""";

var comp = CreateCompilation(source);
comp.VerifyEmitDiagnostics(
// (8,55): error CS0579: Duplicate 'Attr' attribute
// public partial int P { [Attr] get; [Attr] [param: Attr] set; } // 1 (param)
Diagnostic(ErrorCode.ERR_DuplicateAttribute, "Attr").WithArguments("Attr").WithLocation(8, 55),
// (10,6): error CS0579: Duplicate 'Attr' attribute
// [Attr]
// [Attr] // 2
Diagnostic(ErrorCode.ERR_DuplicateAttribute, "Attr").WithArguments("Attr").WithLocation(10, 6),
// (11,29): error CS0579: Duplicate 'Attr' attribute
// public partial int P { [Attr] get => 1; [Attr] [param: Attr] set { } }
// public partial int P { [Attr] get => 1; [Attr] [param: Attr] set { } } // 3, 4 (accessors)
Diagnostic(ErrorCode.ERR_DuplicateAttribute, "Attr").WithArguments("Attr").WithLocation(11, 29),
// (11,46): error CS0579: Duplicate 'Attr' attribute
// public partial int P { [Attr] get => 1; [Attr] [param: Attr] set { } }
Diagnostic(ErrorCode.ERR_DuplicateAttribute, "Attr").WithArguments("Attr").WithLocation(11, 46),
// (11,60): error CS0579: Duplicate 'Attr' attribute
// public partial int P { [Attr] get => 1; [Attr] [param: Attr] set { } }
Diagnostic(ErrorCode.ERR_DuplicateAttribute, "Attr").WithArguments("Attr").WithLocation(11, 60));
// public partial int P { [Attr] get => 1; [Attr] [param: Attr] set { } } // 3, 4 (accessors)
Diagnostic(ErrorCode.ERR_DuplicateAttribute, "Attr").WithArguments("Attr").WithLocation(11, 46));

var property = comp.GetMember<SourcePropertySymbol>("C.P");
AssertEx.Equal(["Attr", "Attr"], property.GetAttributes().SelectAsArray(a => a.ToString()));
Expand All @@ -3357,33 +3361,33 @@ class Attr : Attribute { }
partial class C
{
[Attr]
public partial int this[[Attr] int x, [Attr] int y] { [Attr] get; [Attr] [param: Attr] set; }
public partial int this[[Attr] int x, [Attr] int y] { [Attr] get; [Attr] [param: Attr] set; } // 1, 2, 3 (param)

[Attr]
public partial int this[[Attr] int x, [Attr] int y] { [Attr] get => 1; [Attr] [param: Attr] set { } }
[Attr] // 4
public partial int this[[Attr] int x, [Attr] int y] { [Attr] get => 1; [Attr] [param: Attr] set { } } // 5, 6 (accessor)
}
""";

var comp = CreateCompilation(source);
comp.VerifyEmitDiagnostics(
// (8,30): error CS0579: Duplicate 'Attr' attribute
// public partial int this[[Attr] int x, [Attr] int y] { [Attr] get; [Attr] [param: Attr] set; } // 1, 2, 3 (param)
Diagnostic(ErrorCode.ERR_DuplicateAttribute, "Attr").WithArguments("Attr").WithLocation(8, 30),
// (8,44): error CS0579: Duplicate 'Attr' attribute
// public partial int this[[Attr] int x, [Attr] int y] { [Attr] get; [Attr] [param: Attr] set; } // 1, 2, 3 (param)
Diagnostic(ErrorCode.ERR_DuplicateAttribute, "Attr").WithArguments("Attr").WithLocation(8, 44),
// (8,86): error CS0579: Duplicate 'Attr' attribute
// public partial int this[[Attr] int x, [Attr] int y] { [Attr] get; [Attr] [param: Attr] set; } // 1, 2, 3 (param)
Diagnostic(ErrorCode.ERR_DuplicateAttribute, "Attr").WithArguments("Attr").WithLocation(8, 86),
// (10,6): error CS0579: Duplicate 'Attr' attribute
// [Attr]
// [Attr] // 4
Diagnostic(ErrorCode.ERR_DuplicateAttribute, "Attr").WithArguments("Attr").WithLocation(10, 6),
// (11,30): error CS0579: Duplicate 'Attr' attribute
// public partial int this[[Attr] int x, [Attr] int y] { [Attr] get => 1; [Attr] [param: Attr] set { } }
Diagnostic(ErrorCode.ERR_DuplicateAttribute, "Attr").WithArguments("Attr").WithLocation(11, 30),
// (11,44): error CS0579: Duplicate 'Attr' attribute
// public partial int this[[Attr] int x, [Attr] int y] { [Attr] get => 1; [Attr] [param: Attr] set { } }
Diagnostic(ErrorCode.ERR_DuplicateAttribute, "Attr").WithArguments("Attr").WithLocation(11, 44),
// (11,60): error CS0579: Duplicate 'Attr' attribute
// public partial int this[[Attr] int x, [Attr] int y] { [Attr] get => 1; [Attr] [param: Attr] set { } }
// public partial int this[[Attr] int x, [Attr] int y] { [Attr] get => 1; [Attr] [param: Attr] set { } } // 5, 6 (accessor)
Diagnostic(ErrorCode.ERR_DuplicateAttribute, "Attr").WithArguments("Attr").WithLocation(11, 60),
// (11,77): error CS0579: Duplicate 'Attr' attribute
// public partial int this[[Attr] int x, [Attr] int y] { [Attr] get => 1; [Attr] [param: Attr] set { } }
Diagnostic(ErrorCode.ERR_DuplicateAttribute, "Attr").WithArguments("Attr").WithLocation(11, 77),
// (11,91): error CS0579: Duplicate 'Attr' attribute
// public partial int this[[Attr] int x, [Attr] int y] { [Attr] get => 1; [Attr] [param: Attr] set { } }
Diagnostic(ErrorCode.ERR_DuplicateAttribute, "Attr").WithArguments("Attr").WithLocation(11, 91));
// public partial int this[[Attr] int x, [Attr] int y] { [Attr] get => 1; [Attr] [param: Attr] set { } } // 5, 6 (accessor)
Diagnostic(ErrorCode.ERR_DuplicateAttribute, "Attr").WithArguments("Attr").WithLocation(11, 77));

var property = (SourcePropertySymbol)comp.GetMember<NamedTypeSymbol>("C").Indexers.Single();
AssertEx.Equal(["Attr", "Attr"], property.GetAttributes().SelectAsArray(a => a.ToString()));
Expand Down

0 comments on commit b02a3f4

Please sign in to comment.