Fix overcompilation due to unstable context bound desugaring

Context bounds are desugared into term parameters `evidence$N` and before this
commit, the `N` was chosen to be unique in the current compilation unit. This
isn't great because it means that adding a new definition with a context bound
in the middle of a file would change the desugaring of subsequent definitions
in the same file.

Even worse, when using incremental compilation we could end up with the same
context bound desugared with a different value of `N` on different compilation
runs because the order in which a compilation unit is traversed during Typer is
not fixed but depends on the how the units that are jointly compiled depend on
each other (as demonstrated by the `stable-ctx-bounds` test). This issue
affects all fresh names generated during Typer, but it is especially
problematic for context bounds because they're part of the API and renaming
a method parameter forces the recompilation of all files calling that method.

To fix this, we now only require context bounds parameters to have unique names
among all the parameters of the method. This matches how we already desugar
`def foo(using A, B)` into `def foo(using x$1: A, x$2: B)` regardless of the
context.

Note that fresh names used in other situations are still problematic for
deterministic compilation. Most of the time they're not part of the API checked
by Zinc, but they can still lead to overcompilation if they appear in an
`inline def` since the entire body of the `inline def` constitutes its API. In
the future, we should follow Scala 2's lead and only require names to be fresh
at the method level: scala/scala#6300 (The Scala 2
logic is slightly more complex to handle macros, but I don't think that applies
to Scala 3 macros), see #7661.

Fixes #18080.

Author: smarter

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

@@ -244,19 +244,27 @@ object desugar {
     val DefDef(_, paramss, tpt, rhs) = meth
     val evidenceParamBuf = ListBuffer[ValDef]()
 
+    var seenContextBounds: Int = 0
     def desugarContextBounds(rhs: Tree): Tree = rhs match
       case ContextBounds(tbounds, cxbounds) =>
         val iflag = if sourceVersion.isAtLeast(`future`) then Given else Implicit
         evidenceParamBuf ++= makeImplicitParameters(
           cxbounds, iflag,
-          mkParamName = () => ContextBoundParamName.fresh(),
+          // Just like with `makeSyntheticParameter` on nameless parameters of
+          // using clauses, we only need names that are unique among the
+          // parameters of the method since shadowing does not affect
+          // implicit resolution in Scala 3.
+          mkParamName = () =>
+            val index = seenContextBounds + 1 // Start at 1 like FreshNameCreator.
+            val ret = ContextBoundParamName(EmptyTermName, index)
+            seenContextBounds += 1
+            ret,
           forPrimaryConstructor = isPrimaryConstructor)
         tbounds
       case LambdaTypeTree(tparams, body) =>
         cpy.LambdaTypeTree(rhs)(tparams, desugarContextBounds(body))
       case _ =>
         rhs
-
     val paramssNoContextBounds =
       mapParamss(paramss) {
         tparam => cpy.TypeDef(tparam)(rhs = desugarContextBounds(tparam.rhs))

compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala

@@ -874,7 +874,7 @@ class DottyBytecodeTests extends DottyBytecodeTest {
     }
   }
 
-  @Test def freshNames = {
+  @Test def stableNames = {
     val sourceA =
       """|class A {
          |  def a1[T: Ordering]: Unit = {}
@@ -902,11 +902,11 @@ class DottyBytecodeTests extends DottyBytecodeTest {
           s"Method ${mn.name} has parameter $actualName but expected $expectedName")
       }
 
-      // The fresh name counter should be reset for every compilation unit
+      // Each definition should get the same names since there's no possible clashes.
       assertParamName(a1, "evidence$1")
-      assertParamName(a2, "evidence$2")
+      assertParamName(a2, "evidence$1")
       assertParamName(b1, "evidence$1")
-      assertParamName(b2, "evidence$2")
+      assertParamName(b2, "evidence$1")
     }
   }
 

sbt-test/source-dependencies/stable-ctx-bounds/A.scala

@@ -0,0 +1,5 @@
+package database
+
+object A {
+  def wrapper: B.Wrapper = ???
+}

sbt-test/source-dependencies/stable-ctx-bounds/B.scala

@@ -0,0 +1,29 @@
+package database
+
+object B {
+  trait GetValue[T]
+
+  object GetValue {
+    implicit def inst[T]: GetValue[T] = ???
+  }
+
+  class ResultSet {
+    def getV[A: GetValue]: A = ???
+  }
+
+  trait DBParse[T] {
+    def apply(rs: ResultSet): T
+  }
+
+  class AVG() {
+    def call: String = "AVG"
+  }
+
+  object ClientOwnerId {
+    class CompanyId
+
+    def parseClientOwnerId[T: DBParse]: Unit = {}
+  }
+
+  class Wrapper(companyId: ClientOwnerId.CompanyId)
+}

sbt-test/source-dependencies/stable-ctx-bounds/C.scala

@@ -0,0 +1,8 @@
+package database
+
+object C {
+  def foo: Unit = {
+    val rs: B.ResultSet = ???
+    rs.getV[String]
+  }
+}

sbt-test/source-dependencies/stable-ctx-bounds/build.sbt

@@ -0,0 +1,27 @@
+scalaVersion := sys.props("plugin.scalaVersion")
+
+import sbt.internal.inc.Analysis
+import complete.DefaultParsers._
+
+// Reset compiler iterations, necessary because tests run in batch mode
+val recordPreviousIterations = taskKey[Unit]("Record previous iterations.")
+recordPreviousIterations := {
+  val log = streams.value.log
+  CompileState.previousIterations = {
+    val previousAnalysis = (previousCompile in Compile).value.analysis.asScala
+    previousAnalysis match {
+      case None =>
+        log.info("No previous analysis detected")
+        0
+      case Some(a: Analysis) => a.compilations.allCompilations.size
+    }
+  }
+}
+
+val checkIterations = inputKey[Unit]("Verifies the accumulated number of iterations of incremental compilation.")
+
+checkIterations := {
+  val expected: Int = (Space ~> NatBasic).parsed
+  val actual: Int = ((compile in Compile).value match { case a: Analysis => a.compilations.allCompilations.size }) - CompileState.previousIterations
+  assert(expected == actual, s"Expected $expected compilations, got $actual")
+}

sbt-test/source-dependencies/stable-ctx-bounds/changes/B.scala

@@ -0,0 +1,27 @@
+package database
+
+object B {
+  trait GetValue[T]
+
+  object GetValue {
+    implicit def inst[T]: GetValue[T] = ???
+  }
+
+  class ResultSet {
+    def getV[A: GetValue]: A = ???
+  }
+
+  trait DBParse[T]
+
+  class AVG() {
+    def call: String = "AVG2"
+  }
+
+  object ClientOwnerId {
+    class CompanyId
+
+    def parseClientOwnerId[T: DBParse]: Unit = {}
+  }
+
+  class Wrapper(companyId: ClientOwnerId.CompanyId)
+}

sbt-test/source-dependencies/stable-ctx-bounds/project/CompileState.scala

@@ -0,0 +1,4 @@
+// This is necessary because tests are run in batch mode
+object CompileState {
+  @volatile var previousIterations: Int = -1
+}

sbt-test/source-dependencies/stable-ctx-bounds/test

@@ -0,0 +1,15 @@
+> compile
+> recordPreviousIterations
+
+# change only the body of a method
+$ copy-file changes/B.scala B.scala
+
+# Only B.scala should be recompiled. Previously, this lead to a subsequent
+# compilation round because context bounds were desugared into names unique to
+# the whole compilation unit, and in the first `compile` the two context bounds
+# of B.scala were desugared into `evidence$2` and `evidence$1` in this order
+# (because the definitions were visited out of order), but in the second call
+# to `compile` we traverse them in order as we typecheck B.scala and ended up
+# with `evidence$1` and `evidence$2` instead.
+> compile
+> checkIterations 1

tests/neg/i10901.check

@@ -12,11 +12,11 @@
    |                [T1, T2]
    |                 (x: BugExp4Point2D.ColumnType[T1])
    |                   (y: BugExp4Point2D.ColumnType[T2])
-   |                     (implicit evidence$7: Numeric[T1], evidence$8: Numeric[T2]): BugExp4Point2D.Point2D[T1, T2]
+   |                     (implicit evidence$1: Numeric[T1], evidence$2: Numeric[T2]): BugExp4Point2D.Point2D[T1, T2]
    |                [T1, T2]
    |                 (x: T1)
    |                   (y: BugExp4Point2D.ColumnType[T2])
-   |                     (implicit evidence$5: Numeric[T1], evidence$6: Numeric[T2]): BugExp4Point2D.Point2D[T1, T2]
+   |                     (implicit evidence$1: Numeric[T1], evidence$2: Numeric[T2]): BugExp4Point2D.Point2D[T1, T2]
    |               both match arguments ((x : BugExp4Point2D.IntT.type))((y : BugExp4Point2D.DoubleT.type))
 -- [E008] Not Found Error: tests/neg/i10901.scala:48:38 ----------------------------------------------------------------
 48 |    val pos4: Point2D[Int,Double] = x º 201.1   // error
@@ -31,8 +31,8 @@
    |        Ambiguous overload. The overloaded alternatives of method º in object dsl with types
    |         [T1, T2]
    |          (x: BugExp4Point2D.ColumnType[T1])
-   |            (y: T2)(implicit evidence$9: Numeric[T1], evidence$10: Numeric[T2]): BugExp4Point2D.Point2D[T1, T2]
-   |         [T1, T2](x: T1)(y: T2)(implicit evidence$3: Numeric[T1], evidence$4: Numeric[T2]): BugExp4Point2D.Point2D[T1, T2]
+   |            (y: T2)(implicit evidence$1: Numeric[T1], evidence$2: Numeric[T2]): BugExp4Point2D.Point2D[T1, T2]
+   |         [T1, T2](x: T1)(y: T2)(implicit evidence$1: Numeric[T1], evidence$2: Numeric[T2]): BugExp4Point2D.Point2D[T1, T2]
    |        both match arguments ((x : BugExp4Point2D.IntT.type))((201.1d : Double))
 -- [E008] Not Found Error: tests/neg/i10901.scala:62:16 ----------------------------------------------------------------
 62 |  val y = "abc".foo  // error