Fix nested generic constructor - #3501
Conversation
- Now properly resolving nested types in CEE_NEWOBJ. - Update CLAUDE with reasoning and new finding. Assisted by Claude Opus 4.8.
josesimoes
added
the
Area: Interpreter
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
ChangesNested generic construction
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CEE_NEWOBJ
participant CallerGenericContext
participant MetadataAssemblies
CEE_NEWOBJ->>CallerGenericContext: resolve callee generic arguments
CEE_NEWOBJ->>MetadataAssemblies: search for matching closed TypeSpec
MetadataAssemblies-->>CEE_NEWOBJ: return TypeSpec or no match
CEE_NEWOBJ->>CEE_NEWOBJ: select nested generic context
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
✅ Action performedReview finished.
|
Automated fixes for code style.
…142c8-194a-42fa-a1f7-0da833b3ce89 Code style fixes for nanoframework/nf-interpreter PR#3501
![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 (1)
src/CLR/Core/Interpreter.cpp (1)
3019-3027: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
genericType/arrayElementTypeinvariant violated when a nested closed TypeSpec is found.
calleeInst.arrayElementTypeis pre-set topropagatedArrayElementTypeat line 3027, before nested-generic resolution runs. WhenclosedMatch != nullptr(line 3179), onlycalleeInst.genericTypeis updated —arrayElementTypeis never cleared. If the caller frame had a valid propagatedarrayElementType(e.g. from an SZArrayHelper/array-dispatch context), both fields end up simultaneously valid on the same constructor frame, exactly the corruption case CLAUDE.md documents: "corruptsnewarr's element-type resolution (nativeAccessViolation)."🐛 Proposed fix
if (closedMatch != nullptr) { calleeInst.genericType = closedMatch; nestedGenericTag = closedMatch; + // Invariant: never set both genericType and arrayElementType — see CLAUDE.md §9. + calleeInst.arrayElementType = CLR_RT_TypeDef_Index{}; } else if (argCount == 1) { // Never set alongside genericType above — see CLAUDE.md §9. calleeInst.arrayElementType = resolvedArgs[0]; }Also applies to: 3179-3192
🤖 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 `@src/CLR/Core/Interpreter.cpp` around lines 3019 - 3027, When nested closed TypeSpec resolution finds a non-null closedMatch, update the constructor frame so calleeInst.genericType is set and calleeInst.arrayElementType is cleared, preserving the invariant that both fields are never simultaneously valid. Adjust the logic around calleeInst initialization and the closedMatch handling near ResolveToken to ensure the clear occurs only for this nested closed-TypeSpec path.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 `@src/CLR/Core/Interpreter.cpp`:
- Around line 3086-3127: Update the generic-argument loop in the
owner-resolution block to fully consume each argument subtree before processing
the next argument: repeatedly advance the signature parser until the current
argument’s Available() count reaches zero. Preserve resolution for concrete
class arguments, but mark resolution failed for arrays or open generic
parameters, matching the behavior of the analogous type-signature closure
helpers.
---
Outside diff comments:
In `@src/CLR/Core/Interpreter.cpp`:
- Around line 3019-3027: When nested closed TypeSpec resolution finds a non-null
closedMatch, update the constructor frame so calleeInst.genericType is set and
calleeInst.arrayElementType is cleared, preserving the invariant that both
fields are never simultaneously valid. Adjust the logic around calleeInst
initialization and the closedMatch handling near ResolveToken to ensure the
clear occurs only for this nested closed-TypeSpec path.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c735981c-71b5-49b6-81ea-53d80ec77635
📒 Files selected for processing (2)
src/CLR/Core/CLAUDE.mdsrc/CLR/Core/Interpreter.cpp
2 participants
Description
Motivation and Context
How Has This Been Tested?
Screenshots
Types of changes
Checklist