Skip to content
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

SI-9408 Avoid capturing outer class in local classes. #4652

Merged
merged 3 commits into from
Jul 23, 2015
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
14 changes: 11 additions & 3 deletions src/compiler/scala/tools/nsc/transform/Constructors.scala
Original file line number Diff line number Diff line change
Expand Up @@ -165,11 +165,19 @@ abstract class Constructors extends Statics with Transform with ast.TreeDSL {
return
}

// Note: elision of outer reference is based on a class-wise analysis, if a class might have subclasses,
// it doesn't work. For example, `LocalParent` retains the outer reference in:
//
// class Outer { def test = {class LocalParent; class LocalChild extends LocalParent } }
//
// See run/t9408.scala for related test cases.
val isEffectivelyFinal = clazz.isEffectivelyFinal
def isParamCandidateForElision(sym: Symbol) = (sym.isParamAccessor && sym.isPrivateLocal)
def isOuterCandidateForElision(sym: Symbol) = (sym.isOuterAccessor && sym.owner.isEffectivelyFinal && !sym.isOverridingSymbol)
def isOuterCandidateForElision(sym: Symbol) = (sym.isOuterAccessor && isEffectivelyFinal && !sym.isOverridingSymbol)

val paramCandidatesForElision: Set[ /*Field*/ Symbol] = (clazz.info.decls.toSet filter isParamCandidateForElision)
val outerCandidatesForElision: Set[ /*Method*/ Symbol] = (clazz.info.decls.toSet filter isOuterCandidateForElision)
val decls = clazz.info.decls.toSet
val paramCandidatesForElision: Set[ /*Field*/ Symbol] = (decls filter isParamCandidateForElision)
val outerCandidatesForElision: Set[ /*Method*/ Symbol] = (decls filter isOuterCandidateForElision)

omittables ++= paramCandidatesForElision
omittables ++= outerCandidatesForElision
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ trait TreeAndTypeAnalysis extends Debugging {

if(grouped) {
def enumerateChildren(sym: Symbol) = {
sym.children.toList
sym.sealedChildren.toList
.sortBy(_.sealedSortName)
.filterNot(x => x.isSealed && x.isAbstractClass && !isPrimitiveValueClass(x))
}
Expand Down
4 changes: 2 additions & 2 deletions src/compiler/scala/tools/nsc/typechecker/Checkable.scala
Original file line number Diff line number Diff line change
Expand Up @@ -212,8 +212,8 @@ trait Checkable {
)
/** Are all children of these symbols pairwise irreconcilable? */
def allChildrenAreIrreconcilable(sym1: Symbol, sym2: Symbol) = (
sym1.children.toList forall (c1 =>
sym2.children.toList forall (c2 =>
sym1.sealedChildren.toList forall (c1 =>
sym2.sealedChildren.toList forall (c2 =>
areIrreconcilableAsParents(c1, c2)
)
)
Expand Down
2 changes: 2 additions & 0 deletions src/compiler/scala/tools/nsc/typechecker/Typers.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1694,6 +1694,8 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
psym addChild context.owner
else
pending += ParentSealedInheritanceError(parent, psym)
if (psym.isLocalToBlock && !phase.erasedTypes)
psym addChild context.owner
val parentTypeOfThis = parent.tpe.dealias.typeOfThis

if (!(selfType <:< parentTypeOfThis) &&
Expand Down
8 changes: 5 additions & 3 deletions src/reflect/scala/reflect/internal/Symbols.scala
Original file line number Diff line number Diff line change
Expand Up @@ -980,7 +980,7 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
private def isNotOverridden = (
owner.isClass && (
owner.isEffectivelyFinal
|| owner.isSealed && owner.children.forall(c => c.isEffectivelyFinal && (overridingSymbol(c) == NoSymbol))
|| (owner.isSealed && owner.sealedChildren.forall(c => c.isEffectivelyFinal && (overridingSymbol(c) == NoSymbol)))
)
)

Expand All @@ -992,6 +992,7 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
isPrivate
|| isLocalToBlock
)
|| isClass && originalOwner.isTerm && children.isEmpty // we track known subclasses of term-owned classes, use that infer finality
)
/** Is this symbol effectively final or a concrete term member of sealed class whose children do not override it */
final def isEffectivelyFinalOrNotOverridden: Boolean = isEffectivelyFinal || (isTerm && !isDeferred && isNotOverridden)
Expand Down Expand Up @@ -2495,14 +2496,15 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
def associatedFile: AbstractFile = enclosingTopLevelClass.associatedFile
def associatedFile_=(f: AbstractFile) { abort("associatedFile_= inapplicable for " + this) }

/** If this is a sealed class, its known direct subclasses.
/** If this is a sealed or local class, its known direct subclasses.
* Otherwise, the empty set.
*/
def children: Set[Symbol] = Set()
final def sealedChildren: Set[Symbol] = if (!isSealed) Set.empty else children

/** Recursively assemble all children of this symbol.
*/
def sealedDescendants: Set[Symbol] = children.flatMap(_.sealedDescendants) + this
final def sealedDescendants: Set[Symbol] = if (!isSealed) Set(this) else children.flatMap(_.sealedDescendants) + this

@inline final def orElse(alt: => Symbol): Symbol = if (this ne NoSymbol) this else alt
@inline final def andAlso(f: Symbol => Unit): Symbol = { if (this ne NoSymbol) f(this) ; this }
Expand Down
61 changes: 61 additions & 0 deletions test/files/run/t9408.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
class Outer {
def assertNoFields(c: Class[_]) {
assert(c.getDeclaredFields.isEmpty)
}
def assertHasOuter(c: Class[_]) {
assert(c.getDeclaredFields.exists(_.getName.contains("outer")))
}
class Member
final class FinalMember

def test {
assertHasOuter(classOf[Member])
assertNoFields(classOf[FinalMember])
final class C
assertNoFields(classOf[C])
class D
assertNoFields(classOf[D])
(() => {class E; assertNoFields(classOf[E])}).apply()

// The outer reference elision currently runs on a class-by-class basis. If it cannot rule out that a class has
// subclasses, it will not remove the outer reference. A smarter analysis here could detect if no members of
// a sealed (or effectively sealed) hierarchy use the outer reference, the optimization could be performed.
class Parent
class Child extends Parent
assertHasOuter(classOf[Parent])

// Note: outer references (if they haven't been elided) are used in pattern matching as follows.
// This isn't relevant to term-owned classes, as you can't refer to them with a prefix that includes
// the outer class.
val outer1 = new Outer
val outer2 = new Outer
(new outer1.Member: Any) match {
case _: outer2.Member => sys.error("wrong match!")
case _: outer1.Member => // okay
}

// ... continuing on that theme, note that `Member` isn't considered as a local class, it is owned by a the class
// `LocalOuter`, which itself happens to be term-owned. So we expect that it has an outer reference, and that this
// is respected in type tests.
class LocalOuter {
class Member
final class FinalMember
}
assertNoFields(classOf[LocalOuter])
assertHasOuter(classOf[LocalOuter#Member])
val localOuter1 = new LocalOuter
val localOuter2 = new LocalOuter
(new localOuter1.Member: Any) match {
case _: localOuter2.Member => sys.error("wrong match!")
case _: localOuter1.Member => // okay
}
// Final member classes still lose the outer reference.
assertNoFields(classOf[LocalOuter#FinalMember])
}
}

object Test {
def main(args: Array[String]): Unit = {
new Outer().test
}
}