Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generated names of context bound parameters change on incremental compilation causing unnecessary invalidations #18080

Closed
mrdziuban opened this issue Jun 27, 2023 · 9 comments · Fixed by #18280

Comments

@mrdziuban
Copy link

Compiler version

3.3.0, also happens with 3.3.1-RC1

Minimized code

Unfortunately I haven't been able to come up with a minimal reproduction. This happens in a large, private codebase, and my attempts to replicate the behavior have so far been unsuccessful.

For what it's worth, this is a simplified version of what the code looks like (note that this doesn't reproduce the issue):

object myObject {
  sealed trait Thing {
    def call(): Option[String]
  }
  case class Thing1(o: Option[String]) extends Thing {
    def call(): Option[String] = o.map(s => s"Thing1($s)")
  }

  case class MyType[A](a: A)

  object MyType {
    implicit def myTypeClass[A: MyTypeClass]: MyTypeClass[MyType[A]] = MyTypeClass.derived
  }
}

If I change the string in Thing1.call from "Thing1($s)" to "Thing($s)" (or anything else that doesn't change the signature of Thing1.call) then recompile, I see that the context bound on MyType.myTypeClass has been renamed, causing invalidation of anything that depends on MyType.

Output

I've enabled debug output in sbt with the following settings:

ThisBuild / logLevel := Level.Debug
ThisBuild / incOptions ~= (_.withApiDebug(true).withRelationsDebug(true))

And I see a number of diffs to the API that look like this (shown as a git diff for syntax highlighting):

- [diff] implicit def myTypeClass[ A >: scala.this#Nothing <: scala.this#Any](implicit evidence$63: example.this#MyTypeClass[<A>]): example.this#MyTypeClass[example.myObject$.this#MyType[<A>]]
+ [diff] implicit def myTypeClass[ A >: scala.this#Nothing <: scala.this#Any](implicit evidence$81: example.this#MyTypeClass[<A>]): example.this#MyTypeClass[example.myObject$.this#MyType[<A>]]

Note that the only thing changed is the name of the MyTypeClass parameter, from evidence$63 to evidence$81.

Expectation

Context bound parameter names shouldn't change, or at least they shouldn't cause API invalidation.

@mrdziuban mrdziuban added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Jun 27, 2023
@smarter smarter added area:incremental-compilation and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Jun 27, 2023
@eed3si9n
Copy link
Member

@mrdziuban (Not that I maintain Dotty compiler bridge, but ) Thanks for reporting this!

@odersky
Copy link
Contributor

odersky commented Jun 28, 2023

There are two ways we might want to fix this:

  • Ignore generated names when extracting the API, or rather, map them all to the same placeholder name
  • Make fresh name generation more predictable by having smaller scopes in which fresh names must be unique (right now, it's the compilation unit).

Maybe do both?

@mrdziuban
Copy link
Author

mrdziuban commented Jun 28, 2023

Update on my end -- I've been progressively deleting code from my codebase until the issue goes away and it finally has!

There's one specific file that imports types like myObject.MyType in order to define typeclass instances for them. If I delete that file, or I refactor the code to define the typeclass instance in the MyType companion object instead, then the issue disappears.

This doesn't fix the issue in my full codebase, however, so there must be other contributing factors. Just wanted to share the update in case it helps debug what's going on.

For example in code:

Before -- issue exists

// file that contains myObject
object myObject { /* code as above */ }

// culprit file
import myObject.MyType

object otherObject {
  trait AnotherTypeClass[A]

  object AnotherTypeClass {
    implicit def myTypeInst[A]: AnotherTypeClass[MyType[A]] = ...
  }
}

After -- issue does not exist

// file that contains myObject
import otherObject.AnotherTypeClass

object myObject {
  // ...
  object MyType {
    implicit def myTypeInst[A]: AnotherTypeClass[MyType[A]] = ...
  }
}

// culprit file
object otherObject {
  trait AnotherTypeClass[A]
}

@eed3si9n
Copy link
Member

eed3si9n commented Jul 2, 2023

@mrdziuban I can't seem to reproduce this issue. Could you put a set of complete code that can reproduces this somewhere public please?

@mrdziuban
Copy link
Author

@eed3si9n still working on it, I'll post back here if/when I can do so.

@mrdziuban
Copy link
Author

@eed3si9n I've minimized it as far as I can, down to only 3 files and less than 60 lines of code. Code and instructions for reproducing are in https://github.com/mrdziuban/dotty-issue-18080. Both methods in Queries.scala with context bounds (database.queries$.ResultSet.getV and database.queries$.ClientOwnerId$.parseClientOwnerId) show that the names change -- there's a screenshot in the README showing the debug output

@mrdziuban
Copy link
Author

Also just added a script to that repo that may come in handy when testing fixes. Just running ./test.sh will confirm whether the issue is still reproducible and will show output like this

Screenshot 2023-07-03 at 4 06 17 PM

@eed3si9n
Copy link
Member

eed3si9n commented Jul 3, 2023

@mrdziuban Thanks!

smarter added a commit to dotty-staging/dotty that referenced this issue Jul 24, 2023
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(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 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 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.
smarter added a commit to dotty-staging/dotty that referenced this issue Jul 24, 2023
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.
smarter added a commit to dotty-staging/dotty that referenced this issue Jul 24, 2023
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.
smarter added a commit to dotty-staging/dotty that referenced this issue Jul 25, 2023
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.
smarter added a commit that referenced this issue Jul 25, 2023
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.
@Kordyjan Kordyjan added this to the 3.4.0 milestone Aug 1, 2023
Kordyjan pushed a commit that referenced this issue Dec 1, 2023
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.

[Cherry-picked f322b7b]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants