From db9fd559ec70f033b475b0d91c6049b68955e095 Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Fri, 31 Jan 2014 13:23:33 +0100 Subject: SI-7475 Refactor findMember computation into a class This affords us: - a better chance to share code with `findMembers` again - the ability to break up the logic into smaller methods for scrutability and JIT-tability. The cost is the use of the heap rather than the stack for the working area of the computation. But I didn't notice a difference between `locker.lib` and `quick.lib` after this change. I've left the old implementation in place with an assertion to verify identical results. In the next commit, this stepping stone will be removed, and we'll update `findMembers` to share the code. Some problem areas have been highlighted and will be addressed after the refactoring phase is complete. --- src/reflect/scala/reflect/internal/Types.scala | 21 +- .../scala/reflect/internal/tpe/FindMembers.scala | 223 +++++++++++++++++++++ 2 files changed, 240 insertions(+), 4 deletions(-) create mode 100644 src/reflect/scala/reflect/internal/tpe/FindMembers.scala (limited to 'src/reflect') diff --git a/src/reflect/scala/reflect/internal/Types.scala b/src/reflect/scala/reflect/internal/Types.scala index 17a58d79f6..b7cf8d2197 100644 --- a/src/reflect/scala/reflect/internal/Types.scala +++ b/src/reflect/scala/reflect/internal/Types.scala @@ -82,6 +82,7 @@ trait Types with tpe.GlbLubs with tpe.TypeMaps with tpe.TypeConstraints + with tpe.FindMembers with util.Collections { self: SymbolTable => import definitions._ @@ -248,7 +249,7 @@ trait Types * type instead of the proxy. This gives buried existentials a * chance to make peace with the other types. See SI-5330. */ - private def narrowForFindMember(tp: Type): Type = { + private[internal] def narrowForFindMember(tp: Type): Type = { val w = tp.widen // Only narrow on widened type when we have to -- narrow is expensive unless the target is a singleton type. if ((tp ne w) && containsExistential(w)) w.narrow @@ -989,6 +990,7 @@ trait Types * */ def findMembers(excludedFlags: Long, requiredFlags: Long): Scope = { + // TODO refactor to use `FindMemberBase` def findMembersInternal: Scope = { var members: Scope = null if (Statistics.canEnable) Statistics.incCounter(findMembersCount) @@ -1055,14 +1057,25 @@ trait Types * Find member(s) in this type. If several members matching criteria are found, they are * returned in an OverloadedSymbol * - * @param name The member's name, where nme.ANYNAME means `unspecified` + * @param name The member's name * @param excludedFlags Returned members do not have these flags * @param requiredFlags Returned members do have these flags * @param stableOnly If set, return only members that are types or stable values */ - //TODO: use narrow only for modules? (correct? efficiency gain?) def findMember(name: Name, excludedFlags: Long, requiredFlags: Long, stableOnly: Boolean): Symbol = { - def findMemberInternal: Symbol = { + def findMemberInternal = { + // TODO delete `findMemberInternalOld` + val resultOld = findMemberInternalOld + val resultNew = findMemberInternalNew + if (resultOld.alternatives != resultNew.alternatives) { + findMemberInternalOld + findMemberInternalNew + } + assert(resultOld.alternatives == resultNew.alternatives, s"\nOLD:${resultOld.alternatives}\nNEW${resultNew.alternatives}") + resultNew + } + def findMemberInternalNew = new FindMember(this, name, excludedFlags, requiredFlags, stableOnly).apply() + def findMemberInternalOld: Symbol = { var member: Symbol = NoSymbol var members: List[Symbol] = null var lastM: ::[Symbol] = null diff --git a/src/reflect/scala/reflect/internal/tpe/FindMembers.scala b/src/reflect/scala/reflect/internal/tpe/FindMembers.scala new file mode 100644 index 0000000000..380e11e0d7 --- /dev/null +++ b/src/reflect/scala/reflect/internal/tpe/FindMembers.scala @@ -0,0 +1,223 @@ +/* NSC -- new Scala compiler + * Copyright 2005-2014 LAMP/EPFL + * @author Jason Zaugg + */ +package scala.reflect.internal +package tpe + +import Flags._ +import util.Statistics +import TypesStats._ + +trait FindMembers { + this: SymbolTable => + + /** Implementatation of `Type#{findMember, findMembers}` */ + private[internal] abstract class FindMemberBase[T](tpe: Type, name: Name, excludedFlags: Long, requiredFlags: Long) { + protected val initBaseClasses: List[Symbol] = tpe.baseClasses + + // The first base class, or the symbol of the ThisType + private[this] var _selectorClass: Symbol = null + private def selectorClass: Symbol = { + if (_selectorClass eq null) { + _selectorClass = tpe match { + case tt: ThisType => tt.sym // SI-7507 the first base class is not necessarily the selector class. + case _ => initBaseClasses.head + } + } + _selectorClass + } + + // Cache for the narrowed type of `tp` (in `tp.findMember`). + // This is needed to avoid mismatched existential types are reported in SI-5330. + private[this] var _self: Type = null + protected def self: Type = { + // TODO: use narrow only for modules? (correct? efficiency gain?) (<-- Note: this comment predates SI-5330) + if (_self eq null) _self = narrowForFindMember(tpe) + _self + } + + // Main entry point + def apply(): T = { + if (Statistics.canEnable) Statistics.incCounter(findMemberCount) + val start = if (Statistics.canEnable) Statistics.pushTimer(typeOpsStack, findMemberNanos) else null + try searchConcreteThenDeferred + finally if (Statistics.canEnable) Statistics.popTimer(typeOpsStack, start) + } + + protected def result: T + + // SLS 5.1.3 First, a concrete definition always overrides an abstract definition + private def searchConcreteThenDeferred: T = { + val deferredSeen = walkBaseClasses(requiredFlags, excludedFlags | DEFERRED) + if (deferredSeen) // OPT: the `if` avoids a second pass if the first pass didn't spot any candidates. + walkBaseClasses(requiredFlags | DEFERRED, excludedFlags & ~(DEFERRED.toLong)) + result + } + + /* + * Walk up through the decls of each base class. + * + * Called in two passes: first excluding deferred, then mandating it. + * + * @return true, if a potential deferred member was seen on the first pass that calls for a second pass. + */ + private def walkBaseClasses(required: Long, excluded: Long): Boolean = { + var bcs = initBaseClasses + + // Has we seen a candidate deferred member? + var deferredSeen = false + + while (!bcs.isEmpty) { + val currentBaseClass = bcs.head + val decls = currentBaseClass.info.decls + val findAll = name == nme.ANYname + var entry = if (findAll) decls.elems else decls.lookupEntry(name) + while (entry ne null) { + val sym = entry.sym + val flags = sym.flags + val meetsRequirements = (flags & required) == required + if (meetsRequirements) { + val excl: Long = flags & excluded + val isExcluded: Boolean = excl != 0L + if (!isExcluded && isPotentialMember(sym, flags, currentBaseClass)) { + if (shortCircuit(sym)) return false + else addMemberIfNew(sym) + } else if (excl == DEFERRED) { + deferredSeen = true + } + } + entry = if (findAll) entry.next else decls lookupNextEntry entry + } + + bcs = bcs.tail + } + deferredSeen + } + + /* Should this symbol be returned immediately as the sole result? */ + protected def shortCircuit(sym: Symbol): Boolean + + /* Add this member to the final result, unless an already-found member matches it. */ + protected def addMemberIfNew(sym: Symbol): Unit + + // Is `sym` a potentially member of `baseClass`? + // + // Q. When does a potential member fail to be a an actual member? + // A. if it is subsumed by an member in a subclass. + private def isPotentialMember(sym: Symbol, flags: Long, owner: Symbol): Boolean = { + // conservatively (performance wise) doing this with flags masks rather than `sym.isPrivate` + // to avoid multiple calls to `Symbol#flags`. + val isPrivateLocal = (flags & PrivateLocal) == PrivateLocal + + def admitPrivateLocal(sym: Symbol): Boolean = + ( + (selectorClass == owner) + || !isPrivateLocal + || selectorClass.hasTransOwner(owner) // private[this] only a member from within the class (incorrect!) + ) + + // TODO first condition incorrect wrt self types! In neg/t7507, bippy should not be a member! + // The condition, or a close variant thereof, will still be needed to prevent inheritance of constructors. + owner == initBaseClasses.head || admitPrivateLocal(sym) && sym.name != nme.CONSTRUCTOR + } + + // True unless the already-found member of type `memberType` matches the candidate symbol `other`. + protected def isNewMember(member: Symbol, other: Symbol): Boolean = + ( (other ne member) + && ( (member.owner eq other.owner) + || (other.flags & PRIVATE) != 0 + || !(memberTypeLow(member) matches memberTypeHi(other)) + ) + ) + + // Cache for the member type of a candidate member when comparing against multiple, already-found existing members + // + // TODO this cache is probably unnecessary, `tp.memberType(sym: MethodSymbol)` is already cached internally. + private[this] var _memberTypeHiCache: Type = null + private[this] var _memberTypeHiCacheSym: Symbol = null + + protected def memberTypeHi(sym: Symbol): Type = { + if (_memberTypeHiCacheSym ne sym) { + _memberTypeHiCache = self.memberType(sym) + _memberTypeHiCacheSym = sym + } + _memberTypeHiCache + } + + // member type of the LHS of `matches` call. This is an extension point to enable a cache in + // FindMember. + protected def memberTypeLow(sym: Symbol): Type = self.memberType(sym) + } + + private[reflect] final class FindMember(tpe: Type, name: Name, excludedFlags: Long, requiredFlags: Long, stableOnly: Boolean) + extends FindMemberBase[Symbol](tpe, name, excludedFlags, requiredFlags) { + // Gathering the results into a hand rolled ListBuffer + // TODO Try just using a ListBuffer to see if this low-level-ness is worth it. + private[this] var member0: Symbol = NoSymbol + private[this] var members: List[Symbol] = null + private[this] var lastM: ::[Symbol] = null + + private def clearAndAddResult(sym: Symbol): Unit = { + member0 = sym + members = null + lastM = null + } + + protected def shortCircuit(sym: Symbol): Boolean = (name.isTypeName || (stableOnly && sym.isStable && !sym.hasVolatileType)) && { + clearAndAddResult(sym) + true + } + + protected def addMemberIfNew(sym: Symbol): Unit = + if (member0 eq NoSymbol) { + member0 = sym // The first found member + } else if (members eq null) { + // We've found exactly one member so far... + if (isNewMember(member0, sym)) { + // ... make that two. + lastM = new ::(sym, null) + members = member0 :: lastM + } + } else { + // Already found 2 or more members + var ms: List[Symbol] = members + + var isNew = true + while ((ms ne null) && isNew) { + val member = ms.head + if (!isNewMember(member, sym)) + isNew = false + ms = ms.tail + } + if (isNew) { + val lastM1 = new ::(sym, null) + lastM.tl = lastM1 + lastM = lastM1 + } + } + + // Cache for the member type of the first member we find. + private[this] var _member0Tpe: Type = null + private[this] def member0Tpe: Type = { + assert(member0 != null) + if (_member0Tpe eq null) _member0Tpe = self.memberType(member0) + _member0Tpe + } + + override protected def memberTypeLow(sym: Symbol): Type = + if (sym eq member0) member0Tpe else super.memberTypeLow(sym) + + // Assemble the result from the hand-rolled ListBuffer + protected def result: Symbol = if (members eq null) { + if (member0 == NoSymbol) { + if (Statistics.canEnable) Statistics.incCounter(noMemberCount) + NoSymbol + } else member0 + } else { + if (Statistics.canEnable) Statistics.incCounter(multMemberCount) + lastM.tl = Nil + initBaseClasses.head.newOverloaded(tpe, members) + } + } +} -- cgit v1.2.3 From aebe379a14ab7b2a3bce35a6faa9d9232c748154 Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Fri, 31 Jan 2014 13:46:57 +0100 Subject: SI-7475 findMember and findMembers: estranged no more Swathes of important logic are duplicated between `findMember` and `findMembers` after they separated on grounds of irreconcilable differences about how fast they should run: d905558 Variation #10 to optimze findMember fcb0c01 Attempt #9 to opimize findMember. 71d2ceb Attempt #8 to opimize findMember. 77e5692 Attempty #7 to optimize findMember 275115e Fixing problem that caused fingerprints to fail in e94252e Attemmpt #6 to optimize findMember 73e61b8 Attempt #5 to optimize findMember. 04f0b65 Attempt #4 to optimize findMember 0e3c70f Attempt #3 to optimize findMember 41f4497 Attempt #2 to optimize findMember 1a73aa0 Attempt #1 to optimize findMember This didn't actually bear fruit, and the intervening years have seen the implementations drift. Now is the time to reunite them under the banner of `FindMemberBase`. Each has a separate subclass to customise the behaviour. This is primarily used by `findMember` to cache member types and to assemble the resulting list of symbols in an low-allocation manner. While there I have introduced some polymorphic calls, the call sites are only bi-morphic, and our typical pattern of compilation involves far more `findMember` calls, so I expect that JIT will keep the virtual call cost to an absolute minimum. Test results have been updated now that `findMembers` correctly excludes constructors and doesn't inherit privates. Coming up next: we can actually fix SI-7475! --- src/reflect/scala/reflect/internal/Types.scala | 170 +-------------------- .../scala/reflect/internal/tpe/FindMembers.scala | 25 +++ test/files/presentation/ide-bug-1000531.check | 2 +- test/files/presentation/ping-pong.check | 2 +- .../files/run/reflection-magicsymbols-invoke.check | 1 - 5 files changed, 29 insertions(+), 171 deletions(-) (limited to 'src/reflect') diff --git a/src/reflect/scala/reflect/internal/Types.scala b/src/reflect/scala/reflect/internal/Types.scala index b7cf8d2197..21ce5114e0 100644 --- a/src/reflect/scala/reflect/internal/Types.scala +++ b/src/reflect/scala/reflect/internal/Types.scala @@ -990,65 +990,7 @@ trait Types * */ def findMembers(excludedFlags: Long, requiredFlags: Long): Scope = { - // TODO refactor to use `FindMemberBase` - def findMembersInternal: Scope = { - var members: Scope = null - if (Statistics.canEnable) Statistics.incCounter(findMembersCount) - val start = if (Statistics.canEnable) Statistics.pushTimer(typeOpsStack, findMembersNanos) else null - - //Console.println("find member " + name.decode + " in " + this + ":" + this.baseClasses)//DEBUG - var required = requiredFlags - var excluded = excludedFlags | DEFERRED - var retryForDeferred = true - var self: Type = null - while (retryForDeferred) { - retryForDeferred = false - val bcs0 = baseClasses - var bcs = bcs0 - while (!bcs.isEmpty) { - val decls = bcs.head.info.decls - var entry = decls.elems - while (entry ne null) { - val sym = entry.sym - val flags = sym.flags - if ((flags & required) == required) { - val excl = flags & excluded - if (excl == 0L && - (// omit PRIVATE LOCALS unless selector class is contained in class owning the def. - (bcs eq bcs0) || - (flags & PrivateLocal) != PrivateLocal || - (bcs0.head.hasTransOwner(bcs.head)))) { - if (members eq null) members = newFindMemberScope - var others: ScopeEntry = members.lookupEntry(sym.name) - var symtpe: Type = null - while ((others ne null) && { - val other = others.sym - (other ne sym) && - ((other.owner eq sym.owner) || - (flags & PRIVATE) != 0 || { - if (self eq null) self = narrowForFindMember(this) - if (symtpe eq null) symtpe = self.memberType(sym) - !(self.memberType(other) matches symtpe) - })}) { - others = members lookupNextEntry others - } - if (others eq null) members enter sym - } else if (excl == DEFERRED) { - retryForDeferred = (excludedFlags & DEFERRED) == 0 - } - } - entry = entry.next - } // while (entry ne null) - // excluded = excluded | LOCAL - bcs = bcs.tail - } // while (!bcs.isEmpty) - required |= DEFERRED - excluded &= ~(DEFERRED.toLong) - } // while (retryForDeferred) - if (Statistics.canEnable) Statistics.popTimer(typeOpsStack, start) - if (members eq null) EmptyScope else members - } - + def findMembersInternal = new FindMembers(this, excludedFlags, requiredFlags).apply() if (this.isGround) findMembersInternal else suspendingTypeVars(typeVarsInType(this))(findMembersInternal) } @@ -1063,115 +1005,7 @@ trait Types * @param stableOnly If set, return only members that are types or stable values */ def findMember(name: Name, excludedFlags: Long, requiredFlags: Long, stableOnly: Boolean): Symbol = { - def findMemberInternal = { - // TODO delete `findMemberInternalOld` - val resultOld = findMemberInternalOld - val resultNew = findMemberInternalNew - if (resultOld.alternatives != resultNew.alternatives) { - findMemberInternalOld - findMemberInternalNew - } - assert(resultOld.alternatives == resultNew.alternatives, s"\nOLD:${resultOld.alternatives}\nNEW${resultNew.alternatives}") - resultNew - } - def findMemberInternalNew = new FindMember(this, name, excludedFlags, requiredFlags, stableOnly).apply() - def findMemberInternalOld: Symbol = { - var member: Symbol = NoSymbol - var members: List[Symbol] = null - var lastM: ::[Symbol] = null - if (Statistics.canEnable) Statistics.incCounter(findMemberCount) - val start = if (Statistics.canEnable) Statistics.pushTimer(typeOpsStack, findMemberNanos) else null - - //Console.println("find member " + name.decode + " in " + this + ":" + this.baseClasses)//DEBUG - var membertpe: Type = null - var required = requiredFlags - var excluded = excludedFlags | DEFERRED - var continue = true - var self: Type = null - - while (continue) { - continue = false - val bcs0 = baseClasses - var bcs = bcs0 - // omit PRIVATE LOCALS unless selector class is contained in class owning the def. - def admitPrivateLocal(owner: Symbol): Boolean = { - val selectorClass = this match { - case tt: ThisType => tt.sym // SI-7507 the first base class is not necessarily the selector class. - case _ => bcs0.head - } - selectorClass.hasTransOwner(owner) - } - while (!bcs.isEmpty) { - val decls = bcs.head.info.decls - var entry = decls.lookupEntry(name) - while (entry ne null) { - val sym = entry.sym - val flags = sym.flags - if ((flags & required) == required) { - val excl = flags & excluded - if (excl == 0L && - ( - (bcs eq bcs0) || - (flags & PrivateLocal) != PrivateLocal || - admitPrivateLocal(bcs.head))) { - if (name.isTypeName || (stableOnly && sym.isStable && !sym.hasVolatileType)) { - if (Statistics.canEnable) Statistics.popTimer(typeOpsStack, start) - return sym - } else if (member eq NoSymbol) { - member = sym - } else if (members eq null) { - if ((member ne sym) && - ((member.owner eq sym.owner) || - (flags & PRIVATE) != 0 || { - if (self eq null) self = narrowForFindMember(this) - if (membertpe eq null) membertpe = self.memberType(member) - !(membertpe matches self.memberType(sym)) - })) { - lastM = new ::(sym, null) - members = member :: lastM - } - } else { - var others: List[Symbol] = members - var symtpe: Type = null - while ((others ne null) && { - val other = others.head - (other ne sym) && - ((other.owner eq sym.owner) || - (flags & PRIVATE) != 0 || { - if (self eq null) self = narrowForFindMember(this) - if (symtpe eq null) symtpe = self.memberType(sym) - !(self.memberType(other) matches symtpe) - })}) { - others = others.tail - } - if (others eq null) { - val lastM1 = new ::(sym, null) - lastM.tl = lastM1 - lastM = lastM1 - } - } - } else if (excl == DEFERRED) { - continue = true - } - } - entry = decls lookupNextEntry entry - } // while (entry ne null) - // excluded = excluded | LOCAL - bcs = if (name == nme.CONSTRUCTOR) Nil else bcs.tail - } // while (!bcs.isEmpty) - required |= DEFERRED - excluded &= ~(DEFERRED.toLong) - } // while (continue) - if (Statistics.canEnable) Statistics.popTimer(typeOpsStack, start) - if (members eq null) { - if (member == NoSymbol) if (Statistics.canEnable) Statistics.incCounter(noMemberCount) - member - } else { - if (Statistics.canEnable) Statistics.incCounter(multMemberCount) - lastM.tl = Nil - baseClasses.head.newOverloaded(this, members) - } - } + def findMemberInternal = new FindMember(this, name, excludedFlags, requiredFlags, stableOnly).apply() if (this.isGround) findMemberInternal else suspendingTypeVars(typeVarsInType(this))(findMemberInternal) diff --git a/src/reflect/scala/reflect/internal/tpe/FindMembers.scala b/src/reflect/scala/reflect/internal/tpe/FindMembers.scala index 380e11e0d7..be5a3f0983 100644 --- a/src/reflect/scala/reflect/internal/tpe/FindMembers.scala +++ b/src/reflect/scala/reflect/internal/tpe/FindMembers.scala @@ -150,6 +150,31 @@ trait FindMembers { protected def memberTypeLow(sym: Symbol): Type = self.memberType(sym) } + private[reflect] final class FindMembers(tpe: Type, excludedFlags: Long, requiredFlags: Long) + extends FindMemberBase[Scope](tpe, nme.ANYname, excludedFlags, requiredFlags) { + private[this] var _membersScope: Scope = null + private def membersScope: Scope = { + if (_membersScope eq null) _membersScope = newFindMemberScope + _membersScope + } + + protected def shortCircuit(sym: Symbol): Boolean = false + protected def result: Scope = membersScope + + protected def addMemberIfNew(sym: Symbol): Unit = { + val members = membersScope + var others = members.lookupEntry(sym.name) + var isNew = true + while (others ne null) { + val member = others.sym + if (!isNewMember(member, sym)) + isNew = false + others = members lookupNextEntry others // next existing member with the same name. + } + if (isNew) members.enter(sym) + } + } + private[reflect] final class FindMember(tpe: Type, name: Name, excludedFlags: Long, requiredFlags: Long, stableOnly: Boolean) extends FindMemberBase[Symbol](tpe, name, excludedFlags, requiredFlags) { // Gathering the results into a hand rolled ListBuffer diff --git a/test/files/presentation/ide-bug-1000531.check b/test/files/presentation/ide-bug-1000531.check index ef8a1d12de..d8c7a369f7 100644 --- a/test/files/presentation/ide-bug-1000531.check +++ b/test/files/presentation/ide-bug-1000531.check @@ -3,7 +3,7 @@ reload: CrashOnLoad.scala askTypeCompletion at CrashOnLoad.scala(6,12) ================================================================================ [response] askTypeCompletion at (6,12) -retrieved 118 members +retrieved 117 members [inaccessible] protected[package lang] def clone(): Object [inaccessible] protected[package lang] def finalize(): Unit [inaccessible] protected[this] def reversed: List[B] diff --git a/test/files/presentation/ping-pong.check b/test/files/presentation/ping-pong.check index ab4cfc9e7a..220bdf33b2 100644 --- a/test/files/presentation/ping-pong.check +++ b/test/files/presentation/ping-pong.check @@ -3,7 +3,7 @@ reload: PingPong.scala askTypeCompletion at PingPong.scala(10,23) ================================================================================ [response] askTypeCompletion at (10,23) -retrieved 33 members +retrieved 32 members [inaccessible] private[this] val ping: Ping [inaccessible] protected[package lang] def clone(): Object [inaccessible] protected[package lang] def finalize(): Unit diff --git a/test/files/run/reflection-magicsymbols-invoke.check b/test/files/run/reflection-magicsymbols-invoke.check index 037ba33d21..b153ae0470 100644 --- a/test/files/run/reflection-magicsymbols-invoke.check +++ b/test/files/run/reflection-magicsymbols-invoke.check @@ -80,7 +80,6 @@ Array it's important to print the list of Array's members if some of them change (possibly, adding and/or removing magic symbols), we must update this test constructor Array: (_length: Int)Array[T] -constructor Cloneable: ()java.lang.Cloneable method !=: (x$1: Any)Boolean method ##: ()Int method $asInstanceOf: [T0]()T0 -- cgit v1.2.3 From 8d96380caeb1f03da8916d494e878f57a363f459 Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Fri, 31 Jan 2014 14:29:19 +0100 Subject: SI-7475 Private members are not inheritable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It turns out `findMembers` has been a bit sloppy in recent years and has returned private members from *anywhere* up the base class sequence. Access checks usually pick up the slack and eliminate the unwanted privates. But, in concert with the "concrete beats abstract" rule in `findMember`, the following mishap appeared: scala> :paste // Entering paste mode (ctrl-D to finish) trait T { def a: Int } trait B { private def a: Int = 0 } trait C extends T with B { a } // Exiting paste mode, now interpreting. :9: error: method a in trait B cannot be accessed in C trait C extends T with B { a } ^ I noticed this when compiling Akka against JDK 8; a new private method in the bowels of the JDK was enough to break the build! It turns out that some finesse in needed to interpret SLS 5.2: > The private modifier can be used with any definition or declaration > in a template. They are not inherited by subclasses [...] So, can we simply exclude privates from all but the first base class? No, as that might be a refinement class! The following must be allowed: trait A { private def foo = 0; trait T { self: A => this.foo } } This commit: - tracks when the walk up the base class sequence passes the first non-refinement class, and excludes private members - ... except, if we are at a direct parent of a refinement class itself - Makes a corresponding change to OverridingPairs, to only consider private members if they are owned by the `base` Symbol under consideration. We don't need to deal with the subtleties of refinements there as that code is only used for bona-fide classes. - replaces use of `hasTransOwner` when considering whether a private[this] symbol is a member. The last condition was not grounded in the spec at all. The change is visible in cases like: // Old scala> trait A { private[this] val x = 0; class B extends A { this.x } } :7: error: value x in trait A cannot be accessed in A.this.B trait A { private[this] val x = 0; class B extends A { this.x } } ^ // New scala> trait A { private[this] val x = 0; class B extends A { this.x } } :8: error: value x is not a member of A.this.B trait A { private[this] val x = 0; class B extends A { this.x } } ^ Furthermore, we no longer give a `private[this]` member a free pass if it is sourced from the very first base class. trait Cake extends Slice { private[this] val bippy = () } trait Slice { self: Cake => bippy // BCS: Cake, Slice, AnyRef, Any } The different handling between `private` and `private[this]` still seems a bit dubious. The spec says: > An different form of qualification is private[this]. A member M > marked with this modifier can be accessed only from within the > object in which it is defined. That is, a selection p.M is only > legal if the prefix is this or O.this, for some class O enclosing > the reference. In addition, the restrictions for unqualified > private apply. This sounds like a question of access, not membership. If so, we should admit `private[this]` members from parents of refined types in `FindMember`. AFAICT, not too much rests on the distinction: do we get a "no such member", or "member foo inaccessible" error? I welcome scrutinee of the checkfile of `neg/t7475f.scala` to help put this last piece into the puzzle. One more thing: findMember does not have *any* code the corresponds to the last sentence of: > SLS 5.2 The modifier can be qualified with an identifier C > (e.g. private[C]) that must denote a class or package enclosing > the definition. Members labeled with such a modifier are accessible > respectively only from code inside the package C or only from code > inside the class C and its companion module (ยง5.4). > Such members are also inherited only from templates inside C. When I showed Martin this, he suggested it was an error in the spec, and we should leave the access checking to callers of that inherited qualified-private member. --- .../tools/nsc/transform/OverridingPairs.scala | 7 ++- src/reflect/scala/reflect/internal/Types.scala | 12 ----- .../scala/reflect/internal/tpe/FindMembers.scala | 63 +++++++++++++++++----- .../scala/reflect/runtime/JavaUniverseForce.scala | 31 ----------- test/files/neg/forgot-interpolator.check | 5 +- test/files/neg/forgot-interpolator.scala | 2 +- test/files/neg/t7475c.check | 7 +++ test/files/neg/t7475c.scala | 9 ++++ test/files/neg/t7475d.check | 7 +++ test/files/neg/t7475e.check | 4 ++ test/files/neg/t7475e.scala | 12 +++++ test/files/neg/t7475f.check | 10 ++++ test/files/neg/t7475f.scala | 28 ++++++++++ test/files/neg/t7507.check | 2 +- test/files/pos/t7475a.scala | 11 ++++ test/files/pos/t7475b.scala | 8 +++ test/files/pos/t7475d.scala | 11 ++++ test/files/pos/t7475e.scala | 13 +++++ test/files/presentation/scope-completion-3.check | 28 +--------- .../presentation/scope-completion-import.check | 16 ++---- test/files/presentation/visibility.check | 3 +- test/files/run/reflection-sorted-members.check | 1 - test/files/run/t7475b.check | 2 + test/files/run/t7475b.scala | 11 ++++ test/files/run/t7507.scala | 4 ++ 25 files changed, 203 insertions(+), 104 deletions(-) create mode 100644 test/files/neg/t7475c.check create mode 100644 test/files/neg/t7475c.scala create mode 100644 test/files/neg/t7475d.check create mode 100644 test/files/neg/t7475e.check create mode 100644 test/files/neg/t7475e.scala create mode 100644 test/files/neg/t7475f.check create mode 100644 test/files/neg/t7475f.scala create mode 100644 test/files/pos/t7475a.scala create mode 100644 test/files/pos/t7475b.scala create mode 100644 test/files/pos/t7475d.scala create mode 100644 test/files/pos/t7475e.scala create mode 100644 test/files/run/t7475b.check create mode 100644 test/files/run/t7475b.scala (limited to 'src/reflect') diff --git a/src/compiler/scala/tools/nsc/transform/OverridingPairs.scala b/src/compiler/scala/tools/nsc/transform/OverridingPairs.scala index 870eafbf20..bbd11efa7e 100644 --- a/src/compiler/scala/tools/nsc/transform/OverridingPairs.scala +++ b/src/compiler/scala/tools/nsc/transform/OverridingPairs.scala @@ -24,7 +24,12 @@ abstract class OverridingPairs extends SymbolPairs { /** Symbols to exclude: Here these are constructors and private/artifact symbols, * including bridges. But it may be refined in subclasses. */ - override protected def exclude(sym: Symbol) = sym.isPrivateLocal || sym.isArtifact || sym.isConstructor + override protected def exclude(sym: Symbol) = ( + sym.isPrivateLocal + || sym.isArtifact + || sym.isConstructor + || (sym.isPrivate && sym.owner != base) // Privates aren't inherited. Needed for pos/t7475a.scala + ) /** Types always match. Term symbols match if their member types * relative to `self` match. diff --git a/src/reflect/scala/reflect/internal/Types.scala b/src/reflect/scala/reflect/internal/Types.scala index 21ce5114e0..cf405ade03 100644 --- a/src/reflect/scala/reflect/internal/Types.scala +++ b/src/reflect/scala/reflect/internal/Types.scala @@ -244,18 +244,6 @@ trait Types } } - /** Same as a call to narrow unless existentials are visible - * after widening the type. In that case, narrow from the widened - * type instead of the proxy. This gives buried existentials a - * chance to make peace with the other types. See SI-5330. - */ - private[internal] def narrowForFindMember(tp: Type): Type = { - val w = tp.widen - // Only narrow on widened type when we have to -- narrow is expensive unless the target is a singleton type. - if ((tp ne w) && containsExistential(w)) w.narrow - else tp.narrow - } - /** The base class for all types */ abstract class Type extends TypeApiImpl with Annotatable[Type] { /** Types for which asSeenFrom always is the identity, no matter what diff --git a/src/reflect/scala/reflect/internal/tpe/FindMembers.scala b/src/reflect/scala/reflect/internal/tpe/FindMembers.scala index be5a3f0983..33eccd8bbd 100644 --- a/src/reflect/scala/reflect/internal/tpe/FindMembers.scala +++ b/src/reflect/scala/reflect/internal/tpe/FindMembers.scala @@ -17,6 +17,10 @@ trait FindMembers { protected val initBaseClasses: List[Symbol] = tpe.baseClasses // The first base class, or the symbol of the ThisType + // e.g in: + // trait T { self: C => } + // + // The selector class of `T.this.type` is `T`, and *not* the first base class, `C`. private[this] var _selectorClass: Symbol = null private def selectorClass: Symbol = { if (_selectorClass eq null) { @@ -68,6 +72,13 @@ trait FindMembers { // Has we seen a candidate deferred member? var deferredSeen = false + // All direct parents of refinement classes in the base class sequence + // from the current `walkBaseClasses` + var refinementParents: List[Symbol] = Nil + + // Has the current `walkBaseClasses` encountered a non-refinement class? + var seenFirstNonRefinementClass = false + while (!bcs.isEmpty) { val currentBaseClass = bcs.head val decls = currentBaseClass.info.decls @@ -80,7 +91,7 @@ trait FindMembers { if (meetsRequirements) { val excl: Long = flags & excluded val isExcluded: Boolean = excl != 0L - if (!isExcluded && isPotentialMember(sym, flags, currentBaseClass)) { + if (!isExcluded && isPotentialMember(sym, flags, currentBaseClass, seenFirstNonRefinementClass, refinementParents)) { if (shortCircuit(sym)) return false else addMemberIfNew(sym) } else if (excl == DEFERRED) { @@ -90,6 +101,17 @@ trait FindMembers { entry = if (findAll) entry.next else decls lookupNextEntry entry } + // SLS 5.2 The private modifier can be used with any definition or declaration in a template. + // They are not inherited by subclasses [...] + if (currentBaseClass.isRefinementClass) + // SLS 3.2.7 A compound type T1 with . . . with Tn {R } represents objects with members as given in + // the component types T1, ..., Tn and the refinement {R } + // + // => private members should be included from T1, ... Tn. (SI-7475) + refinementParents :::= currentBaseClass.parentSymbols + else if (currentBaseClass.isClass) + seenFirstNonRefinementClass = true // only inherit privates of refinement parents after this point + bcs = bcs.tail } deferredSeen @@ -105,28 +127,31 @@ trait FindMembers { // // Q. When does a potential member fail to be a an actual member? // A. if it is subsumed by an member in a subclass. - private def isPotentialMember(sym: Symbol, flags: Long, owner: Symbol): Boolean = { + private def isPotentialMember(sym: Symbol, flags: Long, owner: Symbol, + seenFirstNonRefinementClass: Boolean, refinementParents: List[Symbol]): Boolean = { // conservatively (performance wise) doing this with flags masks rather than `sym.isPrivate` // to avoid multiple calls to `Symbol#flags`. + val isPrivate = (flags & PRIVATE) == PRIVATE val isPrivateLocal = (flags & PrivateLocal) == PrivateLocal - def admitPrivateLocal(sym: Symbol): Boolean = - ( - (selectorClass == owner) - || !isPrivateLocal - || selectorClass.hasTransOwner(owner) // private[this] only a member from within the class (incorrect!) - ) - - // TODO first condition incorrect wrt self types! In neg/t7507, bippy should not be a member! - // The condition, or a close variant thereof, will still be needed to prevent inheritance of constructors. - owner == initBaseClasses.head || admitPrivateLocal(sym) && sym.name != nme.CONSTRUCTOR + // TODO Is the special handling of `private[this]` vs `private` backed up by the spec? + def admitPrivate(sym: Symbol): Boolean = + (selectorClass == owner) || ( + !isPrivateLocal // private[this] only a member from within the selector class. (Optimization only? Does the spec back this up?) + && ( + !seenFirstNonRefinementClass + || refinementParents.contains(owner) + ) + ) + + (!isPrivate || admitPrivate(sym)) && (sym.name != nme.CONSTRUCTOR || owner == initBaseClasses.head) } // True unless the already-found member of type `memberType` matches the candidate symbol `other`. protected def isNewMember(member: Symbol, other: Symbol): Boolean = ( (other ne member) && ( (member.owner eq other.owner) - || (other.flags & PRIVATE) != 0 + || (member.flags & PRIVATE) != 0 || !(memberTypeLow(member) matches memberTypeHi(other)) ) ) @@ -148,6 +173,18 @@ trait FindMembers { // member type of the LHS of `matches` call. This is an extension point to enable a cache in // FindMember. protected def memberTypeLow(sym: Symbol): Type = self.memberType(sym) + + /** Same as a call to narrow unless existentials are visible + * after widening the type. In that case, narrow from the widened + * type instead of the proxy. This gives buried existentials a + * chance to make peace with the other types. See SI-5330. + */ + private def narrowForFindMember(tp: Type): Type = { + val w = tp.widen + // Only narrow on widened type when we have to -- narrow is expensive unless the target is a singleton type. + if ((tp ne w) && containsExistential(w)) w.narrow + else tp.narrow + } } private[reflect] final class FindMembers(tpe: Type, excludedFlags: Long, requiredFlags: Long) diff --git a/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala b/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala index fb893cbff1..0fcf215580 100644 --- a/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala +++ b/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala @@ -27,27 +27,8 @@ trait JavaUniverseForce { self: runtime.JavaUniverse => this.settings this.treeInfo - // inaccessible: this.scala$reflect$runtime$Gil$$gil - // inaccessible: this.uniqueLock - // inaccessible: this._skolemizationLevel - // inaccessible: this._undoLog - // inaccessible: this._intersectionWitness - // inaccessible: this._subsametypeRecursions - // inaccessible: this._pendingSubTypes - // inaccessible: this._basetypeRecursions - // inaccessible: this._pendingBaseTypes - // inaccessible: this._lubResults - // inaccessible: this._glbResults - // inaccessible: this._indent - // inaccessible: this._toStringRecursions - // inaccessible: this._toStringSubjects - // inaccessible: this.atomicIds - // inaccessible: this.atomicExistentialIds - // inaccessible: this._recursionTable - // inaccessible: this.mirrors this.rootMirror this.treeBuild - // inaccessible: this.SimpleNameOrdering this.traceSymbols this.perRunCaches this.FreshNameExtractor @@ -60,7 +41,6 @@ trait JavaUniverseForce { self: runtime.JavaUniverse => this.SubpatternsAttachment this.noPrint this.typeDebug - // inaccessible: this.maxFree this.Range // inaccessible: this.posAssigner this.ConsoleWriter @@ -116,7 +96,6 @@ trait JavaUniverseForce { self: runtime.JavaUniverse => this.pendingSuperCall this.emptyValDef this.EmptyTreeTypeSubstituter - // inaccessible: this.duplicator this.UnmappableAnnotArg this.LiteralAnnotArg this.ArrayAnnotArg @@ -127,7 +106,6 @@ trait JavaUniverseForce { self: runtime.JavaUniverse => this.UnmappableAnnotation this.ErroneousAnnotation this.ThrownException - // inaccessible: this.compactify this.tpnme this.fulltpnme this.binarynme @@ -147,7 +125,6 @@ trait JavaUniverseForce { self: runtime.JavaUniverse => this.ProperTypeKind this.TypeConKind this.inferKind - // inaccessible: this.substTypeMapCache this.UnmappableTree this.ErrorType this.WildcardType @@ -184,9 +161,6 @@ trait JavaUniverseForce { self: runtime.JavaUniverse => this.unwrapToStableClass this.unwrapWrapperTypes this.RecoverableCyclicReference - // inaccessible: this._undoLog - // inaccessible: this.numericLoBound - // inaccessible: this.numericHiBound this.TypeConstraint this.normalizeAliases this.dropSingletonType @@ -198,19 +172,16 @@ trait JavaUniverseForce { self: runtime.JavaUniverse => this.typeVarToOriginMap this.ErroneousCollector this.adaptToNewRunMap - // inaccessible: this.commonOwnerMapObj this.SubTypePair this.SymbolKind this.NoSymbol this.CyclicReference - // inaccessible: this.TypeHistory this.SymbolOps this.TermName this.TypeName this.Liftable this.Unliftable this.BooleanFlag - // inaccessible: this.CachedNames this.WeakTypeTag this.TypeTag this.Expr @@ -427,14 +398,12 @@ trait JavaUniverseForce { self: runtime.JavaUniverse => definitions.languageFeatureModule definitions.metaAnnotations definitions.AnnotationDefaultAttr - // inaccessible: definitions.erasurePhase definitions.isPhantomClass definitions.syntheticCoreClasses definitions.syntheticCoreMethods definitions.hijackedCoreClasses definitions.symbolsNotPresentInBytecode definitions.isPossibleSyntheticParent - // inaccessible: definitions.boxedValueClassesSet definitions.abbrvTag definitions.numericWeight definitions.boxedModule diff --git a/test/files/neg/forgot-interpolator.check b/test/files/neg/forgot-interpolator.check index 157cbb4802..8988458982 100644 --- a/test/files/neg/forgot-interpolator.check +++ b/test/files/neg/forgot-interpolator.check @@ -10,9 +10,6 @@ forgot-interpolator.scala:30: warning: `$beppo` looks like an interpolated ident forgot-interpolator.scala:34: warning: `$aleppo` looks like an interpolated identifier! Did you forget the interpolator? def f = "$aleppo is a pepper and a city." // warn 4 ^ -forgot-interpolator.scala:42: warning: `$bar` looks like an interpolated identifier! Did you forget the interpolator? - def f = "$bar is private, shall we warn just in case?" // warn 5 - ^ forgot-interpolator.scala:47: warning: `$hippo` looks like an interpolated identifier! Did you forget the interpolator? def h = "$hippo takes an implicit" // warn 6 ^ @@ -26,5 +23,5 @@ forgot-interpolator.scala:90: warning: `$calico` looks like an interpolated iden def f4 = "I also salute $calico" // warn 9 ^ error: No warnings can be incurred under -Xfatal-warnings. -9 warnings found +8 warnings found one error found diff --git a/test/files/neg/forgot-interpolator.scala b/test/files/neg/forgot-interpolator.scala index 34a7c7aef4..a53054d890 100644 --- a/test/files/neg/forgot-interpolator.scala +++ b/test/files/neg/forgot-interpolator.scala @@ -39,7 +39,7 @@ package test { if (bar > 8) ??? // use it to avoid extra warning } class Baz extends Bar { - def f = "$bar is private, shall we warn just in case?" // warn 5 + def f = "$bar is private, shall we warn just in case?" // no longer a warning, private members aren't inherited! } class G { def g = "$greppo takes an arg" // no warn diff --git a/test/files/neg/t7475c.check b/test/files/neg/t7475c.check new file mode 100644 index 0000000000..472808131a --- /dev/null +++ b/test/files/neg/t7475c.check @@ -0,0 +1,7 @@ +t7475c.scala:6: error: value a is not a member of A.this.B + println(this.a) // wait, what? + ^ +t7475c.scala:7: error: value b is not a member of A.this.B + println(this.b) // wait, what? + ^ +two errors found diff --git a/test/files/neg/t7475c.scala b/test/files/neg/t7475c.scala new file mode 100644 index 0000000000..cd4a8762ca --- /dev/null +++ b/test/files/neg/t7475c.scala @@ -0,0 +1,9 @@ +class A { + private val a: Int = 0 + private[this] val b: Int = 0 + class B extends A { + def foo(a: A) = a.a // okay + println(this.a) // wait, what? + println(this.b) // wait, what? + } +} diff --git a/test/files/neg/t7475d.check b/test/files/neg/t7475d.check new file mode 100644 index 0000000000..6bd1da0d44 --- /dev/null +++ b/test/files/neg/t7475d.check @@ -0,0 +1,7 @@ +t7475d.scala:4: error: value priv is not a member of T.this.TT + (??? : TT).priv + ^ +t7475d.scala:10: error: value priv is not a member of U.this.UU + (??? : UU).priv + ^ +two errors found diff --git a/test/files/neg/t7475e.check b/test/files/neg/t7475e.check new file mode 100644 index 0000000000..48af2be51a --- /dev/null +++ b/test/files/neg/t7475e.check @@ -0,0 +1,4 @@ +t7475e.scala:8: error: value priv is not a member of Base.this.TT + (??? : TT).priv + ^ +one error found diff --git a/test/files/neg/t7475e.scala b/test/files/neg/t7475e.scala new file mode 100644 index 0000000000..e5c4877d6e --- /dev/null +++ b/test/files/neg/t7475e.scala @@ -0,0 +1,12 @@ +trait U { +} + +trait Base { + private val priv = 0 + + type TT = U with T // should exclude `priv` + (??? : TT).priv +} + +trait T extends Base { +} diff --git a/test/files/neg/t7475f.check b/test/files/neg/t7475f.check new file mode 100644 index 0000000000..a07a4480e2 --- /dev/null +++ b/test/files/neg/t7475f.check @@ -0,0 +1,10 @@ +t7475f.scala:12: error: method c1 in class C cannot be accessed in C[T] + c1 // a member, but inaccessible. + ^ +t7475f.scala:13: error: not found: value c2 + c2 // a member, but inaccessible. + ^ +t7475f.scala:26: error: value d2 is not a member of D[Any] + other.d2 // not a member + ^ +three errors found diff --git a/test/files/neg/t7475f.scala b/test/files/neg/t7475f.scala new file mode 100644 index 0000000000..6c5feadf19 --- /dev/null +++ b/test/files/neg/t7475f.scala @@ -0,0 +1,28 @@ +class C[T] extends D[T] { + private def c1 = 0 + private[this] def c2 = 0 +} + +trait D[T] { + self: C[T] => + + private def d1 = 0 + private[this] def d2 = 0 + + c1 // a member, but inaccessible. + c2 // a member, but inaccessible. + + d1 // okay + d2 // okay + + + class C { + d1 + d2 + } + + def x(other: D[Any]) { + other.d1 + other.d2 // not a member + } +} diff --git a/test/files/neg/t7507.check b/test/files/neg/t7507.check index d402869fd4..de30fc7057 100644 --- a/test/files/neg/t7507.check +++ b/test/files/neg/t7507.check @@ -1,4 +1,4 @@ -t7507.scala:6: error: value bippy in trait Cake cannot be accessed in Cake +t7507.scala:6: error: not found: value bippy locally(bippy) ^ one error found diff --git a/test/files/pos/t7475a.scala b/test/files/pos/t7475a.scala new file mode 100644 index 0000000000..810ce9a05c --- /dev/null +++ b/test/files/pos/t7475a.scala @@ -0,0 +1,11 @@ +trait AbstractPublic { + def queue: Any +} +trait ConcretePrivate { + private val queue: Any = () +} + +abstract class Mix + extends ConcretePrivate with AbstractPublic { + final def queue: Any = () +} diff --git a/test/files/pos/t7475b.scala b/test/files/pos/t7475b.scala new file mode 100644 index 0000000000..a34743b8be --- /dev/null +++ b/test/files/pos/t7475b.scala @@ -0,0 +1,8 @@ +trait U { +} + +trait T { + type TT = Any with T with U + private val priv = 0 + (??? : TT).priv +} diff --git a/test/files/pos/t7475d.scala b/test/files/pos/t7475d.scala new file mode 100644 index 0000000000..497c2bf443 --- /dev/null +++ b/test/files/pos/t7475d.scala @@ -0,0 +1,11 @@ +trait T { + type TT = T with Any + private val priv = 0 + (??? : TT).priv +} + +trait U { + type UU = Any with U + private val priv = 0 + (??? : UU).priv +} diff --git a/test/files/pos/t7475e.scala b/test/files/pos/t7475e.scala new file mode 100644 index 0000000000..fbc965c4ca --- /dev/null +++ b/test/files/pos/t7475e.scala @@ -0,0 +1,13 @@ +trait U { + private val priv = 0 + type TT = U with T // should allow `priv` + (??? : TT).priv +} + +trait Base { + +} + +trait T extends Base { + +} diff --git a/test/files/presentation/scope-completion-3.check b/test/files/presentation/scope-completion-3.check index df3007ab4e..b70a7d5c6b 100644 --- a/test/files/presentation/scope-completion-3.check +++ b/test/files/presentation/scope-completion-3.check @@ -3,19 +3,7 @@ reload: Completions.scala askScopeCompletion at Completions.scala(75,2) ================================================================================ [response] askScopeCompletion at (75,2) -retrieved 49 members -[inaccessible] private class Cb2 extends AnyRef -[inaccessible] private class Ct2 extends AnyRef -[inaccessible] private def fb2: Int -[inaccessible] private def ft2: Int -[inaccessible] private object Ob2 -[inaccessible] private object Ot2 -[inaccessible] private type tb2 = Completion1.this.tb2 -[inaccessible] private type tt2 = Completion1.this.tt2 -[inaccessible] private[this] val vb2: Int -[inaccessible] private[this] val vt2: Int -[inaccessible] private[this] var rb2: Int -[inaccessible] private[this] var rt2: Int +retrieved 37 members abstract class Base1 extends AnyRef abstract trait Trait1 extends AnyRef class Cb1 extends AnyRef @@ -58,19 +46,7 @@ type tt1 = Completion1.this.tt1 askScopeCompletion at Completions.scala(104,2) ================================================================================ [response] askScopeCompletion at (104,2) -retrieved 49 members -[inaccessible] private class Cb2 extends AnyRef -[inaccessible] private class Ct2 extends AnyRef -[inaccessible] private def fb2: Int -[inaccessible] private def ft2: Int -[inaccessible] private object Ob2 -[inaccessible] private object Ot2 -[inaccessible] private type tb2 = test.Completion2.tb2 -[inaccessible] private type tt2 = test.Completion2.tt2 -[inaccessible] private[this] val vb2: Int -[inaccessible] private[this] val vt2: Int -[inaccessible] private[this] var rb2: Int -[inaccessible] private[this] var rt2: Int +retrieved 37 members abstract class Base1 extends AnyRef abstract trait Trait1 extends AnyRef class Cb1 extends AnyRef diff --git a/test/files/presentation/scope-completion-import.check b/test/files/presentation/scope-completion-import.check index 220ffc399b..50197e5822 100644 --- a/test/files/presentation/scope-completion-import.check +++ b/test/files/presentation/scope-completion-import.check @@ -3,10 +3,8 @@ reload: Completions.scala askScopeCompletion at Completions.scala(23,4) ================================================================================ [response] askScopeCompletion at (23,4) -retrieved 18 members -[inaccessible] private[this] val pVCCC: Int +retrieved 16 members [inaccessible] private[this] val pVOOO: Int -[inaccessible] private[this] var pRCCC: Int [inaccessible] private[this] var pROOO: Int class C extends AnyRef class Foo extends AnyRef @@ -27,10 +25,8 @@ val o: test.O.type askScopeCompletion at Completions.scala(27,4) ================================================================================ [response] askScopeCompletion at (27,4) -retrieved 17 members -[inaccessible] private[this] val pVCCC: Int +retrieved 15 members [inaccessible] private[this] val pVOOO: Int -[inaccessible] private[this] var pRCCC: Int [inaccessible] private[this] var pROOO: Int class C extends AnyRef class Foo extends AnyRef @@ -126,10 +122,8 @@ val c: test.C askScopeCompletion at Completions.scala(49,4) ================================================================================ [response] askScopeCompletion at (49,4) -retrieved 18 members -[inaccessible] private[this] val pVCCC: Int +retrieved 16 members [inaccessible] private[this] val pVOOO: Int -[inaccessible] private[this] var pRCCC: Int [inaccessible] private[this] var pROOO: Int class C extends AnyRef class Foo extends AnyRef @@ -150,10 +144,8 @@ private[this] var rOOO: Int askScopeCompletion at Completions.scala(59,4) ================================================================================ [response] askScopeCompletion at (59,4) -retrieved 19 members -[inaccessible] private[this] val pVCCC: Int +retrieved 17 members [inaccessible] private[this] val pVOOO: Int -[inaccessible] private[this] var pRCCC: Int [inaccessible] private[this] var pROOO: Int class C extends AnyRef class Foo extends AnyRef diff --git a/test/files/presentation/visibility.check b/test/files/presentation/visibility.check index 1ae1213f2d..b77887f8f7 100644 --- a/test/files/presentation/visibility.check +++ b/test/files/presentation/visibility.check @@ -79,8 +79,7 @@ protected[package lang] def finalize(): Unit askTypeCompletion at Completions.scala(22,11) ================================================================================ [response] askTypeCompletion at (22,11) -retrieved 35 members -[inaccessible] private def secretPrivate(): Unit +retrieved 34 members def +(other: String): String def ->[B](y: B): (accessibility.AccessibilityChecks, B) def ensuring(cond: Boolean): accessibility.AccessibilityChecks diff --git a/test/files/run/reflection-sorted-members.check b/test/files/run/reflection-sorted-members.check index c148e19e69..415e073149 100644 --- a/test/files/run/reflection-sorted-members.check +++ b/test/files/run/reflection-sorted-members.check @@ -1,4 +1,3 @@ value a value b value c -value x diff --git a/test/files/run/t7475b.check b/test/files/run/t7475b.check new file mode 100644 index 0000000000..51993f072d --- /dev/null +++ b/test/files/run/t7475b.check @@ -0,0 +1,2 @@ +2 +2 diff --git a/test/files/run/t7475b.scala b/test/files/run/t7475b.scala new file mode 100644 index 0000000000..a205602b6d --- /dev/null +++ b/test/files/run/t7475b.scala @@ -0,0 +1,11 @@ +trait A { private val x = 1 } +trait B { val x = 2 } +trait C1 extends B with A { println(x) } +trait C2 extends A with B { println(x) } + +object Test { + def main(args: Array[String]): Unit = { + new C1 { } + new C2 { } + } +} diff --git a/test/files/run/t7507.scala b/test/files/run/t7507.scala index 6c1959ddac..a5eab6248f 100644 --- a/test/files/run/t7507.scala +++ b/test/files/run/t7507.scala @@ -4,6 +4,10 @@ trait Cake extends Slice trait Slice { self: Cake => // must have self type that extends `Slice` private[this] val bippy = () // must be private[this] locally(bippy) + class C1 { + locally(bippy) + locally(self.bippy) + } } // Originally reported bug: -- cgit v1.2.3 From 47885ff665a951d2574a7ca5b2ea27c30b67c862 Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Wed, 5 Feb 2014 11:42:02 +0100 Subject: SI-7475 Address review comments in FindMembers - comment the cases in `isNewMember` - check both sides for privacy (doesn't influence behaviour of any tests, but it means that method can be understood without understanding the order of traversal of members.) - hoist `findAll` - break out of the loop in `FindMembers` as soon as we determine that the candidate member is matched by an already-found member and can be discarded. --- .../scala/reflect/internal/tpe/FindMembers.scala | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) (limited to 'src/reflect') diff --git a/src/reflect/scala/reflect/internal/tpe/FindMembers.scala b/src/reflect/scala/reflect/internal/tpe/FindMembers.scala index 33eccd8bbd..de54f3768e 100644 --- a/src/reflect/scala/reflect/internal/tpe/FindMembers.scala +++ b/src/reflect/scala/reflect/internal/tpe/FindMembers.scala @@ -64,12 +64,13 @@ trait FindMembers { * * Called in two passes: first excluding deferred, then mandating it. * - * @return true, if a potential deferred member was seen on the first pass that calls for a second pass. + * @return if a potential deferred member was seen on the first pass that calls for a second pass, + and `excluded & DEFERRED != 0L` */ private def walkBaseClasses(required: Long, excluded: Long): Boolean = { var bcs = initBaseClasses - // Has we seen a candidate deferred member? + // Have we seen a candidate deferred member? var deferredSeen = false // All direct parents of refinement classes in the base class sequence @@ -79,10 +80,11 @@ trait FindMembers { // Has the current `walkBaseClasses` encountered a non-refinement class? var seenFirstNonRefinementClass = false + val findAll = name == nme.ANYname + while (!bcs.isEmpty) { val currentBaseClass = bcs.head val decls = currentBaseClass.info.decls - val findAll = name == nme.ANYname var entry = if (findAll) decls.elems else decls.lookupEntry(name) while (entry ne null) { val sym = entry.sym @@ -150,9 +152,10 @@ trait FindMembers { // True unless the already-found member of type `memberType` matches the candidate symbol `other`. protected def isNewMember(member: Symbol, other: Symbol): Boolean = ( (other ne member) - && ( (member.owner eq other.owner) - || (member.flags & PRIVATE) != 0 - || !(memberTypeLow(member) matches memberTypeHi(other)) + && ( (member.owner eq other.owner) // same owner, therefore overload + || (member.flags & PRIVATE) != 0 // (unqualified) private members never participate in overriding + || (other.flags & PRIVATE) != 0 // ... as overrider or overridee. + || !(memberTypeLow(member) matches memberTypeHi(other)) // do the member types match? If so, its an override. Otherwise it's an overload. ) ) @@ -202,7 +205,7 @@ trait FindMembers { val members = membersScope var others = members.lookupEntry(sym.name) var isNew = true - while (others ne null) { + while ((others ne null) && isNew) { val member = others.sym if (!isNewMember(member, sym)) isNew = false -- cgit v1.2.3