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

Under -Xsource:3, warn that inherited members no longer take precedence over outer definitions in Scala 3 #10220

Merged
merged 7 commits into from Dec 16, 2022
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 9 additions & 7 deletions spec/02-identifiers-names-and-scopes.md
Expand Up @@ -7,7 +7,7 @@ chapter: 2
# Identifiers, Names and Scopes

Names in Scala identify types, values, methods, and classes which are
collectively called _entities_. Names are introduced by local
collectively called _entities_. Names are introduced by
[definitions and declarations](04-basic-declarations-and-definitions.html#basic-declarations-and-definitions),
[inheritance](05-classes-and-objects.html#class-members),
[import clauses](04-basic-declarations-and-definitions.html#import-clauses), or
Expand All @@ -17,14 +17,16 @@ which are collectively called _bindings_.
Bindings of each kind are assigned a precedence which determines
whether one binding can shadow another:

1. Definitions and declarations that are local, inherited, or made
available by a package clause and also defined in the same compilation unit
as the reference to them, have the highest precedence.
1. Definitions and declarations in lexical scope that are not [top-level](09-top-level-definitions.html)
Copy link
Member Author

@lrytz lrytz Feb 17, 2023

Choose a reason for hiding this comment

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

@som-snytt I assume top-level is excluded here because that's what's implemented in Scala 3 (scala/scala3#8622). In Scala 3, the following compiles and new X resolves to C.this.X

package p {
  trait T { class X }
  class C extends T {
    def test = new X
  }
  class X
}

Changing package p to object p makes new X ambiguous. Do you see a good reason for that? cc @odersky

Copy link
Contributor

Choose a reason for hiding this comment

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

If T has a member def name: String = "John Doe", then just where it can be inherited, introducing a different definition of name might be in conflict.

Packagings are not classes and don't suffer from extension, so I should be able to write top-level def name = "Gloria Mundi" without concern about confusing anyone.

But this is ambiguous:

    trait T {
      class X
      def name: String = "John Doe"
    }
    class C extends T {
      def test = new X
      def result = name
    }
    class X
    //def name = "Gloria Mundi"
    val name: T = new T {
      def result = name
    }

I expected it to prefer the inherited name and not the enclosing definition which is a top-level def.

15 |      def result = name
   |                   ^^^^
   |                   Reference to name is ambiguous,
   |                   it is both defined in package object t10220$package
   |                   and inherited subsequently in anonymous class Object with p.T {...}

I don't think they actually got rid of package objects...

Copy link
Member Author

Choose a reason for hiding this comment

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

So you expect an inherited symbol takes precedence over an outer definition in the same source file, if that outer definition is package-owned? Can you say why? I don't yet see what difference it makes if the outer symbol is top-level or not.

I should be able to write top-level def name = "Gloria Mundi" without concern about confusing anyone

What is the difference in that regard between package p { def name = "..." } and object p { def name = "..."}?

Copy link
Member Author

Choose a reason for hiding this comment

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

For the record, I'm on this topic because I'm testing #10297, and I see warnings that didn't show up in 2.13. These new warnings seem helpful to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the proof is in the pudding, or in golf it's the putting.

I have only a rationalization: in package p, I know nothing is inherited in this scope (because we do not package object p extends T). Once someone defines a class, the folks working in that mine must manage that namespace, where stuff is inherited.

I agree there is regularity in seeing object p as just a way to "associate" some definitions, and I think they speak of "package membership", but if the regularity were borne out, then we would love package object p extends T. You would be asking, Why is that different from object p extends T?

But all that is just a rationalization.

What are some examples of pernicious shadowing of top-level defs? I can't wait to see.

Copy link
Member Author

Choose a reason for hiding this comment

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

An example was:

package p

class Code extends Parent {
  def impl = s"The $CommonName"
}

case class CommonName(some: Thing)

A second file:

package p
trait Parent {
  val CommonName = "something"
}

It's maybe clear here that we're not intending to refer to the synthetic case class companion, but this could still be a head scratcher for someone looking at the first file. One could for example do a search in the editor for CommonName and land on the case class.

In this case I moved CommonName to a separate file instead of changing the reference.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aha, that does seem lintable, at least with CommonName in the same file. Conversely, it loses precedence when moved to a different file, but the reference in Parent does not.

I just realized this feature is not released until 2.13.11. Let me take a moment to clear my fever-addled brain.

I notice toString is not exempt.

Also I was about to wonder if the use case is really anon classes, esp SAM types. The user expectation is more obviously, "I am focused on implementing that one method, and probably unaware of inheriting anything."

Copy link
Member Author

@lrytz lrytz Feb 21, 2023

Choose a reason for hiding this comment

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

CommonName in the same file

Yes, I mean to warn only if the outer definition is in the same compilation unit.

I notice toString is not exempt.
if the use case is really anon classes

Those are very good points. I certainly saw it pop up a lot with anonymous classes, and one would expect toString to resolve in the current object. Hmm. Since this is a spec change and not an impl detail (a lint rule), we need to have general and hopefully simple rules. An exception for toString wouldn't work...

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can wait to see if toString really is problematic. So far (Scala 3, my tests of the new Scala 2 warning) it didn't seem so.

have the highest precedence.
1. Definitions and declarations that are either inherited,
or made available by a package clause and also defined in the same compilation unit as the reference to them,
have the next highest precedence.
1. Explicit imports have the next highest precedence.
1. Wildcard imports have the next highest precedence.
1. Bindings made available by a package clause, but not also defined in the
same compilation unit as the reference to them, as well as bindings
supplied by the compiler but not explicitly written in source code,
1. Bindings made available by a package clause,
but not also defined in the same compilation unit as the reference to them,
as well as bindings supplied by the compiler but not explicitly written in source code,
have the lowest precedence.

There are two different name spaces, one for [types](03-types.html#types)
Expand Down
4 changes: 2 additions & 2 deletions src/compiler/scala/tools/nsc/ast/parser/Parsers.scala
Expand Up @@ -2098,7 +2098,7 @@ self =>
if (in.token == SUBTYPE || in.token == SUPERTYPE) wildcardType(start, scala3Wildcard)
else atPos(start) { Bind(tpnme.WILDCARD, EmptyTree) }
} else {
typ() match {
this.typ() match {
case Ident(name: TypeName) if nme.isVariableName(name) =>
atPos(start) { Bind(name, EmptyTree) }
case t => t
Expand Down Expand Up @@ -2289,7 +2289,7 @@ self =>
}
/** The implementation of the context sensitive methods for parsing outside of patterns. */
final val outPattern = new PatternContextSensitive {
def argType(): Tree = typ()
def argType(): Tree = this.typ()
def functionArgType(): Tree = paramType(repeatedParameterOK = false, useStartAsPosition = true)
}
/** The implementation for parsing inside of patterns at points where sequences are allowed. */
Expand Down
107 changes: 77 additions & 30 deletions src/compiler/scala/tools/nsc/typechecker/Contexts.scala
Expand Up @@ -51,13 +51,6 @@ trait Contexts { self: Analyzer =>
}
private lazy val NoJavaMemberFound = (NoType, NoSymbol)

def ambiguousImports(imp1: ImportInfo, imp2: ImportInfo) =
LookupAmbiguous(s"it is imported twice in the same scope by\n$imp1\nand $imp2")
def ambiguousDefnAndImport(owner: Symbol, imp: ImportInfo) =
LookupAmbiguous(s"it is both defined in $owner and imported subsequently by \n$imp")
def ambiguousDefinitions(owner: Symbol, other: Symbol) =
LookupAmbiguous(s"it is both defined in $owner and available as ${other.fullLocationString}")

private lazy val startContext = NoContext.make(
Template(List(), noSelfType, List()) setSymbol global.NoSymbol setType global.NoType,
rootMirror.RootClass,
Expand Down Expand Up @@ -1362,6 +1355,35 @@ trait Contexts { self: Analyzer =>
private[this] var pre: Type = _ // the prefix type of defSym, if a class member
private[this] var cx: Context = _ // the context under consideration
private[this] var symbolDepth: Int = _ // the depth of the directly found symbol
private[this] var foundInPrefix: Boolean = _ // the symbol was found in pre
private[this] var foundInSuper: Boolean = _ // the symbol was found super of context class (inherited)

def ambiguousImports(imp1: ImportInfo, imp2: ImportInfo) =
LookupAmbiguous(s"it is imported twice in the same scope by\n$imp1\nand $imp2")
def ambiguousDefnAndImport(owner: Symbol, imp: ImportInfo) =
LookupAmbiguous(s"it is both defined in $owner and imported subsequently by \n$imp")
def ambiguousDefinitions(sym: Symbol, other: Symbol, otherFoundInSuper: Boolean, otherEnclClass: Symbol) = {
if (otherFoundInSuper) {
val enclDesc = if (otherEnclClass.isAnonymousClass) "anonymous class" else otherEnclClass.toString
val parent = otherEnclClass.parentSymbols.find(_.isNonBottomSubClass(other.owner)).getOrElse(NoSymbol)
val inherit = if (parent.exists && parent != other.owner) s", inherited through parent $parent" else ""
val message =
s"""it is both defined in the enclosing ${sym.owner} and available in the enclosing $enclDesc as $other (defined in ${other.ownsString}$inherit)
|Since 3, symbols inherited from a superclass no longer shadow symbols defined in an outer scope.
|To continue using the symbol from the superclass, write `this.${sym.name}`.""".stripMargin
if (currentRun.isScala3)
Some(LookupAmbiguous(message))
else {
// passing the message to `typedIdent` as attachment, we don't have the position here to report the warning
other.updateAttachment(LookupAmbiguityWarning(
s"""reference to ${sym.name} is ambiguous;
|$message
|Or use `-Wconf:msg=legacy-binding:s` to silence this warning.""".stripMargin))
None
}
} else
Some(LookupAmbiguous(s"it is both defined in ${sym.owner} and available as ${other.fullLocationString}"))
}

def apply(thisContext: Context, name: Name)(qualifies: Symbol => Boolean): NameLookup = {
lookupError = null
Expand All @@ -1370,6 +1392,8 @@ trait Contexts { self: Analyzer =>
pre = NoPrefix
cx = thisContext
symbolDepth = -1
foundInPrefix = false
foundInSuper = false

def finish(qual: Tree, sym: Symbol): NameLookup = (
if (lookupError ne null) lookupError
Expand All @@ -1386,26 +1410,25 @@ trait Contexts { self: Analyzer =>
finish(qual, sym)
}

def lookupInPrefix(name: Name): Symbol = {
if (thisContext.unit.isJava) {
def lookupInPrefix(name: Name): Symbol =
if (thisContext.unit.isJava)
thisContext.javaFindMember(pre, name, qualifies) match {
case (_, NoSymbol) =>
NoSymbol
case (pre1, sym) =>
pre = pre1
sym
}
} else {
else
pre.member(name).filter(qualifies)
}
}

def accessibleInPrefix(s: Symbol) =
thisContext.isAccessible(s, pre, superAccess = false)

def searchPrefix = {
cx = cx.enclClass
val found0 = lookupInPrefix(name)
val found1 = found0 filter accessibleInPrefix
val found1 = found0.filter(accessibleInPrefix)
if (found0.exists && !found1.exists && inaccessible == null)
inaccessible = LookupInaccessible(found0, analyzer.lastAccessCheckDetails)

Expand Down Expand Up @@ -1452,16 +1475,26 @@ trait Contexts { self: Analyzer =>
}

// cx.scope eq null arises during FixInvalidSyms in Duplicators
def nextDefinition(): Unit =
def nextDefinition(lastDef: Symbol, lastPre: Type): Unit = {
var inPrefix = false
defSym = NoSymbol
while (defSym == NoSymbol && (cx ne NoContext) && (cx.scope ne null)) {
pre = cx.enclClass.prefix
defSym = lookupInScope(cx.owner, cx.enclClass.prefix, cx.scope) match {
case NoSymbol => searchPrefix
case found => found
defSym = lookupInScope(cx.owner, pre, cx.scope) match {
case NoSymbol => inPrefix = true; searchPrefix
case found => inPrefix = false; found
}
if (!defSym.exists) cx = cx.outer // push further outward
}
nextDefinition()
if ((defSym.isAliasType || lastDef.isAliasType) && pre.memberType(defSym) =:= lastPre.memberType(lastDef))
defSym = NoSymbol
if (defSym.isStable && lastDef.isStable &&
(lastPre.memberType(lastDef).termSymbol == defSym || pre.memberType(defSym).termSymbol == lastDef))
defSym = NoSymbol
foundInPrefix = inPrefix && defSym.exists
foundInSuper = foundInPrefix && defSym.owner != cx.owner
}
nextDefinition(NoSymbol, NoPrefix)

if (symbolDepth < 0)
symbolDepth = cx.depth
Expand Down Expand Up @@ -1490,12 +1523,17 @@ trait Contexts { self: Analyzer =>
*
* Scala: Bindings of different kinds have a defined precedence order:
*
* 1) Definitions and declarations that are local, inherited, or made available by
* a package clause and also defined in the same compilation unit as the reference, have highest precedence.
* 1) Definitions and declarations in lexical scope that are not top-level have the highest precedence.
* 1b) Definitions and declarations that are either inherited, or made
* available by a package clause and also defined in the same compilation unit
* as the reference to them, have the next highest precedence.
* (Only in -Xsource:3, same precedence as 1 with a warning in Scala 2.)
* 2) Explicit imports have next highest precedence.
* 3) Wildcard imports have next highest precedence.
* 4) Definitions made available by a package clause, but not also defined in the same compilation unit
* as the reference, have lowest precedence. Also "root" imports added implicitly.
* 4) Bindings made available by a package clause,
* but not also defined in the same compilation unit as the reference to them,
* as well as bindings supplied by the compiler but not explicitly written in source code,
* have the lowest precedence.
*/
def foreignDefined = defSym.exists && thisContext.isPackageOwnedInDifferentUnit(defSym) // SI-2458

Expand Down Expand Up @@ -1552,24 +1590,33 @@ trait Contexts { self: Analyzer =>
return ambiguousDefnAndImport(defSym.alternatives.head.owner, imp1)
})

// If the defSym is at 4, and there is a def at 1 in scope due to packaging, then the reference is ambiguous.
if (foreignDefined && !defSym.hasPackageFlag && !thisContext.unit.isJava) {
// If the defSym is at 4, and there is a def at 1b in scope due to packaging, then the reference is ambiguous.
// Also if defSym is at 1b inherited, the reference can be rendered ambiguous by a def at 1a in scope.
val possiblyAmbiguousDefinition =
foundInSuper && cx.owner.isClass ||
foreignDefined && !defSym.hasPackageFlag
if (possiblyAmbiguousDefinition && !thisContext.unit.isJava) {
val defSym0 = defSym
val pre0 = pre
val cx0 = cx
val wasFoundInSuper = foundInSuper
val foundCompetingSymbol: () => Boolean =
if (foreignDefined) () => !foreignDefined
else () => !defSym.isTopLevel && !defSym.owner.isPackageObjectOrClass && !foundInSuper && !foreignDefined
while ((cx ne NoContext) && cx.depth >= symbolDepth) cx = cx.outer
if (wasFoundInSuper)
while ((cx ne NoContext) && (cx.owner eq cx0.owner)) cx = cx.outer
var done = false
while (!done) {
defSym = NoSymbol
nextDefinition()
done = (cx eq NoContext) || defSym.exists && !foreignDefined
nextDefinition(defSym0, pre0)
done = (cx eq NoContext) || defSym.exists && foundCompetingSymbol()
if (!done && (cx ne NoContext)) cx = cx.outer
}
if (defSym.exists && (defSym ne defSym0)) {
val ambiguity =
if (preferDef) ambiguousDefinitions(owner = defSym.owner, defSym0)
else ambiguousDefnAndImport(owner = defSym.owner, imp1)
return ambiguity
if (preferDef) ambiguousDefinitions(defSym, defSym0, wasFoundInSuper, cx0.enclClass.owner)
else Some(ambiguousDefnAndImport(owner = defSym.owner, imp1))
if (ambiguity.nonEmpty) return ambiguity.get
}
defSym = defSym0
pre = pre0
Expand Down
Expand Up @@ -386,7 +386,7 @@ trait MacroAnnotationNamers { self: Analyzer =>
sym.setInfo(new MaybeExpandeeCompleter(tree) {
override def kind = s"maybeExpandeeCompleter for ${sym.accurateKindString} ${sym.rawname}#${sym.id}"
override def maybeExpand(): Unit = {
val companion = if (tree.isInstanceOf[ClassDef]) patchedCompanionSymbolOf(sym, context) else NoSymbol
val companion = if (this.tree.isInstanceOf[ClassDef]) patchedCompanionSymbolOf(sym, context) else NoSymbol

def maybeExpand(annotation: Tree, annottee: Tree, maybeExpandee: Tree): Option[List[Tree]] =
if (context.macrosEnabled) { // TODO: when is this bit flipped -- can we pull this check out farther?
Expand All @@ -395,7 +395,7 @@ trait MacroAnnotationNamers { self: Analyzer =>
if (mann.isClass && mann.hasFlag(MACRO)) {
assert(!currentRun.compiles(mann), mann)
val annm = prepareAnnotationMacro(annotation, mann, sym, annottee, maybeExpandee)
expandAnnotationMacro(tree, annm)
expandAnnotationMacro(this.tree, annm)
// if we encounter an error, we just return None, so that other macro annotations can proceed
// this is unlike macroExpand1 when any error in an expandee blocks expansions
// there it's necessary in order not to exacerbate typer errors
Expand Down Expand Up @@ -424,7 +424,7 @@ trait MacroAnnotationNamers { self: Analyzer =>
enterSyms(expanded) // TODO: we can't reliably expand into imports, because they won't be accounted by definitions below us
case None =>
markNotExpandable(sym)
finishSymbolNotExpandee(tree)
finishSymbolNotExpandee(this.tree)
}

// take care of the companion if it's no longer needed
Expand Down
2 changes: 2 additions & 0 deletions src/compiler/scala/tools/nsc/typechecker/Typers.scala
Expand Up @@ -5501,6 +5501,8 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
case sym => typed1(tree setSymbol sym, mode, pt)
}
case LookupSucceeded(qual, sym) =>
sym.getAndRemoveAttachment[LookupAmbiguityWarning].foreach(w =>
runReporting.warning(tree.pos, w.msg, WarningCategory.Other, context.owner))
(// this -> Foo.this
if (sym.isThisSym)
typed1(This(sym.owner) setPos tree.pos, mode, pt)
Expand Down
2 changes: 2 additions & 0 deletions src/reflect/scala/reflect/internal/StdAttachments.scala
Expand Up @@ -152,4 +152,6 @@ trait StdAttachments {

/** For `val i = 42`, marks field as inferred so accessor (getter) can warn if implicit. */
case object FieldTypeInferred extends PlainAttachment

case class LookupAmbiguityWarning(msg: String) extends PlainAttachment
}
4 changes: 2 additions & 2 deletions src/reflect/scala/reflect/internal/Types.scala
Expand Up @@ -772,8 +772,8 @@ trait Types
def withFilter(p: Type => Boolean) = new FilterMapForeach(p)

class FilterMapForeach(p: Type => Boolean) extends FilterTypeCollector(p){
def foreach[U](f: Type => U): Unit = collect(Type.this) foreach f
def map[T](f: Type => T): List[T] = collect(Type.this) map f
def foreach[U](f: Type => U): Unit = this.collect(Type.this).foreach(f)
def map[T](f: Type => T): List[T] = this.collect(Type.this).map(f)
}

@inline final def orElse(alt: => Type): Type = if (this ne NoType) this else alt
Expand Down
2 changes: 1 addition & 1 deletion src/reflect/scala/reflect/io/ZipArchive.scala
Expand Up @@ -392,7 +392,7 @@ final class ManifestResources(val url: URL) extends ZipArchive(null) {
if (!zipEntry.isDirectory) {
class FileEntry() extends Entry(zipEntry.getName) {
override def lastModified = zipEntry.getTime()
override def input = resourceInputStream(path)
override def input = resourceInputStream(this.path)
override def sizeOption = None
}
val f = new FileEntry()
Expand Down
1 change: 1 addition & 0 deletions src/reflect/scala/reflect/runtime/JavaUniverseForce.scala
Expand Up @@ -79,6 +79,7 @@ trait JavaUniverseForce { self: runtime.JavaUniverse =>
this.RootSelection
this.TypedExpectingUnitAttachment
this.FieldTypeInferred
this.LookupAmbiguityWarning
this.noPrint
this.typeDebug
// inaccessible: this.posAssigner
Expand Down
2 changes: 1 addition & 1 deletion src/scaladoc/scala/tools/nsc/doc/model/ModelFactory.scala
Expand Up @@ -927,7 +927,7 @@ class ModelFactory(val global: Global, val settings: doc.Settings) {
}
else None
def resultType =
makeTypeInTemplateContext(aSym.tpe, inTpl, aSym)
makeTypeInTemplateContext(aSym.tpe, this.inTpl, aSym)
def isImplicit = aSym.isImplicit
}

Expand Down
31 changes: 31 additions & 0 deletions test/files/neg/t11921-alias.check
@@ -0,0 +1,31 @@
t11921-alias.scala:18: warning: reference to TT is ambiguous;
it is both defined in the enclosing object O and available in the enclosing class D as type TT (defined in class C)
Since 3, symbols inherited from a superclass no longer shadow symbols defined in an outer scope.
To continue using the symbol from the superclass, write `this.TT`.
Or use `-Wconf:msg=legacy-binding:s` to silence this warning.
def n(x: TT) = x // ambiguous
^
t11921-alias.scala:38: warning: reference to c is ambiguous;
it is both defined in the enclosing class B and available in the enclosing anonymous class as value c (defined in class A)
Since 3, symbols inherited from a superclass no longer shadow symbols defined in an outer scope.
To continue using the symbol from the superclass, write `this.c`.
Or use `-Wconf:msg=legacy-binding:s` to silence this warning.
def n = c // ambiguous
^
t11921-alias.scala:57: warning: reference to name is ambiguous;
it is both defined in the enclosing method m and available in the enclosing anonymous class as value name (defined in class C)
Since 3, symbols inherited from a superclass no longer shadow symbols defined in an outer scope.
To continue using the symbol from the superclass, write `this.name`.
Or use `-Wconf:msg=legacy-binding:s` to silence this warning.
println(name)
^
t11921-alias.scala:67: warning: reference to name is ambiguous;
it is both defined in the enclosing method m and available in the enclosing anonymous class as value name (defined in class A, inherited through parent class C)
Since 3, symbols inherited from a superclass no longer shadow symbols defined in an outer scope.
To continue using the symbol from the superclass, write `this.name`.
Or use `-Wconf:msg=legacy-binding:s` to silence this warning.
println(name)
^
error: No warnings can be incurred under -Werror.
4 warnings
1 error