Skip to content
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
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,23 @@
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
using System.Reflection;
using Microsoft.Cci;
using Microsoft.CodeAnalysis.PooledObjects;

namespace Microsoft.CodeAnalysis.CSharp.Symbols
{
// Tracked by https://github.com/dotnet/roslyn/issues/78963 : We are not declaring and not initializing the "Name" property yet.
internal sealed class SynthesizedEmbeddedExtensionMarkerAttributeSymbol : SynthesizedEmbeddedAttributeSymbolBase
{
private readonly ImmutableArray<MethodSymbol> _constructors;
private readonly SynthesizedFieldSymbol _nameField;
private readonly NamePropertySymbol _nameProperty;

private const string PropertyName = "Name";
private const string FieldName = "<Name>k__BackingField";

public SynthesizedEmbeddedExtensionMarkerAttributeSymbol(
string name,
Expand All @@ -21,10 +29,11 @@ public SynthesizedEmbeddedExtensionMarkerAttributeSymbol(
TypeSymbol systemStringType)
: base(name, containingNamespace, containingModule, baseType: systemAttributeType)
{
_constructors = ImmutableArray.Create<MethodSymbol>(
new SynthesizedEmbeddedAttributeConstructorSymbol(
this,
m => ImmutableArray.Create(SynthesizedParameterSymbol.Create(m, TypeWithAnnotations.Create(systemStringType), 0, RefKind.None, name: "name"))));
Debug.Assert(FieldName == GeneratedNames.MakeBackingFieldName(PropertyName));

_nameField = new SynthesizedFieldSymbol(this, systemStringType, FieldName);
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 25, 2025

Choose a reason for hiding this comment

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

SynthesizedFieldSymbol

isReadOnly: true? #Closed

_nameProperty = new NamePropertySymbol(_nameField);
_constructors = [new ConstructorSymbol(this, systemStringType, _nameField)];

// Ensure we never get out of sync with the description
Debug.Assert(_constructors.Length == AttributeDescription.ExtensionMarkerAttribute.Signatures.Length);
Expand All @@ -39,5 +48,168 @@ internal override AttributeUsageInfo GetAttributeUsageInfo()
AttributeTargets.Field | AttributeTargets.Event | AttributeTargets.Interface | AttributeTargets.Delegate,
allowMultiple: false, inherited: false);
}

internal override IEnumerable<FieldSymbol> GetFieldsToEmit()
{
return [_nameField];
}

public override ImmutableArray<Symbol> GetMembers()
=> [_nameField, _nameProperty, _nameProperty.GetMethod, _constructors[0]];

public override ImmutableArray<Symbol> GetMembers(string name)
{
return name switch
{
FieldName => [_nameField],
PropertyName => [_nameProperty],
WellKnownMemberNames.InstanceConstructorName => [_constructors[0]],
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 25, 2025

Choose a reason for hiding this comment

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

[_constructors[0]]

_constructors? #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.

Types don't line up

Copy link
Contributor

Choose a reason for hiding this comment

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

Types don't line up

There is a CastUp helper that allows to cast to an array of base class without reallocation

_ => []
};
}

public override IEnumerable<string> MemberNames
=> [_nameField.Name, PropertyName, WellKnownMemberNames.InstanceConstructorName];
Copy link
Member

@jjonescz jjonescz Sep 25, 2025

Choose a reason for hiding this comment

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

Consider using only constants:

Suggested change
=> [_nameField.Name, PropertyName, WellKnownMemberNames.InstanceConstructorName];
=> [FieldName, PropertyName, WellKnownMemberNames.InstanceConstructorName];
``` #Resolved


private sealed class ConstructorSymbol : SynthesizedInstanceConstructor
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 25, 2025

Choose a reason for hiding this comment

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

ConstructorSymbol

Would it make sense to use existing SynthesizedEmbeddedAttributeConstructorWithBodySymbol instead defining new class? #Closed

{
private readonly ImmutableArray<ParameterSymbol> _parameters;
private readonly SynthesizedFieldSymbol _nameField;
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 25, 2025

Choose a reason for hiding this comment

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

private readonly SynthesizedFieldSymbol _nameField;

I think this storage is redundant because the field symbol is available from containing type (through a private field) #Closed


internal ConstructorSymbol(
NamedTypeSymbol containingType,
TypeSymbol systemStringType,
SynthesizedFieldSymbol nameField) :
base(containingType)
{
_parameters = [SynthesizedParameterSymbol.Create(containingType, TypeWithAnnotations.Create(systemStringType), ordinal: 0, RefKind.None, name: "name")];
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 25, 2025

Choose a reason for hiding this comment

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

containingType

The container for parameter looks wrong #Closed

_nameField = nameField;
}

public override ImmutableArray<ParameterSymbol> Parameters => _parameters;

internal override void GenerateMethodBody(TypeCompilationState compilationState, BindingDiagnosticBag diagnostics)
{
GenerateMethodBodyCore(compilationState, diagnostics);
}

internal override void GenerateMethodBodyStatements(SyntheticBoundNodeFactory f, ArrayBuilder<BoundStatement> statements, BindingDiagnosticBag diagnostics)
{
// this._namedField = name;
statements.Add(f.Assignment(f.Field(f.This(), _nameField), f.Parameter(_parameters[0])));
}
}

private sealed class NamePropertySymbol : PropertySymbol
{
private readonly SynthesizedFieldSymbol _backingField;

public NamePropertySymbol(SynthesizedFieldSymbol backingField)
{
_backingField = backingField;
GetMethod = new NameGetAccessorMethodSymbol(this, backingField);
}

public override string Name => PropertyName;
public override TypeWithAnnotations TypeWithAnnotations => _backingField.TypeWithAnnotations;
public override RefKind RefKind => RefKind.None;
public override ImmutableArray<CustomModifier> RefCustomModifiers => [];
public override MethodSymbol GetMethod { get; }
public override MethodSymbol? SetMethod => null;
public override Symbol ContainingSymbol => _backingField.ContainingSymbol;
public override Accessibility DeclaredAccessibility => Accessibility.Internal;
Copy link
Member

@jjonescz jjonescz Sep 25, 2025

Choose a reason for hiding this comment

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

Should it be public to match the runtime API? #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.

The embedded type is internal (see SynthesizedEmbeddedAttributeSymbolBase.DeclaredAccessibility), so public or internal here are equivalent

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should match the accesibility


public override ImmutableArray<Location> Locations => [];
public override ImmutableArray<SyntaxReference> DeclaringSyntaxReferences => [];
public override ImmutableArray<PropertySymbol> ExplicitInterfaceImplementations => [];
public override ImmutableArray<ParameterSymbol> Parameters => [];
public override bool IsIndexer => false;
public override bool IsStatic => false;
public override bool IsVirtual => false;
public override bool IsOverride => false;
public override bool IsAbstract => false;
public override bool IsSealed => false;
public override bool IsExtern => false;
internal override bool IsRequired => false;
internal override bool HasSpecialName => false;
internal override CallingConvention CallingConvention => CallingConvention.HasThis;
internal override bool MustCallMethodsDirectly => false;
internal override bool HasUnscopedRefAttribute => false;
internal override ObsoleteAttributeData? ObsoleteAttributeData => null;
internal override int TryGetOverloadResolutionPriority() => 0;
}

private sealed partial class NameGetAccessorMethodSymbol : SynthesizedMethodSymbol
{
private readonly NamePropertySymbol _nameProperty;
private readonly SynthesizedFieldSymbol _backingField;
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 25, 2025

Choose a reason for hiding this comment

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

private readonly SynthesizedFieldSymbol _backingField;

This field looks redundant #Closed


public NameGetAccessorMethodSymbol(NamePropertySymbol nameProperty, SynthesizedFieldSymbol backingField)
{
_nameProperty = nameProperty;
_backingField = backingField;
}

public override string Name => "get_Name";
internal override bool HasSpecialName => true;
public override MethodKind MethodKind => MethodKind.PropertyGet;
public override Symbol AssociatedSymbol => _nameProperty;
public override Symbol ContainingSymbol => _nameProperty.ContainingSymbol;

internal override bool SynthesizesLoweredBoundBody => true;

internal override void GenerateMethodBody(TypeCompilationState compilationState, BindingDiagnosticBag diagnostics)
{
SyntheticBoundNodeFactory F = new SyntheticBoundNodeFactory(this, this.GetNonNullSyntaxNode(), compilationState, diagnostics);
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 25, 2025

Choose a reason for hiding this comment

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

this.GetNonNullSyntaxNode()

Is there a chance that is is going to produce non-Dummy node? If not, consider using it directly. #Closed

F.CurrentFunction = this.OriginalDefinition;

try
{
// return this._backingField;
F.CloseMethod(F.Return(F.Field(F.This(), _backingField)));
}
catch (SyntheticBoundNodeFactory.MissingPredefinedMember ex)
{
diagnostics.Add(ex.Diagnostic);
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 25, 2025

Choose a reason for hiding this comment

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

diagnostics.Add(ex.Diagnostic);

It looks like other similar places also do F.CloseMethod(F.ThrowNull()); #Closed

}
}

public override bool IsStatic => false;
public override int Arity => 0;
public override bool IsExtensionMethod => false;
public override bool HidesBaseMethodsByName => false;
public override bool IsVararg => false;
public override bool ReturnsVoid => false;
public override bool IsAsync => false;
public override RefKind RefKind => RefKind.None;
public override ImmutableArray<CustomModifier> RefCustomModifiers => [];
public override TypeWithAnnotations ReturnTypeWithAnnotations => _nameProperty.TypeWithAnnotations;
public override FlowAnalysisAnnotations ReturnTypeFlowAnalysisAnnotations => FlowAnalysisAnnotations.None;
public override ImmutableHashSet<string> ReturnNotNullIfParameterNotNull => [];
public override ImmutableArray<TypeWithAnnotations> TypeArgumentsWithAnnotations => [];
public override ImmutableArray<TypeParameterSymbol> TypeParameters => [];
public override ImmutableArray<ParameterSymbol> Parameters => [];
public override ImmutableArray<MethodSymbol> ExplicitInterfaceImplementations => [];
public override ImmutableArray<Location> Locations => [];
public override Accessibility DeclaredAccessibility => Accessibility.Internal;
Copy link
Member

@jjonescz jjonescz Sep 25, 2025

Choose a reason for hiding this comment

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

Should this have the same accessibility as the property?

Suggested change
public override Accessibility DeclaredAccessibility => Accessibility.Internal;
public override Accessibility DeclaredAccessibility => AssociatedSymbol.DeclaredAccessibility;
``` #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.

It is the same and we already know the value, so doesn't seem useful to delegate

Copy link
Contributor

@AlekseyTs AlekseyTs Sep 25, 2025

Choose a reason for hiding this comment

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

Accessibility.Internal

I think accessibility should be public #Closed

Copy link
Member Author

@jcouv jcouv Sep 25, 2025

Choose a reason for hiding this comment

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

Why? Forget it, it's fine

public override bool IsVirtual => false;
public override bool IsOverride => false;
public override bool IsAbstract => false;
public override bool IsSealed => false;
public override bool IsExtern => false;
protected override bool HasSetsRequiredMembersImpl => false;
internal override MethodImplAttributes ImplementationAttributes => default;
internal override bool HasDeclarativeSecurity => false;
internal override MarshalPseudoCustomAttributeData? ReturnValueMarshallingInformation => null;
internal override bool RequiresSecurityObject => false;
internal override CallingConvention CallingConvention => CallingConvention.HasThis;
internal override bool GenerateDebugInfo => false;

public override DllImportData? GetDllImportData() => null;
internal override ImmutableArray<string> GetAppliedConditionalSymbols() => [];
internal override IEnumerable<SecurityAttribute>? GetSecurityInformation() => null;
internal override bool IsMetadataNewSlot(bool ignoreInterfaceImplementationChanges = false) => false;
internal override bool IsMetadataVirtual(IsMetadataVirtualOption option = IsMetadataVirtualOption.None) => false;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3705,12 +3705,12 @@ public void M()
""" + InstrumentationHelperSource;

var checker = new CSharpInstrumentationChecker();
checker.Method(4, 1, snippet: "", expectBodySpan: false)
checker.Method(5, 1, snippet: "", expectBodySpan: false)
.True("42.M();")
.True("Microsoft.CodeAnalysis.Runtime.Instrumentation.FlushPayload();");
checker.Method(6, 1, snippet: "public void M()")
checker.Method(7, 1, snippet: "public void M()")
.True("""System.Console.WriteLine("Test");""");
checker.Method(8, 1)
checker.Method(9, 1)
.True()
.False()
.True()
Expand Down Expand Up @@ -3801,13 +3801,13 @@ public int P
""" + InstrumentationHelperSource;

var checker = new CSharpInstrumentationChecker();
checker.Method(4, 1, snippet: "", expectBodySpan: false)
checker.Method(5, 1, snippet: "", expectBodySpan: false)
.True("_ = 42.P;")
.True("Microsoft.CodeAnalysis.Runtime.Instrumentation.FlushPayload();");
checker.Method(6, 1, snippet: "get")
checker.Method(7, 1, snippet: "get")
.True("""System.Console.WriteLine("Test");""")
.True("return 0;");
checker.Method(8, 1)
checker.Method(9, 1)
.True()
.False()
.True()
Expand Down Expand Up @@ -3862,14 +3862,14 @@ public void M()
""" + InstrumentationHelperSource;

var checker = new CSharpInstrumentationChecker();
checker.Method(4, 1, snippet: "", expectBodySpan: false)
checker.Method(5, 1, snippet: "", expectBodySpan: false)
.True("42.M();")
.True("Microsoft.CodeAnalysis.Runtime.Instrumentation.FlushPayload();");
checker.Method(6, 1, snippet: "public void M()")
checker.Method(7, 1, snippet: "public void M()")
.True("""System.Console.WriteLine("Test");""")
.True("var f = () =>")
.True("f();");
checker.Method(8, 1)
checker.Method(9, 1)
.True()
.False()
.True()
Expand Down Expand Up @@ -3924,13 +3924,13 @@ static void local()
""" + InstrumentationHelperSource;

var checker = new CSharpInstrumentationChecker();
checker.Method(4, 1, snippet: "", expectBodySpan: false)
checker.Method(5, 1, snippet: "", expectBodySpan: false)
.True("42.M();")
.True("Microsoft.CodeAnalysis.Runtime.Instrumentation.FlushPayload();");
checker.Method(6, 1, snippet: "public void M()")
checker.Method(7, 1, snippet: "public void M()")
.True("local();")
.True("""System.Console.WriteLine("Test");""");
checker.Method(9, 1)
checker.Method(10, 1)
.True()
.False()
.True()
Expand Down Expand Up @@ -4005,13 +4005,13 @@ void local()
var source = classic ? classicSource : newSource;

var checker = new CSharpInstrumentationChecker();
checker.Method(classic ? 3 : 4, 1, snippet: "", expectBodySpan: false)
checker.Method(classic ? 3 : 5, 1, snippet: "", expectBodySpan: false)
.True("42.M();")
.True("Microsoft.CodeAnalysis.Runtime.Instrumentation.FlushPayload();");
checker.Method(classic ? 5 : 6, 1, snippet: classic ? "public static void M(this int i)" : "public void M()")
checker.Method(classic ? 5 : 7, 1, snippet: classic ? "public static void M(this int i)" : "public void M()")
.True("local();")
.True("""System.Console.WriteLine(i);""");
checker.Method(classic ? 8 : 9, 1)
checker.Method(classic ? 8 : 10, 1)
.True()
.False()
.True()
Expand Down
Loading