Fix signature parser usage #3215
Conversation
- Replaced wrong check of generics parameter count in parser when it should be in element.
WalkthroughReplaced parser-based generic parameter counts with element-based counts and switched TypeSpec resolution from TypeSpec() row references to direct data-token usage. Updated generic argument parsing, type-name emission, and generic resolution across Type, Execution, and TypeSystem code paths, affecting iteration bounds and the source of tokens during resolution. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Caller (TypeSpec)
participant EE as ExecutionEngine
participant TS as TypeSystem
participant Parser as SigParser
rect rgba(230,240,255,0.6)
note over Caller,TS: Generic resolution using data tokens
Caller->>EE: InitializeLocals (signature with GENERICINST)
EE->>Parser: Advance to GENERICINST
Parser-->>EE: element.GenParamCount
EE->>EE: Loop paramIndex < element.GenParamCount
end
rect rgba(235,255,235,0.6)
note over EE,TS: Resolve VAR against caller
EE->>TS: Resolve VAR (!T / !!M) with caller.genericType->data
TS->>TS: FindGenericParamAtTypeSpec(caller.genericType->data, index)
TS-->>EE: Resolved concrete type
end
rect rgba(255,245,230,0.6)
note over Parser,TS: Type/name emission
TS->>Parser: Parse GENERICINST
Parser-->>TS: element.GenParamCount
TS->>TS: Emit/iterate using element.GenParamCount
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
Automated fixes for code style.
…581d-32e1-41bc-96e0-d984c5f0658f Code style fixes for nanoframework/nf-interpreter PR#3215
|
@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: 0
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/Execution.cpp (1)
2091-2115: Potential null dereference when resolving DATATYPE_VAR without a generic contextElse-branch dereferences methodDefInstance.genericType even when it can be nullptr (or data==0), leading to a crash on locals that include VAR outside a closed generic context.
Apply this guard to avoid NPE and fail fast if resolution isn’t possible:
case DATATYPE_VAR: { // type-level generic parameter in a locals signature (e.g. 'T' inside a generic type) CLR_INT8 genericParamPosition = *sig++; if (methodDefInstance.genericType && methodDefInstance.genericType->data != 0) { CLR_RT_TypeSpec_Instance typeSpec{}; typeSpec.InitializeFromIndex( (const CLR_RT_TypeSpec_Index &)methodDefInstance.genericType->data); typeSpec.assembly->FindGenericParamAtTypeSpec( - methodDefInstance.genericType->data, + methodDefInstance.genericType->data, genericParamPosition, cls, dt); } else { - assembly->FindGenericParamAtTypeSpec( - methodDefInstance.genericType->data, - genericParamPosition, - cls, - dt); + // No closed generic type available; cannot resolve VAR safely. + NANOCLR_SET_AND_LEAVE(CLR_E_WRONG_TYPE); } goto done; }If you prefer a softer fallback, you can instead compute a safe token and attempt resolution, bailing on failure:
- else - { - assembly->FindGenericParamAtTypeSpec( - methodDefInstance.genericType->data, - genericParamPosition, - cls, - dt); - } + else + { + const CLR_UINT32 specData = + (methodDefInstance.genericType != nullptr) ? methodDefInstance.genericType->data : 0; + if (!assembly->FindGenericParamAtTypeSpec(specData, genericParamPosition, cls, dt)) + { + NANOCLR_SET_AND_LEAVE(CLR_E_WRONG_TYPE); + } + }
🧹 Nitpick comments (1)
src/CLR/CorLib/corlib_native_System_Type.cpp (1)
428-461: Refine IsGenericTypeDefinition check to require all args be unbound (VAR/MVAR)Current logic sets the result based on only the first generic argument. Tighten to scan all args and return true only if every arg is VAR/MVAR.
Apply:
// check if it has generic parameters - if (element.GenParamCount == 0) + if (element.GenParamCount == 0) { isTypeDefinition = false; } else { - // keep reading the generic parameters - for (int i = 0; i < element.GenParamCount; i++) - { - if (SUCCEEDED(parser.Advance(element))) - { - if (element.DataType == DATATYPE_VAR || element.DataType == DATATYPE_MVAR) - { - // if we find a VAR or MVAR, then this is a generic type definition - isTypeDefinition = true; - break; - } - else - { - // anything else, this is not a generic type definition - isTypeDefinition = false; - break; - } - } - } + bool allUnbound = true; + for (int i = 0; i < element.GenParamCount; i++) + { + if (FAILED(parser.Advance(element))) + { + allUnbound = false; + break; + } + if (element.DataType != DATATYPE_VAR && element.DataType != DATATYPE_MVAR) + { + allUnbound = false; + break; + } + } + isTypeDefinition = allUnbound; }Please validate against:
- typeof(List<>).IsGenericTypeDefinition == true
- typeof(Dictionary<,>).IsGenericTypeDefinition == true
- typeof(Dictionary<int, string>).IsGenericTypeDefinition == false
- typeof(Dictionary<,>).MakeGenericType(typeof(int), typeof(List<>)).IsGenericTypeDefinition == false
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/CLR/CorLib/corlib_native_System_Type.cpp(2 hunks)src/CLR/Core/Execution.cpp(2 hunks)src/CLR/Core/TypeSystem.cpp(12 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-14T16:27:02.573Z
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3172
File: src/CLR/Core/CLR_RT_HeapBlock.cpp:899-900
Timestamp: 2025-05-14T16:27:02.573Z
Learning: The CLR_RT_TypeDescriptor type in nanoFramework doesn't have a GetElementType() API for extracting array element types.
Applied to files:
src/CLR/Core/TypeSystem.cpp
📚 Learning: 2025-05-14T17:33:51.546Z
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3172
File: src/CLR/Core/TypeSystem.cpp:4556-4589
Timestamp: 2025-05-14T17:33:51.546Z
Learning: When parsing TypeSpec signatures in nanoFramework, the first Advance() call consumes the VAR/MVAR token, followed by additional Advance() calls to walk to the specific generic parameter position.
Applied to files:
src/CLR/Core/TypeSystem.cppsrc/CLR/Core/Execution.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: nf-interpreter (Build_WIN32_nanoCLR)
- GitHub Check: nf-interpreter (Build_TI_SimpleLink_targets TI_CC1352R1_LAUNCHXL_915)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_P4_UART)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_ETHERNET_KIT_1.2)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_S3_ALL)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_H2_THREAD)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_C6_THREAD)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_C3)
- GitHub Check: nf-interpreter (Build_STM32_targets ST_STM32F769I_DISCOVERY)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_BLE_REV0)
- GitHub Check: nf-interpreter (Build_STM32_targets ST_STM32F429I_DISCOVERY)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_PSRAM_REV0)
- GitHub Check: nf-interpreter (Build_Azure_RTOS_targets SL_STK3701A)
- GitHub Check: nf-interpreter (Build_NXP_targets NXP_MIMXRT1060_EVK)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALX)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALTHREE)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
🔇 Additional comments (11)
src/CLR/Core/Execution.cpp (1)
2076-2081: Correct: use element.GenParamCount instead of parser stateLooping over varElement.GenParamCount matches the semantics of the advanced element and avoids parser-state drift on nested generics.
Please sanity‑check nested cases like List<Dictionary<string, List>> locals to confirm the loop consumes exactly the right number of args.
src/CLR/CorLib/corlib_native_System_Type.cpp (1)
141-151: Correct: derive argument count from the advanced elementUsing elem.GenParamCount (after consuming class|valuetype) is the right source for the arity of the current generic instance.
src/CLR/Core/TypeSystem.cpp (9)
803-807: Switch to data token from TypeSpec reference for generic parameter resolution.The code correctly changes from using
TypeSpec()row references to directdatatoken usage for generic parameter resolution. This aligns with the PR description about fixing signature parser usage.
1168-1172: Consistent use of data token for generic parameter mapping.Good change to use
caller->genericType->datainstead ofTypeSpec()for generic parameter resolution. This ensures consistency with the new approach of using direct data tokens.
1289-1290: Correct data token usage in nullable type resolution.The change from
caller->genericType->TypeSpec()tocaller->genericType->datais correct and maintains consistency with the overall fix.
1578-1578: Proper data token usage in method initialization.The change to use
typeSpec.datainstead of calling a method ontypeSpecis consistent with the pattern throughout this fix.
1882-1882: Consistent data token access pattern.The change maintains the consistent pattern of using
.datadirectly instead of method calls for TypeSpec access.
2273-2273: Data token usage in signature token initialization.Correct change to use direct data token access for consistency with the overall fix.
5095-5095: Check generic parameter count bounds using element data.The code now correctly uses
element.GenParamCountinstead ofparser.GenParamCountfor bounds checking. This fixes the issue mentioned in the PR description where the check should be on the element instead of the parser.
5628-5628: Use element parameter count for generic signature building.The changes correctly switch from using
parser.GenParamCounttoelement.GenParamCountwhen iterating through generic parameters and building type signatures. This is consistent with the fix description about using element-based counts instead of parser-based counts.Also applies to: 5635-5635, 5647-5647, 5665-5665
6900-6900: Consistent use of element parameter count in method name building.The changes correctly use
element.GenParamCountinstead ofparser.GenParamCountfor loop bounds when building generic method signatures, maintaining consistency with the overall fix pattern.Also applies to: 6984-6984
2 participants
Description
Motivation and Context
How Has This Been Tested?
[build with MDP buildId 57148]
Screenshots
Types of changes
Checklist
Summary by CodeRabbit
Bug Fixes
Performance and Stability
Refactor