Add NativeAOT and trimming support - #331
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds an incremental source generator for reflection-free model and collection decoding. It adds runtime registration support, NativeAOT projects and tests, packaging integration, diagnostics, documentation, and cross-platform CI validation. ChangesNativeAOT source-generated decoding
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Reader
participant MaxMindDbSourceGenerator
participant SourceGeneratorSupport
participant Decoder
participant NativeAotApp
Reader->>MaxMindDbSourceGenerator: Discover models and Find/FindAll roots
MaxMindDbSourceGenerator->>SourceGeneratorSupport: Emit type, collection, and dictionary registrations
NativeAotApp->>Decoder: Decode MMDB data
Decoder->>SourceGeneratorSupport: Retrieve generated metadata and factories
SourceGeneratorSupport-->>Decoder: Return activators and collection handlers
Decoder-->>NativeAotApp: Return decoded models and collections
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@MaxMind.Db.NativeAot/App/Program.cs`:
- Around line 13-14: Rename the private static fields DecoderAddress and
CityAddress to _decoderAddress and _cityAddress, respectively, and update every
reference to use the new underscore-prefixed names.
In `@MaxMind.Db.NativeAot/Models/MaxMind.Db.NativeAot.Models.csproj`:
- Around line 3-10: Both NativeAOT project files omit required code-quality
properties. In MaxMind.Db.NativeAot/Models/MaxMind.Db.NativeAot.Models.csproj
lines 3-10 and MaxMind.Db.NativeAot/App/MaxMind.Db.NativeAot.App.csproj lines
3-16, add EnforceCodeStyleInBuild, EnableNETAnalyzers, and AnalysisLevel set to
latest alongside TreatWarningsAsErrors.
In `@MaxMind.Db.SourceGenerator.Test/MaxMind.Db.SourceGenerator.Test.csproj`:
- Around line 3-10: Update the PropertyGroup in MaxMind.Db.SourceGenerator.Test
to remove EnableMSTestRunner and retain the xunit.v3-specific runner property
already expected by the project. Add the required code-enforcement analyzer
properties alongside the existing warning and nullable settings, using the
repository’s established analyzer configuration names and values.
In `@MaxMind.Db.SourceGenerator.Test/MaxMindDbSourceGeneratorTest.cs`:
- Around line 131-195: Add coverage to the source-generator tests for both
unhandled collection shapes: create a ReportsValueTypeCollectionForAot test
using ImmutableArray<long> with AOT diagnostics enabled and assert MMDBSG008,
and create an AllowsByteArrayWithoutDiagnostic test using byte[] that asserts
MMDBSG008 is absent and there are no errors. Keep these tests alongside
GeneratesCollectionFactoriesAndAddDelegates and use RunGenerator consistently.
In `@MaxMind.Db.SourceGenerator/AnalyzerReleases.Shipped.md`:
- Line 1: Add MaxMind.Db.SourceGenerator/AnalyzerReleases.*.md to both the
markdownlint and Prettier ignore configurations, preserving the fixed analyzer
release-file format and excluding both shipped and unshipped files from linting
and formatting.
In `@MaxMind.Db.SourceGenerator/AnalyzerReleases.Unshipped.md`:
- Around line 1-14: Add MaxMind.Db.SourceGenerator/AnalyzerReleases.*.md to both
the Prettier and markdownlint ignore configuration so the fixed-format release
files remain unchanged. Apply this for
MaxMind.Db.SourceGenerator/AnalyzerReleases.Unshipped.md lines 1-14 and
MaxMind.Db.SourceGenerator/AnalyzerReleases.Shipped.md line 1; preserve the
Shipped file’s leading semicolon comment.
In `@MaxMind.Db.SourceGenerator/CollectionParser.cs`:
- Around line 10-12: Extract the duplicated TypeDisplayFormat, DisplayType, and
accessibility logic from CollectionParser and ModelParser into a shared internal
SymbolHelpers class. Update both parsers to use these helpers, and make the
shared internal-symbol check require ContainingAssembly to equal
compilation.Assembly, preserving public accessibility behavior.
- Around line 201-211: Update CanConstruct to reject value types before checking
constructor accessibility, so only reference-type collections can produce a
supported construction spec. Preserve the existing abstract, accessibility, and
parameterless-constructor checks for eligible reference types, allowing
value-type collections to fall through to CollectionParseResult.Unsupported and
MMDBSG008.
In `@MaxMind.Db.SourceGenerator/MaxMind.Db.SourceGenerator.csproj`:
- Around line 3-11: Update the PropertyGroup in the source-generator project to
add EnforceCodeStyleInBuild and EnableNETAnalyzers set to true, set
AnalysisLevel to latest, and change LangVersion to 14.0. If LangVersion must
remain 12.0 for Roslyn 4.8.0 compatibility, retain it only with a clear
project-file comment documenting that requirement.
- Around line 14-15: Update the Microsoft.CodeAnalysis.CSharp and
Microsoft.CodeAnalysis.Analyzers PackageReference entries in
MaxMind.Db.SourceGenerator.csproj to set ExcludeAssets="runtime", while
preserving their existing versions and PrivateAssets="all" settings.
In `@MaxMind.Db.SourceGenerator/MaxMindDbSourceGenerator.cs`:
- Around line 110-148: The generated source currently uses platform-dependent
line endings. In MaxMind.Db.SourceGenerator/MaxMindDbSourceGenerator.cs lines
110-148, update the generation flow around the source StringBuilder and
RenderRegistration/RenderCollectionRegistration calls to append a fixed "\n" for
every generated line, including helper output. In
MaxMind.Db.SourceGenerator.Test/MaxMindDbSourceGeneratorTest.cs line 99, no
direct change is required once generation is normalized; keep the assertion
using "\n".
- Around line 60-96: Update Generate’s candidate-type and member-analysis loops
to observe context.CancellationToken and stop promptly when cancellation is
requested. Check the token before processing each candidate and member,
preserving the existing parsing and CollectionParser.Collect behavior when
generation remains active.
- Around line 22-46: Refactor the incremental input setup in the generator
initialization around `types`, `generationInput`, and `Generate` to avoid
caching `INamedTypeSymbol` and `Compilation` values. Discover model declarations
with `ForAttributeWithMetadataName`, resolve required well-known types within
the transform, and pass equatable, symbol-free model metadata through the
pipeline. Remove `TypeSpec.TypeSymbol` and `MemberSpec.TypeSymbol` from cached
records and update downstream transforms/source registration to resolve or use
type information only at generation time.
- Around line 115-126: Update the source-generation path around the emitted
ModuleInitializerAttribute to enforce or clearly document a C# 9.0-or-newer
LangVersion requirement, ensuring generated module initialization is not
silently omitted for C# 8 consumers. If the project supports C# 8, replace this
module-initializer approach with an initialization mechanism compatible with
that language version.
- Around line 233-261: Update ModelParser.ParseProperties to inspect accessible
property symbols marked IsRequired and report a diagnostic whenever the property
model cannot collect and assign that member, including required inherited
properties when no annotated members are present. Add a dedicated diagnostic
descriptor in Diagnostics.cs and document it in AnalyzerReleases.Unshipped.md.
Ensure valid required properties that are collected remain unchanged.
In `@MaxMind.Db.SourceGenerator/ModelParser.cs`:
- Around line 168-184: Update CreateMember to read MapKeyAttribute values from
NamedArguments when the corresponding positional constructor argument is absent,
covering both Name and AlwaysCreate while preserving positional-argument
precedence. Use the attribute-data helpers already used by ModelParser and
ensure repeated attributes such as [MapKey("city", AlwaysCreate = true),
MapKey(AlwaysCreate = true)] resolve the same member shape as reflection.
In `@MaxMind.Db/SourceGeneratorSupport.cs`:
- Around line 26-47: Add XML documentation exception tags to RegisterType,
RegisterCollection, and RegisterDictionary for every exception they can throw:
document ArgumentNullException for each method and ArgumentException for
RegisterType. Keep the existing summary and parameter documentation unchanged.
- Around line 103-111: Update the registration logic in the visible registration
method and the corresponding RegisterCollection and RegisterDictionary methods
so duplicate type registrations are detected instead of silently ignored.
Compare the existing and incoming registration metadata, and throw or log when
they differ while preserving idempotent behavior for equivalent registrations.
In `@releasenotes.md`:
- Around line 3-15: Complete the 5.2.0 entry in the release notes by replacing
YYYY-MM-DD with the actual release date and adding the responsible author and
GitHub issue number alongside the existing feature descriptions, preserving the
required version/date, feature, author, and issue metadata format.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e630260c-8005-4b12-bc46-ed1dfab5487b
📒 Files selected for processing (30)
.github/workflows/test.ymlMaxMind.Db.Benchmark/MaxMind.Db.Benchmark.csprojMaxMind.Db.NativeAot/App/MaxMind.Db.NativeAot.App.csprojMaxMind.Db.NativeAot/App/Program.csMaxMind.Db.NativeAot/Models/MaxMind.Db.NativeAot.Models.csprojMaxMind.Db.NativeAot/Models/Models.csMaxMind.Db.NativeAot/NuGet.ConfigMaxMind.Db.SourceGenerator.Test/MaxMind.Db.SourceGenerator.Test.csprojMaxMind.Db.SourceGenerator.Test/MaxMindDbSourceGeneratorTest.csMaxMind.Db.SourceGenerator/AnalyzerReleases.Shipped.mdMaxMind.Db.SourceGenerator/AnalyzerReleases.Unshipped.mdMaxMind.Db.SourceGenerator/CollectionParser.csMaxMind.Db.SourceGenerator/Diagnostics.csMaxMind.Db.SourceGenerator/MaxMind.Db.SourceGenerator.csprojMaxMind.Db.SourceGenerator/MaxMindDbSourceGenerator.csMaxMind.Db.SourceGenerator/Model.csMaxMind.Db.SourceGenerator/ModelParser.csMaxMind.Db.Test/MaxMind.Db.Test.csprojMaxMind.Db.Test/SourceGeneratorSupportTest.csMaxMind.Db.slnMaxMind.Db/Decoder.csMaxMind.Db/DictionaryActivatorCreator.csMaxMind.Db/ListActivatorCreator.csMaxMind.Db/MaxMind.Db.csprojMaxMind.Db/SourceGeneratorSupport.csMaxMind.Db/TypeActivatorCreator.csMaxMind.Db/buildTransitive/MaxMind.Db.propsREADME.mddev-bin/test-native-aot.shreleasenotes.md
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@MaxMind.Db.SourceGenerator.Test/MaxMindDbSourceGeneratorTest.cs`:
- Around line 197-198: Add XML documentation to the public test method
GeneratedCodeCanPopulateObsoleteProperties, including a concise <summary> that
describes what the test verifies. Follow the project’s documentation conventions
and add any applicable tags required for public members.
- Around line 214-217: Update the test around RunGenerator to assert that the
generated source contains the “legacy” map key and a generated “Legacy =”
assignment, in addition to the existing warning suppression and error
assertions, so omission of the obsolete property cannot pass.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8927928d-2a7c-4176-9f2d-919f519912eb
📒 Files selected for processing (2)
MaxMind.Db.SourceGenerator.Test/MaxMindDbSourceGeneratorTest.csMaxMind.Db.SourceGenerator/MaxMindDbSourceGenerator.cs
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MaxMind.Db/Decoder.cs (1)
609-655: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the instance-based
Addresolution for reflective collection activation.
GetMethod("Add")onICollection<T>cannot resolve explicit implementations such asLinkedList<T>, so fallback array decode can throw for the documentedLinkedList<T>case. Retrieve the instance type before pickingAdd; withLinkedList<T>, useICollection<T>.Addinstead, otherwise use the concreteexpectedType.Add.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MaxMind.Db/Decoder.cs` around lines 609 - 655, Update the reflective collection fallback after _listActivatorCreator.GetActivator in the array-decoding path to resolve Add from the activated instance type. For LinkedList<T>, select ICollection<T>.Add because Add is explicitly implemented; for other collections, use the concrete expectedType.Add method, then invoke it for each decoded value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@MaxMind.Db.Test/SourceGeneratorSupportTest.cs`:
- Around line 76-77: Add XML documentation with a <summary> element before both
public test methods, including
GeneratedDefaultFactoryExceptionsHaveDeserializationContext and the other public
method referenced in the diff, describing each test’s purpose.
- Around line 140-156: Update the test setup around RegisterDictionary and
RegisterType so GeneratedNonGenericDictionary also has a model registration
whose activator throws if selected. Keep the existing model registration for
GeneratedNonGenericDictionaryModel, then assert decoding the dictionary
succeeds, ensuring dictionary activation takes priority over the throwing model
activator.
---
Outside diff comments:
In `@MaxMind.Db/Decoder.cs`:
- Around line 609-655: Update the reflective collection fallback after
_listActivatorCreator.GetActivator in the array-decoding path to resolve Add
from the activated instance type. For LinkedList<T>, select ICollection<T>.Add
because Add is explicitly implemented; for other collections, use the concrete
expectedType.Add method, then invoke it for each decoded value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c26a1ac6-2215-4d79-ab6a-0e82c68d8470
📒 Files selected for processing (22)
.markdownlint-cli2.jsonc.prettierignoreMaxMind.Db.NativeAot/App/MaxMind.Db.NativeAot.App.csprojMaxMind.Db.NativeAot/App/Program.csMaxMind.Db.NativeAot/Models/MaxMind.Db.NativeAot.Models.csprojMaxMind.Db.SourceGenerator.Test/MaxMind.Db.SourceGenerator.Test.csprojMaxMind.Db.SourceGenerator.Test/MaxMindDbSourceGeneratorTest.csMaxMind.Db.SourceGenerator/AnalyzerReleases.Unshipped.mdMaxMind.Db.SourceGenerator/CollectionParser.csMaxMind.Db.SourceGenerator/Diagnostics.csMaxMind.Db.SourceGenerator/MaxMind.Db.SourceGenerator.csprojMaxMind.Db.SourceGenerator/MaxMindDbSourceGenerator.csMaxMind.Db.SourceGenerator/ModelParser.csMaxMind.Db.SourceGenerator/SymbolHelpers.csMaxMind.Db.Test/SourceGeneratorSupportTest.csMaxMind.Db/Decoder.csMaxMind.Db/DictionaryActivatorCreator.csMaxMind.Db/ListActivatorCreator.csMaxMind.Db/SourceGeneratorSupport.csMaxMind.Db/TypeActivatorCreator.csREADME.mdreleasenotes.md
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Manual re-creation of the pr-review-toolkit review.
The AOT/source-generator design holds up, but two things should be settled before merge: the generator emits a false MMDBSG008 for [Inject]/[Network] members, and AOT diagnostics are off in exactly the project layout the README recommends, so a skipped model is silent at build time and — now that IL2xxx/IL3xxx are suppressed — at publish time too.
Inline:
- Generator correctness: false MMDBSG008 on injected/network members;
Find<T>type arguments are never scanned for collection registrations. - AOT signal loss: diagnostics gated on the model project's own properties; blanket IL2070 suppression with no
DynamicallyAccessedMembersanywhere in the library. - Tests: nothing asserts the source-generated path is actually taken.
13 of the 22 commits are fixup! and need autosquashing before merge.
Generated by Claude Code
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
MaxMind.Db.SourceGenerator.Test/MaxMind.Db.SourceGenerator.Test.csproj (1)
12-12: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace the MSTest runner property with the xunit.v3 runner property.
This project uses xunit.v3 (
xunit.v3.mtp-v2).EnableMSTestRunnerapplies to MSTest projects only, so it has no effect here and the Microsoft Testing Platform runner stays disabled. UseUseMicrosoftTestingPlatformRunnerinstead.♻️ Proposed change
- <EnableMSTestRunner>true</EnableMSTestRunner> + <UseMicrosoftTestingPlatformRunner>true</UseMicrosoftTestingPlatformRunner>xunit.v3 UseMicrosoftTestingPlatformRunner property EnableMSTestRunner MSTest only🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MaxMind.Db.SourceGenerator.Test/MaxMind.Db.SourceGenerator.Test.csproj` at line 12, In the project configuration, replace the EnableMSTestRunner property with UseMicrosoftTestingPlatformRunner and preserve its enabled value so the xunit.v3 Microsoft Testing Platform runner is activated.MaxMind.Db.SourceGenerator/ModelParser.cs (1)
182-198: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRead
MapKeyandInjectvalues from named arguments as a fallback.
GetStringArgumentandGetBooleanArgumentonly inspectAttributeData.ConstructorArguments. IfMapKeyAttributeexposesNameorAlwaysCreateas settable properties, a model written as[MapKey(Name = "city")]or[MapKey("city", AlwaysCreate = true)]stores those values inAttributeData.NamedArguments. The generator then falls back to the member name andfalse, while the reflection path reads the property values. The generated activation metadata and the reflection metadata diverge for the same model.Add a named-argument fallback that keeps positional precedence.
🐛 Proposed fix
- private static string? GetStringArgument(AttributeData? attribute, int position) + private static string? GetStringArgument( + AttributeData? attribute, + int position, + string propertyName + ) { - if (attribute == null || attribute.ConstructorArguments.Length <= position) + if (attribute == null) { return null; } - return attribute.ConstructorArguments[position].Value as string; + if (attribute.ConstructorArguments.Length > position) + { + return attribute.ConstructorArguments[position].Value as string; + } + return GetNamedArgument(attribute, propertyName)?.Value as string; } - private static bool GetBooleanArgument(AttributeData? attribute, int position) + private static bool GetBooleanArgument( + AttributeData? attribute, + int position, + string propertyName + ) { - if (attribute == null || attribute.ConstructorArguments.Length <= position) + if (attribute == null) { return false; } - return attribute.ConstructorArguments[position].Value is bool value && value; + var constant = attribute.ConstructorArguments.Length > position + ? attribute.ConstructorArguments[position] + : GetNamedArgument(attribute, propertyName); + return constant?.Value is bool value && value; } + + private static TypedConstant? GetNamedArgument( + AttributeData attribute, + string propertyName + ) + { + foreach (var argument in attribute.NamedArguments) + { + if (string.Equals(argument.Key, propertyName, StringComparison.Ordinal)) + { + return argument.Value; + } + } + return null; + }Run the following script to confirm the attribute surface:
#!/bin/bash # Description: Inspect MapKeyAttribute, InjectAttribute, and NetworkAttribute declarations. set -euo pipefail fd -e cs . MaxMind.Db --exec grep -l -E 'class (MapKeyAttribute|InjectAttribute|NetworkAttribute|ParameterAttribute)' {} \; \ | while IFS= read -r file; do echo "=== $file ===" cat -n "$file" done echo "=== reflection-based reads of the same attributes ===" rg -n -C 3 'MapKeyAttribute|InjectAttribute|AlwaysCreate' MaxMind.Db --glob '*.cs'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MaxMind.Db.SourceGenerator/ModelParser.cs` around lines 182 - 198, Update CreateMember and the GetStringArgument/GetBooleanArgument helpers to fall back to AttributeData.NamedArguments when the corresponding positional argument is absent, using the MapKeyAttribute property names Name and AlwaysCreate and the InjectAttribute property name. Preserve positional constructor arguments as the higher-priority values, then retain the existing source-name and false defaults when neither form is provided.MaxMind.Db.SourceGenerator/MaxMindDbSourceGenerator.cs (1)
23-54: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftKeep Roslyn symbols and
Compilationout of collected pipeline state.
types.Collect()caches everyINamedTypeSymbolin the compilation,collectionRoots.Collect()cachesCollectionRootvalues that hold anITypeSymboland aLocation, and.Combine(context.CompilationProvider)adds theCompilation. Symbols use reference equality and belong to one compilation, so each edit invalidates the cached state and roots the previous compilation snapshot. In an IDE the generator then re-runs the full model and collection analysis on every keystroke, and old compilations stay alive.Extract equatable, symbol-free data inside the transform steps, and pass only that data to
RegisterSourceOutput.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MaxMind.Db.SourceGenerator/MaxMindDbSourceGenerator.cs` around lines 23 - 54, Refactor the generation pipeline around types, collectionRoots, and generationInput so collected values contain only equatable, symbol-free data. In the CreateSyntaxProvider transforms, extract the required type and collection-root metadata instead of returning INamedTypeSymbol, ITypeSymbol, Location, or other Roslyn-owned objects; remove the CompilationProvider combination and update Generate to consume the extracted data directly while preserving its output behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@MaxMind.Db.NativeAot/Models/MaxMind.Db.NativeAot.Models.csproj`:
- Line 5: Update the LangVersion setting from 12.0 to 14.0 in both
MaxMind.Db.NativeAot/Models/MaxMind.Db.NativeAot.Models.csproj at lines 5-5 and
MaxMind.Db.NativeAot/App/MaxMind.Db.NativeAot.App.csproj at lines 6-6.
In `@MaxMind.Db.SourceGenerator.Test/MaxMindDbSourceGeneratorTest.cs`:
- Around line 14-17: Add XML <summary> documentation to the public
MaxMindDbSourceGeneratorTest class and every public [Fact] test method in the
file, including GeneratesConstructorMetadataDeterministically. Use concise
descriptions of the fixture and each test’s behavior, and include any required
param, returns, or exception tags where applicable; alternatively reduce test
method visibility only if the test framework supports it.
In `@MaxMind.Db/Decoder.cs`:
- Around line 590-598: Add an IL2070 unconditional suppression to the reflective
fallback in DecodeArray, alongside its existing IL3050 suppression, covering the
interfaceType.GetMethod("Add") call. Match the suppression pattern and
justification used by the GetConstructor fallbacks in ListActivatorCreator and
DictionaryActivatorCreator, without changing the decoding behavior.
---
Duplicate comments:
In `@MaxMind.Db.SourceGenerator.Test/MaxMind.Db.SourceGenerator.Test.csproj`:
- Line 12: In the project configuration, replace the EnableMSTestRunner property
with UseMicrosoftTestingPlatformRunner and preserve its enabled value so the
xunit.v3 Microsoft Testing Platform runner is activated.
In `@MaxMind.Db.SourceGenerator/MaxMindDbSourceGenerator.cs`:
- Around line 23-54: Refactor the generation pipeline around types,
collectionRoots, and generationInput so collected values contain only equatable,
symbol-free data. In the CreateSyntaxProvider transforms, extract the required
type and collection-root metadata instead of returning INamedTypeSymbol,
ITypeSymbol, Location, or other Roslyn-owned objects; remove the
CompilationProvider combination and update Generate to consume the extracted
data directly while preserving its output behavior.
In `@MaxMind.Db.SourceGenerator/ModelParser.cs`:
- Around line 182-198: Update CreateMember and the
GetStringArgument/GetBooleanArgument helpers to fall back to
AttributeData.NamedArguments when the corresponding positional argument is
absent, using the MapKeyAttribute property names Name and AlwaysCreate and the
InjectAttribute property name. Preserve positional constructor arguments as the
higher-priority values, then retain the existing source-name and false defaults
when neither form is provided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ad852d59-6f23-4df0-b680-e9202a7d51aa
📒 Files selected for processing (33)
.github/workflows/test.yml.markdownlint-cli2.jsonc.prettierignoreMaxMind.Db.Benchmark/MaxMind.Db.Benchmark.csprojMaxMind.Db.NativeAot/App/MaxMind.Db.NativeAot.App.csprojMaxMind.Db.NativeAot/App/Program.csMaxMind.Db.NativeAot/Models/MaxMind.Db.NativeAot.Models.csprojMaxMind.Db.NativeAot/Models/Models.csMaxMind.Db.NativeAot/NuGet.ConfigMaxMind.Db.SourceGenerator.Test/MaxMind.Db.SourceGenerator.Test.csprojMaxMind.Db.SourceGenerator.Test/MaxMindDbSourceGeneratorTest.csMaxMind.Db.SourceGenerator/AnalyzerReleases.Shipped.mdMaxMind.Db.SourceGenerator/AnalyzerReleases.Unshipped.mdMaxMind.Db.SourceGenerator/CollectionParser.csMaxMind.Db.SourceGenerator/Diagnostics.csMaxMind.Db.SourceGenerator/MaxMind.Db.SourceGenerator.csprojMaxMind.Db.SourceGenerator/MaxMindDbSourceGenerator.csMaxMind.Db.SourceGenerator/Model.csMaxMind.Db.SourceGenerator/ModelParser.csMaxMind.Db.SourceGenerator/SymbolHelpers.csMaxMind.Db.Test/MaxMind.Db.Test.csprojMaxMind.Db.Test/SourceGeneratorSupportTest.csMaxMind.Db.slnMaxMind.Db/Decoder.csMaxMind.Db/DictionaryActivatorCreator.csMaxMind.Db/ListActivatorCreator.csMaxMind.Db/MaxMind.Db.csprojMaxMind.Db/SourceGeneratorSupport.csMaxMind.Db/TypeActivatorCreator.csMaxMind.Db/buildTransitive/MaxMind.Db.targetsREADME.mddev-bin/test-native-aot.shreleasenotes.md
There was a problem hiding this comment.
Re-reviewed at d1bcafe: four of the five round-1 findings are addressed, and the branch is now 6 clean commits with no fixup! left, so the autosquash ask is done.
- False MMDBSG008 on
[Inject]/[Network]members — fixed, and now asserted with diagnostics on. - AOT diagnostics gating — fixed by the
buildTransitive.propsto.targetsrename plus the newVerify*AotDiagnosticsguards; the separate-model-library axis is handled by README guidance instead, which is fair given thatPublishAotcannot propagate to referenced projects. Find<T>/FindAll<T>collection roots never scanned — fixed by the newcollectionRootsprovider and three tests.- Nothing asserted the generated path is taken — fixed; the test project runs the generator as an analyzer and asserts a registration no test hand-registers.
- IL2070/IL2067 suppressions root nothing — still open and untouched:
MaxMind.Dbstill has noDynamicallyAccessedMembers(norDynamicDependencyorRequiresUnreferencedCode) anywhere, so the suppressions silence the trimmer's only signal without making the reflection fallback survive trimming. The residual case is narrower now that diagnostics fire, but for the fallbacks the README itself lists, a trimmed build stays quiet and then throws "No constructor found ... with the MaxMind.Db.Constructor attribute" for a constructor that plainly exists in source.
Three new inline comments below, all on the collection-root and model-discovery paths.
Generated by Claude Code
There was a problem hiding this comment.
The round-2 items are resolved, and the delta since d1bcafe turns up no new correctness findings at 0bce137.
Round-2 #1 (null-conditional reader?.Find<T>()) is fixed: the MemberBindingExpressionSyntax arm is in IsReaderInvocationCandidate, and GeneratesCollectionsUsedByConditionalReaderCalls is non-vacuous — the Lookup class has no attributed members, so the two asserted registrations can only come from the collection-root path.
Round-2 #2 (type parameters reaching typeof(...)) is fixed: SymbolHelpers.ContainsTypeParameter recurses array element types, containing types and type arguments, the guard at MaxMindDbSourceGenerator.cs:154-165 runs before CollectionParser.Collect, and ReportsClosedCollectionsContainingTypeParametersForAot would have failed all three of its assertions before the change.
Round-2 #3 (referenced-assembly model roots) is a reasonable won't-fix. I agree the nested-model-graph objection is the real blocker: registering only the root does not cover the member graph, and SymbolHelpers.IsAccessible rejecting a referenced assembly's internal constructors and properties means many real models could not be activated app-side anyway. One clause is not quite right, though — a per-assembly IAssemblySymbol.GetTypeByMetadataName lookup for the well-known MaxMind.Db.Generated.SourceGeneratedRegistration does surface internal metadata types, so a referenced root compiled with the generator is detectable rather than a false positive; that only enables a diagnostic, and it does not change the decision.
Still open is the round-1 IL2070/IL2067 item (TypeActivatorCreator.cs:84, :108, :190, plus the same pattern in Decoder.cs, DictionaryActivatorCreator.cs and ListActivatorCreator.cs). The reworded justifications are more accurate than the text they replaced, but DynamicallyAccessedMembers, DynamicDependency and RequiresUnreferencedCode still have zero hits repo-wide, so the suppressions continue to tell the trimmer this reflection is safe when it is not — the end state is a suppression whose own justification says the fallback does not preserve trimming metadata.
On the request for things to simplify or delete: the reflective machinery is not dead, so there is no large deletion available. I traced every branch — all four Decoder fast paths are reachable and load-bearing (Dictionary<string, object> and List<object> are what keep object-typed members working under AOT, since the reflective fallbacks go through Expression.Compile), and DictionaryActivatorCreator, ListActivatorCreator, TypeActivatorCreator and ReflectionUtil all still serve live cases the registry and fast paths do not cover, with both internal branches of each collection activator reachable.
The six inline comments are what is actually worth doing — roughly 145 hand-written lines plus a per-model reduction in emitted output. The first is the only one I would treat as blocking, because it reshapes new public API that freezes at 5.2.0.
Two smaller ones, not worth their own threads. MaxMindDbSourceGenerator.cs re-checks for null at :110 and :150-153 even though the providers already filtered at :29 and :35; a .Select(static (value, _) => value!) after each Where deletes both guards and the collectionRoots.First(root => root != null)! at :186. And DeserializationMember.Name is write-only — the only writes are TypeActivatorCreator.cs:29 and :40 and nothing reads it, so the property, its constructor argument and the two call sites can all go.
DecodeMap now pays a ConcurrentDictionary lookup (Decoder.cs:319-320, repeated at :361) that misses on every map decoded into a model type, which is the hot path for non-AOT consumers — per CLAUDE.md that is a benchmark question rather than a finding, and hoisting it would make the generic path pay it too.
Generated by Claude Code
|
No new correctness findings at The Still open, and the one thing I would hold on: the IL2070/IL2067 suppressions. The reworded justifications are more honest, but there is still no On simplificationThere is no large deletion available here. I traced every branch expecting the generator to have made some of the reflective machinery dead, and it has not:
So the six inline comments are what is actually there — roughly 145 hand-written lines plus per-model emitted output. The one worth pushing for is Two more that did not warrant their own threads: Not a finding, but possibly worth a benchmark: Generated by Claude Code |
|
Addressed the latest review at 3a6f49a:
On IL2070/IL2067: I am intentionally keeping the narrowly scoped suppressions. The supported trimmed/NativeAOT contract is the source-generated registration path, which returns before these reflection fallbacks. Unregistered trimmed/NativeAOT models are explicitly unsupported and are now called out more clearly in the suppression justifications and README. A The strict |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MaxMind.Db.NativeAot/Models/MaxMind.Db.NativeAot.Models.csproj (1)
1-4: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSet
LangVersionto14.0inDirectory.Build.props.
MaxMind.Db.NativeAot/Directory.Build.propscentralizes the NativeAOT properties, but it still declaresLangVersionas12.0; move this to14.0so the**/*.csprojunder this tree meets the project-file language-version requirement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MaxMind.Db.NativeAot/Models/MaxMind.Db.NativeAot.Models.csproj` around lines 1 - 4, Update MaxMind.Db.NativeAot/Directory.Build.props, which centralizes the NativeAOT project properties, to set LangVersion to 14.0 instead of 12.0; this single change must apply to both MaxMind.Db.NativeAot/Models/MaxMind.Db.NativeAot.Models.csproj and MaxMind.Db.NativeAot/App/MaxMind.Db.NativeAot.App.csproj, with no direct project-file changes required.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@MaxMind.Db/DictionaryActivatorCreator.cs`:
- Around line 21-30: Validate that the type passed to
DictionaryActivatorCreator.GetActivator has exactly two generic arguments before
calling typeof(Dictionary<,>).MakeGenericType or equivalent construction.
Preserve the existing DeserializationException behavior for invalid arity,
including callers outside Decoder.cs such as tests, by placing the check in
DictionaryActivator or guarding every GetActivator caller.
---
Outside diff comments:
In `@MaxMind.Db.NativeAot/Models/MaxMind.Db.NativeAot.Models.csproj`:
- Around line 1-4: Update MaxMind.Db.NativeAot/Directory.Build.props, which
centralizes the NativeAOT project properties, to set LangVersion to 14.0 instead
of 12.0; this single change must apply to both
MaxMind.Db.NativeAot/Models/MaxMind.Db.NativeAot.Models.csproj and
MaxMind.Db.NativeAot/App/MaxMind.Db.NativeAot.App.csproj, with no direct
project-file changes required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bf7c49ed-b348-450e-ad19-fb42c8dcec89
📒 Files selected for processing (34)
.github/workflows/test.yml.markdownlint-cli2.jsonc.prettierignoreMaxMind.Db.Benchmark/MaxMind.Db.Benchmark.csprojMaxMind.Db.NativeAot/App/MaxMind.Db.NativeAot.App.csprojMaxMind.Db.NativeAot/App/Program.csMaxMind.Db.NativeAot/Directory.Build.propsMaxMind.Db.NativeAot/Models/MaxMind.Db.NativeAot.Models.csprojMaxMind.Db.NativeAot/Models/Models.csMaxMind.Db.NativeAot/NuGet.ConfigMaxMind.Db.SourceGenerator.Test/MaxMind.Db.SourceGenerator.Test.csprojMaxMind.Db.SourceGenerator.Test/MaxMindDbSourceGeneratorTest.csMaxMind.Db.SourceGenerator/AnalyzerReleases.Shipped.mdMaxMind.Db.SourceGenerator/AnalyzerReleases.Unshipped.mdMaxMind.Db.SourceGenerator/CollectionParser.csMaxMind.Db.SourceGenerator/Diagnostics.csMaxMind.Db.SourceGenerator/MaxMind.Db.SourceGenerator.csprojMaxMind.Db.SourceGenerator/MaxMindDbSourceGenerator.csMaxMind.Db.SourceGenerator/Model.csMaxMind.Db.SourceGenerator/ModelParser.csMaxMind.Db.SourceGenerator/SymbolHelpers.csMaxMind.Db.Test/MaxMind.Db.Test.csprojMaxMind.Db.Test/SourceGeneratorSupportTest.csMaxMind.Db.slnMaxMind.Db/Decoder.csMaxMind.Db/DictionaryActivatorCreator.csMaxMind.Db/ListActivatorCreator.csMaxMind.Db/MaxMind.Db.csprojMaxMind.Db/SourceGeneratorSupport.csMaxMind.Db/TypeActivatorCreator.csMaxMind.Db/buildTransitive/MaxMind.Db.targetsREADME.mddev-bin/test-native-aot.shreleasenotes.md
|
Follow-up on the CodeRabbit re-review: the C# language-version consistency suggestion was valid. |
There was a problem hiding this comment.
Everything I raised in round 3 is resolved or answered, and re-reading the tree at 4f03d16 turned up no new findings.
I checked the GeneratedMember[] refactor closely, since it was the change I pushed for and it reshapes new public API, and it is correct: all five per-member facts survive with the same meaning, and the array index still supplies DeserializationMember.Position, so BuildMetadata is a line-for-line translation of the old loop with the same four bucket predicates. The per-element validation covers everything the old cross-array length check did, and that failure mode is now unrepresentable rather than merely rejected; Clone() on a struct array is strictly safer than cloning five reference arrays, since the elements cannot be mutated through an aliased reference at all. I traced emission order end to end (ParseConstructor → RenderMembers → BuildMetadata → CreateActivator) for a model with a [MapKey(alwaysCreate: true)], an [Inject] and a [Network], and positions align at every stage; the target-typed new(...) sits exactly at the C# 9 floor the gate claims, and the compat test parses the generated tree at that language version rather than asserting vacuously.
On the DictionaryActivatorCreator arity check I agree with you and with CodeRabbit's withdrawal — GetActivator has exactly one call site and Decoder.cs:378-382 runs the identical check immediately before it, so the internal cache can never be populated for a wrong-arity type.
The two declined items (the ActivatorMetadata cache, CollectionParseResult) are reasonable calls and I am not pursuing them.
On the trimming annotations: you are right, and I am dropping the objection. [DynamicDependency(..., typeof(T))] on a generic method is a CS0416 compile error, so it cannot express "whatever T the caller passes". RequiresUnreferencedCode is call-site-based, so it would warn every caller, including fully source-generated apps that have nothing wrong with them. And DAM has no transitive form, while DecodeMapToType takes the next type from param.MemberType and recurses — so the reflection target set is the whole model graph rather than T, and annotating Find<T> would be both viral and insufficient. The suppressions are defensible under the guidance you cited, which lists "the code path is never executed in trimmed scenarios" as a valid condition, and the System.Text.Json precedent does not overturn that: STJ can annotate at the boundary only because its generated path is visible in the overload's signature, which a registry populated by module initializers is not.
CI is green at 4f03d16 — all 24 checks complete, 23 successful with only the release-only publish job skipped — including both the project-reference and the packaged NativeAOT branches, which exercises both import chains and confirms the emitted source compiles.
Nothing outstanding from my side.
Generated by Claude Code
|
Addressed the latest review and the related correctness audit:
Validation: 282 Release tests passed, plus direct and packaged Linux x64 NativeAOT publishes and executables. |
|
Follow-up on the first refreshed CI run: the new analyzer-free fixture needed a strong name because the Windows |
There was a problem hiding this comment.
No blockers at b821cd3; two low-priority suggestions inline.
The derived-MapKeyAttribute change is more consequential than the changelog line suggests. The generator read AttributeData.ConstructorArguments at the application site, so a derived attribute whose base(...) call rewrites them — the delta's own [Utf8Key("string")] : base("utf8_" + name, true) fixture — previously emitted mapKey="string", alwaysCreate=false against a runtime attribute of Name="utf8_string", AlwaysCreate=true; under AOT that was silently wrong data with no exception, so rejecting these is a correctness fix rather than conservatism. Dropping the whole model is required rather than heavy-handed: GeneratedMember[] is positional and RenderActivator emits new T((T0)values[0], …), so omitting one member shifts every later argument. The JIT fallback is real — TypeActivatorCreator uses GetCustomAttributes<MapKeyAttribute>() and IsDefined(..., true), which match derived types and read the runtime-computed values.
MMDBSG011's descriptor, severity, category and AnalyzerReleases.Unshipped.md row are all consistent with 001-010, and the ID is unique.
The accessibility gates keep internal same-assembly types generated, so no model silently drops to reflection; the only conservative false negative is InternalsVisibleTo, which is pre-existing.
The new ReflectionFallback.TestModels fixture is only worth anything if the analyzer really doesn't reach it, and it doesn't: no repo-root Directory.Build.props, analyzers don't flow transitively over a ProjectReference, and buildTransitive/MaxMind.Db.targets is pack-only so a ProjectReference consumer never imports it. It's also self-validating, since the test asserts TryGetTypeRegistration is false. Signing matches the other projects and release.yml packs only MaxMind.Db, so there's no packaging exposure — one coverage gap worth a clause rather than a thread: the derived-MapKey fallback is exercised on the constructor path only, not the property path (TypeActivatorCreator.cs:209/:251).
All five new tests fail if the behavior they guard is reverted — including DerivedMapKeyAttributeUsesReflectionFallbackSemantics, which asserts the decoded value rather than merely that nothing throws — and the round-4 guidance asserts are genuinely reached, since NoCtorNoAttributeType/NoAnnotatedPropertiesType bail out of ModelParser.Parse and so take the reflective path.
Nothing blocking from my side; the PR is blocked only for want of an approving review.
Generated by Claude Code
|
No blockers at The derived- Other things I checked and cleared:
One coverage gap, not worth its own thread: the derived-MapKey fallback is exercised on the constructor path only, not the property path ( The two inline suggestions are both the same shape as something just fixed here, which is the only reason they seemed worth raising: Generated by Claude Code |
There was a problem hiding this comment.
Blocker: a packaged 5.2.0 fails to compile any netstandard2.0 or netstandard2.1 consumer with error CS0122: 'ModuleInitializerAttribute' is inaccessible due to its protection level — mechanism, evidence, and a validated fix are in the inline comment.
That came out of a migration which is, separately, the strongest evidence I have that this PR does what it set out to do. I moved maxmind/GeoIP2-dotnet (main, c10c8ad, currently on MaxMind.Db 5.1.0) onto a locally packed 5.2.0.
- GeoIP2 needs no source changes at all — a one-line
PackageReferencebump. - All 18 GeoIP2 response and model types get real generated registrations, confirmed two ways: the emitted source has 18
RegisterType, 1RegisterCollection, 1RegisterDictionary, and probing the runtime registration table shows 19 entries (GeoIP2's 18 plus the reader's ownMetadata). The three unregistered types are JSON-only with no MMDB attributes and are correctly skipped. - GeoIP2's suite is 302/302, identical to baseline — no behavior differences, no test changes.
- NativeAOT end to end: the migrated app publishes with one IL warning, GeoIP2's own
StackFramecall, nothing from MaxMind.Db — and the native binary performs live City lookups with 19/19 decoded-value assertions passing. The 5.1.0 control, differing only in reader version, emits 11 IL diagnostics (10 from MaxMind.Db: 4 IL3050, 5 IL2070, 1 IL2067) and crashes on startup withDeserializationException: No constructor found for MaxMind.Db.Metadata. That contrast is the clearest demonstration available of the PR's value. - Zero MMDBSG diagnostics fire on real GeoIP2 code with
MaxMindDbAotDiagnostics=true, on net8/9/10. The zero is not vacuous: planting deliberately bad models makes MMDBSG003/005/011 fire, as hard errors under GeoIP2'sTreatWarningsAsErrors.
One observation rather than a finding: without the diagnostics opt-in, those same deliberately-broken models build with 0 warnings / 0 errors and are silently dropped to reflection. That is the documented gating and you have already reasoned about it — this is just the first time it has been demonstrated rather than argued.
Unrelated aside: ILLinkTreatWarningsAsErrors=true did not promote ILC trim warnings to errors in my test app. Orthogonal to this PR, but possibly worth a look.
Generated by Claude Code
|
Benchmarked a GeoIP2 migration against a full GeoLite2 City database (66 MB, build 2026-07-31, 6,169,380 nodes, 17,153,199 networks). Same GeoIP2 source on both sides, both consuming MaxMind.Db as a packed nupkg so the reference shape is identical; the only difference is Ran with the Steady-state An independent Stopwatch harness gave 0.9520, agreeing to four decimals. Cold start — 59% faster, over 40 fresh interleaved processes per side. 1M lookups — allocation 7,100.9 -> 7,014.4 B/op, Gen0 412 -> 407, peak RSS 113,440 -> 106,224 kB (-7.2 MB, -6.4%), with no overlap between the two sides' ranges across 6 pairs. Where the steady-state win comes from. Mostly not activation — it is the The NativeAOT, standalone (no What these numbers do not establish. One container on 4 shared Xeon cores, one database, one workload, single-threaded, IPv4 only, City lookups only, memory-mapped with the file pre-faulted. The noise floor is roughly ±2-3% and it did bite mid-run: the same binaries gave -1.21% over rounds 1-3 (CI spanning zero) and -5.29% over rounds 4-6, and only 9-12 counterbalanced rounds stabilised it, so the round-level permutation p-values are the conservative test and the within-round bootstrap CIs are optimistic. Record shape alone moves the ratio by 8 points. The -2.83% residual on the array-free path and the -7.2 MB RSS are attributed to generated lambdas versus expression trees and to runtime codegen respectively; that is inference, not profiling. Generated by Claude Code |
There was a problem hiding this comment.
Round 6: everything I raised in round 5 is fixed and build-verified, and I found nothing new.
The CS0122 blocker is resolved. I packed this head and built a downstream consumer multi-targeting netstandard2.0;netstandard2.1;net8.0;net10.0 with TreatWarningsAsErrors against the resulting nupkg: clean on all four, 0 warnings and 0 errors. The same project pointed at a package packed from b821cd3 still fails with error CS0122: 'ModuleInitializerAttribute' is inaccessible due to its protection level on both netstandard TFMs, so the harness is not vacuous. The polyfill is now emitted for the netstandard TFMs and suppressed for net8.0/net10.0, which makes the #pragma warning disable CS0436 that was dead code at the old head load-bearing. MaxMind.Db's own netstandard assemblies still export the internal polyfill, which is fine — the consumer shadows it, and a consumer that already ships its own polyfill gets no emission rather than a collision.
MaxMind.Db.NetStandard.TestModels is a genuine two-assembly regression test, and I confirmed it catches the bug by grafting it onto the old head in plain ProjectReference mode, where it fails with the same CS0122 on both TFMs. Because it is dual-mode it runs in the ordinary solution build, not only under test-native-aot.sh, which is the right place for it.
Both round-5 suggestions are in with tests. I ran the old and new generators side by side over 30 declaration and nesting shapes — class/struct/record struct/abstract/partial primary constructors, file-local and privately nested models, nested types in generics up to four levels including F-bounded and self-nested arguments — and the output differs in exactly the two intended places, with value types and abstract types admitted by the widened predicate and then rejected cleanly rather than crashing; the new tests are not vacuous, since the MMDBSG011 count of 2 splits one per model and the MMDBSG008 count of 3 is 2 plus CS0122 errors at the old head.
At this head the reader builds 0 warnings/0 errors with 283/283 tests, dev-bin/test-native-aot.sh linux-x64 passes with zero IL diagnostics, and a re-run GeoIP2 migration builds clean on all five TFMs with 302/302 tests — where the two netstandard TFMs could not build at all at the previous head.
Nothing outstanding from my side.
Generated by Claude Code
dev-bin/release.sh does the version bump and makes its own "Prepare for $version" commit at release time, so this carries only the notes. The date placeholder is filled in then; release.sh requires it to be today's date.
An annotated struct or record struct was skipped with no diagnostic, even with AOT diagnostics fully enabled. Reflection does activate such a model, so it works under JIT, gets no registration, and gives no build-time signal that it is unsupported after trimming, which is the exact failure the diagnostics exist to prevent. The check runs after the constructor and property scan rather than with the abstract and static filter above it. Candidate discovery admits every type with a base list, so reporting earlier would warn about unrelated declarations. Abstract types keep being skipped silently, since an annotated abstract base is how members are shared and its concrete derived types are what get generated.
| var propertiesByName = new Dictionary<string, IPropertySymbol>(StringComparer.Ordinal); | ||
| for (var current = type; current != null; current = current.BaseType) | ||
| { | ||
| foreach (var property in current.GetMembers().OfType<IPropertySymbol>()) |
There was a problem hiding this comment.
Critical — generator emits uncompilable code for new-shadowed properties.
GetAnnotatedProperties keys by property name and tests ContainsKey before IsAnnotated. So when a derived type declares an unannotated new member, the derived member doesn't claim the name and the annotated base property is selected. But RenderActivator/RenderDefaultsFactory emit a bare Name = ... / instance.Name, which C# binds to the most-derived member.
public class BaseModel { [MapKey("utf8_string")] public string? Value { get; init; } }
public sealed class ShadowModel : BaseModel { public new int Value { get; init; } }produces error CS0029: Cannot implicitly convert type 'string' to 'int' in the generated file, with no MMDBSG diagnostic. A consumer project that compiles on 5.1.0 stops compiling on 5.2.0, and the error points at code they didn't write.
The type-compatible variant (public new string? Value) is worse because it's silent: the generated path assigns the derived member, whereas reflection binds the annotated base one (IsDefined(..., inherit: true) doesn't see a shadowed base attribute).
Suggest skipping an inherited annotated property when a more-derived member of the same name exists — or reporting MMDBSG003 and bailing, so the type falls back to reflection rather than breaking the build.
There was a problem hiding this comment.
Fixed in 8ba21c8, reported as MMDBSG014 and the type is skipped rather than emitted. Property selection now tracks every declared name, so a more derived declaration claims it and an annotated base left unreachable is reported. Overrides are unaffected — GetAttribute already walks the override chain, so an override is annotated in its own right and claims the name; there's a test pinning that.
One correction. For the type-compatible variant, reflection doesn't bind the annotated base property either: GetProperties collapses the same-signature pair to the derived one, and IsDefined(..., inherit: true) is false for it, so reflection finds no matching property at all. The divergence is real, just differently shaped. For the type-incompatible variant your analysis holds exactly and CS0029 reproduces.
— Claude, on behalf of Greg
| foreach (var member in metadata.AlwaysCreatedParameters) | ||
| { | ||
| defaultParameters[member.Position] = null; | ||
| } |
There was a problem hiding this comment.
Critical — AlwaysCreate on a value-type constructor parameter regresses to a thrown exception.
This nulls the default for every AlwaysCreate member, which mirrors the property-based reflection path (TypeActivatorCreator.cs:290-294) but not the constructor-based one. That path keeps a boxed default(T) for non-nullable value types via DefaultValue (TypeActivatorCreator.cs:81-88), so SetAlwaysCreatedParams skips them.
With [Constructor] public M([MapKey("no_such_key", true)] int v):
- without the generator:
V=0 - with the generator:
DeserializationException: No constructor found for System.Int32 with the MaxMind.Db.Constructor attribute and no parameterless constructor found for property-based activation…
No diagnostic is reported even with MaxMindDbAotDiagnostics=true. Every AlwaysCreate usage in the existing suite is a reference type, so nothing catches it.
Suggest only nulling the default when MemberType is a reference type or Nullable<T> — i.e. reuse DefaultValue's rule.
There was a problem hiding this comment.
Fixed in fecfafb, and it was broader than reported. The property-based reflection path did the same unconditional nulling, predating the source generator — so [MapKey("no_such_key", true)] int v returned 0 through a constructor model and threw through either of the other two. All three now consult one rule, TypeActivator.IsNonNullableValueType, so they can't drift again.
Covered on both sides: the reflection models live in the assembly with no generator reference, and the generated case registers by hand. Reverting either guard fails both tests.
— Claude, on behalf of Greg
| { | ||
| var registeredMember = _members[i]; | ||
| var member = new DeserializationMember(i, registeredMember.MemberType); | ||
| if (registeredMember.InjectableName == null && !registeredMember.IsNetwork) |
There was a problem hiding this comment.
Critical — [MapKey] + [Inject]/[Network] on one member silently stops reading the database key.
This excludes injectable and network members from deserializationParameters. The reflection path does not: TypeActivatorCreator.cs:179 adds every constructor parameter to paramNameTypes, keyed by its [MapKey] name or parameter name, regardless of whether it also carries [Inject]/[Network]. Same for the property path at :251-266.
So a model with [MapKey("x")] [Inject("y")] string? Foo reads DB key "x" today; after upgrading to 5.2.0 with the generator active, "x" is dropped from the registration entirely and the member only ever receives the injectable. If the injectable isn't supplied, the field is silently null — no diagnostic, no exception.
MaxMindDbSourceGenerator.cs:145-148 also skips CollectionParser.Collect for those members, so a collection-typed member in that shape gets no registration either.
Either match the reflection path, or report a diagnostic when a member carries [MapKey] alongside [Inject]/[Network]. Silently changing which annotation wins in a minor release seems like the worst of the three options.
There was a problem hiding this comment.
Fixed in ff01657, but as a diagnostic (MMDBSG013) rather than by matching the reflection path, for two reasons.
First, a correction: the member does still receive the injected value on both paths, and a missing injectable throws on both — SetInjectables raises DeserializationException. The real divergence is error behaviour (reflection type-decodes "x" before discarding it, so a malformed value throws there and not here) plus the missing nested registration.
Second, simply including these members again would resurrect the spurious MMDBSG008 on shapes like [Inject("locales")] string[] locales that excluding them was introduced to fix. So a member carrying [MapKey] alongside [Inject]/[Network] now reports MMDBSG013 and the model is dropped, which falls back to reflection and therefore behaves exactly as 5.1.0 did. [Inject]/[Network] without an explicit [MapKey] is unchanged.
— Claude, on behalf of Greg
| <Project> | ||
| <PropertyGroup> | ||
| <MaxMindDbAotDiagnostics | ||
| Condition="'$(MaxMindDbAotDiagnostics)' == '' and ('$(PublishAot)' == 'true' or '$(PublishTrimmed)' == 'true' or '$(IsAotCompatible)' == 'true')" |
There was a problem hiding this comment.
Critical — diagnostics are off in exactly the layout most consumers use.
This enables diagnostics only when PublishAot/PublishTrimmed/IsAotCompatible is true in the project being compiled, and none of those propagate across ProjectReference. The dominant real-world layout is an AOT app plus a plain model class library — so MMDBSG001–012 are silenced in the only compilation that could report them. The app then publishes clean (every reflection path carries UnconditionalSuppressMessage, so ILC is quiet too) and fails at the first Find<T>() in production.
This repo demonstrates it: MaxMind.Db.NetStandard.TestModels — the model library the AOT app consumes — sets none of these properties, and it sits outside MaxMind.Db.NativeAot/Directory.Build.props so the VerifyMaxMindDbAotDiagnostics guard doesn't cover it either. I checked; only MaxMind.Db itself and MaxMind.Db.NativeAot/ set any of the three.
Suggest defaulting MaxMindDbAotDiagnostics to true and letting consumers opt out. There's no real cost to a warning in a JIT build — the user either fixes the model or suppresses the ID. Failing that, at least light up on $(IsTrimmable) too.
Minor: Condition="'$(MaxMindDbAotDiagnostics)' == ''" means an explicit empty assignment (-p:MaxMindDbAotDiagnostics=) reads as unset and gets silently overridden — three states where the user thinks there are two.
There was a problem hiding this comment.
Fixed in 53665eb — on by default, opt out with MaxMindDbAotDiagnostics=false.
Your repro checked out exactly: only MaxMind.Db itself and the projects under MaxMind.Db.NativeAot/ set any of the three properties, and MaxMind.Db.NetStandard.TestModels sits outside that directory so the guard there never covered it.
Adding $(IsTrimmable) would not have closed it, which is worth recording — that project doesn't set it either, and neither does an ordinary model library. The opt-out default is the only thing that reaches the compilation that can report.
— Claude, on behalf of Greg
| SourceProductionContext context | ||
| ) | ||
| { | ||
| var keys = new HashSet<string>(StringComparer.Ordinal); |
There was a problem hiding this comment.
Critical — MMDBSG005 false-positives on [Inject]/[Network] members and silently drops valid models.
HasUniqueMapKeys iterates every member, including injectable and network ones, whose MapKey defaults to the source name (:244). But SourceGeneratorSupport.BuildMetadata (:374) excludes exactly those members from the map-key dictionary — so the generator is stricter than its own runtime.
Repro: public M([MapKey("city")] string? a = null, [Inject("ip_address")] IPAddress? city = null) reports MMDBSG005. The message ("maps more than one member to the database key 'city'") is wrong — the second member maps to no key at all — and the model is dropped from generation, which under AOT means a runtime failure.
Suggest filtering member.InjectableName != null || member.IsNetwork here to match BuildMetadata.
There was a problem hiding this comment.
Fixed in 8d63129. Uniqueness is now computed only over the members that actually reach deserializationParameters, so the generator matches its own runtime by construction rather than by a second filter that can drift. Your repro is now a regression test.
— Claude, on behalf of Greg
| fallback remains available in normal JIT builds but is not guaranteed after | ||
| trimming or with NativeAOT. | ||
| - Open generic model classes are not supported. | ||
| - Models must be classes or records. Annotated structs and record structs are |
There was a problem hiding this comment.
This claim is false for property-based models.
Constructor-based structs do fall back to reflection. Property-based structs and record structs do not — they throw. TypeActivatorCreator.PropertyBasedActivator resolves the parameterless constructor with GetConstructor(..., Type.EmptyTypes, null), which returns null for any struct that doesn't explicitly declare public S() {}. The result is DeserializationException: …no parameterless constructor found for property-based activation… even in a plain JIT build.
ReportsRecordStructPropertyModelsForAot covers that shape but asserts only the diagnostic, never the runtime claim.
Suggested wording: "Annotated structs and record structs are reported as MMDBSG012. Constructor-based structs fall back to reflection; property-based structs fail at run time, because a struct has no parameterless constructor to invoke unless one is declared explicitly."
Also in this section:
- "Open generic model classes are not supported" understates it — no generic model class works, closed or otherwise. Candidates come from syntax declarations so the symbol is always the unbound definition, and
ContainsTypeParameterrejects it.Find<MyModel<string>>never gets a registration. - MMDBSG011 isn't in the limitation list, even though this repo's own AOT sample has to
#pragma warning disable MMDBSG011. - The CLR-array bullet is filed as a generator limitation, but reflection can't populate a CLR array either — it's library-wide.
- "getters and setters" may read as excluding
init, which is supported and is what every example in the README uses.
There was a problem hiding this comment.
Both claims fixed in deecf8d, using your wording for the struct case.
Confirmed the mechanism: PropertyBasedActivator resolves the parameterless constructor with GetConstructor(..., Type.EmptyTypes, null), which is null for any struct that doesn't declare public S() {} explicitly, so a property-based struct throws in a plain JIT build too.
The generic bullet now says no generic model class works, closed or otherwise, and explains why — models are discovered from their declarations, so the generator only ever sees the unbound definition.
— Claude, on behalf of Greg
| @@ -1,5 +1,21 @@ | |||
| # Release Notes | |||
|
|
|||
| ## 5.2.0 (YYYY-MM-DD) | |||
There was a problem hiding this comment.
A few user-facing additions aren't mentioned — the MaxMindDbAotDiagnostics MSBuild property (the only user-facing switch, and the README's central instruction), the MMDBSG### diagnostic range, and the new public SourceGeneratorSupport/GeneratedMember types.
The "CI now publishes and runs a strictly trimmed NativeAOT application on Linux, Windows, and macOS" bullet is CI-internal; CLAUDE.md scopes this file to user-facing changes.
There was a problem hiding this comment.
Fixed in deecf8d. The section now names the MMDBSG001–MMDBSG016 range and the MaxMindDbAotDiagnostics property that controls it, the two new public types, and the AlwaysCreate fix, which changes behaviour for property models that were throwing. The CI bullet is gone — agreed it's internal, and CLAUDE.md scopes this file to user-facing changes.
— Claude, on behalf of Greg
| // type in that assembly. Consistency checks that involve the member set as | ||
| // a whole belong in GeneratedTypeActivatorRegistration, where they run on | ||
| // first use and fail only the offending type. | ||
| TypeRegistrations.TryAdd(type, new GeneratedTypeActivatorRegistration( |
There was a problem hiding this comment.
API shape — worth deciding before 5.2.0 freezes it. Non-blocking, but this is the last chance.
Two things nothing currently enforces:
-
Type identity vs. activator identity. Nothing ties
typeof(X)tovalues => new Y(...). A mismatch surfaces as anInvalidCastExceptionfrom the caller'sFind<T>with no mention of the bad registration. Making these generic —RegisterType<T>(Func<object?[], T> activator, …),RegisterCollection<TCollection, TElement>,RegisterDictionary<TKey, TValue, TDictionary>— turns that into a compile-time guarantee for free, since the generatedvalues => new X(...)infersT = X. It also makeselementType/keyType/valueTypeunforgeable and lets the generated lambdas drop their casts. -
GeneratedMember's flag triple.injectableName/isNetwork/alwaysCreateare three orthogonal flags describing what's really a discriminated choice, and the illegal combinations are silently mis-resolved rather than rejected — e.g.injectableName != null && isNetworkputs the member in both collections and network wins at decode time;isNetworkwith a meaningfulmapKeysilently drops the key. AMemberKindenum plus named factories (GeneratedMember.ForMapKey(...),.ForInjectable(...),.ForNetwork(...)) makes those unrepresentable and reads better thannew("x", typeof(string), null, false, false).
Smaller, same deadline: GeneratedMember is a fairly generic name to plant in the root MaxMind.Db namespace — MaxMind.Db.SourceGeneration might age better.
Also worth documenting on RegisterType regardless of the above: all three registries use TryAdd, so first-registration-wins and a conflicting later registration is silently discarded; and the object?[] handed to the activator is ArrayPool-rented, so values.Length is not members.Length and the array is returned to the pool the instant the activator returns. Generated code indexes values[0..n-1] and is fine, but any hand-written activator that iterates it or retains it is subtly wrong.
There was a problem hiding this comment.
Done in 0980e57 — thanks for pushing on this before it froze.
Registration is generic: RegisterType<T>, RegisterCollection<TCollection, TElement> and RegisterDictionary<TDictionary, TKey, TValue>, so type identity and the element, key and value types are compile-time guarantees and the generated lambda infers T. GeneratedMember now has Mapped/Injected/Networked factories, so the illegal combinations you listed are unrepresentable, and BuildMetadata switches on the kind rather than testing flags in an order that mattered.
One deliberate asymmetry: the add delegates stay untyped. Making them generic moves the object-to-element cast out of generated code and into a wrapper delegate sitting on the per-element decode path; the factory is generic because it runs once per collection. Benchmarked paired against the pre-review head over three passes and the difference is inside the noise band, with identical allocations.
— Claude, on behalf of Greg
| Accessibility.ProtectedOrInternal; | ||
| } | ||
|
|
||
| internal static bool IsTypeAccessible( |
There was a problem hiding this comment.
Minor style: IsTypeAccessible calls IsAccessible declared above it at :14, which inverts the caller-before-callee ordering in CLAUDE.md. Swapping the two methods fixes it.
Same rule in ModelParser.cs: HasAttribute (:309) calls GetAttribute (:289), and SupportsRequiredMembers (:367) calls HasAttribute. Because GetAttribute has callers both above and below, no reordering satisfies the rule there — deleting the one-line HasAttribute wrapper (two call sites) and inlining GetAttribute(...) != null resolves both.
There was a problem hiding this comment.
Fixed in 51d8c08. IsTypeAccessible now precedes IsAccessible, and in ModelParser I took your second suggestion — the one-line HasAttribute wrapper is gone and its two call sites use GetAttribute(...) != null, which resolves the ordering rather than shuffling declarations around it.
— Claude, on behalf of Greg
| readonly native_aot_rid="${1:?usage: test-native-aot.sh <runtime-identifier>}" | ||
| repository_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" | ||
| readonly repository_root | ||
| readonly package_version="5.2.0-aot-ci" |
There was a problem hiding this comment.
Credit where due — set -euo pipefail, ${1:?usage...}, and Check(!RuntimeFeature.IsDynamicCodeSupported, ...) in the app mean this genuinely can't silently pass as a JIT build. That's better than most AOT test harnesses.
A few drift/cleanup items:
package_versionis hard-coded at5.2.0-aot-ci, decoupled fromVersionPrefix, and$repository_root/artifacts/native-aot-packagesis never cleaned — so a stale.nupkgfrom an aborted run can coexist with a fresh one under the same filename.rm -rf "$package_directory"before themkdir -pcosts nothing.restore_directory="$(mktemp -d)"on line 12 is never removed and there's notrap … EXIT; leaks a full package cache per run.- Line 37 hard-codes
net8.0in the publish path, so a TFM bump inDirectory.Build.propsfails with a bare "No such file or directory".set -emakes it loud, but the message won't point at the cause. - The
netstandard_models_projectbuild proves it compiles but never asserts it produced registrations. Since the app consumes it byProjectReferenceanyway, asserting the generated file exists (-p:EmitCompilerGeneratedFiles=trueplus a file check) would catch a packaging regression directly.
Unrelated but adjacent: dev-bin/release.sh:117's sed -i for PackageValidationBaselineVersion exits 0 whether or not it matched, and it runs after gh release create has already tagged and published. The only symptom of a miss is git commit failing with "nothing to commit" — which doesn't name the property. A grep -q guard before the sed would make it fail for the right reason.
There was a problem hiding this comment.
All five fixed in 411acfd: package_version is derived from the library's VersionPrefix instead of a literal that had already drifted from it, the package directory is removed before packing, a trap cleans up the mktemp restore directory, the publish path's target framework is read from the app project with a clear message if the directory is missing, and the .NET Standard consumer build now asserts the generator produced registrations, mirroring the check the app's own model project already had.
— Claude, on behalf of Greg
Nulling an AlwaysCreate slot is what tells SetAlwaysCreatedParams to construct
that member. A non-nullable value type has no model to construct and no null state
to signal absence with, so nulling it sent the decoder off to activate something
like System.Int32 as a model, throwing DeserializationException.
The constructor-based reflection path never had this problem: it derives defaults
from DefaultValue, which keeps a boxed default(T), so SetAlwaysCreatedParams skips
the slot. Both the source-generated path and the property-based reflection path
overrode that to null unconditionally, so `[MapKey("no_such_key", true)] int v`
returned 0 through a constructor model and threw through either of the other two.
Both now consult the same rule, exposed as TypeActivator.IsNonNullableValueType so
the three paths cannot drift again. The property-based reflection regression is
older than the source generator and is fixed here too.
Covered on both sides: the reflection models live in the assembly that has no
generator reference, and the generated case registers by hand. Reverting either
guard fails both tests.
HasUniqueMapKeys counted every member, and a member with no explicit [MapKey]
defaults its map key to its source name. So a model like
M([MapKey("city")] string? name, [Inject("ip_address")] IPAddress? city)
reported "maps more than one member to the database key 'city'" and was dropped
from generation, which under NativeAOT means a runtime failure for a model that is
perfectly valid.
The message was wrong as well as the verdict: the injectable member maps to no key
at all. BuildMetadata already excludes injectable and network members from the
decode dictionary, so uniqueness now considers the same set the runtime does and
the generator can no longer be stricter than itself.
An injectable or network member reads no database key, so an explicit MapKey alongside one of them is ambiguous. The two activation paths resolved it differently: reflection added the key to its dictionary, decoded the value, then let SetInjectables or SetNetwork overwrite the slot, while generated code dropped the key entirely. The end value matched, but a malformed database value threw on one path and not the other, and a collection-typed member in that shape got no registration. Rather than reproduce the reflection path's wasted decode — which would also resurrect the spurious MMDBSG008 that skipping these members was introduced to fix — report the combination and drop the model. Falling back to reflection is exactly the 5.1.0 behaviour, so nothing changes for existing code beyond gaining a warning that points at the ambiguity. Members carrying Inject or Network without an explicit MapKey are unaffected.
GetAnnotatedProperties keyed by name and tested ContainsKey before IsAnnotated, so
an unannotated `new` member in a derived type did not claim the name and the
annotated base property was selected instead. Generated code emits an unqualified
member reference, which C# binds to the most derived member, so:
class BaseModel { [MapKey("utf8_string")] string? Value { get; init; } }
sealed class ShadowModel : BaseModel { public new int Value { get; init; } }
emitted `Value = (string)values[0]!` into `new ShadowModel { ... }` and failed with
CS0029 in a file the consumer did not write, with no diagnostic to explain it. A
project that builds against 5.1.0 would stop building.
Property selection now tracks every declared name, so a more derived declaration
claims the name and an annotated base property left unreachable is reported
instead of emitted. Overrides are unaffected: GetAttribute already walks the
override chain, so an override is annotated in its own right and claims the name,
which the second new test pins.
The type-parameter branch guarded its diagnostic on `root.TypeSymbol is
INamedTypeSymbol`, but a bare method type parameter is an ITypeParameterSymbol, so
the canonical wrapper
T Lookup<T>(Reader r, IPAddress ip) => r.Find<T>(ip);
produced no warning at all. Wrapping Find<T> in a repository or service is the
normal way to encapsulate this library, so a developer could set PublishAot=true,
see a clean build, and have every lookup through the wrapper unregistered. A test
asserted that silence, so it was pinned as intended behaviour.
The guard is gone — the branch exists because the type argument is unresolved, and
that is worth reporting however it is spelled.
These cases also stop borrowing MMDBSG008, whose message describes a collection
that cannot be created and populated. For Find<Foo<T>> the type need not be a
collection at all, and the real cause is the type parameter, so they get their own
ID and wording. The former behaviour is still covered for a non-AOT build, where
the silence is deliberate.
Report resolved a location with symbol.Locations.FirstOrDefault(IsInSource), which is null when the offending symbol comes from a referenced assembly. An inherited property (MMDBSG003), an inherited required member (MMDBSG010), an inherited derived-MapKey attribute (MMDBSG011) or a hidden base property (MMDBSG014) then landed at Location.None, with no file or line. The README tells consumers to treat these as build errors, which is hard to act on without a location. The model type is always declared in the compilation being analysed, so it now stands in when the reported symbol has no source of its own. The existing inherited-property test asserted only the diagnostic ID, so it passed either way; it now asserts the location is in source and points at the derived declaration.
SupportsRequiredMembers filtered OfType<IPropertySymbol>(), so a required field never triggered MMDBSG010. The generated constructor call or object initializer then failed with CS9035 — a compile error in generated code with nothing to explain it. Both member kinds are now checked, which also makes MMDBSG010's title and the README bullet accurate: they say "members", not "properties".
A positional record whose attributes bind to its primary-constructor parameters —
internal sealed record Model([MapKey("v")] string Value);
— has no constructor carrying ConstructorAttribute and no annotated properties, so
it fell out of the parser with no output and no diagnostic even with diagnostics
fully enabled. Under NativeAOT that is a runtime failure, and the guidance in the
fallback exception tells the user to resolve MMDBSG diagnostics of which there were
none.
Silence is still right for the general case, because candidate discovery admits far
more types than are models. Annotated constructor parameters are the signal that
the author meant this type to be deserialized, so only that shape is reported. A
second test pins that an unrelated type with a base list and a plain constructor
stays silent.
The diagnostics were keyed off PublishAot, PublishTrimmed or IsAotCompatible in the project being compiled. None of those propagate across a ProjectReference, so in the layout most consumers use — an AOT application plus a plain model class library — every MMDBSG diagnostic was silenced in the only compilation that could report them. The application then published clean, because the reflection paths all carry UnconditionalSuppressMessage and ILC has nothing to say about them, and failed at the first Find<T> in production. This repository demonstrated it: MaxMind.Db.NetStandard.TestModels, the model library the NativeAOT application consumes, sets none of the three, and it sits outside MaxMind.Db.NativeAot/Directory.Build.props so the diagnostics guard there did not cover it either. They are now on unless a consumer opts out. Adding IsTrimmable to the old condition would not have helped: that project does not set it either, and neither does any ordinary model library. A warning in a JIT build that will never be trimmed is a far smaller cost than a silent AOT failure, and the opt-out is one property.
MaxMind.Db.Test references the source generator, and ClassActivator prefers a generated registration, so essentially the whole pre-existing suite moved onto generated activation. What lost reflection coverage is what every consumer still gets until they rebuild: Inject and Network injection, BigInteger and uint128, byte[], numeric widening, nested model activation and AlwaysCreate. MaxMind.Db.ReflectionFallback.TestModels previously covered a string and a List<long> subclass, by constructor and by properties, plus default preservation. It now mirrors TypeHolder and PropTypeHolder in full, and the assertions match the values ReaderTest asserts for the generated path, so the two paths are pinned against the same expectations. This is the coverage that would have caught the two divergences fixed earlier in this branch: every AlwaysCreate member in the suite was a reference type, and no model combined MapKey with Inject.
Nothing has shipped yet, so this is the last chance to shape this API without a breaking change. Registration is now generic. Nothing previously tied typeof(X) to the activator that builds it, so a mismatch surfaced as an InvalidCastException out of the caller's Find<T> with no mention of the bad registration. The generated lambda infers T, so RegisterType<T>, RegisterCollection<TCollection, TElement> and RegisterDictionary<TDictionary, TKey, TValue> make type identity and the element, key and value types a compile-time guarantee instead. The add delegates deliberately stay untyped. Making them generic would move the object-to-element cast from generated code into a wrapper delegate, adding a call on the per-element decode path; the factory is generic because it runs once per collection, where a wrapper costs nothing. GeneratedMember's three orthogonal flags described what is really a choice of one source, and the illegal combinations resolved silently: an injectable name together with isNetwork put the member in both collections and network won, and a network member with a meaningful map key dropped the key. Mapped, Injected and Networked make those states unrepresentable, and BuildMetadata switches on the kind rather than testing flags in an order that mattered.
CollectionRoot travels through the incremental pipeline as a plain class with reference equality, holding an ITypeSymbol and a Location, so every recompute was a guaranteed cache miss. This is a prerequisite rather than a win on its own: the output node also combines CompilationProvider, which changes on every keystroke, so it re-runs regardless. Both that and the raw INamedTypeSymbol flowing through the types provider need parsing to move into the syntax transform and diagnostics to be carried as data, which is a redesign of ModelParser and CollectionParser rather than a local change. The reasoning is recorded at the pipeline and at the candidate predicate, including why the base-list arm cannot simply be narrowed to an attribute-name scan: that would drop inherited models, which are supported and tested.
The analyzer is packed from a hard-coded bin\$(Configuration)\netstandard2.0 path. A missing file already fails loudly — NuGet reports NU5019 or a path error — so the package cannot silently ship without an analyzer. The real hazard is narrower: if the generator's target framework or output layout changes, the assembly left behind at the old path is stale but present, and would be packed without complaint. The path now comes from one property, and a guard asks the generator project for its actual GetTargetPath and fails the pack if the two disagree. Verified by pointing the property at a different framework with a stale copy in place, which the guard rejects with both paths named. Resolving the path through the project system instead was the other option, but neither TargetsForTfmSpecificContentInPackage nor a GenerateNuspecDependsOn hook placed the file for this multi-targeted project, and a packaging mechanism that silently produces nothing is a worse trade than a hard-coded path with a guard.
MaxMind.Db.NativeAot/App and MaxMind.Db.NativeAot/Models were absent from the solution, so Program.cs and Models.cs — around 300 lines of real test logic — were never compiled by dotnet build MaxMind.Db.sln, and were invisible to CodeQL's autobuild. Only dev-bin/test-native-aot.sh built them. A plain solution build works because Directory.Build.props falls back to a ProjectReference when MaxMindDbPackageVersion is unset, which is the case outside that script. Precious is unaffected either way: .precious.toml configures only the prettier markdown, JSON and YAML commands, with no C# rule, so solution membership was never what determined whether these files were linted.
Five things, all of which either go stale silently or leave rubbish behind: - package_version was the literal 5.2.0-aot-ci while VersionPrefix said 5.1.0. It is now derived from the library, so the two cannot disagree. - artifacts/native-aot-packages was never cleaned, and a stale package from an aborted run has the same file name as the fresh one, so a restore could pick up either. It is removed before packing. - The mktemp restore directory was never removed and there was no trap, leaking a full package cache per run. - The publish path hard-coded net8.0, so a target framework bump would fail with a bare "No such file or directory" pointing nowhere useful. It is read from the app project, and a missing publish directory now says so. - Building the .NET Standard consumer proved it compiles but never that the generator ran, so a packaging regression there would pass silently. It now asserts the generated registrations exist, matching the check the app's own model project already performs.
Two README claims were wrong. Annotated structs do not simply fall back to reflection: a constructor-based one does and works, but a property-based struct or record struct throws, in a plain JIT build as much as under NativeAOT, because reflection never surfaces a struct's implicit parameterless constructor. And "open generic model classes are not supported" implied closed ones work, when no generic model class works at all — models are discovered from their declarations, so the generator only ever sees the unbound definition. The release notes omitted the things a consumer acts on: the MMDBSG diagnostic range and the MaxMindDbAotDiagnostics property that controls it, the two new public types, and the AlwaysCreate fix, which changes behaviour for property models that were throwing. The bullet about CI publishing a trimmed application on three platforms is internal to this repository, and CLAUDE.md scopes this file to user-facing changes.
IsTypeAccessible calls IsAccessible, so it now comes first. ModelParser had no ordering that satisfied the rule, because GetAttribute had callers both above and below it. Deleting the one-line HasAttribute wrapper and inlining its two call sites resolves that rather than shuffling declarations around.
RegisteredCollectionsTakePrecedenceOverReflection registered an IReadOnlyList<long> and a GeneratedDictionary and then asserted on values that the reflection fallback produces identically: ListActivatorCreator builds a List<long> and DictionaryActivatorCreator finds the parameterless constructor. Bypassing both registration lookups in Decoder left it green. The factories now return canary subclasses and the test asserts the concrete types, so it fails if the generated path is not taken — verified by pointing both lookups at a type that never matches, which fails this test and the non-generic dictionary one and nothing else. Also adds concurrency coverage for the metadata cache, which uses Volatile.Read and Interlocked.CompareExchange and had none. It deliberately does not assert reference identity across threads: a caller that loses the compare-exchange legitimately sees a different instance, so the assertion is that every caller gets complete, correct metadata.
It is process-wide, write-once and set from module initializers, and tests set it too, so it looks like shared mutable state that could make test order matter. Setting it cannot change the outcome for any other type: decoding still requires a registration keyed by the exact type, so an unrelated type misses the lookup and the branch behaves exactly as it would with the flag unset. The flag only decides whether that lookup runs at all.
The benchmark job set no MAXMIND_BENCHMARK_IP_ADDRESSES, so it fell back to 1,000 pseudo-random addresses. All of them miss in the 26-record test database, so no record is found, no model is ever constructed, and BenchmarkDotNet reports zero allocations: the job measured tree traversal and nothing else. These addresses hit records, so the decode path this release changes is actually exercised.
|
Thanks — this was a genuinely valuable review. All 20 threads are answered inline at aa5bb57, one commit per change. Six findings came out differently than described, and I've put the evidence in the relevant threads. Three are worth knowing about up front, because they would otherwise have had us change working code:
Two things I didn't do, both explained in thread: reporting annotated interfaces and abstract classes with no derived type (both would false-positive on legitimate shapes), and the full incremental-generator rework, which is a redesign of Otherwise: the four behaviour bugs are fixed with tests that fail without the fix, MMDBSG013 through MMDBSG016 are new, diagnostics are on by default, the reflection fallback has full Verification at aa5bb57: build 0 warnings, 316 tests, Separately fixed: the CI benchmark job passed no — Claude, on behalf of Greg |
Adding the NativeAOT projects to the solution put them in autobuild's path, and the App project copies test databases out of the MaxMind-DB submodule, which this workflow did not fetch. Autobuild failed with MSB3030 for MaxMind-DB-test-decoder.mmdb and GeoIP2-City-Test.mmdb. Matches what the test workflow already does. The alternative was to make the copy conditional on the files existing, but that trades a loud build failure for an application that builds and then fails at run time with something less obvious.
Reporting a bare type parameter was wrong, and building MaxMind.GeoIP2 against this branch showed it: DatabaseReader.Execute<T> is the canonical generic wrapper, and MMDBSG015 fired there and failed the build, because GeoIP2 treats warnings as errors. It is the only Find<T> call in that library, so every lookup broke. The diagnostic was a false positive. Model registration comes from declarations, not from lookup sites, so the wrapper is irrelevant to it: the same build emits registrations for all ten response types and all eight nested models, and every instantiation of Execute<T> is one of them. "No registration can be generated for this call" was true of the call site and misleading about the outcome. A bare type parameter is silent again. What it cannot rule out is a collection type argument, or a model from an assembly that never ran the generator, and neither is visible from the wrapper — so the README now explains that a wrapper carries models but not collection result types, and why no diagnostic is issued. A constructed type that still contains a type parameter, such as Find<Dictionary<string, T>>, is still reported. With this, GeoIP2 builds against the branch with zero warnings and zero MMDBSG diagnostics on all five target frameworks.
2 participants
Summary
NativeAOT coverage
The integration application consumes a locally packed prerelease package and places its models in a separate assembly. It verifies:
IReadOnlyList<T>,ICollection<T>,LinkedList<T>, dictionary interfaces, and concrete dictionariesDictionary<string, object>, andFindAll<T>[1, 2, 3], the nestedmapX/utf8_stringXvalue, CityLondon, subdivisionEngland, and network81.2.69.160/27The model assembly also fails its build if the generator emits no registrations, so the analyzer going missing from the package cannot pass as a green run.
Reflection fallback under NativeAOT
A model without a generated registration fails at reflection metadata lookup, not at
Expression.Compile:Full trimming removes the parameterless constructor before dynamic code generation is
ever reached, so a
RuntimeFeature.IsDynamicCodeSupportedswitch falling back toConstructorInfo.Invokewould have had nothing to invoke. Source-generated registrationis the only supported trimmed/AOT path, and the fallback now fails with actionable
guidance.
TestReflectionFallbackFailsWithGuidancepins this.Benchmarks
The measurement that matters is the GeoIP2 migration benchmark in the comment
thread, against a full 66 MB GeoLite2 City database over 1M lookups: steady-state
DatabaseReader.City()-4.79% (bootstrap 95% CI [-5.52%, -3.83%], permutationp = 0.0021) and cold start -59%, with the constructor -46.8%, the first lookup
-71.3%, and allocation to first hit -70.8%. Skipping
Expression.Compileis whatdrives the cold-start win.
Separately,
MaxMind.Db.BenchmarkoverGeoIP2-City-Test.mmdbcaught a decode-pathregression that the GeoIP2 run predates:
Decoder.DecodeMapprobed for a generateddictionary registration on every decoded map, costing about 0.6 us of a 30 us City
lookup.
IsGenericTypeis false for an ordinary model, so the probe always ran.Only a non-generic type can need that lookup, so it is now gated on a flag.
That harness cannot resolve the steady-state win above: the 26-record test database
makes decode a small share of each lookup and run-to-run noise is about +/-1.7%, so
treat it as a regression check on the decode path rather than a measure of the
feature. Two related caveats — the CI benchmark job passes no
MAXMIND_BENCHMARK_IP_ADDRESSES, so its 1,000 random IPs all miss the test databaseand no model is ever constructed (
Allocatedreports-), and only paired runsalternating both trees inside one invocation were trustworthy; unpaired comparisons
flipped sign between replicates.
Testing
dotnet build MaxMind.Db.sln -c Release— 0 warnings, 0 errorsdotnet test MaxMind.Db.sln -c Release— 292 passeddotnet pack MaxMind.Db/MaxMind.Db.csproj -c Release— analyzer atanalyzers/dotnet/csandbuildTransitive/MaxMind.Db.targetspresentbash dev-bin/test-native-aot.sh linux-x64— .NET Standard 2.0/2.1 package consumers built, then strict NativeAOT publishing completed with zero IL warnings and the native executable passedprecious lint --all— 16 checks, exit 0 (includes prettier markdown/yaml/json andshellcheck dev-bin/test-native-aot.sh)Summary by CodeRabbit