Fix box/unbox Nullable<T> types #3199
Conversation
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 You can disable this status message by setting the WalkthroughSupport for Changes
Sequence Diagram(s)sequenceDiagram
participant IL_Interpreter
participant TypeSystem
participant ObjectHeap
IL_Interpreter->>TypeSystem: Is target type Nullable<T>?
alt Boxing
IL_Interpreter->>ObjectHeap: Check HasValue on Nullable<T>
alt HasValue == false
IL_Interpreter->>ObjectHeap: Box null reference
else HasValue == true
IL_Interpreter->>ObjectHeap: Box underlying value
end
else Unboxing
IL_Interpreter->>ObjectHeap: Create Nullable<T> instance
IL_Interpreter->>ObjectHeap: Set HasValue and assign value if present
end
sequenceDiagram
participant TypeDef_Instance
participant Assembly
participant SignatureParser
TypeDef_Instance->>SignatureParser: Parse TypeSpec token
alt Is Nullable<T> signature
SignatureParser->>TypeDef_Instance: Advance to generic parameter
alt Generic parameter is VAR/MVAR
TypeDef_Instance->>TypeDef_Instance: Resolve via caller context
else
TypeDef_Instance->>Assembly: Resolve class/value type directly
end
TypeDef_Instance-->>Caller: Return resolved Nullable<T> type
else
TypeDef_Instance-->>Caller: Fail to resolve
end
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@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 (4)
src/CLR/Core/Interpreter.cpp (2)
2877-2883: Simplify value assignment in unbox operation.The temporary
unboxedValuevariable is unnecessary. After unboxing,evalPos[0]already contains the unboxed value.else { // unbox the T value... NANOCLR_CHECK_HRESULT(evalPos[0].PerformUnboxing(destinationType)); - CLR_RT_HeapBlock unboxedValue = {}; - unboxedValue.Assign(evalPos[0]); - // assign the copied unboxed value - nullableObject.Dereference()[Sys_Nullable::FIELD__value].Assign(unboxedValue); + nullableObject.Dereference()[Sys_Nullable::FIELD__value].Assign(evalPos[0]); // set HasValue to true nullableObject.Dereference()[Sys_Nullable::FIELD__hasValue].SetBoolean(true); }
2912-2912: Address the TODO for nullable type support in CEE_UNBOX_ANY.The TODO correctly identifies that CEE_UNBOX_ANY needs nullable type support for completeness.
Would you like me to help implement nullable type handling for CEE_UNBOX_ANY similar to what was done for CEE_UNBOX?
src/CLR/Core/TypeSystem.cpp (2)
1072-1173: Consider refactoring to reduce code duplication with ResolveToken.The
ResolveNullableTypemethod contains logic very similar to parts ofResolveToken, particularly the generic parameter resolution sections (lines 1114-1158). This duplication could lead to maintenance issues.Additionally, the method assumes the first element is
DATATYPE_VALUETYPEbut doesn't validate this assumption until after parsing begins, which could lead to confusing error states.Consider extracting the common generic parameter resolution logic into a helper method:
+private: + bool ResolveGenericParameter( + CLR_RT_SignatureParser::Element& elem, + const CLR_RT_MethodDef_Instance* caller, + CLR_RT_TypeDef_Index& resolvedType); bool CLR_RT_TypeDef_Instance::ResolveNullableType( CLR_UINT32 tk, CLR_RT_Assembly *assm, const CLR_RT_MethodDef_Instance *caller) { // ... existing validation code ... + if (elem.DataType == DATATYPE_VAR || elem.DataType == DATATYPE_MVAR) + { + CLR_RT_TypeDef_Index resolvedType; + if (ResolveGenericParameter(elem, caller, resolvedType)) + { + data = resolvedType.data; + assembly = g_CLR_RT_TypeSystem.m_assemblies[resolvedType.Assembly() - 1]; + target = assembly->GetTypeDef(resolvedType.Type()); + return true; + } + return false; + }
1101-1106: Early validation could improve error clarity.The method checks if the first element is
DATATYPE_VALUETYPEonly after advancing the parser. Consider validating this constraint earlier to provide clearer error messaging.if (FAILED(parser.Advance(elem))) { return false; } if (elem.DataType != DATATYPE_VALUETYPE) { - // If it's not a value type, we can't resolve it as a nullable type ClearInstance(); return false; }Consider adding a more descriptive comment explaining why only value types are supported for nullable type resolution.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/CLR/CorLib/corlib_native.h(1 hunks)src/CLR/Core/Interpreter.cpp(3 hunks)src/CLR/Core/TypeSystem.cpp(4 hunks)src/CLR/Include/nanoCLR_Runtime.h(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#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.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3062
File: src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice.cpp:77-93
Timestamp: 2025-01-09T13:10:31.388Z
Learning: In nanoFramework SPI transactions, NULL buffers are valid cases when there is no data to send or receive. The code should handle NULL buffers by setting corresponding sizes to 0 instead of throwing exceptions.
src/CLR/CorLib/corlib_native.h (5)
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#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.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#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.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3023
File: targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp:191-225
Timestamp: 2024-09-25T11:28:38.536Z
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.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#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.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3023
File: targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp:191-225
Timestamp: 2024-10-12T19:00:39.000Z
Learning: When working with `nanoCLR_GetNativeAssemblyInformation`, fixed-size assembly names are required, so code that deals with variable-length names cannot be used.
src/CLR/Core/Interpreter.cpp (12)
undefined
<retrieved_learning>
Learnt from: josesimoes
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.
</retrieved_learning>
<retrieved_learning>
Learnt from: josesimoes
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.
</retrieved_learning>
<retrieved_learning>
Learnt from: josesimoes
PR: #3023
File: targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp:191-225
Timestamp: 2024-10-12T19:00:39.000Z
Learning: When working with nanoCLR_GetNativeAssemblyInformation, fixed-size assembly names are required, so code that deals with variable-length names cannot be used.
</retrieved_learning>
<retrieved_learning>
Learnt from: frobijn
PR: #3023
File: targets/netcore/nanoFramework.nanoCLR.Host/NativeAssemblyDetails.cs:0-0
Timestamp: 2024-10-08T15:52:09.445Z
Learning: In the context of NativeAssemblyDetails.Get(), exceptions from Interop.nanoCLR.nanoCLR_GetNativeAssemblyInformation are very unlikely, and additional exception handling may not be necessary.
</retrieved_learning>
<retrieved_learning>
Learnt from: frobijn
PR: #3023
File: targets/netcore/nanoFramework.nanoCLR.Host/NativeAssemblyDetails.cs:0-0
Timestamp: 2024-09-23T19:44:42.051Z
Learning: In the context of NativeAssemblyDetails.Get(), exceptions from Interop.nanoCLR.nanoCLR_GetNativeAssemblyInformation are very unlikely, and additional exception handling may not be necessary.
</retrieved_learning>
<retrieved_learning>
Learnt from: josesimoes
PR: #3062
File: src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice.cpp:77-93
Timestamp: 2025-01-09T13:10:31.388Z
Learning: In nanoFramework SPI transactions, NULL buffers are valid cases when there is no data to send or receive. The code should handle NULL buffers by setting corresponding sizes to 0 instead of throwing exceptions.
</retrieved_learning>
<retrieved_learning>
Learnt from: josesimoes
PR: #3062
File: src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice.cpp:106-188
Timestamp: 2025-01-09T13:32:43.711Z
Learning: In nanoFramework, CLR_RT_HeapBlock_Array::Pin() method returns void and cannot fail. It should be called without error handling.
</retrieved_learning>
<retrieved_learning>
Learnt from: AdrianSoundy
PR: #3152
File: targets/ESP32/ESP32_P4/nanoCLR/nanoFramework.Graphics/Graphics_Memory.cpp:0-0
Timestamp: 2025-05-01T03:31:34.528Z
Learning: In the nanoFramework ESP32 codebase, assertions are preferred over graceful error handling for critical memory allocations (like graphics memory) where the system cannot function without the allocated resources.
</retrieved_learning>
<retrieved_learning>
Learnt from: josesimoes
PR: #3023
File: targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp:191-225
Timestamp: 2024-09-25T11:28:38.536Z
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.
</retrieved_learning>
<retrieved_learning>
Learnt from: josesimoes
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.
</retrieved_learning>
<retrieved_learning>
Learnt from: josesimoes
PR: #3172
File: src/CLR/Core/TypeSystem.cpp:971-999
Timestamp: 2025-05-14T17:26:54.181Z
Learning: In the nanoFramework codebase, when handling HRESULT values from function calls like parser.Advance(elem), it's preferable to use if (parser.Advance(elem) != S_OK) for local error handling rather than using the NANOCLR_CHECK_HRESULT macro, which would immediately propagate errors up the call stack.
</retrieved_learning>
<retrieved_learning>
Learnt from: josesimoes
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.
</retrieved_learning>
src/CLR/Include/nanoCLR_Runtime.h (9)
undefined
<retrieved_learning>
Learnt from: josesimoes
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.
</retrieved_learning>
<retrieved_learning>
Learnt from: josesimoes
PR: #3023
File: targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp:191-225
Timestamp: 2024-10-12T19:00:39.000Z
Learning: When working with nanoCLR_GetNativeAssemblyInformation, fixed-size assembly names are required, so code that deals with variable-length names cannot be used.
</retrieved_learning>
<retrieved_learning>
Learnt from: josesimoes
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.
</retrieved_learning>
<retrieved_learning>
Learnt from: josesimoes
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.
</retrieved_learning>
<retrieved_learning>
Learnt from: josesimoes
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.
</retrieved_learning>
<retrieved_learning>
Learnt from: josesimoes
PR: #3023
File: targets/netcore/nanoFramework.nanoCLR/nanoCLR_native.cpp:191-225
Timestamp: 2024-09-25T11:28:38.536Z
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.
</retrieved_learning>
<retrieved_learning>
Learnt from: frobijn
PR: #3023
File: targets/netcore/nanoFramework.nanoCLR.Host/NativeAssemblyDetails.cs:0-0
Timestamp: 2024-09-23T19:44:42.051Z
Learning: In the context of NativeAssemblyDetails.Get(), exceptions from Interop.nanoCLR.nanoCLR_GetNativeAssemblyInformation are very unlikely, and additional exception handling may not be necessary.
</retrieved_learning>
<retrieved_learning>
Learnt from: frobijn
PR: #3023
File: targets/netcore/nanoFramework.nanoCLR.Host/NativeAssemblyDetails.cs:0-0
Timestamp: 2024-10-08T15:52:09.445Z
Learning: In the context of NativeAssemblyDetails.Get(), exceptions from Interop.nanoCLR.nanoCLR_GetNativeAssemblyInformation are very unlikely, and additional exception handling may not be necessary.
</retrieved_learning>
<retrieved_learning>
Learnt from: josesimoes
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.
</retrieved_learning>
src/CLR/Core/TypeSystem.cpp (2)
undefined
<retrieved_learning>
Learnt from: josesimoes
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.
</retrieved_learning>
<retrieved_learning>
Learnt from: josesimoes
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.
</retrieved_learning>
⏰ 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). (18)
- GitHub Check: nf-interpreter (Build_STM32_targets ST_STM32F769I_DISCOVERY)
- GitHub Check: nf-interpreter (Build_WIN32_nanoCLR)
- GitHub Check: nf-interpreter (Build_STM32_targets ST_STM32F429I_DISCOVERY)
- GitHub Check: nf-interpreter (Build_Azure_RTOS_targets SL_STK3701A)
- 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_ESP32_targets ESP32_PSRAM_REV0)
- GitHub Check: nf-interpreter (Build_TI_SimpleLink_targets TI_CC1352R1_LAUNCHXL_915)
- 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 (6)
src/CLR/CorLib/corlib_native.h (1)
647-653: LGTM! Well-implemented native struct for Nullable support.The struct correctly defines the field indices for
System.Nullable<T>following the established patterns in the codebase. The field indices (1 forhasValue, 2 forvalue) match the expected internal layout and are consistent with other value type structures in this file.src/CLR/Include/nanoCLR_Runtime.h (2)
1694-1694: LGTM! Well-positioned addition to well-known types.Adding
Nullableto theCLR_RT_WellKnownTypesstruct is the correct approach for trackingSystem.Nullable<T>as a fundamental runtime type. The positioning afterEnumand before exception types follows a logical grouping pattern.
2118-2118: LGTM! Appropriate method declaration for nullable type resolution.The
ResolveNullableTypemethod declaration follows the established pattern of theResolveTokenmethod above it, with consistent parameter types and naming conventions. This specialized method for resolving nullable types is a sensible addition to complement the existing type resolution functionality.src/CLR/Core/Interpreter.cpp (1)
8-8: Good use of typedef for readability.The typedef alias improves code readability by shortening the long type name.
src/CLR/Core/TypeSystem.cpp (2)
387-387: LGTM: Well-known type addition looks correct.The addition of
System.Nullable1to the type index lookup follows the established pattern and correctly maps to theNullable` member in the well-known types structure.
979-996: Manual Verification Required: Confirm Array and ByRef Handling in TypeSpec ParsingI wasn’t able to find any existing special‐case logic for
DATATYPE_SZARRAYorDATATYPE_BYREFin the TypeSpec parsing code after the recent simplification. Because removing the explicit loop may silently drop handling for these tokens, please:
- Review the
TypeSystem.cppsignature‐parsing logic around the newparser.Advance(elem)call (lines 979–996)- Ensure that array (
SZARRAY) and by‐ref (BYREF) annotations are still correctly consumed and resolved- Add or update unit tests to cover TypeSpec signatures containing arrays and by‐ref types
- TypeDef_Instance ResolveToken now properly handles TypeSpec tokens for class and value types. - Add ResolveNullableType() to resolve T for Nullable<T> types. - Add Nullable`1 to well known types array. - Add System_Nullable_1 to mscorlib class declaration. - Rework box/unbox handler code to properly deal with nullable types.
Description
Motivation and Context
How Has This Been Tested?
Span<T>.[build with MDP buildId 56369]
Screenshots
Types of changes
Checklist
Summary by CodeRabbit
New Features
Improvements