Merge pull request #10447 from som-snytt/issue/12813

Nuance import selector errors in Namers

src/compiler/scala/tools/nsc/ast/parser/Parsers.scala

@@ -2803,46 +2803,59 @@ self =>
      *  }}}
      */
     def importSelectors(): List[ImportSelector] = {
-      val selectors0 = inBracesOrNil(commaSeparated(importSelector()))
-
-      // Treat an import of `*, given` or `given, *` as if it was an import of `*`
-      // since the former in Scala 3 has the same semantics as the latter in Scala 2.
-      val selectors =
-        if (currentRun.isScala3 && selectors0.exists(_.isWildcard))
-          selectors0.filterNot(sel => sel.name == nme.`given` && sel.rename == sel.name)
-        else
-          selectors0
-
-      for (t <- selectors.init if t.isWildcard) syntaxError(t.namePos, "Wildcard import must be in last position")
-      selectors
+      def isWilder(sel: ImportSelector) = sel.isWildcard || currentRun.isScala3 && !sel.isRename && sel.name == nme.`given`
+      // error on duplicate target names, import x.{a=>z, b=>z}, and fix import x.{given, *} to x._
+      def checkSelectors(xs: List[ImportSelector]): List[ImportSelector] = xs match {
+        case h :: t =>
+          // wildcards must come last, and for -Xsource:3, take trailing given, * (or *, given) as _
+          if (isWilder(h)) {
+            if (t.exists(!isWilder(_)))
+              syntaxError(h.namePos, "wildcard import must be in last position")
+            xs.filter(_.isWildcard) match {
+              case ws @ (_ :: Nil) => ws
+              case Nil =>
+                syntaxError(h.namePos, "given requires a wildcard selector")
+                ImportSelector.wildAt(h.namePos) :: Nil
+              case ws @ (w :: _) =>
+                syntaxError(w.namePos, "duplicate wildcard selector")
+                w :: Nil
+            }
+          }
+          else {
+            if (!h.isMask)
+              t.find(_.rename == h.rename).foreach { duplicate =>
+                val msg =
+                  if (h.isRename || duplicate.isRename) s"${h.rename} is an ambiguous name on import"
+                  else s"${h.rename} is imported twice"
+                syntaxError(h.namePos, msg)
+              }
+            h :: checkSelectors(t)
+          }
+        case _ => Nil
+      }
+      checkSelectors(inBracesOrNil(commaSeparated(importSelector())))
     }
 
     def wildcardOrIdent() =
-      if (in.token == USCORE || currentRun.isScala3 && isRawStar) { in.nextToken() ; nme.WILDCARD }
+      if (in.token == USCORE || currentRun.isScala3 && isRawStar) { in.nextToken(); nme.WILDCARD }
       else ident()
 
     /** {{{
      *  ImportSelector ::= Id [`=>` Id | `=>` `_`]
      *  }}}
      */
     def importSelector(): ImportSelector = {
-      val start        = in.offset
-      val bbq          = in.token == BACKQUOTED_IDENT
-      val name         = wildcardOrIdent()
-      var renameOffset = -1
-
-      val rename =
+      val start = in.offset
+      val bbq   = in.token == BACKQUOTED_IDENT
+      val name  = wildcardOrIdent()
+      val (rename, renameOffset) =
         if (in.token == ARROW || (currentRun.isScala3 && isRawIdent && in.name == nme.as)) {
           in.nextToken()
-          renameOffset = in.offset
-          if (name == nme.WILDCARD && !bbq) syntaxError(renameOffset, "Wildcard import cannot be renamed")
-          wildcardOrIdent()
-        }
-        else if (name == nme.WILDCARD && !bbq) null
-        else {
-          renameOffset = start
-          name
+          if (name == nme.WILDCARD && !bbq) syntaxError(in.offset, "Wildcard import cannot be renamed")
+          (wildcardOrIdent(), in.offset)
         }
+        else if (name == nme.WILDCARD && !bbq) (null, -1)
+        else (name, start)
 
       ImportSelector(name, start, rename, renameOffset)
     }

src/compiler/scala/tools/nsc/typechecker/ContextErrors.scala

@@ -1335,12 +1335,7 @@ trait ContextErrors extends splain.SplainErrors {
           ByNameParameter, AbstractVar = Value
       }
 
-      object DuplicatesErrorKinds extends Enumeration {
-        val RenamedTwice, AppearsTwice = Value
-      }
-
       import SymValidateErrors._
-      import DuplicatesErrorKinds._
       import symtab.Flags
 
       def TypeSigError(tree: Tree, ex: TypeError) = {
@@ -1433,16 +1428,6 @@ trait ContextErrors extends splain.SplainErrors {
           ": parameter may only be referenced in a subsequent parameter section"
         issueSymbolTypeError(sym,  "illegal dependent method type" + errorAddendum)(context)
       }
-
-      def DuplicatesError(tree: Tree, name: Name, kind: DuplicatesErrorKinds.Value) = {
-        val msg = kind match {
-          case RenamedTwice => "is renamed twice"
-          case AppearsTwice => "appears twice as a target of a renaming"
-          case x            => throw new MatchError(x)
-        }
-
-        issueNormalTypeError(tree, name.decode + " " + msg)
-      }
     }
 
     def InferredImplicitError(tree: Tree, inferred: Type, cx: Context): Unit =

src/compiler/scala/tools/nsc/typechecker/Contexts.scala

@@ -1106,7 +1106,7 @@ trait Contexts { self: Analyzer =>
       def collect(sels: List[ImportSelector]): List[ImplicitInfo] = sels match {
         case List() =>
           List()
-        case sel :: Nil if sel.isWildcard =>
+        case sel :: _ if sel.isWildcard =>
           // Using pre.implicitMembers seems to exposes a problem with out-dated symbols in the IDE,
           // see the example in https://www.assembla.com/spaces/scala-ide/tickets/1002552#/activity/ticket
           // I haven't been able to boil that down the an automated test yet.
@@ -1255,12 +1255,11 @@ trait Contexts { self: Analyzer =>
       )
 
     /** If the given import is permitted, fetch the symbol and filter for accessibility.
+     *  Tests `exists` to complete SymbolLoaders, which sets the symbol's access flags (scala/bug#12736)
      */
-    private[Contexts] def importedAccessibleSymbol(imp: ImportInfo, sym: => Symbol): Symbol = {
+    private[Contexts] def importedAccessibleSymbol(imp: ImportInfo, sym: => Symbol): Symbol =
       if (isExcludedRootImport(imp)) NoSymbol
       else sym.filter(s => s.exists && isAccessible(s, imp.qual.tpe, superAccess = false))
-      // `exists` above completes SymbolLoaders, which sets the symbol's access flags (scala/bug#12736)
-    }
 
     private def isExcludedRootImport(imp: ImportInfo): Boolean =
       imp.isRootImport && excludedRootImportsCached.get(unit).exists(_.contains(imp.qual.symbol))
@@ -1988,7 +1987,7 @@ trait Contexts { self: Analyzer =>
 
     override def hashCode = tree.##
     override def equals(other: Any) = other match {
-      case that: ImportInfo => (tree == that.tree)
+      case that: ImportInfo => tree == that.tree
       case _                => false
     }
     override def toString = tree.toString

src/compiler/scala/tools/nsc/typechecker/Infer.scala

@@ -543,10 +543,10 @@ trait Infer extends Checkable {
      */
     def methTypeArgs(fn: Tree, tparams: List[Symbol], formals: List[Type], restpe: Type,
                      argtpes: List[Type], pt: Type): AdjustedTypeArgs = {
-      val tvars = tparams map freshVar
       if (!sameLength(formals, argtpes))
         throw new NoInstance("parameter lists differ in length")
 
+      val tvars = tparams.map(freshVar)
       val restpeInst = restpe.instantiateTypeParams(tparams, tvars)
 
       // first check if typevars can be fully defined from the expected type.
@@ -576,10 +576,9 @@ trait Infer extends Checkable {
 
         // Note that isCompatible side-effects: subtype checks involving typevars
         // are recorded in the typevar's bounds (see TypeConstraint)
-        if (!isCompatible(tp1, pt1)) {
+        if (!isCompatible(tp1, pt1))
           throw new DeferredNoInstance(() =>
             "argument expression's type is not compatible with formal parameter type" + foundReqMsg(tp1, pt1))
-        }
       }
       val targs = solvedTypes(tvars, tparams, varianceInTypes(formals), upper = false, lubDepth(formals) max lubDepth(argtpes))
       if (settings.warnInferAny && !fn.isEmpty) {

src/compiler/scala/tools/nsc/typechecker/Namers.scala

@@ -537,7 +537,6 @@ trait Namers extends MethodSynthesis {
     }
 
     private def checkSelectors(tree: Import): Unit = {
-      import DuplicatesErrorKinds._
       val Import(expr, selectors) = tree
       val base = expr.tpe
 
@@ -594,22 +593,7 @@ trait Namers extends MethodSynthesis {
             checkNotRedundant(tree.pos withPoint fromPos, from, to)
         }
       }
-      selectors foreach checkSelector
-
-      def noDuplicates(): Unit = {
-        def loop(xs: List[ImportSelector]): Unit = xs match {
-          case Nil      => ()
-          case hd :: tl =>
-            if (!hd.isWildcard && tl.exists(x => !x.isWildcard && x.name == hd.name))
-              DuplicatesError(tree, hd.name, RenamedTwice)
-            else if (hd.isRename && tl.exists(x => x.isRename && x.rename == hd.rename))
-              DuplicatesError(tree, hd.rename, AppearsTwice)
-            else loop(tl)
-        }
-        loop(selectors)
-      }
-      // checks on the whole set
-      noDuplicates()
+      selectors.foreach(checkSelector)
     }
 
     def copyMethodCompleter(copyDef: DefDef): TypeCompleter = {

src/compiler/scala/tools/nsc/typechecker/Typers.scala

@@ -3311,7 +3311,7 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
       }
     }
 
-    def typedImport(imp : Import): Import = transformed.remove(imp) match {
+    def typedImport(imp: Import): Import = transformed.remove(imp) match {
       case Some(imp1: Import) => imp1
       case _                  => log(s"unhandled import: $imp in $unit"); imp
     }

src/reflect/scala/reflect/internal/Trees.scala

@@ -510,7 +510,7 @@ trait Trees extends api.Trees {
   }
 
   case class ImportSelector(name: Name, namePos: Int, rename: Name, renamePos: Int) extends ImportSelectorApi {
-    assert(name == nme.WILDCARD && rename == null || rename != null, s"Bad import selector $name => $rename")
+    assert(isWildcard || rename != null, s"Bad import selector $name => $rename")
     def isWildcard = name == nme.WILDCARD && rename == null
     def isMask = name != nme.WILDCARD && rename == nme.WILDCARD
     def isSpecific = !isWildcard
@@ -523,8 +523,8 @@ trait Trees extends api.Trees {
       else target != null && sameName(rename, target)
   }
   object ImportSelector extends ImportSelectorExtractor {
-    val wild     = ImportSelector(nme.WILDCARD, -1, null, -1)
-    val wildList = List(wild) // OPT This list is shared for performance.
+    private val wild = ImportSelector(nme.WILDCARD, -1, null, -1)
+    val wildList = List(wild) // OPT This list is shared for performance. Used for unpositioned synthetic only.
     def wildAt(pos: Int) = ImportSelector(nme.WILDCARD, pos, null, -1)
     def mask(name: Name) = ImportSelector(name, -1, nme.WILDCARD, -1)
   }

test/files/neg/t12813.check

@@ -0,0 +1,16 @@
+t12813.scala:5: error: a is imported twice
+import O.{a, a}   // error
+          ^
+t12813.scala:8: error: b is an ambiguous name on import
+import O.{a => b, a => b}   // error
+          ^
+t12813.scala:10: error: b is an ambiguous name on import
+import O.{a => b, toString => b}    // error
+          ^
+t12813.scala:11: error: toString is an ambiguous name on import
+import O.{a => toString, toString}  // error
+          ^
+t12813.scala:12: error: toString is an ambiguous name on import
+import O.{toString, a => toString}  // error
+          ^
+5 errors

test/files/neg/t12813.scala

@@ -0,0 +1,19 @@
+//
+
+object O { val a = 1 }
+
+import O.{a, a}   // error
+import O.{a => b, a}  // ok
+import O.{a, a => b}  // ok
+import O.{a => b, a => b}   // error
+import O.{a => b, a => c}   // ok
+import O.{a => b, toString => b}    // error
+import O.{a => toString, toString}  // error
+import O.{toString, a => toString}  // error
+import O.{a => _, toString => _}    // ok
+import O.{given, a, _}    // ok
+import O.{given, toString, a, _}    // ok
+import O.{a, given, *}    // ok
+import O.{a, *, given}    // ok
+import O.{a, given, *, _} // ok
+import O.{a, given}       // ok

test/files/neg/t12813b.check

@@ -0,0 +1,28 @@
+t12813b.scala:5: error: a is imported twice
+import O.{a, a}   // error
+          ^
+t12813b.scala:8: error: b is an ambiguous name on import
+import O.{a => b, a => b}   // error
+          ^
+t12813b.scala:10: error: b is an ambiguous name on import
+import O.{a => b, toString => b}    // error
+          ^
+t12813b.scala:11: error: toString is an ambiguous name on import
+import O.{a => toString, toString}  // error
+          ^
+t12813b.scala:12: error: toString is an ambiguous name on import
+import O.{toString, a => toString}  // error
+          ^
+t12813b.scala:14: error: wildcard import must be in last position
+import O.{given, a, _}    // error 3
+          ^
+t12813b.scala:15: error: wildcard import must be in last position
+import O.{given, toString, a, _}    // error 3
+          ^
+t12813b.scala:18: error: duplicate wildcard selector
+import O.{a, given, *, _} // error 3
+                    ^
+t12813b.scala:19: error: given requires a wildcard selector
+import O.{a, given}       // error 3
+             ^
+9 errors

test/files/neg/t12813b.scala

@@ -0,0 +1,19 @@
+// scalac: -Xsource:3
+
+object O { val a = 1 }
+
+import O.{a, a}   // error
+import O.{a => b, a}  // ok
+import O.{a, a => b}  // ok
+import O.{a => b, a => b}   // error
+import O.{a => b, a => c}   // ok
+import O.{a => b, toString => b}    // error
+import O.{a => toString, toString}  // error
+import O.{toString, a => toString}  // error
+import O.{a => _, toString => _}    // ok
+import O.{given, a, _}    // error 3
+import O.{given, toString, a, _}    // error 3
+import O.{a, given, *}    // ok
+import O.{a, *, given}    // ok
+import O.{a, given, *, _} // error 3
+import O.{a, given}       // error 3

test/files/neg/t9636.scala

@@ -1,4 +1,4 @@
-// scalac: -Xlint -Xfatal-warnings
+// scalac: -Werror -Xlint
 //
 
 import java.io._

test/files/pos/import-future.scala

@@ -26,7 +26,7 @@ class C {
 
 object starring {
 
-  import scala.concurrent.{*, given}, duration.{given, Duration as D, *}, ExecutionContext.Implicits.*
+  import scala.concurrent.{*, given}, duration.{Duration as D, given, *}, ExecutionContext.Implicits.*
 
   val f = Future(42)
   val r = Await.result(f, D.Inf)