Fix accessing a null element in a byref array

[josesimoes]

Description

• Code now grabs element type from array instead of element when element is null.

Motivation and Context

How Has This Been Tested?

• Tested with new unit tests from Sys.Collections.
• Tested against Merge generics support in mscorlib CoreLibrary#245
• [build with MDP buildId 57901]

Screenshots

Types of changes

• Improvement (non-breaking change that improves a feature, code or algorithm)
• Bug fix (non-breaking change which fixes an issue with code or algorithm)
• New feature (non-breaking change which adds functionality to code)
• Breaking change (fix or feature that would cause existing functionality to change)
• Config and build (change in the configuration and build system, has no impact on code or features)
• Dev Containers (changes related with Dev Containers, has no impact on code or features)
• Dependencies/declarations (update dependencies or assembly declarations and changes associated, has no impact on code or features)
• Documentation (changes or updates in the documentation, has no impact on code or features)

Checklist

• My code follows the code style of this project (only if there are changes in source code).
• My changes require an update to the documentation (there are changes that require the docs website to be updated).
• I have updated the documentation accordingly (the changes require an update on the docs in this repo).
• I have read the CONTRIBUTING document.
• I have tested everything locally and all new and existing tests passed (only if there are changes in source code).

Summary by CodeRabbit

• Bug Fixes
  • Enhanced null element handling in reference arrays to improve system stability and prevent potential crashes during type resolution.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The CLR_RT_TypeDescriptor::InitializeFromObject method in TypeSystem.cpp now handles null elements in reference arrays by using the array's element type information as a fallback instead of dereferencing null, preventing null pointer access.

Changes

Cohort / File(s)	Summary
Null safety improvement in type resolution src/CLR/Core/TypeSystem.cpp	Modified DATATYPE_ARRAY_BYREF handling to check for null dereferenced elements and use array's element type information as fallback, avoiding null pointer dereference.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Verify that using array->ReflectionDataConst().data.type is semantically correct as a fallback for null elements
• Confirm that breaking from the loop is the intended control flow for this scenario
• Check for any existing tests covering this edge case or if new tests are needed

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: fixing null element access in byref arrays, which matches the PR's core objective.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[josesimoes]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/CLR/Core/TypeSystem.cpp (1)

2549-2557: Good fix for null element in reference arrays; avoid deref crash.

This correctly falls back to the array’s element type instead of dereferencing a null element. Two small tweaks will make the behavior more consistent and align with code style:

• Use nullptr instead of NULL.
• Don’t assign reflex here. Copying the array’s ReflectionData into m_reflex later (outside the switch) propagates the array “levels” (e.g., 1 for T[]) even though we’re intentionally returning the element type (consistent with the non-null path that returns the concrete element’s type). Setting only cls avoids that mismatch.

Suggested diff:

-                    if (obj->Dereference() == NULL)
+                    if (obj->Dereference() == nullptr)
                     {
-                        // Use the array's element type information
-                        reflex = &array->ReflectionDataConst();
-                        cls = &reflex->data.type;
+                        // Use the array's element type (not the array itself). Keep 'reflex' null
+                        // to avoid carrying array levels when returning an element type.
+                        cls = &(array->ReflectionDataConst().data.type);
                         break;
                     }

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5f9d281 and 6b123de.

📒 Files selected for processing (1)

• src/CLR/Core/TypeSystem.cpp (1 hunks)

🧰 Additional context used

🧠 Learnings (3)

📓 Common learnings

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.

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.

📚 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/TypeSystem.cpp

📚 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/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). (6)

• 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)