Skip to content

Conversation

@davidwrighton
Copy link
Member

  • Fixes GitHub_35384
  • Only required using the correct helper routines. The rest of the implementation fell out from there

- Fixes GitHub_35384
- Only required using the correct helper routines. The rest of the implementation fell out from there
@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @BrzVlad, @janvorli, @kg
See info in area-owners.md if you want to be subscribed.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds support for explicit this calli instructions to valuetype methods in the CoreCLR interpreter. The primary change refactors the code to use hasImplicitThis() instead of the more verbose hasThis() && !hasExplicitThis() pattern, which correctly distinguishes between methods with implicit this pointers (standard instance methods) and those with explicit this pointers (used in calli scenarios).

Key Changes

  • Refactored signature checks to use the cleaner hasImplicitThis() helper method
  • Added validation to ensure method definitions don't have explicit this (which is only valid for call sites)
  • Updated argument counting and processing logic to correctly handle explicit this in call scenarios

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/coreclr/interpreter/naming.cpp Simplified the check for implicit this in method name printing by using hasImplicitThis() instead of the compound condition
src/coreclr/interpreter/compiler.cpp Updated call emission logic to use hasImplicitThis() for argument counting and type checking, and added a BADCODE check to prevent explicit this in method definitions

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@davidwrighton davidwrighton merged commit 8eb90a6 into dotnet:main Dec 5, 2025
103 of 106 checks passed
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