orleans
orleans copied to clipboard
dotnet
Fix support for externally declared records
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 https://github.com/dotnet/roslyn/discussions/72374#discussioncomment-8655916) and it also doesn't even report record struct symbols as records at all (see https://github.com/dotnet/roslyn/issues/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
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.
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 😉
Any updates on this @ReubenBond? This is blocking a release of my cloud actors thingy :)
I pushed a failing (#ifdef'd out) test for the generic case, which I believe is fixed in 9.0: https://github.com/dotnet/runtime/pull/99468
@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.
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?
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?
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.
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.)
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.
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.
Would be awesome to know the milestone this will ship with, if you happen to know, to track when I can start using it 🙏