Refactorings and fixes to erased definition handling #23404
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The motivation for the phase was always unclear to me. No tests broke after the change. Piggybacked on the phase was the unrelated "anonymous class in inline method" warning. I moved it in changed form to FirstTransform. It is more efficient now since it does not need an auxiliary tree traversal.
We assumed all type applications were pure, which is claerly false.
E.g.
def fn[T] = println("hi")
fn[Int]
This triggered a problem in coverage tests which now assumes a use of an erased
function is not erased (probably because it now shows up in the coverage info).
The problem was fixed by mapping the erased defs to inline defs.
I found out the problem with coverage: If an argument is not a pure expression, it gets lifted
into a prefix val. And the prefix val is not erased even if the parameter is erased. So that way
things escape an erased context. I believe this is a fundamental problem that would also affect
lifting due to other reasons (e.g. eta expansion or handling default arguments). We either
have to fix the lifting to mainain erased status, or restrict erased arguments to pure expressions
that don't need lifting.
If one of the methods is an inline method that is not retained, no need to report a double definition.
natsukagami
reviewed
Jun 24, 2025
Comment on lines
+21
to
+37
| 0 | ||
| macro-late-suspend/UsesTest.scala | ||
| example | ||
| UsesTest$package | ||
| Object | ||
| example.UsesTest$package | ||
| <init> | ||
| 22 | ||
| 22 | ||
| 3 | ||
| <none> | ||
| Literal | ||
| true | ||
| 0 | ||
| false | ||
|
|
||
|
|
There was a problem hiding this comment.
I'm not sure what this test does. Is it important that the output changes?
There was a problem hiding this comment.
I am not sure. It might have to do with the fact that we don't delete code in PruneErasedDefs anymore. But if anything this hshould be more precise now.
| @@ -56,6 +56,6 @@ object ccConfig: | |||
|
|
|||
| /** Not used currently. Handy for trying out new features */ | |||
| def newScheme(using ctx: Context): Boolean = | |||
| ctx.settings.XdropComments.value | |||
| Feature.sourceVersion.stable.isAtLeast(SourceVersion.`3.7`) | |||
There was a problem hiding this comment.
This seems to be a random change
There was a problem hiding this comment.
yes, it's not needed elsewhere though.
|
erased defs are not yet removed. That comes later. |
natsukagami
approved these changes
Jun 24, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These are cleanups and bug-fixes. They don't change the semantics of erasedDefinitions