-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Try to fix #24573: Don't treat SAM class with imcompatible Unit result types as a platform SAM #24624
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Still WIP |
…re.Boxing.adaptClosure
There was a problem hiding this 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 fixes issue #24573 by adding stricter checks for platform SAM (Single Abstract Method) compatibility. The core change prevents SAM classes with incompatible Unit result types (or other types that LambdaMetaFactory cannot auto-adapt) from being treated as platform SAMs, requiring them to be expanded to anonymous classes instead.
Key changes:
- Added
samDoesNotNeedExpansionmethod inTypeErasureto check if LambdaMetaFactory can handle required signature adaptations - Integrated the new check into
JavaPlatform.isSamto filter out incompatible SAM types - Added comprehensive test coverage for various SAM type scenarios
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| compiler/src/dotty/tools/dotc/core/TypeErasure.scala | Adds samDoesNotNeedExpansion method that checks whether overridden methods have compatible erased signatures for LambdaMetaFactory adaptation; converts object declaration to Scala 3 indentation style |
| compiler/src/dotty/tools/dotc/config/JavaPlatform.scala | Integrates the new SAM expansion check into isSam method; adds import statement |
| tests/run/i24573.scala | Comprehensive test file with various SAM trait configurations testing parameter and result type adaptations |
| tests/run/i24573.check | Expected output for the test cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * for LambdaMetaFactory to work. This method returns true if any overridden method | ||
| * has an incompatible erased signature that LMF cannot auto-adapt. |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation comment is misleading. Line 593-594 states "This method returns true if any overridden method has an incompatible erased signature that LMF cannot auto-adapt", but the actual behavior is the opposite: the method returns true when LambdaMetaFactory CAN handle all overridden methods (i.e., when the SAM does NOT need expansion).
The contradiction is between "any overridden method has an incompatible erased signature" (which would indicate a problem) and "the SAM class does not need to be expanded" (line 600), which would be the opposite.
The correct description should be: "This method returns true if all overridden methods have compatible erased signatures that LMF can auto-adapt (or don't need adaptation)."
| * for LambdaMetaFactory to work. This method returns true if any overridden method | |
| * has an incompatible erased signature that LMF cannot auto-adapt. | |
| * for LambdaMetaFactory to work. This method returns true if all overridden methods | |
| * have compatible erased signatures that LMF can auto-adapt (or don't need adaptation). |
| import classpath.AggregateClassPath | ||
| import core.* | ||
| import Symbols.*, Types.*, Contexts.*, StdNames.* | ||
| import Phases.* |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import Phases.* appears to be unused in this file. Consider removing it if it's not needed for the new functionality.
| import Phases.* |
Try to fix #24573, not sure if this is a correct fix.
Try a more strict check for platform SAM