Fix overcompilation due to unstable context bound desugaring #18280
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bishabosha
reviewed
Jul 25, 2023
bishabosha
reviewed
Jul 25, 2023
| @@ -0,0 +1,9 @@ | |||
| > compile | |||
There was a problem hiding this comment.
may be worth noting here the predicted order of typer visiting definitions in run 1 vs run 2, to illustrate how before the change it actually had unstable names between runs.
bishabosha
approved these changes
Jul 25, 2023
There are various places in the compiler and assorted tools where we assume that an EvidenceParamName is the name of a context bound, but in practice we also used the same NameKind in other cases such as for inferred contextual functions. This commit cleans things up by replacing EvidenceParamName by: - ContextBoundParamName - ContextFunctionParamName - CanThrowEvidenceParamName - and the existing WildcardParamName Note that Scala 2 also uses "evidence$" prefixes to represent context bounds, this is why some pretty-printing code that aims to resugar context bounds coming from both Scala 2 and 3 does a syntactic check for `ContextBoundParamName.separator` instead of a semantic check on the NameKind itself, this could perhaps be handled in a nicer way using unmangle in the Scala2Unpickler.
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
force-pushed
the
stable-ctx-bounds
branch
from
b833b8c
to
f322b7b
Compare
|
This change is causing compatibility issues when running a Play project with Scala 3.3.1 with Play artifacts build with Scala 3.3.2: @smarter Can you take a look? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context bounds are desugared into term parameters
evidence$Nand before thiscommit, the
Nwas chosen to be unique in the current compilation unit. Thisisn'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
Non different compilationruns 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-boundstest). This issueaffects 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)intodef foo(using x$1: A, x$2: B)regardless of thecontext.
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 defsince the entire body of theinline defconstitutes its API. Inthe 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.