Fix newobj handler for generic constructors

[josesimoes]

Description

• Now setting the correct context to allow NewObject to properly resolve all generic types.

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]
• Related with Support for generics <T> Home#782.

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

Release Notes

• Bug Fixes
  • Improved generic class instantiation to correctly resolve type contexts during object construction, ensuring constructor calls properly bind to the appropriate generic context.

[coderabbitai]

Walkthrough

The newobj instruction handler for generic classes has been modified to prioritize the caller's stack context generic type when constructing objects. The logic now attempts to resolve a valid generic type context before creating the object, falling back to previous behavior when unavailable.

Changes

Cohort / File(s)	Change Summary
Generic object construction in newobj src/CLR/Core/Interpreter.cpp	Modified object instantiation for generic classes to compute genericTypeForContext by preferring caller's stack context generic type over callee's. Initializes temporary genericTypeInstance from context index when valid, passes it to NewObject, or creates object without generic type if context unavailable. Adjusts control flow for context resolution.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Key focus areas:
  • Logic correctness of the generic type context resolution (caller vs. callee preference)
  • Proper handling of the temporary genericTypeInstance initialization and scope
  • Fallback behavior when context is unavailable—ensure backward compatibility
  • Edge cases where generic type context may be partially populated or invalid

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Fix newobj handler for generic constructors' directly and clearly describes the main change: fixing the newobj handler specifically for generic constructors, which matches the core objective of properly resolving generic types in object construction.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9892998 and e849c76.

📒 Files selected for processing (1)

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

⏰ 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_ESP32_targets ESP32_BLE_REV0)
• GitHub Check: nf-interpreter (Build_STM32_targets ST_STM32F769I_DISCOVERY)
• 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_STM32_targets ST_STM32F429I_DISCOVERY)
• GitHub Check: nf-interpreter (Build_NXP_targets NXP_MIMXRT1060_EVK)
• 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 (Check_Code_Style)
• 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 (1)

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

2467-2495: LGTM! Correct fix for generic constructor context resolution.

The implementation correctly addresses the issue where generic constructors need the caller's closed generic type rather than the potentially open generic type from the method reference. The logic is sound:

1. Correct prioritization: Preferring stack->m_call.genericType (caller's context) over calleeInst.genericType is appropriate for constructor calls, as the caller knows the closed generic type (e.g., List<int>) while the constructor's token may reference an open generic.

2. Proper fallback: The else-if at line 2479 provides appropriate fallback behavior, and the nullptr check at line 2484 ensures non-generic classes continue to work correctly by calling NewObject without a generic type.

3. Memory safety: The stack-allocated genericTypeInstance is properly scoped and only used within this block.

4. Clear documentation: The comment at lines 2467-2470 effectively explains the rationale for the change.

The PR description indicates this has been tested with new unit tests and against CoreLibrary#245, which provides good confidence in the fix.

---

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.