Work to add new features and fixes in type system and execution engine related with generics #3240
Conversation
- Now extracts the element TypeDef directly from the array's ReflectionData(), which contains the actual element type.
Automated fixes for code style.
- Add checks for null objects and perform accordingly.
Automated fixes for code style.
- Now uses generic context for correct and complete type resolution. - Fix hash computation for field instance. - Fix call for static field getter in ldsfld, ldsflda, stsfld and callvirt. *** PR separately ***
- Now setting the correct context to allow NewObject to properly resolve all generic types. *** PR separately ***
…eter into add-arrayhelper
- Code now grabs element type from array instead of element when element is null.
- Now getting generic context from parameter. - When finding a VAR or MVAR nows decodes type from context.
- Now when it encounters a GENERICINST it passes the generic context (if available) and the resolution is left to the callee.
- Alse reworked the code to remove redundant processing as now everything is handled in the call to InitializeFromSignatureToken.
- Now checks if the method is generic and there is a generic context available.
…ution - Included some refactoring to remove wrong code.
|
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 You can disable this status message by setting the WalkthroughIntroduces comprehensive generic type and method context propagation throughout the CLR runtime. Key additions include delegate generic method specifications, stack frame generic context storage, context-aware type and method resolution APIs, on-demand generic static field allocation, and enhanced parameter resolution for closed generic instantiations across execution, type system, and interpreter layers. Changes
Sequence DiagramsequenceDiagram
actor Caller
participant StackFrame
participant Interpreter
participant TypeSystem
participant GenericContext as Generic<br/>Context
Caller->>StackFrame: Push (with generic context)
activate StackFrame
StackFrame->>StackFrame: Initialize m_genericTypeSpecStorage
StackFrame->>GenericContext: Store context for resolution
deactivate StackFrame
Caller->>Interpreter: Execute IL (CallVirt/Castclass)
activate Interpreter
rect rgb(200, 220, 255)
note over Interpreter: New: Infer closed TypeSpec<br/>from caller context
Interpreter->>GenericContext: Query caller's object & TypeDef
Interpreter->>GenericContext: Build effectiveCallerGeneric
end
Interpreter->>TypeSystem: Resolve token with context
activate TypeSystem
rect rgb(220, 200, 255)
note over TypeSystem: New: Context-aware<br/>VAR/MVAR resolution
TypeSystem->>GenericContext: GetGenericParam(position, context)
GenericContext-->>TypeSystem: Resolved type + datatype
end
TypeSystem-->>Interpreter: Type resolved
deactivate TypeSystem
Interpreter->>StackFrame: Create new frame
activate StackFrame
StackFrame->>GenericContext: Copy arrayElementType (SZArray)
StackFrame->>GenericContext: Store propagatedArrayElementType
deactivate StackFrame
Interpreter-->>Caller: Return with restored context
deactivate Interpreter
rect rgb(200, 255, 200)
note over Interpreter,GenericContext: Context preserved through<br/>delegate dispatch & MVAR resolution
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Automated fixes for code style.
…d7a15-a5b9-446d-8b4d-ea84912619b2 Code style fixes for nanoframework/nf-interpreter PR#3240
|
@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: 3
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)
66-79: Add null-check for genericInstance before dereferencing.The code dereferences
genericInstanceon line 67 without validating it's non-null. While the calling context may ensure this for VAR cases, defensive programming would add a null-check to prevent potential crashes if the invariant is violated.Apply this diff to add a safety check:
if (dt == DATATYPE_VAR) { + if (genericInstance == nullptr || !NANOCLR_INDEX_IS_VALID(*genericInstance)) + { + NANOCLR_SET_AND_LEAVE(CLR_E_FAIL); + } + CLR_RT_TypeSpec_Instance genericTs; if (!genericTs.InitializeFromIndex(*genericInstance))
🧹 Nitpick comments (12)
src/CLR/Core/Thread.cpp (1)
166-219: Generic context propagation from delegate to stack frame looks correct; minor simplification possibleThe way you:
- copy
m_genericTypeSpecinto a temporary, pass it throughinst.genericTypeintoPush(), and then move it intom_genericTypeSpecStorageon the new frame, and- propagate
m_genericMethodSpecontoinst.methodSpecand then tostackTop->m_call.methodSpecis sound and avoids dangling pointers.
Given that
CLR_RT_StackFrame::Pushdoes not currently consultcallInst.methodSpec, you could simplify the method-spec path by skipping the localdelegateMethodSpecand the earlyinst.methodSpecassignment, and just set the frame field when needed:- CLR_RT_MethodSpec_Index delegateMethodSpec; - ... - delegateMethodSpec.Clear(); - ... - if (pDelegate->m_genericMethodSpec.data != 0) - { - delegateMethodSpec = pDelegate->m_genericMethodSpec; - inst.methodSpec = delegateMethodSpec; - } + ... + if (pDelegate->m_genericMethodSpec.data != 0) + { + inst.methodSpec = pDelegate->m_genericMethodSpec; + } @@ - if (delegateMethodSpec.data != 0) + if (pDelegate->m_genericMethodSpec.data != 0) { CLR_RT_StackFrame *stackTop = this->CurrentFrame(); - stackTop->m_call.methodSpec = delegateMethodSpec; + stackTop->m_call.methodSpec = pDelegate->m_genericMethodSpec; }This keeps behavior while reducing duplication. If you expect
Pushto start honoringmethodSpeclater, the current version is also fine.src/CLR/Diagnostics/Info.cpp (3)
445-738: Generic-aware FieldRef/TypeSpec dumping is a good improvement; verify MVAR-in-SZARRAY handling and tidy minor detailsThe new
DumpTokenlogic forTBL_FieldRefandTBL_TypeSpecsubstantially improves diagnostics in generic contexts:
- For
FieldRef, using the caller’s closedgenericTypeplus the field’sOwner()TypeSpec to buildTypeName::Fieldis exactly what you want when dumping things likeEmptyArray<T>.Value.- For non-TypeSpec owners you sensibly fall back to the caller’s
genericType, and otherwise revert to the old FieldDef-based path.A couple of small points worth checking:
- VAR vs MVAR inside SZARRAY
In the top-level
DATATYPE_VAR/DATATYPE_MVARcases you distinguish between:
- VAR:
CLR_RT_TypeSpec_Instance typeSpec+GetGenericParam(position, paramElement).- MVAR:
FindGenericParamAtMethodDef(methodDefInstance, position, gpIndex)thenBuildTypeName(gp.classTypeDef, ...).In the
DATATYPE_SZARRAYcase, bothVARandMVARinner elements go through:CLR_RT_TypeSpec_Instance typeSpec; if (typeSpec.InitializeFromIndex(*methodDefInstance.genericType) && typeSpec.GetGenericParam(gpIndex, paramElement)) { BuildTypeName(paramElement.Class, ...); }i.e., only the closed declaring type’s
TypeSpecis consulted, and the method’s generic context (methodDefInstance.methodSpec) is not referenced.If
GetGenericParam()is only defined over type-level generic arguments (VAR), this would leave SZARRAYs over method generics (e.g.!!0[]) unresolved or mis-resolved. If, in this PR, you extendedGetGenericParam()to also handle method-level generics given the right context, you might want to pass whatever method-spec context it needs here; otherwise, consider aligning the MVAR-in-SZARRAY handling with the top-level MVAR path (usingFindGenericParamAtMethodDef) for consistency.
- Minor cleanups
ownerAsmin the TypeSpec case is assigned but never used and can be dropped.static CLR_RT_TypeSpec_Index s_ownerTypeSpec;in the FieldRef/TypeSpec-owner branch doesn’t need to bestatic; a plain localCLR_RT_TypeSpec_Index ownerTypeSpec;would avoid any theoretical cross-thread interference in tracing without changing behavior.None of this blocks the change, but it would be good to confirm the intended coverage of
GetGenericParam()and optionally tidy the dead/staticlocals.
948-971: CALL/CALLVIRT dumping via ResolveToken + METHOD is nicer; guard against missing contextThe new
DumpOpcodeDirectbehavior forCEE_CALL/CEE_CALLVIRT:CLR_UINT32 token = CLR_ReadTokenCompressed(ip, op); CLR_RT_MethodDef_Instance mdInst{}; if (mdInst.ResolveToken(token, call.assembly, call.genericType)) { CLR_RT_DUMP::METHOD(mdInst, call.genericType); } else { DumpToken(token, call); }is a solid upgrade: when you have a valid
callcontext you now print the fully-qualified method name (respecting generic type context) instead of a raw token.One edge case to double‑check:
DumpOpcodepasses either
stack->m_callwhens_CLR_RT_fTrace_Instructions >= c_CLR_RT_Trace_Verbose, or- a cleared
CLR_RT_MethodDef_Instanceotherwise.In the latter case,
call.assemblymay be null/invalid. This is fine forDumpToken(token, call)(it only looks atgenericType) but could be problematic ifResolveToken()assumes a non-nullassemblyand dereferences it.To be defensive while preserving the new behavior, you could gate the resolve on
NANOCLR_INDEX_IS_VALID(call)and fall back toDumpTokenotherwise, e.g.:CLR_UINT32 token = CLR_ReadTokenCompressed(ip, op); if (NANOCLR_INDEX_IS_VALID(call) && mdInst.ResolveToken(token, call.assembly, call.genericType)) { CLR_RT_DUMP::METHOD(mdInst, call.genericType); } else { DumpToken(token, call); }This keeps current behavior for verbose traces and avoids depending on
ResolveTokenhandling a null assembly in the non‑verbose case.
1050-1122: New METHOD overload and delegate OBJECT dump integrationThe new
CLR_RT_DUMP::METHOD(const CLR_RT_MethodDef_Instance&, ...)implementation that forwards toBuildMethodName(mdInst, genericType, ...)looks correct and ties in neatly with the new call sites (e.g.DumpOpcodeDirectandDumpToken).In the delegate OBJECT case you still call:
CLR_RT_DUMP::METHOD(dlg->DelegateFtn(), nullptr);Given
CLR_RT_HeapBlock_Delegatenow carriesm_genericTypeSpec/m_genericMethodSpec, you might later want to enhance this to use the instance overload and propagatedlg->m_genericTypeSpecso delegate dumps show the closed generic form:CLR_RT_MethodDef_Instance mdInst; if (mdInst.InitializeFromIndex(dlg->DelegateFtn())) { CLR_RT_DUMP::METHOD(mdInst, &dlg->m_genericTypeSpec); }Not required for correctness, but could make delegate diagnostics more informative in generic-heavy scenarios.
src/CLR/Core/Execution.cpp (2)
312-334: Clarify comment and verify error handling for VAR resolution.Line 312's comment "First, try to resolve using the method's generic type context" could be misleading. This resolves type-level generic parameters (VAR) using the method's enclosing type's generic context, not method-level parameters.
Additionally, the fallback on line 333 unconditionally fails with
CLR_E_FAIL. Verify whether there are valid scenarios where a VAR local should be resolvable without a genericType context (e.g., in non-generic methods that reference generic types).Consider this diff for clarity:
- // First, try to resolve using the method's generic type context + // Resolve type-level generic parameter (VAR) using the method's enclosing type context if (methodDefInstance.genericType && NANOCLR_INDEX_IS_VALID(*methodDefInstance.genericType) &&
66-79: Consider extracting common VAR resolution logic.The VAR resolution logic in
InitializeReference(lines 66-79) andInitializeLocals(lines 316-330) is very similar. Both follow the pattern:
- Initialize TypeSpec from genericInstance
- Call GetGenericParam
- Extract Class and DataType
Consider extracting this into a helper function like
ResolveGenericTypeParameterto reduce duplication and improve maintainability.Example helper signature:
HRESULT ResolveGenericTypeParameter( const CLR_RT_TypeSpec_Instance &genericInstance, CLR_UINT8 paramPosition, CLR_RT_TypeDef_Index &outClass, NanoCLRDataType &outDataType) { // Common resolution logic here }Also applies to: 316-330
src/CLR/Core/Interpreter.cpp (1)
2131-2185: Consider profiling TypeSpec lookup performance only if generic collection calls become a bottleneck.The TypeSpec search executes during interface method calls on generic instances, but only after passing 9 guards that restrict this to a narrow case. The loop breaks on first match, and linear TypeSpec searches are used elsewhere in the codebase (e.g., GarbageCollector.cpp for marking generic static fields).
If profiling reveals this path is hot and impacts performance on constrained devices, a cached mapping from (object TypeDef) → TypeSpec_Index would eliminate the linear search. For now, this is exploratory code that works correctly; optimization can wait for evidence of actual bottleneck.
src/CLR/Include/nanoCLR_Runtime.h (5)
1438-1447: Generic static field access: context-aware overload looks consistentExtending
GetStaticFieldByFieldDefandAllocateGenericStaticFieldsOnDemandwithcontextTypeSpecandcontextMethodmatches the rest of the generic-context work and keeps old call sites valid via defaults. Please just ensure the implementation treats these context pointers as strictly ephemeral (not stored beyond the call) and copies any data it needs, to avoid lifetime issues when callers pass stack-localCLR_RT_TypeSpec_Index/CLR_RT_MethodDef_Instanceobjects.
2090-2098:BuildMethodNameoverload forMethodDef_Instanceis a useful additionAdding
BuildMethodName(const CLR_RT_MethodDef_Instance& mdInst, …)should simplify call sites that already have an instance. Ensure the implementation normalizes to the existingMethodDef_Index‑based path so both overloads produce identical strings for the same method/generic context (to avoid subtle mismatches in logging or hashing).
2270-2275:CLR_RT_MethodDef_Instance::arrayElementType– ensure proper initialization/resetThe new
arrayElementTypefield for SZArrayHelper rebinding is a good way to cache the elementTypeDef. Please make sure:
- All
InitializeFromIndexoverloads andClearInstance()setarrayElementType.datadeterministically (e.g., zero for non‑array methods), so no stale element type can leak between reuses of aCLR_RT_MethodDef_Instance.- Any code that relies on
arrayElementTypechecks for validity (e.g.,data != 0) before use.
2335-2353:CLR_RT_MethodSpec_Instance::GetGenericArgument– confirm coverage for non-TypeDef arguments
GetGenericArgument(argumentPosition, typeDef, dataType)is a useful accessor for MethodSpec generic arguments. Two things to verify:
- Like
GetGenericParam, theargumentPositionconvention (0‑ vs 1‑based) matches how MethodSpec signatures are parsed.- The method clearly defines behavior when the argument is not a simple
TypeDef(e.g., array, generic instance, VAR/MVAR). If those cases are unsupported, consider asserting or documenting that the API is only for “class/value-type” arguments, so callers don’t accidentally mis-handle other shapes.
2628-2635:m_genericTypeSpecStoragecomment duplication and usageIntroducing
m_genericTypeSpecStorageas stable storage for generic type context onCLR_RT_StackFrameis a solid approach to avoid GC‑moved owners (e.g., delegate fields). Two minor points:
- There appears to be a duplicated comment line (“Stable storage for generic type context (not GC‑relocated)”) directly above the field; consider removing one copy to keep the comment concise.
- Please verify that any code assigning to
m_call.genericTypeand using this storage always writes theTypeSpec_Indexvalue (not a pointer) intom_genericTypeSpecStorageand then pointsm_call.genericTypeat it, and thatClearInstance/frame setup paths reset this field appropriately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/CLR/Core/CLR_RT_HeapBlock_Delegate.cpp(1 hunks)src/CLR/Core/CLR_RT_StackFrame.cpp(1 hunks)src/CLR/Core/Execution.cpp(5 hunks)src/CLR/Core/Interpreter.cpp(21 hunks)src/CLR/Core/Thread.cpp(2 hunks)src/CLR/Core/TypeSystem.cpp(37 hunks)src/CLR/Diagnostics/Diagnostics_stub.cpp(1 hunks)src/CLR/Diagnostics/Info.cpp(4 hunks)src/CLR/Include/nanoCLR_Checks.h(1 hunks)src/CLR/Include/nanoCLR_Runtime.h(11 hunks)src/CLR/Include/nanoCLR_Runtime__HeapBlock.h(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-01-22T03:38:57.394Z
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3074
File: src/CLR/Core/GarbageCollector_Info.cpp:107-167
Timestamp: 2025-01-22T03:38:57.394Z
Learning: In nanoFramework's memory management code, DataSize() validation is comprehensively handled through CLR_RT_HeapCluster::ValidateBlock() and other caller code. Additional size checks in ValidateCluster() are redundant as the validation is already performed at multiple levels.
Applied to files:
src/CLR/Include/nanoCLR_Runtime__HeapBlock.hsrc/CLR/Core/CLR_RT_HeapBlock_Delegate.cpp
📚 Learning: 2025-05-14T17:33:51.546Z
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 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/Interpreter.cppsrc/CLR/Diagnostics/Info.cppsrc/CLR/Core/TypeSystem.cppsrc/CLR/Core/Execution.cppsrc/CLR/Include/nanoCLR_Runtime.h
📚 Learning: 2025-06-26T09:16:55.184Z
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3190
File: src/CLR/Core/TypeSystem.cpp:0-0
Timestamp: 2025-06-26T09:16:55.184Z
Learning: In nanoFramework's CLR attribute parsing (src/CLR/Core/TypeSystem.cpp), the sentinel value 0xFFFF in string tokens represents a null string. When encountered, this should result in a true null reference (using SetObjectReference(nullptr)) rather than an empty string instance, and the boxing operation should be skipped via early return.
Applied to files:
src/CLR/Core/Interpreter.cpp
📚 Learning: 2025-05-14T16:27:02.573Z
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 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/Interpreter.cppsrc/CLR/Diagnostics/Info.cppsrc/CLR/Core/TypeSystem.cppsrc/CLR/Include/nanoCLR_Runtime.h
📚 Learning: 2024-09-25T11:28:38.536Z
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3023
File: targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp:191-225
Timestamp: 2024-09-25T11:28:38.536Z
Learning: When working with `nanoCLR_GetNativeAssemblyInformation`, fixed-size assembly names are required, so code that deals with variable-length names cannot be used.
Applied to files:
src/CLR/Diagnostics/Info.cppsrc/CLR/Core/TypeSystem.cpp
📚 Learning: 2024-10-08T15:52:09.445Z
Learnt from: josesimoes
Repo: nanoframework/nf-interpreter PR: 3023
File: targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp:191-225
Timestamp: 2024-10-08T15:52:09.445Z
Learning: In `nanoCLR_GetNativeAssemblyInformation`, there is no need to return the number of bytes written, as the memory buffer is zeroed, making the string buffer terminated.
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). (20)
- GitHub Check: nf-interpreter (Build_WIN32_nanoCLR)
- GitHub Check: nf-interpreter (Build_Azure_RTOS_targets SL_STK3701A)
- 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_NXP_targets NXP_MIMXRT1060_EVK)
- GitHub Check: nf-interpreter (Build_TI_SimpleLink_targets TI_CC1352R1_LAUNCHXL_915)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets MXCHIP_AZ3166)
- 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 (20)
src/CLR/Core/CLR_RT_HeapBlock_Delegate.cpp (1)
51-60: Delegate generic method spec correctly cleared on instantiationExplicitly clearing
m_genericMethodSpecalongsidem_genericTypeSpecprevents recycled delegates from carrying stale generic method context; this initialization looks correct and sufficient.src/CLR/Core/CLR_RT_StackFrame.cpp (1)
121-123: Stack-frame generic type context correctly initializedClearing
m_genericTypeSpecStoragewhen pushing a new frame ensures the per-frame generic context starts in a known “invalid” state, avoiding accidental reuse of previous values.src/CLR/Include/nanoCLR_Checks.h (1)
27-35: New METHOD overload for MethodDef_Instance is consistentThe added
CLR_RT_DUMP::METHOD(const CLR_RT_MethodDef_Instance&, const CLR_RT_TypeSpec_Index*)cleanly complements the existing index-based overload and matches the implementations in diagnostics code.src/CLR/Include/nanoCLR_Runtime__HeapBlock.h (1)
1913-1920: Delegate now carries both type and method generic contextAdding
m_genericMethodSpec(with explanatory comments) alongsidem_genericTypeSpecis consistent with the existing allocation pattern (sizeof(CLR_RT_HeapBlock_Delegate)) and enables delegate-aware MVAR resolution without affecting GC relocation.src/CLR/Diagnostics/Diagnostics_stub.cpp (1)
193-199: Stub for new METHOD overload matches existing patternThe weak
CLR_RT_DUMP::METHOD(const CLR_RT_MethodDef_Instance&, ...)stub is correctly wired (parameters ignored, profiling macro invoked) and consistent with the other diagnostics stubs.src/CLR/Core/Execution.cpp (3)
339-370: LGTM! Well-structured MVAR resolution with proper fallback.The method-level generic parameter (MVAR) resolution is well-implemented:
- Clear primary path using MethodSpec for concrete instantiations (lines 345-358)
- Graceful fallback to GenericParam table for open generic methods (lines 359-370)
- Accurate comment describing the parameter type
This provides better error handling than the VAR case and should work correctly for both closed and open generic method scenarios.
628-638: LGTM! CastToType signature change consistent with IsInstanceOf.The addition of the
callerparameter toCastToType(lines 628-629) and its propagation to theIsInstanceOfcall (line 638) maintains consistency across the type-checking APIs. This enables proper generic context resolution during cast operations.
574-587: The review comment contains material inaccuracies and should be disregarded.The referenced lines 574-587 contain the
CreateEntryPointArgsfunction, not theIsInstanceOfsignature changes. The actual changes are at lines approximately 3575-3610 in Execution.cpp.More importantly, the API changes are not breaking changes. The new
IsInstanceOfoverload is backward compatible because it includes a default parameter value (const CLR_RT_MethodDef_Instance *caller = nullptr). It coexists with the existing 3-parameter signatures as an overload, so existing call sites continue to work without modification. The single verified call site with the caller parameter (line 3638) correctly implements the new functionality.No verification of call site updates is necessary.
Likely an incorrect or invalid review comment.
src/CLR/Core/Interpreter.cpp (6)
2295-2345: LGTM! Correct handling of SZArrayHelper element type propagation.The logic correctly prioritizes the element type derived from the runtime array's reflection data (SZArrayHelper case) over the propagated element type from the parent frame, with appropriate fallback handling.
2386-2423: LGTM! Robust generic context setup with stable storage.The three-level priority for generic context selection is clearly documented, and the atomic copy-to-stable-storage pattern prevents dangling pointer issues when the stack frame is reused.
2450-2458: LGTM! Correct arrayElementType preservation on return.The logic correctly preserves the arrayElementType context from the returning frame back to the caller, ensuring generic context flows properly through the call chain.
3163-3187: LGTM! Correct save/restore pattern for arrayElementType inference.Both BOX and UNBOX.ANY correctly:
- Save the previous arrayElementType context
- Attempt to infer the element type from the runtime value being boxed/unboxed
- Restore the previous context on failure paths
- Restore the previous context after successful resolution
The defensive checks ensure that failed type extraction doesn't corrupt the context.
Also applies to: 3336-3360
3910-3924: GetGenericParam implementation correctly handles all edge cases.Verification confirms the API properly handles:
- Out-of-bounds: Line 937 checks
parameterPosition >= element.GenParamCountand returns false- Open generics: Line 925 validates
element.DataType == DATATYPE_GENERICINST, rejecting uninstantiated generic types- Recursive generics: The returned
elementcan itself be a GENERICINST for nested cases likeIEnumerable<IEnumerable<T>>The loop (lines 944-950) correctly walks through parameters with successive
Advance()calls to reach the target position. The refactored code at lines 3910-3924 properly uses this API and checks the return value at line 3921.
3519-3555: <!-- [scratchpad_start]
[task verify_ldelem_element_extraction_vs_szarrayhelper]
[observations]
SZArrayHelper flow (lines 2295-2318):
- Gets pArray via pThis[0].Dereference()
- Accesses pArray->ReflectionDataConst() to get reflection metadata
- Extracts reflex.data.type (the element TypeDef) when levels==1
- This is the correct way to get Int32 from List[]
LDELEM flow (lines 3519-3526):
- Calls ExtractTypeIndexFromObject(evalPos[0], cls)
- ExtractTypeIndexFromObject returns obj->ObjectCls()
- For array of List, ObjectCls() returns List, NOT Int32
[analysis]
- SZArrayHelper uses reflection metadata: pArray->ReflectionDataConst()->data.type
- LDELEM uses object instance type: obj->ObjectCls()
- These are fundamentally different!
- SZArrayHelper gets the element type (Int32)
- LDELEM would get the array element object's type (List), which is NOT the generic parameter
- The review comment correctly identifies this mismatch: code doesn't mirror SZArrayHelper flow
- The code comment says "List[] -> Int32" but the implementation extracts List
[pending]
- Confirm what evalPos[0] actually contains at LDELEM (should be array object)
- Check if there's a way to get ReflectionData from evalPos[0] like SZArrayHelper does
[actions]
Find LDELEM stack setup and confirm evalPos[0] is the array object
src/CLR/Include/nanoCLR_Runtime.h (6)
1747-1752: AddingSZArrayHelperto well‑known types is appropriateIncluding
CLR_RT_TypeDef_Index SZArrayHelperhere is a good fit for the new SZ-array IList dispatch. Verify thatTypeSystem_Initialize(or equivalent init path) now populates this field and that any runtime SZArray rebinding paths consistently useg_CLR_RT_WellKnownTypes.SZArrayHelperinstead of hard-coded lookups.
2075-2081: NewBuildTypeNameoverload with TypeSpec and contextThe
BuildTypeName(const CLR_RT_TypeSpec_Index&, …, levels, contextTypeSpec, contextMethodDef)overload is a clean way to generate names for closed generic instantiations with proper VAR/MVAR substitution. Given how TypeSpec signatures are parsed (initialAdvance()over VAR/MVAR followed by position walking), double-check thatlevelsandcontextTypeSpecare interpreted consistently withCLR_RT_SignatureParser::GenericParamPositionto avoid off‑by‑one in generic parameter names. Based on learnings.
2150-2153:ComputeHashForClosedGenericType– confirm collision behavior and context useExposing
ComputeHashForClosedGenericTypewith optionalcontextTypeSpecandcontextMethodis the right hook for generic static fields and generic‑cctor tracking. Please verify that:
- The hash incorporates all generic arguments (including nested generic instances and array element types).
- VAR/MVAR are interpreted using the provided context, matching the execution/type‑resolution paths.
- The resulting hash is stable across boots for the same metadata layout (important for any persisted state keyed by this value).
2178-2196:CLR_RT_TypeSpec_Instance::GetGenericParam– clarify index semanticsThe new
GetGenericParam(CLR_INT32 parameterPosition, CLR_RT_SignatureParser::Element& element)API is a useful primitive for resolving generic arguments from TypeSpecs. Given the way TypeSpec signatures are walked (initialAdvance()consuming VAR/MVAR before stepping to the specific position), please document and confirm whetherparameterPositionis 0‑based or 1‑based, and ensure its usage matchesGenericParamPositioninCLR_RT_SignatureParser::Elementto avoid subtle off‑by‑one bugs. Based on learnings.
2457-2469: TypeDescriptor init from TypeSpec/signature: context parameterization aligns with generic workAdding
contextTypeSpectoInitializeFromTypeSpecandInitializeFromSignatureParseris consistent with the need to interpret VAR/MVAR relative to a closed generic owner. This should unblock more accurate type descriptions for generic instances. Please ensure:
- All generic-aware call sites (e.g.,
IsInstanceOf, boxing/unboxing, SZArrayHelper dispatch) now pass the correctcontextTypeSpecwhen available.- The implementation falls back to the old behavior when
contextTypeSpec == nullptr, so non‑generic and existing paths remain unchanged.
4233-4250: ExtendingIsInstanceOf/CastToTypewith caller context is correct; check call‑site coverageThe new overloads:
static bool IsInstanceOf(CLR_RT_HeapBlock& obj, CLR_RT_Assembly* assm, CLR_UINT32 token, bool isInstInstruction, const CLR_RT_MethodDef_Instance* caller = nullptr);static HRESULT CastToType(CLR_RT_HeapBlock& ref, CLR_UINT32 tk, CLR_RT_Assembly* assm, bool isInstInstruction, const CLR_RT_MethodDef_Instance* caller);are exactly what’s needed to resolve VAR/MVAR tokens in
isinst/castclassusing the calling method’s generic context. A few things to confirm:
- All IL interpreter/execution paths that handle
isinst,castclass, and related token‑based casts now pass a validcallerwhen the token may reference MVAR/VAR; relying on the defaultnullptrshould be limited to non‑generic tokens.IsInstanceOfToken(just below) is updated to route through this overload, avoiding duplicate logic.CastToType’s non‑optionalcallerparameter forces callers to be explicit; that’s good—ensure any non‑generic use sites deliberately passnullptror a known context, rather than leaving this ambiguous.
- Adjust code accordingly (and add it where missing it).
- Was getting the typespec from the wrong assembly.
- Adjust callers accordingly.
…eter into add-arrayhelper
Automated fixes for code style.
…60780-03cb-44d5-a94e-2a6342668af0 Code style fixes for nanoframework/nf-interpreter PR#3240
2 participants
Description
Motivation and Context
List<T>unit tests and related helper classes in mscorlib, likeSZArrayHelper, along with interfaces for generic collections. All the nuances and more broad usage of the type system and execution engine that these surfaced, prompted for the fixes and new code that is here. This work caused cascading issues when running all available unit tests. It was therefore, necessary to add more code to handle the various edge cases and more broad handling of generics. Working backwards to the core library surfaced even more edge cases and alternative execution paths. Because all of this work was related and somehow convoluted, it wasn't practical to commit in smaller batches and more atomic PRs.How Has This Been Tested?
List<T>.Screenshots
Types of changes
Checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.