Skip to content

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

Closed
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/dotty/tools/dotc/Compiler.scala
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ class Compiler {
new CheckReentrant), // Internal use only: Check that compiled program has no data races involving global vars
List(new RefChecks, // Various checks mostly related to abstract members and overriding
new CheckStatic, // Check restrictions that apply to @static members
new CheckIllegalOverrides, // Check restrictions that apply to phantom types and value classes
new ElimRepeated, // Rewrite vararg parameters and arguments
new NormalizeFlags, // Rewrite some definition flags
new ExtensionMethods, // Expand methods of value classes with extension methods
Expand Down
11 changes: 6 additions & 5 deletions src/dotty/tools/dotc/core/Definitions.scala
Original file line number Diff line number Diff line change
Expand Up @@ -633,9 +633,9 @@ class Definitions {
name.startsWith(prefix) && name.drop(prefix.length).forall(_.isDigit)
}

def isBottomClass(cls: Symbol) =
def isBottomClass(cls: Symbol) =
cls == NothingClass || cls == NullClass
def isBottomType(tp: Type) =
def isBottomType(tp: Type) =
tp.derivesFrom(NothingClass) || tp.derivesFrom(NullClass)

def isFunctionClass(cls: Symbol) = isVarArityClass(cls, tpnme.Function)
Expand Down Expand Up @@ -772,7 +772,8 @@ class Definitions {
SingletonClass,
EqualsPatternClass,
EmptyPackageVal,
OpsPackageClass)
OpsPackageClass
)

/** Lists core methods that don't have underlying bytecode, but are synthesized on-the-fly in every reflection universe */
lazy val syntheticCoreMethods = AnyMethods ++ ObjectMethods ++ List(String_+, throwMethod)
Expand All @@ -785,8 +786,8 @@ class Definitions {
if (!_isInitialized) {
// force initialization of every symbol that is synthesized or hijacked by the compiler
val forced = syntheticCoreClasses ++ syntheticCoreMethods ++ ScalaValueClasses()
// Enter all symbols from the scalaShadowing package in the scala package

// Enter all symbols from the scalaShadowing package in the scala package
for (m <- ScalaShadowingPackageClass.info.decls)
ScalaPackageClass.enter(m)

Expand Down
41 changes: 41 additions & 0 deletions src/dotty/tools/dotc/transform/CheckIllegalOverrides.scala
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 {
Copy link
Contributor

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.

import ast.tpd._

override def phaseName = "checkPhantom"
Copy link
Contributor

Choose a reason for hiding this comment

The 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) = {
Copy link
Contributor

@DarkDimius DarkDimius Nov 9, 2016

Choose a reason for hiding this comment

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

I'd propose to slightly change the way such checks are done.
The problem with this approach is that the other mini-phases in the same block couldn't rely on the facts that you've checking, as they run interleaved with this phase.

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

Copy link
Contributor

Choose a reason for hiding this comment

The 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.
Eg, there actually is a class Nothing$ that isn't source defined but backend relies on it existing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 i1688c.scala just to make sure.

illegalOverride.foreach { overriden =>
ctx.error(i"Cannot define symbol overriding $overriden class `$tpe`", tree.pos)
}

tree
}
}
5 changes: 5 additions & 0 deletions tests/neg/i1688.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package scala

sealed trait Null // error

trait Char // error
3 changes: 3 additions & 0 deletions tests/neg/i1688b.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
package java.lang

trait Byte // error