Fix #8617: Check that there is no inheritance/definition shadowing

odersky · anatoliykmetyuk · commit 9d28345e6574 · 2020-04-02T17:47:01.000+02:00
This implements the additional rule for name resolution:

It is an error if an identifier `x` is available as an inherited member
in an inner scope and the same name `x` is defined in an outer scope, unless

 - the inherited member (has an overloaded alternative that) coincides with
   (an overloaded alternative of) the definition `x`
 - `x` is global, i.e. a member of a package.
diff --git a/compiler/src/dotty/tools/dotc/ast/Desugar.scala b/compiler/src/dotty/tools/dotc/ast/Desugar.scala
@@ -112,7 +112,7 @@ object desugar {
         val originalOwner = sym.owner
         def apply(tp: Type) = tp match {
           case tp: NamedType if tp.symbol.exists && (tp.symbol.owner eq originalOwner) =>
-            val defctx = ctx.outersIterator.dropWhile(_.scope eq ctx.scope).next()
+            val defctx = this.ctx.outersIterator.dropWhile(_.scope eq this.ctx.scope).next()
             var local = defctx.denotNamed(tp.name).suchThat(_.isParamOrAccessor).symbol
             if (local.exists) (defctx.owner.thisType select local).dealiasKeepAnnots
             else {
diff --git a/compiler/src/dotty/tools/dotc/core/Denotations.scala b/compiler/src/dotty/tools/dotc/core/Denotations.scala
@@ -1243,7 +1243,7 @@ object Denotations {
       throw new MergeError(sym1, sym2, sym1.info, sym2.info, pre) {
         override def addendum(implicit ctx: Context) =
           i"""
-             |they are both defined in ${sym1.effectiveOwner} but have matching signatures
+             |they are both defined in ${this.sym1.effectiveOwner} but have matching signatures
              |  ${denot1.info} and
              |  ${denot2.info}${super.addendum}"""
       }
diff --git a/compiler/src/dotty/tools/dotc/parsing/Parsers.scala b/compiler/src/dotty/tools/dotc/parsing/Parsers.scala
@@ -3784,7 +3784,7 @@ object Parsers {
             i"refinement cannot have default arguments"
           case tree: ValOrDefDef =>
             if tree.rhs.isEmpty then ""
-            else "refinement in cannot have a right-hand side"
+            else "refinement cannot have a right-hand side"
           case tree: TypeDef =>
             if !tree.isClassDef then ""
             else "refinement cannot be a class or trait"
diff --git a/compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala b/compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala
@@ -57,7 +57,7 @@ enum ErrorMessageID extends java.lang.Enum[ErrorMessageID] {
     CyclicReferenceInvolvingID,
     CyclicReferenceInvolvingImplicitID,
     SuperQualMustBeParentID,
-    AmbiguousImportID,
+    AmbiguousReferenceID,
     MethodDoesNotTakeParametersId,
     AmbiguousOverloadID,
     ReassignmentToValID,
diff --git a/compiler/src/dotty/tools/dotc/reporting/messages.scala b/compiler/src/dotty/tools/dotc/reporting/messages.scala
@@ -250,13 +250,13 @@ object messages {
     // these are usually easier to analyze.
     object reported extends TypeMap:
       def setVariance(v: Int) = variance = v
-      val constraint = ctx.typerState.constraint
+      val constraint = this.ctx.typerState.constraint
       def apply(tp: Type): Type = tp match
         case tp: TypeParamRef =>
           constraint.entry(tp) match
             case bounds: TypeBounds =>
-              if variance < 0 then apply(ctx.typeComparer.fullUpperBound(tp))
-              else if variance > 0 then apply(ctx.typeComparer.fullLowerBound(tp))
+              if variance < 0 then apply(this.ctx.typeComparer.fullUpperBound(tp))
+              else if variance > 0 then apply(this.ctx.typeComparer.fullLowerBound(tp))
               else tp
             case NoType => tp
             case instType => apply(instType)
@@ -1244,8 +1244,8 @@ object messages {
 
   import typer.Typer.BindingPrec
 
-  class AmbiguousImport(name: Name, newPrec: BindingPrec, prevPrec: BindingPrec, prevCtx: Context)(implicit ctx: Context)
-    extends ReferenceMsg(AmbiguousImportID) {
+  class AmbiguousReference(name: Name, newPrec: BindingPrec, prevPrec: BindingPrec, prevCtx: Context)(implicit ctx: Context)
+    extends ReferenceMsg(AmbiguousReferenceID) {
 
     /** A string which explains how something was bound; Depending on `prec` this is either
       *      imported by <tree>
@@ -1254,6 +1254,7 @@ object messages {
     private def bindingString(prec: BindingPrec, whereFound: Context, qualifier: String = "") = {
       val howVisible = prec match {
         case BindingPrec.Definition => "defined"
+        case BindingPrec.Inheritance => "inherited"
         case BindingPrec.NamedImport => "imported by name"
         case BindingPrec.WildImport => "imported"
         case BindingPrec.PackageClause => "found"
@@ -1266,18 +1267,21 @@ object messages {
     }
 
     def msg =
-      i"""|Reference to ${em"$name"} is ambiguous
+      i"""|Reference to ${em"$name"} is ambiguous,
           |it is both ${bindingString(newPrec, ctx)}
           |and ${bindingString(prevPrec, prevCtx, " subsequently")}"""
 
     def explain =
       em"""|The compiler can't decide which of the possible choices you
-           |are referencing with $name.
+           |are referencing with $name: A definition of lower precedence
+           |in an inner scope, or a definition with higher precedence in
+           |an outer scope.
            |Note:
-           |- Definitions take precedence over imports
-           |- Named imports take precedence over wildcard imports
-           |- You may replace a name when imported using
-           |  ${hl("import")} scala.{ $name => ${name.show + "Tick"} }
+           | - Definitions in an enclosing scope take precedence over inherited definitions
+           | - Definitions take precedence over imports
+           | - Named imports take precedence over wildcard imports
+           | - You may replace a name when imported using
+           |   ${hl("import")} scala.{ $name => ${name.show + "Tick"} }
            |"""
   }
 
diff --git a/compiler/src/dotty/tools/dotc/transform/PCPCheckAndHeal.scala b/compiler/src/dotty/tools/dotc/transform/PCPCheckAndHeal.scala
@@ -138,7 +138,7 @@ class PCPCheckAndHeal(@constructorOnly ictx: Context) extends TreeMapWithStages(
       tp match {
         case tp: TypeRef if tp.symbol.isSplice =>
           if (tp.isTerm)
-            ctx.error(i"splice outside quotes", pos)
+            this.ctx.error(i"splice outside quotes", pos)
           if level > 0 then getQuoteTypeTags.getTagRef(tp.prefix.asInstanceOf[TermRef])
           else tp
         case tp: TypeRef if tp.symbol == defn.QuotedTypeClass.typeParams.head =>
diff --git a/compiler/src/dotty/tools/dotc/typer/Implicits.scala b/compiler/src/dotty/tools/dotc/typer/Implicits.scala
@@ -414,7 +414,7 @@ object Implicits {
             case t: TypeParamRef =>
               constraint.entry(t) match {
                 case NoType => t
-                case bounds: TypeBounds => ctx.typeComparer.fullBounds(t)
+                case bounds: TypeBounds => this.ctx.typeComparer.fullBounds(t)
                 case t1 => t1
               }
             case t: TypeVar =>
diff --git a/compiler/src/dotty/tools/dotc/typer/Inferencing.scala b/compiler/src/dotty/tools/dotc/typer/Inferencing.scala
@@ -67,7 +67,7 @@ object Inferencing {
       def apply(tvars: Set[TypeVar], tp: Type) = tp match {
         case tp: TypeVar
         if !tp.isInstantiated &&
-            ctx.typeComparer.bounds(tp.origin)
+            this.ctx.typeComparer.bounds(tp.origin)
               .namedPartsWith(ref => params.contains(ref.symbol))
               .nonEmpty =>
           tvars + tp
@@ -172,13 +172,13 @@ object Inferencing {
       def traverse(tp: Type): Unit = {
         tp match {
           case param: TypeParamRef =>
-            val constraint = ctx.typerState.constraint
+            val constraint = this.ctx.typerState.constraint
             constraint.entry(param) match {
               case TypeBounds(lo, hi)
               if (hi frozen_<:< lo) =>
-                val inst = ctx.typeComparer.approximation(param, fromBelow = true)
+                val inst = this.ctx.typeComparer.approximation(param, fromBelow = true)
                 typr.println(i"replace singleton $param := $inst")
-                ctx.typerState.constraint = constraint.replace(param, inst)
+                this.ctx.typerState.constraint = constraint.replace(param, inst)
               case _ =>
             }
           case _ =>
@@ -333,7 +333,7 @@ object Inferencing {
       def setVariance(v: Int) = variance = v
       def apply(vmap: VarianceMap, t: Type): VarianceMap = t match {
         case t: TypeVar
-        if !t.isInstantiated && ctx.typerState.constraint.contains(t) =>
+        if !t.isInstantiated && this.ctx.typerState.constraint.contains(t) =>
           val v = vmap(t)
           if (v == null) vmap.updated(t, variance)
           else if (v == variance || v == 0) vmap
diff --git a/compiler/src/dotty/tools/dotc/typer/TypeAssigner.scala b/compiler/src/dotty/tools/dotc/typer/TypeAssigner.scala
@@ -123,8 +123,8 @@ trait TypeAssigner {
           range(defn.NothingType, apply(classBound(tp.cls.classInfo)))
         case tp: SkolemType if partsToAvoid(mutable.Set.empty, tp.info).nonEmpty =>
           range(defn.NothingType, apply(tp.info))
-        case tp: TypeVar if ctx.typerState.constraint.contains(tp) =>
-          val lo = ctx.typeComparer.instanceType(
+        case tp: TypeVar if this.ctx.typerState.constraint.contains(tp) =>
+          val lo = this.ctx.typeComparer.instanceType(
             tp.origin, fromBelow = variance > 0 || variance == 0 && tp.hasLowerBound)
           val lo1 = apply(lo)
           if (lo1 ne lo) lo1 else tp
diff --git a/compiler/src/dotty/tools/dotc/typer/Typer.scala b/compiler/src/dotty/tools/dotc/typer/Typer.scala
@@ -49,7 +49,7 @@ object Typer {
    *  accessed by an Ident.
    */
   enum BindingPrec {
-    case NothingBound, PackageClause, WildImport, NamedImport, Definition
+    case NothingBound, PackageClause, WildImport, NamedImport, Inheritance, Definition
 
     def isImportPrec = this == NamedImport || this == WildImport
   }
@@ -125,6 +125,9 @@ class Typer extends Namer
     val refctx = ctx
     val noImports = ctx.mode.is(Mode.InPackageClauseName)
 
+    def ambiguous(newPrec: BindingPrec, prevPrec: BindingPrec, prevCtx: Context)(using Context) =
+      refctx.error(AmbiguousReference(name, newPrec, prevPrec, prevCtx), posd.sourcePos)
+
     /** A symbol qualifies if it really exists and is not a package class.
      *  In addition, if we are in a constructor of a pattern, we ignore all definitions
      *  which are methods and not accessors (note: if we don't do that
@@ -135,10 +138,9 @@ class Typer extends Namer
      *  package as a type from source is always an error.
      */
     def qualifies(denot: Denotation): Boolean =
-      reallyExists(denot) &&
-        !(pt.isInstanceOf[UnapplySelectionProto] &&
-          (denot.symbol is (Method, butNot = Accessor))) &&
-        !(denot.symbol.is(PackageClass))
+      reallyExists(denot)
+      && !(pt.isInstanceOf[UnapplySelectionProto] && denot.symbol.is(Method, butNot = Accessor))
+      && !denot.symbol.is(PackageClass)
 
     /** Find the denotation of enclosing `name` in given context `ctx`.
      *  @param previous    A denotation that was found in a more deeply nested scope,
@@ -161,18 +163,18 @@ class Typer extends Namer
        *                   the previous (inner) definition. This models what scalac does.
        */
       def checkNewOrShadowed(found: Type, newPrec: BindingPrec, scala2pkg: Boolean = false)(implicit ctx: Context): Type =
-        if (!previous.exists || ctx.typeComparer.isSameRef(previous, found)) found
-        else if ((prevCtx.scope eq ctx.scope) &&
-                 (newPrec == Definition ||
-                  newPrec == NamedImport && prevPrec == WildImport))
+        if !previous.exists || ctx.typeComparer.isSameRef(previous, found) then
+           found
+        else if (prevCtx.scope eq ctx.scope)
+                && (newPrec == Definition || newPrec == NamedImport && prevPrec == WildImport)
+        then
           // special cases: definitions beat imports, and named imports beat
           // wildcard imports, provided both are in contexts with same scope
           found
-        else {
-          if (!scala2pkg && !previous.isError && !found.isError)
-            refctx.error(AmbiguousImport(name, newPrec, prevPrec, prevCtx), posd.sourcePos)
+        else
+          if !scala2pkg && !previous.isError && !found.isError then
+            ambiguous(newPrec, prevPrec, prevCtx)
           previous
-        }
 
       /** Recurse in outer context. If final result is same as `previous`, check that it
        *  is new or shadowed. This order of checking is necessary since an
@@ -192,7 +194,8 @@ class Typer extends Namer
           NoType
         else
           val pre = imp.site
-          var denot = pre.memberBasedOnFlags(name, required, EmptyFlags).accessibleFrom(pre)(refctx)
+          var denot = pre.memberBasedOnFlags(name, required, EmptyFlags)
+            .accessibleFrom(pre)(using refctx)
             // Pass refctx so that any errors are reported in the context of the
             // reference instead of the context of the import scope
           if checkBounds && denot.exists then
@@ -290,11 +293,35 @@ class Typer extends Namer
           // with an error on CI which I cannot replicate locally (not even
           // with the exact list of files given).
           val isNewDefScope =
-            if (curOwner.is(Package) && !curOwner.isRoot) curOwner ne ctx.outer.owner
-            else ((ctx.scope ne lastCtx.scope) || (curOwner ne lastCtx.owner)) &&
-                  !isTransparentPackageObject
+            if curOwner.is(Package) && !curOwner.isRoot then
+              curOwner ne ctx.outer.owner
+            else
+              ((ctx.scope ne lastCtx.scope) || (curOwner ne lastCtx.owner))
+              && !isTransparentPackageObject
+
+          // Does reference `tp` refer only to inherited symbols?
+          def isInherited(denot: Denotation) =
+            def isCurrent(mbr: SingleDenotation): Boolean =
+              !mbr.symbol.exists || mbr.symbol.owner == ctx.owner
+            denot match
+              case denot: SingleDenotation => !isCurrent(denot)
+              case denot => !denot.hasAltWith(isCurrent)
+
+          def checkNoOuterDefs(denot: Denotation, last: Context, prevCtx: Context): Unit =
+            val outer = last.outer
+            val owner = outer.owner
+            if (owner eq last.owner) && (outer.scope eq last.scope) then
+              checkNoOuterDefs(denot, outer, prevCtx)
+            else if !owner.is(Package) then
+              val scope = if owner.isClass then owner.info.decls else outer.scope
+              if scope.lookup(name).exists then
+                val symsMatch = scope.lookupAll(name).exists(denot.containsSym)
+                if !symsMatch then
+                  ambiguous(Definition, Inheritance, prevCtx)(using outer)
+              else
+                checkNoOuterDefs(denot, outer, prevCtx)
 
-          if (isNewDefScope) {
+          if isNewDefScope then
             val defDenot = ctx.denotNamed(name, required)
             if (qualifies(defDenot)) {
               val found =
@@ -309,20 +336,22 @@ class Typer extends Namer
                       curOwner
                   effectiveOwner.thisType.select(name, defDenot)
                 }
-              if (!curOwner.is(Package) || isDefinedInCurrentUnit(defDenot))
+              if !curOwner.is(Package) || isDefinedInCurrentUnit(defDenot) then
                 result = checkNewOrShadowed(found, Definition) // no need to go further out, we found highest prec entry
-              else {
+                found match
+                  case found: NamedType if ctx.owner.isClass && isInherited(found.denot) =>
+                    checkNoOuterDefs(found.denot, ctx, ctx)
+                  case _ =>
+              else
                 if (ctx.scala2CompatMode && !foundUnderScala2.exists)
                   foundUnderScala2 = checkNewOrShadowed(found, Definition, scala2pkg = true)
                 if (defDenot.symbol.is(Package))
                   result = checkNewOrShadowed(previous orElse found, PackageClause)
                 else if (prevPrec.ordinal < PackageClause.ordinal)
                   result = findRefRecur(found, PackageClause, ctx)(ctx.outer)
-              }
             }
-          }
 
-          if (result.exists) result
+          if result.exists then result
           else {  // find import
             val outer = ctx.outer
             val curImport = ctx.importInfo
diff --git a/compiler/src/dotty/tools/dotc/typer/VarianceChecker.scala b/compiler/src/dotty/tools/dotc/typer/VarianceChecker.scala
@@ -44,7 +44,7 @@ object VarianceChecker {
               .find(_.name.toTermName == paramName)
               .map(_.sourcePos)
               .getOrElse(tree.sourcePos)
-            ctx.error(em"${paramVarianceStr}variant type parameter $paramName occurs in ${occursStr}variant position in ${tl.resType}", pos)
+            this.ctx.error(em"${paramVarianceStr}variant type parameter $paramName occurs in ${occursStr}variant position in ${tl.resType}", pos)
           }
           def apply(x: Boolean, t: Type) = x && {
             t match {
@@ -115,10 +115,10 @@ class VarianceChecker()(implicit ctx: Context) {
         val required = compose(relative, this.variance)
         def tvar_s = s"$tvar (${varianceLabel(tvar.flags)} ${tvar.showLocated})"
         def base_s = s"$base in ${base.owner}" + (if (base.owner.isClass) "" else " in " + base.owner.enclosingClass)
-        ctx.log(s"verifying $tvar_s is ${varianceLabel(required)} at $base_s")
-        ctx.log(s"relative variance: ${varianceLabel(relative)}")
-        ctx.log(s"current variance: ${this.variance}")
-        ctx.log(s"owner chain: ${base.ownersIterator.toList}")
+        this.ctx.log(s"verifying $tvar_s is ${varianceLabel(required)} at $base_s")
+        this.ctx.log(s"relative variance: ${varianceLabel(relative)}")
+        this.ctx.log(s"current variance: ${this.variance}")
+        this.ctx.log(s"owner chain: ${base.ownersIterator.toList}")
         if (tvar.isOneOf(required)) None
         else Some(VarianceError(tvar, required))
       }
diff --git a/compiler/src/dotty/tools/dotc/util/SourceFile.scala b/compiler/src/dotty/tools/dotc/util/SourceFile.scala
@@ -33,7 +33,7 @@ object ScriptSourceFile {
       }
       else 0
     new SourceFile(file, content drop headerLength) {
-      override val underlying = new SourceFile(file, content)
+      override val underlying = new SourceFile(this.file, this.content)
     }
   }
 }
diff --git a/compiler/src/dotty/tools/io/ZipArchive.scala b/compiler/src/dotty/tools/io/ZipArchive.scala
@@ -208,7 +208,7 @@ final class ManifestResources(val url: URL) extends ZipArchive(null) {
       if (!zipEntry.isDirectory) {
         val f = new Entry(zipEntry.getName, dir) {
           override def lastModified = zipEntry.getTime()
-          override def input        = resourceInputStream(path)
+          override def input        = resourceInputStream(this.path)
           override def sizeOption   = None
         }
         dir.entries(f.name) = f
diff --git a/tests/neg/ambiref.check b/tests/neg/ambiref.check
@@ -0,0 +1,24 @@
+-- [E049] Reference Error: tests/neg/ambiref.scala:8:14 ----------------------------------------------------------------
+8 |      println(x)  // error
+  |              ^
+  |              Reference to x is ambiguous,
+  |              it is both defined in object Test
+  |              and inherited subsequently in class D
+
+longer explanation available when compiling with `-explain`
+-- [E049] Reference Error: tests/neg/ambiref.scala:10:14 ---------------------------------------------------------------
+10 |      println(x)  // error
+   |              ^
+   |              Reference to x is ambiguous,
+   |              it is both defined in object Test
+   |              and inherited subsequently in anonymous class test1.C {...}
+
+longer explanation available when compiling with `-explain`
+-- [E049] Reference Error: tests/neg/ambiref.scala:17:14 ---------------------------------------------------------------
+17 |      println(y)  // error
+   |              ^
+   |              Reference to y is ambiguous,
+   |              it is both defined in method c
+   |              and inherited subsequently in anonymous class D {...}
+
+longer explanation available when compiling with `-explain`
diff --git a/tests/neg/ambiref.scala b/tests/neg/ambiref.scala
diff --git a/tests/pos/i2232.scala b/tests/pos/i2232.scala
diff --git a/tests/pos/t0165.scala b/tests/pos/t0165.scala
diff --git a/tests/pos/typeclass-encoding2.scala b/tests/pos/typeclass-encoding2.scala
diff --git a/tests/run-custom-args/tasty-interpreter/interpreter/TastyInterpreter.scala b/tests/run-custom-args/tasty-interpreter/interpreter/TastyInterpreter.scala
diff --git a/tests/run/unambiref.check b/tests/run/unambiref.check
diff --git a/tests/run/unambiref.scala b/tests/run/unambiref.scala

Original file line number	Diff line number	Diff line change
`@@ -1243,7 +1243,7 @@ object Denotations {`
`1243`	`1243`	`throw new MergeError(sym1, sym2, sym1.info, sym2.info, pre) {`
`1244`	`1244`	`override def addendum(implicit ctx: Context) =`
`1245`	`1245`	`i"""`
`1246`		`- \|they are both defined in ${sym1.effectiveOwner} but have matching signatures`
	`1246`	`+ \|they are both defined in ${this.sym1.effectiveOwner} but have matching signatures`
`1247`	`1247`	`\| ${denot1.info} and`
`1248`	`1248`	`\| ${denot2.info}${super.addendum}"""`
`1249`	`1249`	`}`
Original file line number	Diff line number	Diff line change
`@@ -414,7 +414,7 @@ object Implicits {`
`414`	`414`	`case t: TypeParamRef =>`
`415`	`415`	`constraint.entry(t) match {`
`416`	`416`	`case NoType => t`
`417`		`- case bounds: TypeBounds => ctx.typeComparer.fullBounds(t)`
	`417`	`+ case bounds: TypeBounds => this.ctx.typeComparer.fullBounds(t)`
`418`	`418`	`case t1 => t1`
`419`	`419`	`}`
`420`	`420`	`case t: TypeVar =>`