Skip to content

Conversation

@josesimoes
Copy link
Member

@josesimoes josesimoes commented Oct 9, 2025

Description

  • Initializing a type descriptor from a signature token now properly handles a VAR in the signature (now uses the assembly from the caller generic type).
  • Checking the match for type descriptors now looks into the correct type handler.

Motivation and Context

How Has This Been Tested?

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
    • Improved detection of closed generic classes during matching, reducing runtime mismatches and boxing-related errors.
    • Corrected resolution of generic type parameters across assemblies to prevent incorrect type selection.
    • Fewer edge-case failures when using generics in user code and libraries.
    • Enhances compatibility and stability with third-party libraries that heavily use generics.

- Initializing a type descriptor from a signature token now properly handles a VAR in the signature (now uses the assembly from the caller generic type).
- Checking the match for type descriptors now looks into the correct type handler.
@josesimoes josesimoes added the Area: Common libs Everything related with common libraries label Oct 9, 2025
@nfbot nfbot added the Type: bug label Oct 9, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 9, 2025

Walkthrough

Adjusts generic handling in the runtime: updates TypeSpec VAR resolution to use the caller’s generic assembly context, and modifies TypeDescriptorsMatch to promote CLASS to GENERICINST based on NANOCLR_INDEX_IS_VALID for the generic handler.

Changes

Cohort / File(s) Summary
Generic type resolution (TypeSpec VAR)
src/CLR/Core/TypeSystem.cpp
Resolves VAR (!T) via the caller’s generic type assembly context instead of the current assembly; delegates FindGenericParamAtTypeSpec to g_CLR_RT_TypeSystem.m_assemblies[caller->genericType->Assembly() - 1].
Type match promotion (CLASS→GENERICINST)
src/CLR/Core/CLR_RT_HeapBlock.cpp
In TypeDescriptorsMatch, changes promotion condition for CLASS to GENERICINST from checking handler token non-empty to NANOCLR_INDEX_IS_VALID(actualType.m_handlerGenericType).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Caller
  participant TypeSystem
  participant TypeSpec
  participant Assemblies as g_CLR_RT_TypeSystem.m_assemblies[caller.genericType.Assembly()-1]

  Caller->>TypeSystem: Resolve TypeSpec
  TypeSystem->>TypeSpec: Parse next element
  alt VAR (!T) encountered
    TypeSystem->>Assemblies: FindGenericParamAtTypeSpec(...)
    Assemblies-->>TypeSystem: Generic param mapping (concrete type)
    TypeSystem-->>Caller: Resolved type (from caller's generic context)
  else Non-VAR path
    TypeSystem-->>Caller: Resolve via existing flow
  end
Loading
sequenceDiagram
  autonumber
  participant Matcher as TypeDescriptorsMatch
  participant Actual as actualType
  participant Expected as expectedType

  Matcher->>Actual: Inspect DataType
  alt actualDataType == DATATYPE_CLASS and handler index valid
    note right of Matcher: NANOCLR_INDEX_IS_VALID(Actual.m_handlerGenericType)
    Matcher->>Matcher: Promote CLASS -> GENERICINST
  end
  Matcher->>Expected: Proceed with generic-inst matching
  Matcher-->>Matcher: Return match result
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

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 clearly indicates that the pull request addresses fixes in the type descriptor logic related to generics, which aligns with the main changes to descriptor matching and generic parameter resolution in the codebase. It is concise and directly references the core area modified without unnecessary noise.
✨ 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 f187be0 and a2dd00f.

📒 Files selected for processing (2)
  • src/CLR/Core/CLR_RT_HeapBlock.cpp (1 hunks)
  • src/CLR/Core/TypeSystem.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). (19)
  • 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_NXP_targets NXP_MIMXRT1060_EVK)
  • 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 (1)
src/CLR/Core/CLR_RT_HeapBlock.cpp (1)

794-794: LGTM! Improved generic type validation.

The change from direct field comparison (actualType.m_handlerGenericType.data != CLR_EmptyToken) to using the NANOCLR_INDEX_IS_VALID macro is a solid improvement. This provides better encapsulation and likely includes additional validation beyond just checking for an empty token, ensuring that closed-generic types boxed as CLASS are correctly promoted to GENERICINST for proper type matching.


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

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

@josesimoes
Copy link
Member Author

@coderabbitai review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 9, 2025

✅ 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.

@josesimoes josesimoes merged commit 58ab8de into nanoframework:develop Oct 9, 2025
25 checks passed
@josesimoes josesimoes deleted the fixes-generics branch October 9, 2025 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: Common libs Everything related with common libraries hacktoberfest-accepted Type: bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants