Fix support for externally declared records #9104

kzu · 2024-08-06T19:00:55Z

Fixes improper codec codegen for records declared in referenced projects/assemblies. Roslyn does not guarantee the symbols contain the backing fields for generated properties (see dotnet/roslyn#72374 (comment)) and it also doesn't even report record struct symbols as records at all (see dotnet/roslyn#69326).

This makes for a very inconsistent experience when dealing with types defined in external assemblies that don't use the Orleans SDK themselves.

We implement a heuristics here to determine primary constructors that can be relied upon to detect them consistently:

A ctor with non-zero parameters
All parameters match by name exactly with corresponding properties
All matching properties have getter AND setter annotated with [CompilerGenerated].

In addition, since the backing field isn't available at all in these records, and the corresponding property isn't settable (it's generated as init set), we leverage unsafe accessors (see https://learn.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.unsafeaccessorattribute?view=net-8.0) instead. The code checks whether the FieldAccessorDescription has an initializer syntax or not to determine whether to generate the original code or the new accessor version.

The signature of the accessor matches the delegate that is generated for the regular backing field case, so there is no need to modify other call sites.

Fixes #9092

Microsoft Reviewers: Open in CodeFlow

kzu · 2024-08-06T19:04:58Z

There's a bit of code duplication in the PR, but I tried to keep it in line with that was already there (i.e. the field accessor generation is also duplicated for the original use case too), and I didn't find a generic "SymbolExtensions" to centralize the primary constructor lookup, for example.

Let me know if I should seek to unify that and the existing code a bit too.

kzu · 2024-08-06T20:36:59Z

Failed test seems unrelated:

[xUnit.net 00:00:09.23]     UnitTests.StuckGrainTests.StuckGrainTests.StuckGrainTest_StuckDetectionOnDeactivation [FAIL]

It runs fine on my machine 🤔

Perhaps use dotnet-retest 😉

kzu · 2024-08-17T22:51:26Z

Any updates on this @ReubenBond? This is blocking a release of my cloud actors thingy :)

ReubenBond · 2024-08-20T19:14:47Z

I'll review this asap and see when we can get a release out

ReubenBond · 2024-08-27T21:54:19Z

I pushed a failing (#ifdef'd out) test for the generic case, which I believe is fixed in 9.0: dotnet/runtime#99468

ReubenBond · 2024-08-27T21:55:59Z

src/Orleans.CodeGenerator/CodeGenerator.cs

+                                            .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 =>
+                                                properties.Any(prop => prop.Name.Equals(prm.Name, StringComparison.Ordinal) && prop.IsCompilerGenerated())));


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

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

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.

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

src/Orleans.CodeGenerator/Model/SerializableTypeDescription.cs

FIxes improper codec codegen for records declared in referenced projects/assemblies. Roslyn does not guarantee the symbols contain the backing fields for generated properties (see dotnet/roslyn#72374 (comment)) and it also doesn't even report `record struct` symbols as records at all (see dotnet/roslyn#69326). This makes for a very inconsistent experience when dealing with types defined in external assemblies that don't use the Orleans SDK themselves. We implement a heuristics here to determine primary constructors that can be relied upon to detect them consistently: 1. A ctor with non-zero parameters 2. All parameters match by name exactly with corresponding properties 3. All matching properties have getter AND setter annotated with [CompilerGenerated]. In addition, since the backing field isn't available at all in these records, and the corresponding property isn't settable (it's generated as `init set`), we leverage unsafe accessors (see https://learn.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.unsafeaccessorattribute?view=net-8.0) instead. The code checks whether the `FieldAccessorDescription` has an initializer syntax or not to determine whether to generate the original code or the new accessor version. The signature of the accessor matches the delegate that is generated for the regular backing field case, so there is no need to modify other call sites. Fixes #9092

In externally defined symbols, we need to also check for `[CompilerGenerated]`.

This allows running F5 on the generator project and troubleshoot issues. By checking for an already attached debugger, we can debug also scenarios of code generation where the roslyn component starts it with DTB=true.

codymullins · 2024-09-03T15:15:05Z

@ReubenBond do you foresee this PR being an issue to merge?

alrz · 2024-09-04T00:59:49Z

src/Orleans.CodeGenerator/CodeGenerator.cs

+                                            .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 =>


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 );

ReubenBond · 2024-09-04T15:57:01Z

@ReubenBond do you foresee this PR being an issue to merge?

@codymullins no, I just need to run more tests to give myself confidence that this will not accidentally break backwards compatibility. I'll try to get to that today.

kzu · 2024-09-10T10:10:35Z

To add to the argument for merging this PR: the externally defined records that are now considered for generation would have previously been entirely omitted, causing runtime errors. So I think the risk is virtually non existent?

alrz · 2024-09-16T11:48:28Z

I hit this today, it occurred to me that GenerateSerializer already validates the type if it's applicable (if I'm not mistaken, that means either Id is present or it's a record). Why is there a need to validate externally declared types as well? Furthermore, my impression was that GenerateSerializer will actually do the generation as well in the current assembly and then a client will use those?

kzu · 2024-09-17T00:37:07Z

If the type is externally defined only, and the client is generic and does not do codegen itself, you would face this issue, since the serializers won't exist in the assembly where the records are defined.

alrz · 2024-09-17T08:10:43Z

client is generic and does not do codegen itself

Since it already references orleans bits for GenerateSerializer attribute, wouldn't it make sense to also do the codegen right there? That is, include the generator inside Abstractions package.

See https://github.com/dotnet/extensions/blob/main/src/Libraries/Microsoft.Extensions.Telemetry.Abstractions/Microsoft.Extensions.Telemetry.Abstractions.csproj for another instance of this pattern.

If that's too much of a change I'd argue at least the validation should happen via an analyzer once the attribute is applied so you don't need to validate externally - the presence of the attribute is effectively a guarantee.

(This is also generally how language features expose their contract for example for NRT, the compiler validates on declaration only, then reads the medatada on the client side, there's no concept of external validation because you wouldn't have enough info to do it after the fact which I think is the core issue here.)

kzu · 2024-09-20T14:49:02Z

That's a different issue. The [GenerateSerializer] attribute exists in the abstractions assembly. It does not require that users reference the orleans SDK. This a nice thing in my scenario (https://github.com/devlooped/CloudActors/) since I want the business logic assembly to remain Orleans agnostic (save for that attribute, which I even source-generate to a partial class so users don't even have to use it, ideally).

If that was not an explicit goal/supported scenario, then there would be no reason for the attribute to be separate from the Orleans SDK/generator itself, since using one would require using the other.

ReubenBond · 2024-09-30T17:52:02Z

Apologies for the hiatus, my newborn arrived early, so I've had my hands full. I pushed a couple of additional changes:

Add a test for an external readonly record struct
Remove the Where(x => x.IsImplicitlyDeclared) on the ctor. I diff'd the codegen output and this one stuck out: it caused some default-constructable types to no longer be default-constructable. Tests all pass with this deleted

I'm ready to merge when CI completes.

kzu · 2024-09-30T18:24:59Z

Congrats!! 👶 rocks ❤️

ReubenBond · 2024-10-01T21:59:54Z

Many thanks, @kzu!

kzu · 2024-10-03T19:02:11Z

Would be awesome to know the milestone this will ship with, if you happen to know, to track when I can start using it 🙏

kzu mentioned this pull request Aug 6, 2024

Issue with generated codec for referenced project record message type #9092

Closed

kzu force-pushed the external-record-serializer branch from f85d424 to d2fb2f0 Compare August 6, 2024 19:25

kzu force-pushed the external-record-serializer branch 2 times, most recently from 8527186 to 58a972b Compare August 17, 2024 22:50

ReubenBond force-pushed the external-record-serializer branch from 58a972b to 60aedf2 Compare August 20, 2024 19:32

ReubenBond self-assigned this Aug 21, 2024

ReubenBond reviewed Aug 27, 2024

View reviewed changes

src/Orleans.CodeGenerator/Model/SerializableTypeDescription.cs Outdated Show resolved Hide resolved

kzu added 3 commits August 28, 2024 15:19

Skip compiler-generated ctors in records

9e64309

In externally defined symbols, we need to also check for `[CompilerGenerated]`.

Ignore design-time build value when debugging generator

7b48cc2

This allows running F5 on the generator project and troubleshoot issues. By checking for an already attached debugger, we can debug also scenarios of code generation where the roslyn component starts it with DTB=true.

kzu force-pushed the external-record-serializer branch from a4976d6 to 28ffda8 Compare August 28, 2024 18:19

kzu mentioned this pull request Sep 2, 2024

Separate package for interfaces? devlooped/CloudActors#74

Closed

alrz reviewed Sep 4, 2024

View reviewed changes

ReubenBond force-pushed the external-record-serializer branch from 28ffda8 to f0ca3a4 Compare September 30, 2024 17:36

Add additional test cases for generic & readonly scenario

050c1c9

ReubenBond force-pushed the external-record-serializer branch from f0ca3a4 to 050c1c9 Compare September 30, 2024 17:36

Remove IsImplicitlyDeclared check

e919b73

ReubenBond approved these changes Sep 30, 2024

View reviewed changes

ReubenBond merged commit ae262c8 into dotnet:main Oct 1, 2024
22 checks passed

kzu deleted the external-record-serializer branch October 3, 2024 19:01

github-actions bot locked and limited conversation to collaborators Nov 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix support for externally declared records #9104

Fix support for externally declared records #9104

kzu commented Aug 6, 2024 •

edited by dotnet-policy-service bot

Loading

kzu commented Aug 6, 2024

kzu commented Aug 6, 2024 •

edited

Loading

kzu commented Aug 17, 2024

ReubenBond commented Aug 20, 2024 •

edited

Loading

ReubenBond commented Aug 27, 2024

ReubenBond Aug 27, 2024

kzu Aug 27, 2024

kzu Aug 27, 2024

kzu Aug 28, 2024

codymullins commented Sep 3, 2024

alrz Sep 4, 2024

ReubenBond commented Sep 4, 2024

kzu commented Sep 10, 2024

alrz commented Sep 16, 2024

kzu commented Sep 17, 2024

alrz commented Sep 17, 2024 •

edited

Loading

kzu commented Sep 20, 2024 •

edited

Loading

ReubenBond commented Sep 30, 2024 •

edited

Loading

kzu commented Sep 30, 2024

ReubenBond commented Oct 1, 2024

kzu commented Oct 3, 2024

Fix support for externally declared records #9104

Fix support for externally declared records #9104

Conversation

kzu commented Aug 6, 2024 • edited by dotnet-policy-service bot Loading

Microsoft Reviewers: Open in CodeFlow

kzu commented Aug 6, 2024

kzu commented Aug 6, 2024 • edited Loading

kzu commented Aug 17, 2024

ReubenBond commented Aug 20, 2024 • edited Loading

ReubenBond commented Aug 27, 2024

ReubenBond Aug 27, 2024

Choose a reason for hiding this comment

kzu Aug 27, 2024

Choose a reason for hiding this comment

kzu Aug 27, 2024

Choose a reason for hiding this comment

kzu Aug 28, 2024

Choose a reason for hiding this comment

codymullins commented Sep 3, 2024

alrz Sep 4, 2024

Choose a reason for hiding this comment

ReubenBond commented Sep 4, 2024

kzu commented Sep 10, 2024

alrz commented Sep 16, 2024

kzu commented Sep 17, 2024

alrz commented Sep 17, 2024 • edited Loading

kzu commented Sep 20, 2024 • edited Loading

ReubenBond commented Sep 30, 2024 • edited Loading

kzu commented Sep 30, 2024

ReubenBond commented Oct 1, 2024

kzu commented Oct 3, 2024

kzu commented Aug 6, 2024 •

edited by dotnet-policy-service bot

Loading

kzu commented Aug 6, 2024 •

edited

Loading

ReubenBond commented Aug 20, 2024 •

edited

Loading

alrz commented Sep 17, 2024 •

edited

Loading

kzu commented Sep 20, 2024 •

edited

Loading

ReubenBond commented Sep 30, 2024 •

edited

Loading