Fix asserts for TypeSpecs not in lookup table#233
Conversation
📝 WalkthroughWalkthroughTwo files in the metadata processor are modified to improve synchronization of type specification lookups. Changes ensure both internal dictionaries are updated consistently when seeding from member references, and document why certain lookup failures are benign. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
MetadataProcessor.Shared/Tables/nanoTypeSpecificationsTable.cs (2)
329-341: 🛠️ Refactor suggestion | 🟠 Major
FillTypeSpecsFromTypeshas the same direct-Addgap.Line 337 (
_idByTypeSpecifications.Add(genericInstanceType, sigId);) adds only to the primary dictionary and skips_indexByTypeReference, mirroring the issue the PR fixes elsewhere. Route this throughAddIfNewas well to keep the two maps coherent and make the downstreamTryGetTypeReferenceIdlookups deterministic.♻️ Proposed fix
if (genericInstanceType != null && !_idByTypeSpecifications.ContainsKey(genericInstanceType) && !genericInstanceType.IsToExclude()) { ushort sigId = _context.SignaturesTable.GetOrCreateSignatureId(genericInstanceType); - _idByTypeSpecifications.Add(genericInstanceType, sigId); + AddIfNew(genericInstanceType, sigId); // also pull in its element‐type and args ExpandNestedTypeSpecs(genericInstanceType); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MetadataProcessor.Shared/Tables/nanoTypeSpecificationsTable.cs` around lines 329 - 341, In FillTypeSpecsFromTypes there's a direct call to _idByTypeSpecifications.Add(genericInstanceType, sigId) that skips updating the parallel map _indexByTypeReference; replace this direct Add with the existing helper (call AddIfNew(genericInstanceType, sigId)) so both _idByTypeSpecifications and _indexByTypeReference are updated atomically and keep TryGetTypeReferenceId deterministic; ensure you still call ExpandNestedTypeSpecs(genericInstanceType) afterward and handle genericInstanceMethod / genericInstanceType as before.
266-304: 🛠️ Refactor suggestion | 🟠 MajorSame divergence still exists in the follow-up loops within
FillTypeSpecsFromMemberReferences.The two subsequent loops (GenericInstanceType seeding from
TypeReferencesTableat lines 272/280, and theallGenericInstancesunion at line 301) still call_idByTypeSpecifications.Add(...)directly and never update_indexByTypeReference. That reproduces exactly the failure mode documented in the new comments innanoAssemblyBuilder.cs(scenario 2: "Added only to_idByTypeSpecifications…TryGetTypeReferenceIdcannot find it"), so any caller relying onTryGetTypeReferenceIdfor these entries will still miss them untilResetTypeSpecificationsTable()runs.Consider routing these through
AddIfNewfor consistency and to fully eliminate the benign-lookup-miss case the PR is papering over.♻️ Proposed refactor
foreach (GenericInstanceType genericInstanceType in _context.TypeReferencesTable.Items.OfType<GenericInstanceType>()) { if (!_idByTypeSpecifications.ContainsKey(genericInstanceType) && !genericInstanceType.IsToExclude()) { // create or get the signature ID for this instanced type ushort sigId = _context.SignaturesTable.GetOrCreateSignatureId(genericInstanceType); - _idByTypeSpecifications.Add(genericInstanceType, sigId); + AddIfNew(genericInstanceType, sigId); // (and don’t forget to pull in any nested generic-parameter args) foreach (GenericParameter arg in genericInstanceType.GenericArguments.OfType<GenericParameter>()) { if (!_idByTypeSpecifications.ContainsKey(arg) && !arg.IsToExclude()) { ushort argSig = _context.SignaturesTable.GetOrCreateSignatureId(arg); - _idByTypeSpecifications.Add(arg, argSig); + AddIfNew(arg, argSig); } } } } @@ foreach (TypeReference typeRefItem in allGenericInstances) { if (!_idByTypeSpecifications.ContainsKey(typeRefItem) && !typeRefItem.IsToExclude()) { ushort sigId = _context.SignaturesTable.GetOrCreateSignatureId(typeRefItem); - _idByTypeSpecifications.Add(typeRefItem, sigId); + AddIfNew(typeRefItem, sigId); ExpandNestedTypeSpecs(typeRefItem); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MetadataProcessor.Shared/Tables/nanoTypeSpecificationsTable.cs` around lines 266 - 304, In FillTypeSpecsFromMemberReferences the loops that seed GenericInstanceType and GenericParameter currently call _idByTypeSpecifications.Add(...) directly (in the first foreach and in the allGenericInstances loop), which omits updating _indexByTypeReference and causes TryGetTypeReferenceId misses; replace those direct adds with the existing helper AddIfNew(...) so entries go through the canonical insertion path (use AddIfNew(genericInstanceType, sigId) and AddIfNew(arg, argSig) and AddIfNew(typeRefItem, sigId) respectively) and keep the ExpandNestedTypeSpecs(typeRefItem) call as-is.
🤖 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/nanoAssemblyBuilder.cs`:
- Around line 372-378: The Debug.Fail removal is fine, but to make scenario 2
truly benign you must stop masking the lookup-table inconsistency by routing all
direct writes to the type-spec ID map through the canonical add helper: replace
the direct uses of _idByTypeSpecifications.Add (the calls currently in
nanoTypeSpecificationsTable: the adds at the sites that correspond to the
earlier lines 272, 280, 301, 337) with the existing AddIfNew helper so that
AddIfNew enforces uniqueness and consistent state; ensure AddIfNew is invoked
from FillTypeSpecsFromMemberReferences and any other code paths that previously
called _idByTypeSpecifications.Add so TryGetTypeReferenceId no longer relies on
a separate implicit window opened by ResetTypeSpecificationsTable.
---
Outside diff comments:
In `@MetadataProcessor.Shared/Tables/nanoTypeSpecificationsTable.cs`:
- Around line 329-341: In FillTypeSpecsFromTypes there's a direct call to
_idByTypeSpecifications.Add(genericInstanceType, sigId) that skips updating the
parallel map _indexByTypeReference; replace this direct Add with the existing
helper (call AddIfNew(genericInstanceType, sigId)) so both
_idByTypeSpecifications and _indexByTypeReference are updated atomically and
keep TryGetTypeReferenceId deterministic; ensure you still call
ExpandNestedTypeSpecs(genericInstanceType) afterward and handle
genericInstanceMethod / genericInstanceType as before.
- Around line 266-304: In FillTypeSpecsFromMemberReferences the loops that seed
GenericInstanceType and GenericParameter currently call
_idByTypeSpecifications.Add(...) directly (in the first foreach and in the
allGenericInstances loop), which omits updating _indexByTypeReference and causes
TryGetTypeReferenceId misses; replace those direct adds with the existing helper
AddIfNew(...) so entries go through the canonical insertion path (use
AddIfNew(genericInstanceType, sigId) and AddIfNew(arg, argSig) and
AddIfNew(typeRefItem, sigId) respectively) and keep the
ExpandNestedTypeSpecs(typeRefItem) call as-is.
🪄 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: Repository: nanoframework/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ab5fa57e-6007-404c-a125-320b143ee10e
📒 Files selected for processing (2)
MetadataProcessor.Shared/Tables/nanoTypeSpecificationsTable.csMetadataProcessor.Shared/nanoAssemblyBuilder.cs
- Fix FillTypeSpecsFromMemberReferences to use AddIfNew instead of adding directly to _idByTypeSpecifications, so RID!=0 TypeSpecs from member references are now reachable via TryGetTypeReferenceId. - Remove asserts in Minimize() and replace with explanatory comment in case someone debugging this code hits the else.
josesimoes
force-pushed
the
fix-asserts-typespecs
branch
from
b4dddb9 to
b184c68
Compare
josesimoes
changed the title
2 participants
Description
Motivation and Context
How Has This Been Tested?
Screenshots
Types of changes
Checklist: