Issue with generated codec for referenced project record message type #9092
Comments
Related codegen is also moved there, with a detection for the project having the server package installed. Due to an issue with Orleans codec generation (see dotnet/orleans#9092), end to end tests are failing since state is not being properly serialized for actor messages. We can only ship this rework once that's fixed. Otherwise, we'll need to move serialization codegen back to the actor/domain project (which unfortunately brings a larger dependency on Orleans, which is undesirable).
Related codegen is also moved there, with a detection for the project having the server package installed. Due to an issue with Orleans codec generation (see dotnet/orleans#9092), end to end tests are failing since state is not being properly serialized for actor messages. We can only ship this rework once that's fixed. Otherwise, we'll need to move serialization codegen back to the actor/domain project (which unfortunately brings a larger dependency on Orleans, which is undesirable).
|
That looks precisely it! Thanks for the quick reply 🙏 |
|
Ok @ReubenBond I think I found the root cause: dotnet/roslyn#74634 |
|
And the actual root cause might be that (at least in .NET SDK 9.0), the library produces a reference assembly by default and that's what passed to the generator, which obviously has no private impl. details including the backing fields. |
|
There is probably not much which can be done about that, other than perhaps suppressing ref assemblies in the project with codegen: dotnet/roslyn#72374 (comment) |
|
Turns out that doesn't solve it either: kzu/BackingFieldRecordSymbolMissing@a2f9b6c It has to be something else then, but I'm puzzled what it can be! |
|
Ok, so how about something like this: 1 - A user that references the SerializationAbstractions but NOT the codegen/SDK, can be assumed to be someone who will use the serialization annotations but not the codegen in that project itself, which will cause them to run into this very issue.
The only drawback I see to this approach is that it would emit an unused field in the record in the otherwise pretty pristine "messages/contracts" project. But not much else. Instead of having a half-broken support for Thoughts? PS: tried creating a fake IFieldSymbol too but it seems even trickier. |
|
Perhaps we can find a clean/sure-fire way to detect that the target is a record type, find the primary constructor, and create unsafe accessors for the properties since records expose their ctor params as init-only properties. Eg, this code allows us to set var x = new External(45);
SetAmount(x, 35);
Console.WriteLine(x.Amount);
[UnsafeAccessor(UnsafeAccessorKind.Method, Name = "set_Amount")]
extern static void SetAmount(External instance, int value); |
|
That sounds much more solid, yeah! |
|
Shocking, Roslyn isn't buying my fake symbol 😅
|
|
@ReubenBond should I give this a try or are you going to work on it soon-ish? |
|
Please give it a go if you can. I can assist if you hit any issues. Thank you, @kzu! |
|
ok, I've spent some time understanding how things currently work in the generator. Do you want to keep the existing field accessor approach or should it be replaced wholesale with the new "unsafe accessor"? |
|
Hey @ReubenBond, I've got a question regarding including primary ctor parameters. The For the following record struct, however, that property returns public record struct Person2ExternalStruct(int Age, string Name)This seems to be "by design" (however inconsistent) in roslyn. I've verified this to be the case indeed. This means that the provided failing test still fails even if a (non-struct) record I added now does work with my changes. I think Orleans should try to hide these generator/roslyn inconsistencies and have a unifying approach in either case. I think the following heuristics for determining a ctor is a primary ctor should be very reliable:
#3 in particular is key to discarding a constructor with an argument which by chance happens to have the same name as a property. Thoughts? |
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 dotnet#9092
|
I think I got a potential fix 💪 #9104 |
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 dotnet#9092
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 dotnet#9092
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 dotnet#9092
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 dotnet#9092
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 dotnet#9092
Problem: generated serializer in server project for a referenced assembly opted-in to codegen via
[assembly: GenerateCodeForDeclaringAssembly]fails to generate proper codec.Repro solution at https://github.com/kzu/OrleansCodeGenIssue
Copy of the readme from there:
Solution contains:
Models: a class library project using only the
Microsoft.Orleans.Serialization.Abstractionspackageso record types used in grains messages can be annotated with
[GenerateSerializer]. This is intendedas a contracts assembly, so we want to keep the Orleans dependencies to a minimum.
Hosting Orleans project: this contains the grain and full codegen.
The grain implements two strategies (methods) that showcase the issue (which is a codegen one):
Deposit(Deposit message): the message type is a record in a referenced project, annotated withwith
[GenerateSerializer]. The type/assembly is referenced and opted-in for referenced assemblycodegen via
[assembly: GenerateCodeForDeclaringAssembly(typeof(Deposit))]Deposit2(Deposit2 message): the message type is a record in the same project as the grain,also annotated with
[GenerateSerializer].Other than the declaring project, there is no difference between the two.
Reproduce the bug:
Note how it's always an empty response.
Every refresh appends more to the balance, which is the correct response.
After hitting 3., you can go back to 2. and see that what you get is the last balance updated by
Deposit2(since there is only one state, to eliminate issues with state persistence). But you can never increment.
The generated codec for one in-project vs the referenced one differs as follows:
The referenced type is not writing the Amount value at all:
The text was updated successfully, but these errors were encountered: