Various fixes for generics handling #3185
Conversation
- Fix several APIs to deal with closed generic types and find types of generic VAR types. - Fix wrong setting of assembly index when resolving a MethodRef from a TypeSpec. - Fix bug with dummy assembly index in ResolveMethodRef(). - CLR_RT_Assembly::FindMethodDef() can now find MethodDef from TypeSpec in the same or referenced assembly. - Major rework of CLR_RT_TypeSystem::BuildTypeName() for TypeSpec. - Major rework of BuildMethodName().
josesimoes
added
the
Area: Interpreter
WalkthroughThe changes enhance the CLR type system's handling of generic types and methods. They refine generic method initialization, method reference resolution, type name construction, and validation for type/method indices. Several method signatures are updated to support improved parsing, resolution, and name-building for generics, ensuring more robust and accurate behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant TypeSystem
participant Assembly
participant MethodDefInstance
Caller->>TypeSystem: BuildMethodName(md, genericType, szBuffer, iBuffer)
TypeSystem->>MethodDefInstance: InitializeFromIndex(md, genericType)
MethodDefInstance->>Assembly: FindGenericParamAtTypeSpec(typeSpecIndex, paramPos)
Assembly-->>MethodDefInstance: Return resolved typeDef and dataType
MethodDefInstance-->>TypeSystem: Initialization complete
TypeSystem->>TypeSystem: BuildTypeName(typeIndex, szBuffer, iBuffer)
TypeSystem-->>Caller: Return method name
sequenceDiagram
participant Caller
participant MethodDefInstance
participant Assembly
Caller->>MethodDefInstance: ResolveToken(token, assembly, callerGeneric)
MethodDefInstance->>Assembly: ResolveMethodRef(token, callerGeneric)
Assembly-->>MethodDefInstance: Return method index and assembly index
MethodDefInstance-->>Caller: Token resolved
✨ Finishing Touches
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/CLR/Core/TypeSystem.cpp(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- 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/Core/TypeSystem.cpp (6)
1269-1320: Good improvement for generic type initializationThe refactored
InitializeFromIndexmethod now properly handles generic types by:
- Creating a
CLR_RT_TypeSpec_Instanceto parse the type specification- Using signature parser to extract generic instance elements
- Correctly handling VAR types by resolving to the real owner type via
FindGenericParamAtTypeSpec- Properly rebinding the method definition index to the declaring assembly
This is a solid improvement that should prevent issues with generic method resolution.
3304-3304: Improved consistency for dummy assembly indexChanging from
0xffffto0xffffffffaligns with theCLR_UINT32type and provides a clearer sentinel value that won't conflict with valid assembly indices.
5031-5045: Excellent defensive programming for TypeSpec method resolutionThe addition of bounds checking for
typeDefIdxprevents potential crashes from out-of-range indices. The fallback to TypeSpec parsing provides a robust alternative path for resolving methods in generic types. This should improve stability when dealing with complex generic type hierarchies.
4796-4796: Minor formatting improvementAdding a trailing newline for better code readability.
6269-6304: Major improvement in generic type name resolutionThe enhanced
BuildTypeNamenow properly resolves generic type parameters (VAR) to their concrete types in closed generic instantiations. This means that instead of seeing cryptic parameter placeholders like!T, users will see the actual type names likeSystem.Int32. This significantly improves debugging and error messages.The recursive approach for building nested generic type names is well-structured.
6379-6439: Significant rework of method name building for genericsThis refactoring properly handles the complexity of building method names for generic types:
- Correct type association: The code now verifies that the provided
genericTypeactually corresponds to the method's declaring type before using it- Fallback mechanism: If the types don't match, it falls back to non-generic initialization, preventing incorrect type associations
- Proper ownership resolution: Uses
InitializeFromMethodto correctly determine the method's ownerThis should resolve issues with method name generation for generic types and their instantiations.
| @@ -1401,7 +1424,7 @@ bool CLR_RT_MethodDef_Instance::ResolveToken( | |||
| return false; | |||
| } | |||
|
|
|||
| Set(assemblyIndex - 1, methodIndex.Method()); | |||
| Set(assemblyIndex, methodIndex.Method()); | |||
There was a problem hiding this comment.
Critical bug fix: Corrected assembly index calculation
The removal of - 1 from assemblyIndex fixes an off-by-one error when setting the method index for TypeSpec-owned method references. This ensures the correct assembly is referenced.
🤖 Prompt for AI Agents
In src/CLR/Core/TypeSystem.cpp at line 1427, fix the off-by-one error by
removing the subtraction of 1 from assemblyIndex when calling Set with
assemblyIndex and methodIndex.Method(). This correction ensures the correct
assembly is referenced for TypeSpec-owned method references.
Description
Motivation and Context
How Has This Been Tested?
Span<T>.[build with MDP buildId 56229]
Screenshots
Types of changes
Checklist
Summary by CodeRabbit