From 9009aa79bcf6ba79ffc4a497c7d7dbfff58dd64c Mon Sep 17 00:00:00 2001 From: Meta Vens Date: Sun, 13 Sep 2026 21:19:08 +0200 Subject: [PATCH] Generate derived documents' members and require [Key] on every one The generator walked only members declared on the document class and emitted a codec only when those were dense, so a derived document's generated descriptor hashed a different schema id and catalog than the reflective one. It now discovers members along the base-type chain the way CultDocumentRegistry reflects them (public instance fields and get/set properties, inherited included, [IgnoreMember] excluded), and density is over that full set: RoutingWeapon : RoutingGear gets a codec and the same SchemaId and catalog (Name@0, Damage@1) from both builds. Unkeyed members made the two orderings disagree (generator by name, registry by metadata token). Both implicit-slot paths are deleted: an unkeyed persisted member of a [CultDocument] type is generator error GCC001, and the registry throws the same message. The consumer audit found no unkeyed document with a persisted store. Tests: GeneratedAndReflectiveDescriptorsAgreeForDerivedDocuments failed on ea5a07b ("no generated codec for the derived document") and passes through UpsertWeaponThroughGearHandleReloadsAsWeapon; UnkeyedDocumentMemberIsRejected drives the generator through Roslyn and compares its diagnostic with the registry's exception. The registry's reflective BuildDescriptor is internal and visible to the tests for that comparison. The registry-collision fixture's emitted field gains its [Key]; schema-compatibility fixture ids are unchanged. Co-Authored-By: Claude Opus 5 --- .../CultDocumentMessagePackGenerator.cs | 105 ++++++++---------- src/GameCult.Caching/CultCache.cs | 52 +++------ src/GameCult.Caching/GameCult.Caching.csproj | 1 + tests/GameCult.Caching.Tests/AssemblyInfo.cs | 1 + .../CultDocumentRegistryTests.cs | 4 +- .../GameCult.Caching.Tests.csproj | 4 + .../StoreRoutingTests.cs | 19 +++- .../UnkeyedDocumentMemberTests.cs | 66 +++++++++++ 8 files changed, 154 insertions(+), 98 deletions(-) create mode 100644 tests/GameCult.Caching.Tests/UnkeyedDocumentMemberTests.cs diff --git a/src/GameCult.Caching.MessagePack.Generator/CultDocumentMessagePackGenerator.cs b/src/GameCult.Caching.MessagePack.Generator/CultDocumentMessagePackGenerator.cs index d7ed451e..2ffe63ae 100644 --- a/src/GameCult.Caching.MessagePack.Generator/CultDocumentMessagePackGenerator.cs +++ b/src/GameCult.Caching.MessagePack.Generator/CultDocumentMessagePackGenerator.cs @@ -1,3 +1,4 @@ +#nullable enable using System; using System.Collections.Generic; using System.Collections.Immutable; @@ -49,7 +50,8 @@ public sealed class CultDocumentMessagePackGenerator : IIncrementalGenerator return null; } - var members = DiscoverMembers(typeSymbol); + var unkeyedMembers = new List(); + var members = DiscoverMembers(typeSymbol, unkeyedMembers); var nameMember = members.FirstOrDefault(member => member.IsName); var canConstructForDeserialization = typeSymbol.TypeKind == TypeKind.Struct || typeSymbol.InstanceConstructors.Any(constructor => @@ -71,68 +73,50 @@ public sealed class CultDocumentMessagePackGenerator : IIncrementalGenerator .ToImmutableArray(), members.ToImmutableArray(), canConstructForDeserialization, - hasDenseSlots); + hasDenseSlots, + unkeyedMembers.Select(name => UnkeyedMemberMessage(typeSymbol.Name, name)).ToImmutableArray(), + typeSymbol.Locations.FirstOrDefault()); } - private static ImmutableArray DiscoverMembers(INamedTypeSymbol typeSymbol) + // The same members CultDocumentRegistry reflects: public instance fields and get/set properties, inherited included. + private static ImmutableArray DiscoverMembers(INamedTypeSymbol typeSymbol, List unkeyedMembers) { - var candidates = new List(); - foreach (var member in typeSymbol.GetMembers()) + var shaped = new List(); + for (var type = typeSymbol; type != null && type.SpecialType != SpecialType.System_Object; type = type.BaseType) { - if (member is IFieldSymbol field) + foreach (var member in type.GetMembers()) { - if (field.IsStatic || field.DeclaredAccessibility != Accessibility.Public || IsIgnored(field)) - { + ITypeSymbol memberType; + if (member is IFieldSymbol field && !field.IsStatic && field.DeclaredAccessibility == Accessibility.Public) + memberType = field.Type; + else if (member is IPropertySymbol property && !property.IsStatic && !property.IsIndexer && !property.IsOverride && + property.DeclaredAccessibility == Accessibility.Public && property.GetMethod != null && property.SetMethod != null) + memberType = property.Type; + else continue; - } - candidates.Add(MemberShapeSeed.From(field, field.Type)); - continue; - } - - if (member is IPropertySymbol property) - { - if (property.IsStatic || - property.IsIndexer || - property.DeclaredAccessibility != Accessibility.Public || - property.GetMethod == null || - property.SetMethod == null || - IsIgnored(property)) - { + if (IsIgnored(member)) continue; - } - - candidates.Add(MemberShapeSeed.From(property, property.Type)); + var slot = GetExplicitSlot(member); + if (slot == null) + unkeyedMembers.Add(member.Name); + else + shaped.Add(MemberShapeSeed.From(member, memberType).WithSlot(slot.Value)); } } - var explicitMembers = candidates - .Where(member => member.ExplicitSlot.HasValue) - .OrderBy(member => member.ExplicitSlot!.Value) - .ThenBy(member => member.SortKey, StringComparer.Ordinal) - .ToArray(); - var implicitMembers = candidates - .Where(member => !member.ExplicitSlot.HasValue) - .OrderBy(member => member.SortKey, StringComparer.Ordinal) - .ToArray(); - var nextSlot = explicitMembers.Length == 0 ? 0 : explicitMembers.Max(member => member.ExplicitSlot!.Value) + 1; - var shaped = new List(candidates.Count); - - foreach (var member in explicitMembers) - { - shaped.Add(member.WithSlot(member.ExplicitSlot!.Value)); - } - - foreach (var member in implicitMembers) - { - shaped.Add(member.WithSlot(nextSlot++)); - } - - return shaped - .OrderBy(member => member.Slot) - .ToImmutableArray(); + return shaped.OrderBy(member => member.Slot).ToImmutableArray(); } + // CultDocumentRegistry.UnkeyedMemberMessage throws the same text. + private static string UnkeyedMemberMessage(string documentTypeName, string memberName) => + $"Cult document {documentTypeName} member {memberName} has no [Key]; every persisted member of a [CultDocument] type needs an explicit [Key(n)]."; + +#pragma warning disable RS2008 + private static readonly DiagnosticDescriptor UnkeyedMember = new( + "GCC001", "Cult document member has no [Key]", "{0}", "GameCult.Caching", DiagnosticSeverity.Error, isEnabledByDefault: true); +#pragma warning restore RS2008 + private static bool IsIgnored(ISymbol member) { return GetAttribute(member, "MessagePack.IgnoreMemberAttribute") != null; @@ -160,12 +144,19 @@ public sealed class CultDocumentMessagePackGenerator : IIncrementalGenerator private static void EmitProvider(SourceProductionContext context, Compilation compilation, ImmutableArray shapes) { - var documents = shapes + var shaped = shapes .Where(shape => shape != null) .GroupBy(shape => shape.DocumentTypeName, StringComparer.Ordinal) .Select(group => group.First()) .OrderBy(shape => shape.DocumentTypeName, StringComparer.Ordinal) .ToArray(); + foreach (var shape in shaped) + { + foreach (var message in shape.UnkeyedMemberMessages) + context.ReportDiagnostic(Diagnostic.Create(UnkeyedMember, shape.Location, message)); + } + + var documents = shaped.Where(shape => shape.UnkeyedMemberMessages.IsEmpty).ToArray(); if (documents.Length == 0) { @@ -425,7 +416,6 @@ public sealed class CultDocumentMessagePackGenerator : IIncrementalGenerator string name, string schemaTypeName, string typeSyntaxName, - int? explicitSlot, string sortKey, bool isName, string? indexAlias, @@ -437,7 +427,6 @@ public sealed class CultDocumentMessagePackGenerator : IIncrementalGenerator Name = name; SchemaTypeName = schemaTypeName; TypeSyntaxName = typeSyntaxName; - ExplicitSlot = explicitSlot; SortKey = sortKey; IsName = isName; IndexAlias = indexAlias; @@ -450,7 +439,6 @@ public sealed class CultDocumentMessagePackGenerator : IIncrementalGenerator public string Name { get; } public string SchemaTypeName { get; } public string TypeSyntaxName { get; } - public int? ExplicitSlot { get; } public string SortKey { get; } public bool IsName { get; } public string? IndexAlias { get; } @@ -501,7 +489,6 @@ public sealed class CultDocumentMessagePackGenerator : IIncrementalGenerator member.Name, GetSchemaTypeName(memberType), memberType.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat), - GetExplicitSlot(member), BuildAccessorStem(member), GetAttribute(member, "GameCult.Caching.CultNameAttribute") != null, indexAlias, @@ -546,8 +533,12 @@ public sealed class CultDocumentMessagePackGenerator : IIncrementalGenerator ImmutableArray indexAccessors, ImmutableArray members, bool canConstructForDeserialization, - bool hasDenseSlots) + bool hasDenseSlots, + ImmutableArray unkeyedMemberMessages, + Location? location) { + UnkeyedMemberMessages = unkeyedMemberMessages; + Location = location; DocumentTypeName = documentTypeName; SchemaName = schemaName; SchemaVersion = schemaVersion; @@ -575,6 +566,8 @@ public sealed class CultDocumentMessagePackGenerator : IIncrementalGenerator public ImmutableArray Members { get; } public bool CanConstructForDeserialization { get; } public bool HasDenseSlots { get; } + public ImmutableArray UnkeyedMemberMessages { get; } + public Location? Location { get; } } private sealed class IndexAccessorShape diff --git a/src/GameCult.Caching/CultCache.cs b/src/GameCult.Caching/CultCache.cs index 199e010f..70d8530d 100644 --- a/src/GameCult.Caching/CultCache.cs +++ b/src/GameCult.Caching/CultCache.cs @@ -481,7 +481,7 @@ namespace GameCult.Caching : BuildDescriptor(definition); } - private static CultDocumentDescriptor BuildDescriptor(Type type) + internal static CultDocumentDescriptor BuildDescriptor(Type type) { var attribute = type.GetCustomAttribute() ?? throw new InvalidOperationException( @@ -906,44 +906,26 @@ namespace GameCult.Caching foreach (var field in type.GetFields(BindingFlags.Instance | BindingFlags.Public)) { - if (IsIgnored(field)) - { - continue; - } - - members.Add(PersistedMember.FromMember(field, field.FieldType, value => field.GetValue(value), GetKeyValue(field))); + if (!IsIgnored(field)) + members.Add(PersistedMember.FromMember(field, field.FieldType, value => field.GetValue(value), RequireKey(type, field))); } foreach (var property in type.GetProperties(BindingFlags.Instance | BindingFlags.Public)) { - if (property.GetMethod == null || property.SetMethod == null || IsIgnored(property)) - { - continue; - } - - members.Add(PersistedMember.FromMember(property, property.PropertyType, value => property.GetValue(value), GetKeyValue(property))); + if (property.GetMethod != null && property.SetMethod != null && !IsIgnored(property)) + members.Add(PersistedMember.FromMember(property, property.PropertyType, value => property.GetValue(value), RequireKey(type, property))); } - var explicitMembers = members.Where(member => member.ExplicitSlot.HasValue).OrderBy(member => member.ExplicitSlot.GetValueOrDefault()).ToArray(); - var implicitMembers = members.Where(member => !member.ExplicitSlot.HasValue).OrderBy(member => member.MetadataToken).ToArray(); - var assigned = new List(members.Count); - var nextSlot = explicitMembers.Length == 0 ? 0 : explicitMembers.Max(member => member.ExplicitSlot.GetValueOrDefault()) + 1; - - foreach (var member in explicitMembers) - { - member.Slot = member.ExplicitSlot.GetValueOrDefault(); - assigned.Add(member); - } - - foreach (var member in implicitMembers) - { - member.Slot = nextSlot++; - assigned.Add(member); - } - - return assigned.OrderBy(member => member.Slot).ToArray(); + return members.OrderBy(member => member.Slot).ToArray(); } + // The generator reports the same text as diagnostic GCC001. + internal static string UnkeyedMemberMessage(string documentTypeName, string memberName) => + $"Cult document {documentTypeName} member {memberName} has no [Key]; every persisted member of a [CultDocument] type needs an explicit [Key(n)]."; + + private static int RequireKey(Type documentType, MemberInfo member) => + GetKeyValue(member) ?? throw new InvalidOperationException(UnkeyedMemberMessage(documentType.Name, member.Name)); + private static bool IsIgnored(MemberInfo member) { return member.GetCustomAttributes().Any(attribute => @@ -988,8 +970,6 @@ namespace GameCult.Caching { public MemberInfo Member { get; set; } = default!; public Type MemberType { get; set; } = default!; - public int MetadataToken { get; set; } - public int? ExplicitSlot { get; set; } public int Slot { get; set; } public bool IsName { get; set; } public string? IndexAlias { get; set; } @@ -1003,7 +983,7 @@ namespace GameCult.Caching MemberInfo member, Type memberType, Func getValue, - int? explicitSlot) + int slot) { var referenceAttribute = member.GetCustomAttribute(); var targetType = ResolveReferenceTarget(memberType, referenceAttribute?.TargetType); @@ -1012,9 +992,7 @@ namespace GameCult.Caching { Member = member, MemberType = memberType, - MetadataToken = member.MetadataToken, - ExplicitSlot = explicitSlot, - Slot = explicitSlot ?? -1, + Slot = slot, IsName = member.GetCustomAttribute() != null, IndexAlias = ResolveIndexAlias(member), IsReference = targetType != null || referenceAttribute != null, diff --git a/src/GameCult.Caching/GameCult.Caching.csproj b/src/GameCult.Caching/GameCult.Caching.csproj index 68c4b198..12616c6a 100644 --- a/src/GameCult.Caching/GameCult.Caching.csproj +++ b/src/GameCult.Caching/GameCult.Caching.csproj @@ -7,5 +7,6 @@ + \ No newline at end of file diff --git a/tests/GameCult.Caching.Tests/AssemblyInfo.cs b/tests/GameCult.Caching.Tests/AssemblyInfo.cs index d6821542..f3a1dfa1 100644 --- a/tests/GameCult.Caching.Tests/AssemblyInfo.cs +++ b/tests/GameCult.Caching.Tests/AssemblyInfo.cs @@ -1,3 +1,4 @@ +#nullable enable using GameCult.Caching.MessagePack; using GameCult.Caching.Tests; using MessagePack; diff --git a/tests/GameCult.Caching.Tests/CultDocumentRegistryTests.cs b/tests/GameCult.Caching.Tests/CultDocumentRegistryTests.cs index 1a1e83a3..d980db3e 100644 --- a/tests/GameCult.Caching.Tests/CultDocumentRegistryTests.cs +++ b/tests/GameCult.Caching.Tests/CultDocumentRegistryTests.cs @@ -142,7 +142,9 @@ public sealed class CultDocumentRegistryTests new object[] { schemaName, schemaVersion })); if (addStringField) { - type.DefineField("Value", typeof(string), FieldAttributes.Public); + type.DefineField("Value", typeof(string), FieldAttributes.Public).SetCustomAttribute(new CustomAttributeBuilder( + typeof(global::MessagePack.KeyAttribute).GetConstructor(new[] { typeof(int) })!, + new object[] { 0 })); } return type.CreateType()!; diff --git a/tests/GameCult.Caching.Tests/GameCult.Caching.Tests.csproj b/tests/GameCult.Caching.Tests/GameCult.Caching.Tests.csproj index 4b3d92d3..31d33a30 100644 --- a/tests/GameCult.Caching.Tests/GameCult.Caching.Tests.csproj +++ b/tests/GameCult.Caching.Tests/GameCult.Caching.Tests.csproj @@ -4,6 +4,8 @@ net10.0 false GameCult.Caching.Tests + + $(NoWarn);RS1036;RS1041 @@ -12,6 +14,8 @@ + + diff --git a/tests/GameCult.Caching.Tests/StoreRoutingTests.cs b/tests/GameCult.Caching.Tests/StoreRoutingTests.cs index 205a0c99..9598d4d8 100644 --- a/tests/GameCult.Caching.Tests/StoreRoutingTests.cs +++ b/tests/GameCult.Caching.Tests/StoreRoutingTests.cs @@ -71,6 +71,21 @@ namespace GameCult.Caching.Tests Assert.That(((RoutingWeapon)loaded!).Name, Is.EqualTo("lance"), "the inherited member did not round-trip"); } + [Test] + public async Task GeneratedAndReflectiveDescriptorsAgreeForDerivedDocuments() + { + var generated = CultDocumentRegistry.ForTypes(new[] { typeof(RoutingWeapon) }).GetRequired(); + var reflective = CultDocumentRegistry.BuildDescriptor(typeof(RoutingWeapon)); + + Assert.That(generated.GeneratedPayloadSerializer, Is.Not.Null, "no generated codec for the derived document"); + Assert.That(generated.GeneratedPayloadDeserializer, Is.Not.Null); + Assert.That(generated.SchemaId, Is.EqualTo(reflective.SchemaId)); + Assert.That(generated.CanonicalSchemaJson, Is.EqualTo(reflective.CanonicalSchemaJson)); + Assert.That(generated.Members.Select(member => $"{member.MemberName}@{member.Slot}"), Is.EqualTo(new[] { "Name@0", "Damage@1" })); + Assert.That(reflective.Members.Select(member => $"{member.MemberName}@{member.Slot}"), Is.EqualTo(new[] { "Name@0", "Damage@1" })); + await UpsertWeaponThroughGearHandleReloadsAsWeapon(); + } + [Test] public async Task GetByNameMatchesAssignableTypes() { @@ -728,10 +743,7 @@ namespace GameCult.Caching.Tests public string Text = string.Empty; } - // RoutingWeapon's own slots are not dense, so it gets no generated codec and serializes through - // MessagePack's dynamic resolver, which needs a public [MessagePackObject]. [CultDocument("tests.routing_gear", "tests.routing_gear.v1")] - [MessagePackObject] public class RoutingGear { [Key(0)] @@ -739,7 +751,6 @@ namespace GameCult.Caching.Tests } [CultDocument("tests.routing_weapon", "tests.routing_weapon.v1")] - [MessagePackObject] public sealed class RoutingWeapon : RoutingGear { [Key(1)] diff --git a/tests/GameCult.Caching.Tests/UnkeyedDocumentMemberTests.cs b/tests/GameCult.Caching.Tests/UnkeyedDocumentMemberTests.cs new file mode 100644 index 00000000..b0fb5b3d --- /dev/null +++ b/tests/GameCult.Caching.Tests/UnkeyedDocumentMemberTests.cs @@ -0,0 +1,66 @@ +#nullable enable +using System; +using System.IO; +using System.Linq; +using System.Reflection; +using System.Reflection.Emit; +using GameCult.Caching; +using GameCult.Caching.MessagePack.Generator; +using MessagePack; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using NUnit.Framework; + +namespace GameCult.Caching.Tests +{ + public class UnkeyedDocumentMemberTests + { + [Test] + public void UnkeyedDocumentMemberIsRejected() + { + var reflective = Assert.Throws(() => CultDocumentRegistry.ForTypes(new[] { EmitUnkeyedDocument() }))!; + + const string source = @" +using GameCult.Caching; +using MessagePack; +[CultDocument(""tests.unkeyed"", ""tests.unkeyed.v1"")] +public class UnkeyedDocument +{ + [Key(0)] public string Name = string.Empty; + public int Count; +}"; + var references = ((string)AppContext.GetData("TRUSTED_PLATFORM_ASSEMBLIES")!) + .Split(Path.PathSeparator) + .Append(typeof(CultDocumentAttribute).Assembly.Location) + .Append(typeof(KeyAttribute).Assembly.Location) + .Distinct(StringComparer.OrdinalIgnoreCase) + .Select(path => MetadataReference.CreateFromFile(path)); + var compilation = CSharpCompilation.Create( + "UnkeyedConsumer", + new[] { CSharpSyntaxTree.ParseText(source) }, + references, + new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary)); + CSharpGeneratorDriver.Create(new CultDocumentMessagePackGenerator()) + .RunGeneratorsAndUpdateCompilation(compilation, out _, out var diagnostics); + + var generated = diagnostics.Single(diagnostic => diagnostic.Id == "GCC001"); + Assert.That(generated.Severity, Is.EqualTo(DiagnosticSeverity.Error)); + Assert.That(generated.GetMessage(), Is.EqualTo(reflective.Message)); + Assert.That(reflective.Message, Does.Contain("UnkeyedDocument member Count has no [Key]")); + } + + private static Type EmitUnkeyedDocument() + { + var module = AssemblyBuilder.DefineDynamicAssembly(new AssemblyName("UnkeyedDocuments"), AssemblyBuilderAccess.Run) + .DefineDynamicModule("UnkeyedDocuments"); + var type = module.DefineType("UnkeyedDocument", TypeAttributes.Public | TypeAttributes.Class); + type.SetCustomAttribute(new CustomAttributeBuilder( + typeof(CultDocumentAttribute).GetConstructor(new[] { typeof(string), typeof(string) })!, + new object[] { "tests.unkeyed", "tests.unkeyed.v1" })); + type.DefineField("Name", typeof(string), FieldAttributes.Public) + .SetCustomAttribute(new CustomAttributeBuilder(typeof(KeyAttribute).GetConstructor(new[] { typeof(int) })!, new object[] { 0 })); + type.DefineField("Count", typeof(int), FieldAttributes.Public); + return type.CreateType()!; + } + } +}