Add warning for anonymous inline classes (#16723) #20291
Conversation
| def checkNoInlineAnnoClasses(tree: DefDef)(using Context): Unit = | ||
| if tree.symbol.is(Inline) then | ||
| new TreeTraverser { | ||
| def traverse(tree: Tree)(using Context): Unit = | ||
| tree match | ||
| case tree: TypeDef if tree.symbol.isAnonymousClass => | ||
| report.warning(new InlinedAnonClassWarning(), tree.symbol.sourcePos) | ||
| case _ => traverseChildren(tree) | ||
| }.traverse(tree) | ||
|
|
There was a problem hiding this comment.
I don't think this is the correct place to put this check in, I think it makes more sense to add it in RefChecks'.
There was a problem hiding this comment.
Sure, I don't feel strongly so I have moved it, although if this method is ever called from somewhere other than where it was originally defined then the warning itself would very likely need to change.
tests/neg/i13044.check
Outdated
| -- [E195] Potential Issue Warning: tests/neg/i13044.scala:26:8 --------------------------------------------------------- | ||
| 26 | new Schema[A] { | ||
| | ^ | ||
| | New anonymous class definition will be duplicated at each inline site | ||
| | | ||
| | longer explanation available when compiling with `-explain` | ||
| -- [E195] Potential Issue Warning: tests/neg/i13044.scala:32:8 --------------------------------------------------------- | ||
| 32 | new Schema[A] { | ||
| | ^ | ||
| | New anonymous class definition will be duplicated at each inline site | ||
| | | ||
| | longer explanation available when compiling with `-explain` |
There was a problem hiding this comment.
It is better to replace the anonymous class in the test than adding the warnings here. You can do that by simply replacing them by ???
There was a problem hiding this comment.
For this particular test, I thought it was necessary to keep it as is since replacing those line with ??? will cause the compilation to pass. I could add @nowarn as an alternative to modifying the checkfile, but I don't know the exact context in which the test was constructed so I am hesitant to change it too significantly in case it ends up passing but not testing what it was intended to test.
There was a problem hiding this comment.
Mmmh, but the test behaves as expected if we change it to ??? (I can reproduce it correctly locally)
There was a problem hiding this comment.
Huh, maybe I'm misunderstanding, what I'm testing looks like this: https://github.com/aherlihy/scala3/compare/i15503...aherlihy:scala3:i16723-2?expand=1
Is that what you're running as well?
There was a problem hiding this comment.
It does not pass for me, not sure why it would behave differently
|
Can you also squash everything in a single commit |
|
@hamzaremmal Thank you for the review! Is there some strong preference for rebase merge over squash merge in the repo? Normally irrelevant commits and their messages will be removed upon squashing. |
| @@ -51,6 +52,7 @@ class PruneErasedDefs extends MiniPhase with SymTransformer { thisTransform => | |||
| else cpy.ValDef(tree)(rhs = trivialErasedTree(tree.rhs)) | |||
|
|
|||
| override def transformDefDef(tree: DefDef)(using Context): Tree = | |||
| RefChecks.checkNoInlineAnnoClasses(tree) | |||
There was a problem hiding this comment.
Oh, I think my previous comment was not clear. I meant why adding the check in the PruneErasedDefs and not in RefChecks ?
There was a problem hiding this comment.
Ah, I see! I asked @nicolasstucki and he mentioned that if it happened at a later stage then some inlined definitions wouldn't be available (e.g. it's best to do it as late as possible, and this is the latest phase before it's no longer possible), but I'm not 100% sure about that.
tests/neg/i13044.check
Outdated
| -- [E195] Potential Issue Warning: tests/neg/i13044.scala:26:8 --------------------------------------------------------- | ||
| 26 | new Schema[A] { | ||
| | ^ | ||
| | New anonymous class definition will be duplicated at each inline site | ||
| | | ||
| | longer explanation available when compiling with `-explain` | ||
| -- [E195] Potential Issue Warning: tests/neg/i13044.scala:32:8 --------------------------------------------------------- | ||
| 32 | new Schema[A] { | ||
| | ^ | ||
| | New anonymous class definition will be duplicated at each inline site | ||
| | | ||
| | longer explanation available when compiling with `-explain` |
There was a problem hiding this comment.
Mmmh, but the test behaves as expected if we change it to ??? (I can reproduce it correctly locally)
I agree, but this was a recommendation from @nicolasstucki to follow for this repository, maybe he can explain it here |
Fixes #16723