diff options
author | Paul Phillips <paulp@improving.org> | 2013-02-19 12:57:21 -0800 |
---|---|---|
committer | Paul Phillips <paulp@improving.org> | 2013-02-19 13:07:55 -0800 |
commit | 0eff6cd49d32c20d5648b57a01b5e80339a1cca7 (patch) | |
tree | 44e41e47efafccc9ccd343e97de69e486e972104 | |
parent | bafebe1c161f8db0be758c30fe5cc51082a56427 (diff) | |
download | scala-0eff6cd49d32c20d5648b57a01b5e80339a1cca7.tar.gz scala-0eff6cd49d32c20d5648b57a01b5e80339a1cca7.tar.bz2 scala-0eff6cd49d32c20d5648b57a01b5e80339a1cca7.zip |
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".
-rw-r--r-- | src/compiler/scala/tools/nsc/typechecker/Contexts.scala | 3 | ||||
-rw-r--r-- | src/compiler/scala/tools/nsc/typechecker/RefChecks.scala | 9 | ||||
-rw-r--r-- | src/reflect/scala/reflect/internal/Symbols.scala | 133 | ||||
-rw-r--r-- | test/files/run/all-overridden.check | 1 | ||||
-rw-r--r-- | test/files/run/all-overridden.scala | 11 |
5 files changed, 97 insertions, 60 deletions
diff --git a/src/compiler/scala/tools/nsc/typechecker/Contexts.scala b/src/compiler/scala/tools/nsc/typechecker/Contexts.scala index 711085e6c9..b070bd1b49 100644 --- a/src/compiler/scala/tools/nsc/typechecker/Contexts.scala +++ b/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. ) ) diff --git a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala index 60a73036f8..dd16e9de30 100644 --- a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala +++ b/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") diff --git a/src/reflect/scala/reflect/internal/Symbols.scala b/src/reflect/scala/reflect/internal/Symbols.scala index 4f6dab3e7c..fbf14e8156 100644 --- a/src/reflect/scala/reflect/internal/Symbols.scala +++ b/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) diff --git a/test/files/run/all-overridden.check b/test/files/run/all-overridden.check new file mode 100644 index 0000000000..1b620b1176 --- /dev/null +++ b/test/files/run/all-overridden.check @@ -0,0 +1 @@ +method g diff --git a/test/files/run/all-overridden.scala b/test/files/run/all-overridden.scala new file mode 100644 index 0000000000..1b798ef748 --- /dev/null +++ b/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 + } +} |