Skip to content

Fix #1750: Alternative fix for cyclic references due to illegal class overrides #1913

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

Merged
merged 1 commit into from
Jan 30, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
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
12 changes: 11 additions & 1 deletion compiler/src/dotty/tools/dotc/sbt/ExtractAPI.scala
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import core._, core.Decorators._
import Annotations._, Contexts._, Flags._, Phases._, Trees._, Types._, Symbols._
import Names._, NameOps._, StdNames._
import typer.Inliner
import typer.ErrorReporting.cyclicErrorMsg

import dotty.tools.io.Path
import java.io.PrintWriter
Expand Down Expand Up @@ -190,7 +191,16 @@ private class ExtractAPICollector(implicit val ctx: Context) extends ThunkHolder
def apiClassStructure(csym: ClassSymbol): api.Structure = {
val cinfo = csym.classInfo

val bases = linearizedAncestorTypes(cinfo)
val bases =
try linearizedAncestorTypes(cinfo)
catch {
case ex: CyclicReference =>
// See neg/i1750a for an example where a cyclic error can arise.
// The root cause in this example is an illegal "override" of an inner trait
ctx.error(cyclicErrorMsg(ex), csym.pos)
defn.ObjectType :: Nil
}

val apiBases = bases.map(apiType)

// Synthetic methods that are always present do not affect the API
Expand Down
17 changes: 13 additions & 4 deletions compiler/src/dotty/tools/dotc/transform/OverridingPairs.scala
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import Flags._, Symbols._, Contexts._, Types._, Scopes._, Decorators._
import util.HashSet
import collection.mutable
import collection.immutable.BitSet
import typer.ErrorReporting.cyclicErrorMsg
import scala.annotation.tailrec

/** A module that can produce a kind of iterator (`Cursor`),
Expand Down Expand Up @@ -122,10 +123,18 @@ object OverridingPairs {
if (nextEntry ne null) {
nextEntry = decls.lookupNextEntry(nextEntry)
if (nextEntry ne null) {
overridden = nextEntry.sym
if (overriding.owner != overridden.owner && matches(overriding, overridden)) {
visited += overridden
if (!hasCommonParentAsSubclass(overriding.owner, overridden.owner)) return
try {
overridden = nextEntry.sym
if (overriding.owner != overridden.owner && matches(overriding, overridden)) {
visited += overridden
if (!hasCommonParentAsSubclass(overriding.owner, overridden.owner)) return
}
}
catch {
case ex: CyclicReference =>
// See neg/i1750a for an example where a cyclic error can arise.
// The root cause in this example is an illegal "override" of an inner trait
ctx.error(cyclicErrorMsg(ex), base.pos)
Copy link
Contributor

Choose a reason for hiding this comment

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

As a general rule in development, may I understand that try/catch can only be used as the last resort? Just want to know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's complicated. Cyclic references can only be handled with try-catch, because at the points they are raised we typically do not have a way to back out cleanly. So if a cyclic reference is a possibility we need to handle it with try/catch. And we should try to minimize the places where cyclic references can be raised.

I would think twice before we introduce other exceptions and handle them with try/catch.

}
} else {
curEntry = curEntry.prev
Expand Down
5 changes: 5 additions & 0 deletions compiler/src/dotty/tools/dotc/typer/Checking.scala
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,11 @@ object Checking {
checkNoConflict(Final, Sealed)
checkNoConflict(Private, Protected)
checkNoConflict(Abstract, Override)
if (sym.isType && !sym.is(Deferred))
for (cls <- sym.allOverriddenSymbols.filter(_.isClass)) {
fail(i"$sym cannot have the same name as ${cls.showLocated} -- class definitions cannot be overridden")
sym.setFlag(Private) // break the overriding relationship by making sym Private
}
}

/** Check the type signature of the symbol `M` defined by `tree` does not refer
Expand Down
2 changes: 2 additions & 0 deletions compiler/src/dotty/tools/dotc/typer/RefChecks.scala
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,8 @@ object RefChecks {
if (!isOverrideAccessOK) {
overrideAccessError()
} else if (other.isClass) {
// direct overrides were already checked on completion (see Checking.chckWellFormed)
// the test here catches indirect overriddes between two inherited base types.
overrideError("cannot be used here - class definitions cannot be overridden")
} else if (!other.is(Deferred) && member.isClass) {
overrideError("cannot be used here - classes can only override abstract types")
Expand Down
3 changes: 1 addition & 2 deletions tests/neg/customArgs/overrideClass.scala
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,7 @@
}
trait FooB extends FooA {
type A <: Ax;
trait Ax extends super.Ax { def xxx : Int; } // error: cyclic inheritance: trait Ax extends itself
// (Note that inheriting a class of the same name is no longer allowed)
trait Ax extends super.Ax { def xxx : Int; } // error: cyclic inheritance: trait Ax extends itself) // error: class definitions cannot be overridden
abstract class InnerB extends InnerA {
// type B <: A;
val a : A = doB;
Expand Down
12 changes: 12 additions & 0 deletions tests/neg/i1750.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
trait Lang1 {
trait Exp
trait Visitor { def f(left: Exp): Unit }
class Eval1 extends Visitor { self =>
def f(left: Exp) = ()
}
}
trait Lang2 extends Lang1 {
class Visitor extends Eval1 { Visitor => // error: classes cannot be overridden
}
}

13 changes: 13 additions & 0 deletions tests/neg/i1750a.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
trait Lang1 {
trait Exp
trait Visitor { def f(left: Exp): Unit }
class Eval1 extends Visitor { self =>
def f(left: Exp) = ()
}
}
trait Lang3 { self: Lang1 =>
class Visitor extends Eval1 { Visitor => // error: classes cannot be overridden
}
}
trait Lang4 extends Lang1 with Lang3

2 changes: 1 addition & 1 deletion tests/neg/i1806.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@ trait A {
class Inner
}
trait B extends A {
class Inner extends super.Inner // error
class Inner extends super.Inner // error // error
}

5 changes: 0 additions & 5 deletions tests/neg/overrides.scala
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,6 @@ package p2 { // all being in the same package compiles fine
}
}

abstract class T3 extends T2 {
class A { // error: classes cannot be overridden
bug()
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why remove the class here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It now masks the other errors because the error is raised now in typer, whereas the others would be raised in RefChecks.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, it makes sense.


class A[T] {
Expand Down
2 changes: 1 addition & 1 deletion tests/neg/t7278.scala
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
class A { class E }
class B extends A { class E }
class B extends A { class EB }
trait C { type E = Int }
trait D { type E = String }
trait EC { type E }
Expand Down