Fix nested generic TypeSpec emission#235
Conversation
📝 WalkthroughWalkthroughThis PR updates the metadata processor to implement ECMA-335 cumulative indexing rules for nested generic types. It refactors signature encoding to compute absolute VAR indices by walking declaring-type chains and ensures generic instances include enclosing type arguments. Test assertions are made version-independent, and a new regression test validates nested generic struct TypeSpec correctness. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@MetadataProcessor.Shared/Tables/nanoSignaturesTable.cs`:
- Line 450: The cast to byte when writing cumulative indices (e.g., the
writer.WriteByte((byte)absolutePosition) call and the similar writes around
lines 548-549) can silently truncate values; before casting, validate the int
value is within 0..255 and fail fast if not (throw an informative
ArgumentOutOfRangeException or custom exception that includes the variable name
and actual value), or switch to writing a wider type if larger ranges are
expected; update the code paths that compute absolutePosition/cumulativeIndex to
perform this bounds check and include the variable name and offending value in
the thrown message.
- Around line 443-448: The code currently assumes varParam.Owner is a
TypeDefinition (declaringTypeDef) and therefore skips the MethodDefinition case;
update handling for varParam.Owner so it checks its runtime type: if Owner is a
TypeDefinition, traverse its DeclaringType chain and sum each
DeclaringType.GenericParameters.Count into absolutePosition (using the existing
declaringTypeDef loop), and if Owner is a MethodDefinition, first set a local
methodDecl = (MethodDefinition)varParam.Owner, then take
methodDecl.DeclaringType and traverse that DeclaringType chain adding each
DeclaringType.GenericParameters.Count to absolutePosition; ensure you still
account for the method-level generic parameters correctly (treat method generic
params separately as needed) and reference varParam.Owner, TypeDefinition,
MethodDefinition, declaringTypeDef, and GenericParameters.Count when making
these changes.
- Around line 526-545: The current check uses
declaringTypeRef.HasGenericParameters which is false for closed
GenericInstanceType and blocks recovering enclosing generic args; change the
logic so that you don't gate recovery on HasGenericParameters alone — first
detect if declaringTypeRef is a GenericInstanceType and recover its
GenericArguments (assign to enclosingArgs) even when HasGenericParameters is
false, and only fall back to HasGenericParameters-based logic otherwise; update
the code paths around declaringTypeRef, declaringGenericInst, enclosingArgs,
elementType and genericType.GenericArguments so enclosed-type arguments from a
GenericInstanceType are always considered.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: Repository: nanoframework/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5962d447-59d0-43b0-a74f-62ade66fa786
📒 Files selected for processing (5)
MetadataProcessor.Shared/Tables/nanoSignaturesTable.csMetadataProcessor.Tests/Core/Utility/DumperTests.csMetadataProcessor.Tests/TestNFApp/NestedGenericTypeSpecTests.csMetadataProcessor.Tests/TestNFApp/Program.csMetadataProcessor.Tests/TestNFApp/TestNFApp.nfproj
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. An unexpected error occurred while generating fixes: Not Found - https://docs.github.com/rest/git/refs#get-a-reference |
- Emit cumulative generic-argument count and write enclosing-type arguments before nested-type arguments; compute and write absolute VAR indices for nested generic parameters. - Fix signature for nested helper types. - Fix container index for For generic methods on non-generic declaring types. - Add regression test (NestedGenericTypeSpecTests) to test app exercising nested generic struct TypeSpec. - Improve DumpAssemblyTest() unit test to grab indexed from metadatables instead of being hardcoded.
- Types sharing a nested name (e.g. Enumerator) but differing only in their enclosing generic type (e.g. Container1<T> vs Container2<TKey,TValue>) were incorrectly mapped to the same TypeSpec entry. The root cause was that the parent type's concrete generic arguments were recovered from the uninstantiated element type's declaring type instead of from the instantiated GenericInstanceType.DeclaringType, causing signatures to be identical and MemberRefs to resolve to the wrong type at runtime. - Added test class to prevent regression.
38e7c8f to
ba15616
Compare
Description
Motivation and Context
Dictionary<TKey, TValue>in System.Collections.How Has This Been Tested?
Dictionary<TKey, TValue>.Screenshots
Types of changes
Checklist: