More fixes with generics #3209
More fixes with generics #3209
Conversation
- Token resolution for TypeSpec now takes caller parameter. - TypeSpec Instance now caches element TypeDef on token resolution (when parsing VAR). - TypeSpec now stores level to deal with arrays from TypeSpecs. - ClearInstance() now clears all elements.
- Calls appropriate reflection and returning the correct reflection for the closed type instance.
- When creating instance now uses proper reflection data with cached element type. - Rename local var for clarity. - Revert fix added before to tackle creation of jagged arrays from TypeSpec.
WalkthroughThe change set updates TypeSpec resolution to be context-aware (caller-based), adds array TypeSpec handling in the interpreter, modifies array instance creation to use synthesized reflections from TypeSpec, and expands the type system with new fields and updated ResolveToken signatures. Public headers are updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant IL as IL Interpreter
participant TS as CLR_RT_TypeSpec_Instance
participant TD as CLR_RT_TypeDef_Instance
participant MD as Caller Method (context)
rect rgba(230,240,255,0.6)
note over IL: CEE_LDTOKEN on TypeSpec
IL->>TS: ResolveToken(tsToken, assm, caller=MD)
alt VAR/MVAR in TypeSpec
TS->>MD: Query generic bindings
TS->>TD: Map generic param to concrete type
TD-->>TS: Resolved typeDef
else Non-generic or concrete TypeSpec
TS-->>IL: levels, cachedElementType / genericTypeDef
end
end
alt Array TypeSpec (levels > 0)
IL->>IL: Build REFLECTION_TYPE { levels, element = (genericTypeDef | cachedElementType) }
IL-->>IL: Push reflection to eval stack
else Non-array TypeSpec
IL->>IL: Determine reflection from concrete/generic type
IL-->>IL: Push reflection to eval stack
end
sequenceDiagram
autonumber
participant VM as Runtime
participant TS as CLR_RT_TypeSpec_Instance
participant AR as CLR_RT_HeapBlock_Array
rect rgba(240,255,240,0.6)
note over VM: Array.CreateInstance with TypeSpec token
VM->>TS: ResolveToken(tsToken, assm, caller)
TS-->>VM: levels, cachedElementType
VM->>AR: CreateInstance(reference, length, assm, tk, caller)
AR->>AR: Synthesize REFLECTION_TYPE { kind=TYPE, levels, data.type=cachedElementType }
AR->>AR: SetReflection(reflex)
AR-->>VM: Array instance created
end
rect rgba(255,245,230,0.6)
note over AR: Non-REFLECTION_TYPE rejected early
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks (1 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ 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.
|
@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: 2
🧹 Nitpick comments (3)
src/CLR/Include/nanoCLR_Runtime.h (1)
2089-2094: Initialize and document new fields (levels, cachedElementType)Confirm
ClearInstance()and all initializers zero these new members; add a brief comment describing their semantics (array-rank; cached element TypeDef for VAR/MVAR or non-generic cases).src/CLR/Core/Interpreter.cpp (1)
3310-3313: Fix typo in commentMinor nit in the added comment: “caller’sc”.
Apply:
-// this has to provide the closed instance of the type in the context of the caller'sc +// this has to provide the closed instance of the type in the context of the caller'ssrc/CLR/Core/CLR_RT_HeapBlock_Array.cpp (1)
120-123: Array-rank increment: sanity check (nit)
ref.ReflectionData().levels++can overflowCLR_UINT16(ReflectionDef uses 16 bits). Unlikely but consider guarding or asserting in debug builds.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/CLR/Core/CLR_RT_HeapBlock_Array.cpp(2 hunks)src/CLR/Core/Interpreter.cpp(1 hunks)src/CLR/Core/TypeSystem.cpp(6 hunks)src/CLR/Include/nanoCLR_Runtime.h(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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.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_nanoCLR_CLI)
- 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_ESP32_targets ESP32_BLE_REV0)
- GitHub Check: nf-interpreter (Build_WIN32_nanoCLR)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_PSRAM_REV0)
- GitHub Check: nf-interpreter (Build_STM32_targets ST_STM32F769I_DISCOVERY)
- GitHub Check: nf-interpreter (Build_NXP_targets NXP_MIMXRT1060_EVK)
- GitHub Check: nf-interpreter (Build_Azure_RTOS_targets SL_STK3701A)
- GitHub Check: nf-interpreter (Build_TI_SimpleLink_targets TI_CC1352R1_LAUNCHXL_915)
- GitHub Check: nf-interpreter (Build_STM32_targets ST_STM32F429I_DISCOVERY)
- 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 (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
🔇 Additional comments (12)
src/CLR/Include/nanoCLR_Runtime.h (1)
2089-2101: Propagate the newcallerargument to every CLR_RT_TypeSpec_Instance::ResolveToken call site.
The optionalcallermust be forwarded from callers (especially array/TypeSpec resolution paths) to avoid resolving open generics in the wrong context. Automated scan returned no ResolveToken usages—verify manually.src/CLR/Core/Interpreter.cpp (1)
3310-3365: Context-aware TypeSpec resolution for ldtoken: LGTMPassing
&stack->m_calland synthesizing array reflections vialevels/element TypeDef is the right fix for closed-type instances and arrays.src/CLR/Core/CLR_RT_HeapBlock_Array.cpp (1)
27-35: Array CreateInstance(reflection) strictness: LGTMEarly exit on non-
REFLECTION_TYPEand direct use ofreflex.data.typefor rank-1 is sound given the new upstream synthesis.src/CLR/Core/TypeSystem.cpp (9)
619-621: Reset generic instance flag correctly after parsing the instance.Good fix. This ensures the
IsGenericInstflag is properly reset after processing a generic instance, preventing incorrect state from persisting to subsequent parsing operations.
714-721: Add error handling for signature parsing in TypeSpec initialization.The addition of error handling for
parser.Advance()is a good defensive programming practice. This ensures that malformed TypeSpec signatures are caught early and the instance is properly cleared on failure.
722-722: Store levels from parsed element for array handling.Caching the
levelsvalue from the parsed element is essential for proper array TypeSpec handling, especially for jagged arrays originating from TypeSpecs.
735-735: Use ClearInstance() for consistent cleanup.Good consistency improvement. Using
ClearInstance()instead of direct field nulling ensures all fields are properly reset, reducing the chance of leaving the instance in an inconsistent state.
747-750: Comprehensive cleanup in ClearInstance().The additions to
ClearInstance()ensure that all new fields (levels,genericTypeDef, and resettingdata) are properly cleared. This prevents stale data from persisting across instance reuses.
752-756: Add caller context to ResolveToken for generic resolution.The addition of the
callerparameter toResolveTokenenables context-aware type resolution, which is crucial for properly resolving type-generic slots (!T) and method-generic slots (!!T) in generic contexts.
775-776: Cache levels for array handling in TypeSpec resolution.Good addition. Storing the
levelsvalue during token resolution ensures array dimension information is preserved for later use.
784-821: Implement VAR handling for type-generic parameters.The implementation correctly resolves type-generic parameters (
!T) against the caller's closed generic type. The error handling for null caller or genericType is appropriate, ensuring the function fails gracefully when the required context is missing.The resolution logic:
- Extracts the generic parameter position
- Uses the caller's bound genericType to find the actual type
- Updates assembly and target references
- Stores the resolved type in
cachedElementTypeThis is a critical fix for proper generic type instantiation.
815-821: Cache element type for non-generic instances.Good addition. Storing the
cachedElementTypefor non-generic instances ensures that the element type information is available when needed, particularly for array creation from TypeSpecs.
2 participants
Description
Motivation and Context
How Has This Been Tested?
[build with MDP buildId 56949]
Screenshots
Types of changes
Checklist
Summary by CodeRabbit
New Features
Bug Fixes