Permalink
Browse files

Fix and optimization in overriding logic.

Given:

  trait Foo { def f: Int = 5 }
  trait Bar extends Foo { def f: Int }

I noticed allOverriddenSymbols for the abstract f defined in Bar
was returning the method from Foo, even though an abstract method
cannot override a concrete one. There were other bits of code
which accidentally depended on this outcome. Now allOverriddenSymbols
for Bar is empty.

The optimization is that whether or not a symbol overrides
any other symbols is known at creation time and does not change.
We now spend a lot less time looking for overridden symbols in
base classes by storing that value, "isOverridingSymbol".
  • Loading branch information...
1 parent bafebe1 commit 0eff6cd49d32c20d5648b57a01b5e80339a1cca7 @paulp paulp committed Feb 19, 2013
View
3 src/compiler/scala/tools/nsc/typechecker/Contexts.scala
@@ -573,8 +573,7 @@ trait Contexts { self: Analyzer =>
( superAccess
|| pre.isInstanceOf[ThisType]
|| phase.erasedTypes
- || isProtectedAccessOK(sym)
- || (sym.allOverriddenSymbols exists isProtectedAccessOK)
+ || (sym.overrideChain exists isProtectedAccessOK)
// that last condition makes protected access via self types work.
)
)
View
9 src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
@@ -726,8 +726,11 @@ abstract class RefChecks extends InfoTransform with scala.reflect.internal.trans
else if (clazz.isTrait && !(clazz isSubClass AnyValClass)) {
// For non-AnyVal classes, prevent abstract methods in interfaces that override
// final members in Object; see #4431
- for (decl <- clazz.info.decls.iterator) {
- val overridden = decl.overriddenSymbol(ObjectClass)
+ for (decl <- clazz.info.decls) {
+ // Have to use matchingSymbol, not a method involving overridden symbols,
+ // because the scala type system understands that an abstract method here does not
+ // override a concrete method in Object. The jvm, however, does not.
+ val overridden = decl.matchingSymbol(ObjectClass, ObjectClass.tpe)
if (overridden.isFinal)
unit.error(decl.pos, "trait cannot redefine final method from class AnyRef")
}
@@ -1499,8 +1502,8 @@ abstract class RefChecks extends InfoTransform with scala.reflect.internal.trans
// on Unit, in which case we had better let it slide.
val isOk = (
sym.isGetter
- || sym.allOverriddenSymbols.exists(over => !(over.tpe.resultType =:= sym.tpe.resultType))
|| (sym.name containsName nme.DEFAULT_GETTER_STRING)
+ || sym.allOverriddenSymbols.exists(over => !(over.tpe.resultType =:= sym.tpe.resultType))
)
if (!isOk)
unit.warning(sym.pos, s"side-effecting nullary methods are discouraged: suggest defining as `def ${sym.name.decode}()` instead")
View
133 src/reflect/scala/reflect/internal/Symbols.scala
@@ -853,12 +853,13 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
/** Is this a term symbol only defined in a refinement (so that it needs
* to be accessed by reflection)?
*/
- def isOnlyRefinementMember: Boolean =
+ def isOnlyRefinementMember: Boolean = (
isTerm && // type members are not affected
owner.isRefinementClass && // owner must be a refinement class
(owner.info decl name) == this && // symbol must be explicitly declared in the refinement (not synthesized from glb)
- allOverriddenSymbols.isEmpty && // symbol must not override a symbol in a base class
+ !isOverridingSymbol && // symbol must not override a symbol in a base class
!isConstant // symbol must not be a constant. Question: Can we exclude @inline methods as well?
+ )
final def isStructuralRefinementMember = owner.isStructuralRefinement && isPossibleInRefinement && isPublic
final def isPossibleInRefinement = !isConstructor && !isOverridingSymbol
@@ -960,14 +961,6 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
def ownerChain: List[Symbol] = this :: owner.ownerChain
def originalOwnerChain: List[Symbol] = this :: originalOwner.getOrElse(this, rawowner).originalOwnerChain
- // All the symbols overridden by this symbol and this symbol at the head,
- // or Nil if this is NoSymbol.
- def overrideChain = (
- if (this eq NoSymbol) Nil
- else if (!owner.isClass) this :: Nil
- else this :: allOverriddenSymbols
- )
-
// Non-classes skip self and return rest of owner chain; overridden in ClassSymbol.
def enclClassChain: List[Symbol] = owner.enclClassChain
@@ -2070,81 +2063,111 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
* @param ofclazz The class containing the symbol's definition
* @param site The base type from which member types are computed
*/
- final def matchingSymbol(ofclazz: Symbol, site: Type): Symbol = {
- //OPT cut down on #closures by special casing non-overloaded case
- // was: ofclazz.info.nonPrivateDecl(name) filter (sym =>
- // !sym.isTerm || (site.memberType(this) matches site.memberType(sym)))
- val result = ofclazz.info.nonPrivateDecl(name)
- def qualifies(sym: Symbol) = !sym.isTerm || (site.memberType(this) matches site.memberType(sym))
- if ((result eq NoSymbol) || !result.isOverloaded && qualifies(result)) result
- else result filter qualifies
- }
+ final def matchingSymbol(ofclazz: Symbol, site: Type): Symbol =
+ matchingSymbolInternal(site, ofclazz.info nonPrivateDecl name)
/** The non-private member of `site` whose type and name match the type of this symbol. */
final def matchingSymbol(site: Type, admit: Long = 0L): Symbol =
- site.nonPrivateMemberAdmitting(name, admit).filter(sym =>
- !sym.isTerm || (site.memberType(this) matches site.memberType(sym)))
+ matchingSymbolInternal(site, site.nonPrivateMemberAdmitting(name, admit))
- /** The symbol, in class `ofclazz`, that is overridden by this symbol.
+ private def matchingSymbolInternal(site: Type, candidate: Symbol): Symbol = {
+ def qualifies(sym: Symbol) = !sym.isTerm || ((site memberType this) matches (site memberType sym))
+ //OPT cut down on #closures by special casing non-overloaded case
+ if (candidate.isOverloaded) candidate filter qualifies
+ else if (qualifies(candidate)) candidate
+ else NoSymbol
+ }
+
+ /** The symbol, in class `baseClass`, that is overridden by this symbol.
*
- * @param ofclazz is a base class of this symbol's owner.
+ * @param baseClass is a base class of this symbol's owner.
*/
- final def overriddenSymbol(ofclazz: Symbol): Symbol =
- if (isClassConstructor) NoSymbol else matchingSymbol(ofclazz, owner.thisType)
+ final def overriddenSymbol(baseClass: Symbol): Symbol = (
+ // concrete always overrides abstract, so don't let an abstract definition
+ // claim to be overriding an inherited concrete one.
+ matchingInheritedSymbolIn(baseClass) filter (res => res.isDeferred || !this.isDeferred)
+ )
+
+ private def matchingInheritedSymbolIn(baseClass: Symbol): Symbol =
+ if (canMatchInheritedSymbols) matchingSymbol(baseClass, owner.thisType) else NoSymbol
/** The symbol overriding this symbol in given subclass `ofclazz`.
*
* @param ofclazz is a subclass of this symbol's owner
*/
- final def overridingSymbol(ofclazz: Symbol): Symbol =
- if (isClassConstructor) NoSymbol else matchingSymbol(ofclazz, ofclazz.thisType)
+ final def overridingSymbol(ofclazz: Symbol): Symbol = (
+ if (canMatchInheritedSymbols)
+ matchingSymbol(ofclazz, ofclazz.thisType)
+ else
+ NoSymbol
+ )
- /** Returns all symbols overriden by this symbol. */
- final def allOverriddenSymbols: List[Symbol] = (
- if ((this eq NoSymbol) || !owner.isClass) Nil
- else {
- def loop(xs: List[Symbol]): List[Symbol] = xs match {
- case Nil => Nil
- case x :: xs =>
- overriddenSymbol(x) match {
- case NoSymbol => loop(xs)
- case sym => sym :: loop(xs)
- }
- }
- loop(owner.ancestors)
- }
+ /** If false, this symbol cannot possibly participate in an override,
+ * either as overrider or overridee. For internal use; you should consult
+ * with isOverridingSymbol. This is used by isOverridingSymbol to escape
+ * the recursive knot.
+ */
+ private def canMatchInheritedSymbols = (
+ (this ne NoSymbol)
+ && owner.isClass
+ && !this.isClass
+ && !this.isConstructor
+ )
+
+ // All the symbols overridden by this symbol and this symbol at the head,
+ // or Nil if this is NoSymbol.
+ def overrideChain = (
+ if (this eq NoSymbol) Nil
+ else if (isOverridingSymbol) this :: allOverriddenSymbols
+ else this :: Nil
)
+ /** Returns all symbols overridden by this symbol. */
+ final def allOverriddenSymbols: List[Symbol] = {
+ def loop(xs: List[Symbol]): List[Symbol] = xs match {
+ case Nil => Nil
+ case x :: xs =>
+ overriddenSymbol(x) match {
+ case NoSymbol => loop(xs)
+ case sym => sym :: loop(xs)
+ }
+ }
+ if (isOverridingSymbol) loop(owner.ancestors) else Nil
+ }
+
/** Equivalent to allOverriddenSymbols.nonEmpty, but more efficient. */
- // !!! When if ever will this answer differ from .isOverride?
- // How/where is the OVERRIDE flag managed, as compared to how checks
- // based on type membership will evaluate?
- def isOverridingSymbol = owner.isClass && (
- owner.ancestors exists (cls => matchingSymbol(cls, owner.thisType) != NoSymbol)
+ lazy val isOverridingSymbol = (
+ canMatchInheritedSymbols
+ && owner.ancestors.exists(base => overriddenSymbol(base) != NoSymbol)
)
+
/** Equivalent to allOverriddenSymbols.head (or NoSymbol if no overrides) but more efficient. */
def nextOverriddenSymbol: Symbol = {
- if ((this ne NoSymbol) && owner.isClass) owner.ancestors foreach { base =>
- val sym = overriddenSymbol(base)
- if (sym != NoSymbol)
- return sym
+ @tailrec def loop(bases: List[Symbol]): Symbol = bases match {
+ case Nil => NoSymbol
+ case base :: rest =>
+ val sym = overriddenSymbol(base)
+ if (sym == NoSymbol) loop(rest) else sym
}
- NoSymbol
+ if (isOverridingSymbol) loop(owner.ancestors) else NoSymbol
}
/** Returns all symbols overridden by this symbol, plus all matching symbols
* defined in parents of the selftype.
*/
- final def extendedOverriddenSymbols: List[Symbol] =
- if (!owner.isClass) Nil
- else owner.thisSym.ancestors map overriddenSymbol filter (_ != NoSymbol)
+ final def extendedOverriddenSymbols: List[Symbol] = (
+ if (canMatchInheritedSymbols)
+ owner.thisSym.ancestors map overriddenSymbol filter (_ != NoSymbol)
+ else
+ Nil
+ )
/** The symbol accessed by a super in the definition of this symbol when
* seen from class `base`. This symbol is always concrete.
* pre: `this.owner` is in the base class sequence of `base`.
*/
final def superSymbol(base: Symbol): Symbol = {
- var bcs = base.info.baseClasses.dropWhile(owner != _).tail
+ var bcs = base.info.baseClasses dropWhile (owner != _) drop 1
var sym: Symbol = NoSymbol
while (!bcs.isEmpty && sym == NoSymbol) {
if (!bcs.head.isImplClass)
View
1 test/files/run/all-overridden.check
@@ -0,0 +1 @@
+method g
View
11 test/files/run/all-overridden.scala
@@ -0,0 +1,11 @@
+import scala.reflect.runtime.universe._
+
+object Test {
+ trait Foo { def f: Int = 5 ; def g: Int }
+ trait Bar extends Foo { def f: Int ; def g: Int = 5 }
+
+ def main(args: Array[String]): Unit = {
+ // We should see g, but not f or $init$.
+ typeOf[Bar].declarations.toList.flatMap(_.allOverriddenSymbols) foreach println
+ }
+}

0 comments on commit 0eff6cd

Please sign in to comment.