Skip to content

Conversation

@josesimoes
Copy link
Member

@josesimoes josesimoes commented Oct 3, 2025

Description

  • Hardened check for generic type when using MethodDef.
  • Simplified getting and setting TypeSpec for generic type.
  • Now using pointers all the way instead of creating new TypeSpecs along the path.
  • Remove wrong this accessor.

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 correctness of generic method/type handling, avoiding misresolution when the generic context is invalid or empty.
    • Preserved and propagated generic context during instance/virtual method calls for accurate dispatch with generics.
    • Tightened validation to prevent treating empty tokens as generics, reducing edge-case runtime errors.
    • Enhanced diagnostic output to more reliably display generic owner/type information for methods.

@josesimoes josesimoes added the Area: Common libs Everything related with common libraries label Oct 3, 2025
@nfbot nfbot added the Type: bug label Oct 3, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 3, 2025

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

Tightens generic-type validity checks across execution, type system, and diagnostics, and propagates generic context during virtual call resolution. Conditions now require valid indices and non-empty tokens before treating types as generic. Diagnostics printing respects these constraints. No public APIs changed.

Changes

Cohort / File(s) Summary
Generic local initialization checks
src/CLR/Core/Execution.cpp
InitializeLocals now treats a type as generic only if the index is valid (NANOCLR_INDEX_IS_VALID) and token is not CLR_EmptyToken, bypassing generic-parameter handling otherwise.
Generic context propagation on call resolution
src/CLR/Core/Interpreter.cpp
During virtual method selection, preserves and applies existing generic context to calleeInst when valid; otherwise falls back to InitializeFromIndex(calleeReal). Previously always used InitializeFromIndex(calleeReal).
Type system generic handling and validation
src/CLR/Core/TypeSystem.cpp
Simplifies assignment in InitializeFromIndex; in ResolveToken assigns genericType directly based on winner. BuildMethodName adds validity check on dereferenced genericType and uses CLR_EmptyToken instead of literal; strengthens generic handling.
Diagnostics token dump tightening
src/CLR/Diagnostics/Info.cpp
DumpToken for MethodRef now requires non-null, valid genericType with non-CLR_EmptyToken before using as ownerTypeSpec for printing.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant IL as IL Interpreter
  participant TS as TypeSystem
  participant MR as Method Resolver
  participant Callee as calleeInst

  IL->>MR: Resolve instance/virtual call
  MR->>TS: Select virtual method (calleeReal)
  alt Generic context available and valid
    MR->>Callee: Initialize with (calleeReal, *genericType)
  else No valid generic context
    MR->>Callee: InitializeFromIndex(calleeReal)
  end
  IL->>Callee: Invoke
Loading
sequenceDiagram
  autonumber
  participant Exec as Execution.InitializeLocals
  participant TS as TypeSystem

  Exec->>Exec: Check local's genericType
  alt Index valid AND token != CLR_EmptyToken
    Exec->>TS: Handle as generic parameter (init from method/type context)
  else
    Exec->>Exec: Bypass generic-param handling
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% 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 accurately reflects the main change by indicating that the pull request fixes how generic types are handled in MethodDef instances and is concise and clear without unnecessary detail.

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 3, 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.

- Hardned check for generic type when using MethodDef.
- Simplified getting and setting TypeSpec for generic type.
- Now using pointers all the way instead of creating new TypeSpecs along the path.
- Remove wrong this accessor.
@josesimoes josesimoes force-pushed the fix-generictype-in-methoddef branch from 046de42 to af81651 Compare October 3, 2025 09:57
@josesimoes josesimoes merged commit f187be0 into nanoframework:develop Oct 3, 2025
24 checks passed
@josesimoes josesimoes deleted the fix-generictype-in-methoddef branch October 3, 2025 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants