Merge pull request #580 from scala/backport-lts-3.3-23121

Backport "Invent given pattern name in for comprehension" to 3.3 LTS

Author: tgodzik

compiler/src/dotty/tools/dotc/ast/Desugar.scala

@@ -1653,18 +1653,19 @@ object desugar {
        *  that refers to the bound variable for the pattern. Wildcard Binds are
        *  also replaced by Binds with fresh names.
        */
-      def makeIdPat(pat: Tree): (Tree, Ident) = pat match {
-        case bind @ Bind(name, pat1) =>
-          if name == nme.WILDCARD then
-            val name = UniqueName.fresh()
-            (cpy.Bind(pat)(name, pat1).withMods(bind.mods), Ident(name))
-          else (pat, Ident(name))
+      def makeIdPat(pat: Tree): (Tree, Ident) = pat match
+        case pat @ Bind(nme.WILDCARD, body) =>
+          val name =
+            body match
+            case Typed(Ident(nme.WILDCARD), tpt) if pat.mods.is(Given) => inventGivenName(tpt)
+            case _ => UniqueName.fresh()
+          (cpy.Bind(pat)(name, body).withMods(pat.mods), Ident(name))
+        case Bind(name, _) => (pat, Ident(name))
         case id: Ident if isVarPattern(id) && id.name != nme.WILDCARD => (id, id)
         case Typed(id: Ident, _) if isVarPattern(id) && id.name != nme.WILDCARD => (pat, id)
         case _ =>
           val name = UniqueName.fresh()
           (Bind(name, pat), Ident(name))
-      }
 
       /** Make a pattern filter:
        *    rhs.withFilter { case pat => true case _ => false }

compiler/src/dotty/tools/dotc/reporting/messages.scala

@@ -2087,8 +2087,27 @@ extends NamingMsg(AlreadyDefinedID):
         i" in ${conflicting.associatedFile}"
       else if conflicting.owner == owner then ""
       else i" in ${conflicting.owner}"
+    def print(tpe: Type): String =
+      def addParams(tpe: Type): List[String] = tpe match
+        case tpe: MethodType =>
+          val s = if tpe.isContextualMethod then i"(${tpe.paramInfos}%, %) =>" else ""
+          s :: addParams(tpe.resType)
+        case tpe: PolyType =>
+          i"[${tpe.paramNames}%, %] =>" :: addParams(tpe.resType)
+        case tpe =>
+          i"$tpe" :: Nil
+      addParams(tpe).mkString(" ")
     def note =
-      if owner.is(Method) || conflicting.is(Method) then
+      if conflicting.is(Given) && name.startsWith("given_") then
+        i"""|
+            |
+            |Provide an explicit, unique name to given definitions,
+            |since the names assigned to anonymous givens may clash. For example:
+            |
+            |      given myGiven: ${print(atPhase(typerPhase)(conflicting.info))}  // define an instance
+            |      given myGiven @ ${print(atPhase(typerPhase)(conflicting.info))} // as a pattern variable
+            |"""
+      else if owner.is(Method) || conflicting.is(Method) then
         "\n\nNote that overloaded methods must all be defined in the same group of toplevel definitions"
       else ""
     if conflicting.isTerm != name.isTermName then

tests/neg/i23119.check

@@ -0,0 +1,20 @@
+-- [E161] Naming Error: tests/neg/i23119.scala:8:4 ---------------------------------------------------------------------
+8 |    given Option[List[Int]] = Some(List(x)) // error
+  |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+  |    given_Option_List is already defined as given instance given_Option_List
+  |
+  |    Provide an explicit, unique name to given definitions,
+  |    since the names assigned to anonymous givens may clash. For example:
+  |
+  |          given myGiven: Option[List[String]]  // define an instance
+  |          given myGiven @ Option[List[String]] // as a pattern variable
+-- [E161] Naming Error: tests/neg/i23119.scala:18:8 --------------------------------------------------------------------
+18 |  given [A] => List[A] = ??? // error
+   |  ^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |  given_List_A is already defined as given instance given_List_A
+   |
+   |  Provide an explicit, unique name to given definitions,
+   |  since the names assigned to anonymous givens may clash. For example:
+   |
+   |        given myGiven: [A] => List[A]  // define an instance
+   |        given myGiven @ [A] => List[A] // as a pattern variable

tests/neg/i23119.scala

@@ -0,0 +1,19 @@
+//> using options -explain
+
+@main def test = println:
+  for x <- 1 to 2
+    // works with explicit name
+    //ols @ given Option[List[String]] = Some(List(x.toString))
+    given Option[List[String]] = Some(List(x.toString))
+    given Option[List[Int]] = Some(List(x)) // error
+  yield summon[Option[List[String]]].map(ss => ss.corresponds(given_Option_List.get)((a, b) => a == b.toString))
+
+// The naming clash is noticed when defining local values for "packaging":
+//  given_Option_List is already defined as given instance given_Option_List
+// Previously the naming clash was noticed when extracting values in the map or do function:
+//  duplicate pattern variable: given_Option_List
+
+def also =
+  given [A] => List[A] = ???
+  given [A] => List[A] = ??? // error
+  ()

tests/pos/i23119.scala

@@ -0,0 +1,29 @@
+//> using options -Wunused:patvars -Werror
+
+def make: IndexedSeq[FalsePositive] =
+  for {
+    i <- 1 to 2
+    given Int = i
+    fp = FalsePositive()
+  } yield fp
+
+def broken =
+  for
+    i <- List(42)
+    (x, y) = "hello" -> "world"
+  yield
+    s"$x, $y" * i
+
+def alt: IndexedSeq[FalsePositive] =
+  given String = "hi"
+  for
+    given Int <- 1 to 2
+    j: Int = summon[Int] // simple assign because irrefutable
+    _ = j + 1
+    k :: Nil = j :: Nil : @unchecked // pattern in one var
+    fp = FalsePositive(using k)
+  yield fp
+
+class FalsePositive(using Int):
+  def usage(): Unit =
+    println(summon[Int])