Fix MVAR resolution in MethodDef_Instance::ResolveToken#3398
Conversation
- MVAR (!!0) resolution now checks the MethodSpec first (authoritative IL-encoded generic argument) and only falls back to arrayElementType when no MethodSpec exists.
|
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 Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR tightens generic method argument resolution in ChangesMVAR Resolution Logic
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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.
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/TypeSystem.cpp (1)
2265-2288:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail unresolved
MethodSpec-to-VARchains instead of storing an empty class token.If
GetGenericArgument()returnsDATATYPE_VARand the caller type context is missing or still doesn't resolve to a concrete type, this path currently falls back tomsArgElem.Class/paramElem.Class. Those fields are not guaranteed to be populated for openVAR/GENERICINST, soresolvedArgs[a]can stay empty and later match the wrong closedTypeSpecinstead of failing resolution.Suggested fix
- if (msInst.GetGenericArgument(argElem.GenericParamPosition, msArgElem)) + if (msInst.GetGenericArgument(argElem.GenericParamPosition, msArgElem)) { if (msArgElem.DataType == DATATYPE_VAR && caller->genericType != nullptr && NANOCLR_INDEX_IS_VALID(*caller->genericType)) { @@ if (callerTsInst.InitializeFromIndex(*caller->genericType) && callerTsInst.GetGenericParam( msArgElem.GenericParamPosition, paramElem)) { - resolvedArgs[a] = paramElem.Class; + if (NANOCLR_INDEX_IS_VALID(paramElem.Class)) + { + resolvedArgs[a] = paramElem.Class; + } + else + { + allResolved = false; + } } else { allResolved = false; } } - else + else if (NANOCLR_INDEX_IS_VALID(msArgElem.Class)) { resolvedArgs[a] = msArgElem.Class; } + else + { + allResolved = false; + } }🤖 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/TypeSystem.cpp` around lines 2265 - 2288, When GetGenericArgument() yields DATATYPE_VAR but the caller type context is missing or cannot resolve to a concrete type, don't fall back to using msArgElem.Class or paramElem.Class (which can be empty); instead mark resolution as failed. In the block checking msArgElem.DataType == DATATYPE_VAR and caller->genericType, if callerTsInst.InitializeFromIndex(...) or callerTsInst.GetGenericParam(...) fails, set allResolved = false and do not assign resolvedArgs[a]; also if msArgElem.DataType == DATATYPE_VAR and the caller context is null/invalid, set allResolved = false rather than assigning msArgElem.Class. This ensures unresolved VAR/GENERICINST chains cause resolution to fail (use symbols: GetGenericArgument, DATATYPE_VAR, caller->genericType, CLR_RT_TypeSpec_Instance, InitializeFromIndex, GetGenericParam, resolvedArgs[a], allResolved).
🤖 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.
Outside diff comments:
In `@src/CLR/Core/TypeSystem.cpp`:
- Around line 2265-2288: When GetGenericArgument() yields DATATYPE_VAR but the
caller type context is missing or cannot resolve to a concrete type, don't fall
back to using msArgElem.Class or paramElem.Class (which can be empty); instead
mark resolution as failed. In the block checking msArgElem.DataType ==
DATATYPE_VAR and caller->genericType, if callerTsInst.InitializeFromIndex(...)
or callerTsInst.GetGenericParam(...) fails, set allResolved = false and do not
assign resolvedArgs[a]; also if msArgElem.DataType == DATATYPE_VAR and the
caller context is null/invalid, set allResolved = false rather than assigning
msArgElem.Class. This ensures unresolved VAR/GENERICINST chains cause resolution
to fail (use symbols: GetGenericArgument, DATATYPE_VAR, caller->genericType,
CLR_RT_TypeSpec_Instance, InitializeFromIndex, GetGenericParam, resolvedArgs[a],
allResolved).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5a3dcec8-58cd-48e1-aaca-8e27b445c95f
📒 Files selected for processing (1)
src/CLR/Core/TypeSystem.cpp
2 participants
Description
Motivation and Context
How Has This Been Tested?
Screenshots
Types of changes
Checklist