Fix support for externally declared records

[kzu]

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

Microsoft Reviewers: Open in CodeFlow

[kzu]

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]

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]

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

[ReubenBond]

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

[ReubenBond]

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

[ReubenBond]

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

[kzu]

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

[kzu]

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.

[kzu]

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

[codymullins]

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

[alrz]

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]

@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]

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]

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]

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]

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