-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix #1688: don't allow override of synthetic or primitive classes #1689
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
package dotty.tools.dotc | ||
package transform | ||
|
||
import TreeTransforms._ | ||
import core._ | ||
import Contexts._, Definitions._, Decorators._ | ||
|
||
/** Checks if the user is trying to illegally override synthetic classes, | ||
* primitives or their boxed equivalent | ||
*/ | ||
class CheckIllegalOverrides extends MiniPhaseTransform { | ||
import ast.tpd._ | ||
|
||
override def phaseName = "checkPhantom" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be nice to have phaseName and class name in sync. |
||
|
||
override def transformTypeDef(tree: TypeDef)(implicit ctx: Context, info: TransformerInfo) = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd propose to slightly change the way such checks are done. As you see, your implementation only needs the symbol and this means that you can make it into an SymTransformer which checks the invariants. Checking in a SymTransformer enforces that even phases in the same mini-phase block that are after you may rely on invariants that you introduce. |
||
val defn = ctx.definitions | ||
val tpe = tree.tpe | ||
val fullName = tree.symbol.showFullName | ||
|
||
val syntheticClasses = defn.syntheticCoreClasses.map(_.showFullName) | ||
val primitives = defn.ScalaValueClasses().map(_.showFullName) | ||
val boxedPrimitives = defn.ScalaBoxedClasses().map(_.showFullName) | ||
|
||
val illegalOverride = | ||
if (syntheticClasses.contains(fullName)) | ||
Some("synthetic") | ||
else if (primitives.contains(fullName)) | ||
Some("primitive") | ||
else if (boxedPrimitives.contains(fullName)) | ||
Some("boxed primitive") | ||
else | ||
None | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be also nice to check that no-one tries to introduce a companion to those classes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we're comparing the full paths of the symbols we get this for free, but I added a neg test |
||
illegalOverride.foreach { overriden => | ||
ctx.error(i"Cannot define symbol overriding $overriden class `$tpe`", tree.pos) | ||
} | ||
|
||
tree | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
package scala | ||
|
||
sealed trait Null // error | ||
|
||
trait Char // error |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
package java.lang | ||
|
||
trait Byte // error |
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.
I wouldn't call this an override, it's more of a re-definition. But I'm also fine with the current name.