Skip to content

Commit

Permalink
Fix overcompilation due to unstable context bound desugaring
Browse files Browse the repository at this point in the history
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 scala#7661.

Fixes scala#18080.
  • Loading branch information
smarter committed Jul 24, 2023
1 parent 81a8509 commit cceb20d
Show file tree
Hide file tree
Showing 10 changed files with 127 additions and 10 deletions.
12 changes: 10 additions & 2 deletions compiler/src/dotty/tools/dotc/ast/Desugar.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -874,7 +874,7 @@ class DottyBytecodeTests extends DottyBytecodeTest {
}
}

@Test def freshNames = {
@Test def stableNames = {
val sourceA =
"""|class A {
| def a1[T: Ordering]: Unit = {}
Expand Down Expand Up @@ -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")
}
}

Expand Down
5 changes: 5 additions & 0 deletions sbt-test/source-dependencies/stable-ctx-bounds/A.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package database

object A {
def wrapper: B.Wrapper = ???
}
29 changes: 29 additions & 0 deletions sbt-test/source-dependencies/stable-ctx-bounds/B.scala
Original file line number Diff line number Diff line change
@@ -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)
}
8 changes: 8 additions & 0 deletions sbt-test/source-dependencies/stable-ctx-bounds/C.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package database

object C {
def foo: Unit = {
val rs: B.ResultSet = ???
rs.getV[String]
}
}
27 changes: 27 additions & 0 deletions sbt-test/source-dependencies/stable-ctx-bounds/build.sbt
Original file line number Diff line number Diff line change
@@ -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")
}
27 changes: 27 additions & 0 deletions sbt-test/source-dependencies/stable-ctx-bounds/changes/B.scala
Original file line number Diff line number Diff line change
@@ -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)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// This is necessary because tests are run in batch mode
object CompileState {
@volatile var previousIterations: Int = -1
}
9 changes: 9 additions & 0 deletions sbt-test/source-dependencies/stable-ctx-bounds/test
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
> compile
> recordPreviousIterations

# change only the body of a method
$ copy-file changes/B.scala B.scala

# Only B.scala should be recompiled
> compile
> checkIterations 1
8 changes: 4 additions & 4 deletions tests/neg/i10901.check
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down

0 comments on commit cceb20d

Please sign in to comment.