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

Fix support for externally declared records #9104

Merged
merged 5 commits into from
Oct 1, 2024
Merged

Fix support for externally declared records #9104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6357654
Fix support for externally declared records
kzu Aug 6, 2024
9e64309
Skip compiler-generated ctors in records
kzu Aug 5, 2024
7b48cc2
Ignore design-time build value when debugging generator
kzu Aug 6, 2024
050c1c9
Add additional test cases for generic & readonly scenario
ReubenBond Aug 27, 2024
e919b73
Remove IsImplicitlyDeclared check
ReubenBond Sep 30, 2024
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
7 changes: 7 additions & 0 deletions Orleans.sln
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,8 @@ Project("{6EC3EE1D-3C4E-46DD-8F32-0CC8E7565705}") = "Orleans.Serialization.FShar
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Orleans.Serialization.MessagePack", "src\Orleans.Serialization.MessagePack\Orleans.Serialization.MessagePack.csproj", "{F50F81B6-E9B5-4143-B66B-A1AD913F6E9C}"
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "TestSerializerExternalModels", "test\Misc\TestSerializerExternalModels\TestSerializerExternalModels.csproj", "{5D587DDE-036D-4694-A314-8DDF270AC031}"
EndProject
Global
GlobalSection(SolutionConfigurationPlatforms) = preSolution
Debug|Any CPU = Debug|Any CPU
Expand Down Expand Up @@ -632,6 +634,10 @@ Global
{F50F81B6-E9B5-4143-B66B-A1AD913F6E9C}.Debug|Any CPU.Build.0 = Debug|Any CPU
{F50F81B6-E9B5-4143-B66B-A1AD913F6E9C}.Release|Any CPU.ActiveCfg = Release|Any CPU
{F50F81B6-E9B5-4143-B66B-A1AD913F6E9C}.Release|Any CPU.Build.0 = Release|Any CPU
{5D587DDE-036D-4694-A314-8DDF270AC031}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{5D587DDE-036D-4694-A314-8DDF270AC031}.Debug|Any CPU.Build.0 = Debug|Any CPU
{5D587DDE-036D-4694-A314-8DDF270AC031}.Release|Any CPU.ActiveCfg = Release|Any CPU
{5D587DDE-036D-4694-A314-8DDF270AC031}.Release|Any CPU.Build.0 = Release|Any CPU
EndGlobalSection
GlobalSection(SolutionProperties) = preSolution
HideSolutionNode = FALSE
Expand Down Expand Up @@ -748,6 +754,7 @@ Global
{84B44F1D-B7FE-40E3-82F0-730A55AC8613} = {316CDCC7-323F-4264-9FC9-667662BB1F80}
{B2D53D3C-E44A-4C9B-AAEE-28FB8C1BDF62} = {A6573187-FD0D-4DF7-91D1-03E07E470C0A}
{F50F81B6-E9B5-4143-B66B-A1AD913F6E9C} = {4CD3AA9E-D937-48CA-BB6C-158E12257D23}
{5D587DDE-036D-4694-A314-8DDF270AC031} = {70BCC54E-1618-4742-A079-07588065E361}
EndGlobalSection
GlobalSection(ExtensibilityGlobals) = postSolution
SolutionGuid = {7BFB3429-B5BB-4DB1-95B4-67D77A864952}
Expand Down
35 changes: 31 additions & 4 deletions src/Orleans.CodeGenerator/CodeGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,10 +145,10 @@ public CompilationUnitSyntax GenerateCode(CancellationToken cancellationToken)
if (symbol.IsRecord)
{
// If there is a primary constructor then that will be declared before the copy constructor
// A record always generates a copy constructor and marks it as implicitly declared
// A record always generates a copy constructor and marks it as compiler generated
// todo: find an alternative to this magic
var potentialPrimaryConstructor = symbol.Constructors[0];
if (!potentialPrimaryConstructor.IsImplicitlyDeclared)
if (!potentialPrimaryConstructor.IsImplicitlyDeclared && !potentialPrimaryConstructor.IsCompilerGenerated())
{
constructorParameters = potentialPrimaryConstructor.Parameters;
}
Expand All @@ -160,6 +160,23 @@ public CompilationUnitSyntax GenerateCode(CancellationToken cancellationToken)
{
constructorParameters = annotatedConstructors[0].Parameters;
}
else
{
// record structs from referenced assemblies do not return IsRecord=true
// above. See https://github.com/dotnet/roslyn/issues/69326
// So we implement the same heuristics from ShouldIncludePrimaryConstructorParameters
// to detect a primary constructor.
var properties = symbol.GetMembers().OfType<IPropertySymbol>().ToImmutableArray();
var primaryConstructor = symbol.GetMembers()
.OfType<IMethodSymbol>()
.Where(m => m.MethodKind == MethodKind.Constructor && m.Parameters.Length > 0)
// Check for a ctor where all parameters have a corresponding compiler-generated prop.
.FirstOrDefault(ctor => ctor.Parameters.All(prm =>
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this variation easier to read, and possibly a little faster.

    .FirstOrDefault(ctor =>
        ctor.Parameters
            .Join(properties,
                  prm => prm.Name,
                  prop => prop.Name,
                  (prm, prop) => prop)
            .Count(prop => prop.IsCompilerGenerated) == ctor.Parameters.Length
    );

properties.Any(prop => prop.Name.Equals(prm.Name, StringComparison.Ordinal) && prop.IsCompilerGenerated())));
Copy link
Member

Choose a reason for hiding this comment

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

How reliable is this? Could some language/compiler change in the future break it and cause a wire compatibility issue for users?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't find anything more reliable than that :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As sharplab shows, there's nothing else we can depend on, unless the runtime adds something: https://sharplab.io/#v2:EYLgtghglgdgNAExAagD4AEDMACATgUwGMB7XBbAMWOIAp0BGABmwCEJcBKAbiA=. The property itself isn't [CompilerGenerated], and I doubt anyone would manually write an accessor annotated with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW, I think the proposed heuristics is as specific and detailed as possible to detect the situation given the current compiler output.


if (primaryConstructor != null)
constructorParameters = primaryConstructor.Parameters;
}
}
}

Expand Down Expand Up @@ -273,8 +290,18 @@ bool ShouldIncludePrimaryConstructorParameters(INamedTypeSymbol t)
}
}

// Default to true for records, false otherwise.
return t.IsRecord;
// Default to true for records.
if (t.IsRecord)
return true;

var properties = t.GetMembers().OfType<IPropertySymbol>().ToImmutableArray();

return t.GetMembers()
.OfType<IMethodSymbol>()
.Where(m => m.MethodKind == MethodKind.Constructor && m.Parameters.Length > 0)
// Check for a ctor where all parameters have a corresponding compiler-generated prop.
.Any(ctor => ctor.Parameters.All(prm =>
properties.Any(prop => prop.Name.Equals(prm.Name, StringComparison.Ordinal) && prop.IsCompilerGenerated())));
}
}
}
Expand Down
30 changes: 29 additions & 1 deletion src/Orleans.CodeGenerator/CopierGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,11 +117,39 @@ static MemberDeclarationSyntax GetFieldDeclaration(GeneratedFieldDescription des
{
switch (description)
{
case FieldAccessorDescription accessor:
case FieldAccessorDescription accessor when accessor.InitializationSyntax != null:
return
FieldDeclaration(VariableDeclaration(accessor.FieldType,
SingletonSeparatedList(VariableDeclarator(accessor.FieldName).WithInitializer(EqualsValueClause(accessor.InitializationSyntax)))))
.AddModifiers(Token(SyntaxKind.PrivateKeyword), Token(SyntaxKind.StaticKeyword), Token(SyntaxKind.ReadOnlyKeyword));
case FieldAccessorDescription accessor when accessor.InitializationSyntax == null:
//[UnsafeAccessor(UnsafeAccessorKind.Method, Name = "set_Amount")]
//extern static void SetAmount(External instance, int value);
return
MethodDeclaration(
PredefinedType(Token(SyntaxKind.VoidKeyword)),
accessor.AccessorName)
.AddModifiers(Token(SyntaxKind.PrivateKeyword), Token(SyntaxKind.ExternKeyword), Token(SyntaxKind.StaticKeyword))
.AddAttributeLists(AttributeList(SingletonSeparatedList(
Attribute(IdentifierName("System.Runtime.CompilerServices.UnsafeAccessor"))
.AddArgumentListArguments(
AttributeArgument(
MemberAccessExpression(
SyntaxKind.SimpleMemberAccessExpression,
IdentifierName("System.Runtime.CompilerServices.UnsafeAccessorKind"),
IdentifierName("Method"))),
AttributeArgument(
LiteralExpression(
SyntaxKind.StringLiteralExpression,
Literal($"set_{accessor.FieldName}")))
.WithNameEquals(NameEquals("Name"))))))
.WithParameterList(
ParameterList(SeparatedList(new[]
{
Parameter(Identifier("instance")).WithType(accessor.ContainingType),
Parameter(Identifier("value")).WithType(description.FieldType)
})))
.WithSemicolonToken(Token(SyntaxKind.SemicolonToken));
default:
return FieldDeclaration(VariableDeclaration(description.FieldType, SingletonSeparatedList(VariableDeclarator(description.FieldName))))
.AddModifiers(Token(SyntaxKind.PrivateKeyword), Token(SyntaxKind.ReadOnlyKeyword));
Expand Down
12 changes: 12 additions & 0 deletions src/Orleans.CodeGenerator/FieldIdAssignmentHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,18 @@ private bool ExtractFieldIdAnnotations()
{
_symbols[member] = (id.Value, false);
}
else if (PropertyUtility.GetMatchingPrimaryConstructorParameter(prop, _constructorParameters) is { } prm)
{
id = CodeGenerator.GetId(_libraryTypes, prop);
if (id.HasValue)
{
_symbols[member] = (id.Value, true);
}
else
{
_symbols[member] = ((uint)_constructorParameters.IndexOf(prm), true);
}
}
}

if (member is IFieldSymbol field)
Expand Down
1 change: 1 addition & 0 deletions src/Orleans.CodeGenerator/Orleans.CodeGenerator.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
<DevelopmentDependency>true</DevelopmentDependency>
<IsOrleansFrameworkPart>false</IsOrleansFrameworkPart>
<EnforceExtendedAnalyzerRules>true</EnforceExtendedAnalyzerRules>
<IsRoslynComponent>true</IsRoslynComponent>
</PropertyGroup>

<ItemGroup>
Expand Down
3 changes: 2 additions & 1 deletion src/Orleans.CodeGenerator/OrleansSourceGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ public void Execute(GeneratorExecutionContext context)
return;
}

if (context.AnalyzerConfigOptions.GlobalOptions.TryGetValue("build_property.orleans_designtimebuild", out var isDesignTimeBuild)
if (!Debugger.IsAttached &&
context.AnalyzerConfigOptions.GlobalOptions.TryGetValue("build_property.orleans_designtimebuild", out var isDesignTimeBuild)
&& string.Equals("true", isDesignTimeBuild, StringComparison.OrdinalIgnoreCase))
{
return;
Expand Down
8 changes: 8 additions & 0 deletions src/Orleans.CodeGenerator/Properties/launchSettings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"profiles": {
"Roslyn": {
"commandName": "DebugRoslynComponent",
"targetProject": "..\\..\\test\\Orleans.Serialization.UnitTests\\Orleans.Serialization.UnitTests.csproj"
}
}
}
16 changes: 16 additions & 0 deletions src/Orleans.CodeGenerator/PropertyUtility.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,22 @@ public static class PropertyUtility
return GetMatchingProperty(field, field.ContainingType.GetMembers());
}

public static bool IsCompilerGenerated(this ISymbol? symbol)
=> symbol?.GetAttributes().Any(a => a.AttributeClass?.Name == "CompilerGeneratedAttribute") == true;

public static bool IsCompilerGenerated(this IPropertySymbol? property)
=> property?.GetMethod.IsCompilerGenerated() == true && property.SetMethod.IsCompilerGenerated();

public static IParameterSymbol? GetMatchingPrimaryConstructorParameter(IPropertySymbol property, IEnumerable<IParameterSymbol> constructorParameters)
{
if (!property.IsCompilerGenerated())
return null;

return constructorParameters.FirstOrDefault(p =>
string.Equals(p.Name, property.Name, StringComparison.Ordinal) &&
SymbolEqualityComparer.Default.Equals(p.Type, property.Type));
}

public static IPropertySymbol? GetMatchingProperty(IFieldSymbol field, IEnumerable<ISymbol> memberSymbols)
{
var propertyName = PropertyMatchRegex.Match(field.Name);
Expand Down
59 changes: 49 additions & 10 deletions src/Orleans.CodeGenerator/SerializerGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,11 +137,39 @@ static MemberDeclarationSyntax GetFieldDeclaration(GeneratedFieldDescription des
SingletonSeparatedList(VariableDeclarator(type.FieldName)
.WithInitializer(EqualsValueClause(TypeOfExpression(type.CodecFieldType))))))
.AddModifiers(Token(SyntaxKind.PrivateKeyword), Token(SyntaxKind.ReadOnlyKeyword));
case FieldAccessorDescription accessor:
case FieldAccessorDescription accessor when accessor.InitializationSyntax != null:
return
FieldDeclaration(VariableDeclaration(accessor.FieldType,
SingletonSeparatedList(VariableDeclarator(accessor.FieldName).WithInitializer(EqualsValueClause(accessor.InitializationSyntax)))))
.AddModifiers(Token(SyntaxKind.PrivateKeyword), Token(SyntaxKind.StaticKeyword), Token(SyntaxKind.ReadOnlyKeyword));
case FieldAccessorDescription accessor when accessor.InitializationSyntax == null:
//[UnsafeAccessor(UnsafeAccessorKind.Method, Name = "set_Amount")]
//extern static void SetAmount(External instance, int value);
return
MethodDeclaration(
PredefinedType(Token(SyntaxKind.VoidKeyword)),
accessor.AccessorName)
.AddModifiers(Token(SyntaxKind.PrivateKeyword), Token(SyntaxKind.ExternKeyword), Token(SyntaxKind.StaticKeyword))
.AddAttributeLists(AttributeList(SingletonSeparatedList(
Attribute(IdentifierName("System.Runtime.CompilerServices.UnsafeAccessor"))
.AddArgumentListArguments(
AttributeArgument(
MemberAccessExpression(
SyntaxKind.SimpleMemberAccessExpression,
IdentifierName("System.Runtime.CompilerServices.UnsafeAccessorKind"),
IdentifierName("Method"))),
AttributeArgument(
LiteralExpression(
SyntaxKind.StringLiteralExpression,
Literal($"set_{accessor.FieldName}")))
.WithNameEquals(NameEquals("Name"))))))
.WithParameterList(
ParameterList(SeparatedList(new[]
{
Parameter(Identifier("instance")).WithType(accessor.ContainingType),
Parameter(Identifier("value")).WithType(description.FieldType)
})))
.WithSemicolonToken(Token(SyntaxKind.SemicolonToken));
default:
return FieldDeclaration(VariableDeclaration(description.FieldType, SingletonSeparatedList(VariableDeclarator(description.FieldName))))
.AddModifiers(Token(SyntaxKind.PrivateKeyword), Token(SyntaxKind.ReadOnlyKeyword));
Expand Down Expand Up @@ -1069,11 +1097,16 @@ public CodecFieldTypeFieldDescription(TypeSyntax fieldType, string fieldName, Ty

internal sealed class FieldAccessorDescription : GeneratedFieldDescription
{
public FieldAccessorDescription(TypeSyntax fieldType, string fieldName, ExpressionSyntax initializationSyntax) : base(fieldType, fieldName)
=> InitializationSyntax = initializationSyntax;
public FieldAccessorDescription(TypeSyntax containingType, TypeSyntax fieldType, string fieldName, string accessorName, ExpressionSyntax initializationSyntax = null) : base(fieldType, fieldName)
{
ContainingType = containingType;
AccessorName = accessorName;
InitializationSyntax = initializationSyntax;
}

public override bool IsInjected => false;

public readonly string AccessorName;
public readonly TypeSyntax ContainingType;
public readonly ExpressionSyntax InitializationSyntax;
}

Expand Down Expand Up @@ -1325,7 +1358,6 @@ public ExpressionSyntax GetSetter(ExpressionSyntax instance, ExpressionSyntax va
return AssignmentExpression(
SyntaxKind.SimpleAssignmentExpression,
instance.Member(Field.Name),

value);
}

Expand Down Expand Up @@ -1357,20 +1389,26 @@ public ExpressionSyntax GetSetter(ExpressionSyntax instance, ExpressionSyntax va
public FieldAccessorDescription GetGetterFieldDescription()
{
if (IsGettableField || IsGettableProperty) return null;
return GetFieldAccessor(ContainingType, TypeSyntax, MemberName, GetterFieldName, LibraryTypes, false);
return GetFieldAccessor(ContainingType, TypeSyntax, MemberName, GetterFieldName, LibraryTypes, false,
IsPrimaryConstructorParameter && IsProperty);
}

public FieldAccessorDescription GetSetterFieldDescription()
{
if (IsSettableField || IsSettableProperty) return null;
return GetFieldAccessor(ContainingType, TypeSyntax, MemberName, SetterFieldName, LibraryTypes, true);
return GetFieldAccessor(ContainingType, TypeSyntax, MemberName, SetterFieldName, LibraryTypes, true,
IsPrimaryConstructorParameter && IsProperty);
}

public static FieldAccessorDescription GetFieldAccessor(INamedTypeSymbol containingType, TypeSyntax fieldType, string fieldName, string accessorName, LibraryTypes library, bool setter)
public static FieldAccessorDescription GetFieldAccessor(INamedTypeSymbol containingType, TypeSyntax fieldType, string fieldName, string accessorName, LibraryTypes library, bool setter, bool useUnsafeAccessor = false)
{
var valueType = containingType.IsValueType;
var containingTypeSyntax = containingType.ToTypeSyntax();

if (useUnsafeAccessor)
return new(containingTypeSyntax, fieldType, fieldName, accessorName);

var valueType = containingType.IsValueType;

var delegateType = (setter ? (valueType ? library.ValueTypeSetter_2 : library.Action_2) : (valueType ? library.ValueTypeGetter_2 : library.Func_2))
.ToTypeSyntax(containingTypeSyntax, fieldType);

Expand All @@ -1381,7 +1419,8 @@ public static FieldAccessorDescription GetFieldAccessor(INamedTypeSymbol contain
InvocationExpression(fieldAccessorUtility.Member(accessorMethod))
.AddArgumentListArguments(Argument(TypeOfExpression(containingTypeSyntax)), Argument(fieldName.GetLiteralExpression())));

return new(delegateType, accessorName, accessorInvoke);
// Existing case, accessor is the field in both cases
return new(containingTypeSyntax, delegateType, accessorName, accessorName, accessorInvoke);
}
}
}
Expand Down
4 changes: 4 additions & 0 deletions test/Misc/TestSerializerExternalModels/IsExternalInit.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
namespace System.Runtime.CompilerServices;

// required for record serialization support for downlevel
internal static class IsExternalInit {}
Loading
Loading