In-source sbt compiler bridge [ci: last-only] #10472
Merged
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
Documentation TBD. Rewritten from sbt/zinc@f8a907a
We need to call it just once and this is already being done. Rewritten from sbt/zinc@9cee296
This commit merges 1.0 branch into class-based-dependencies branch.
The class-based-dependencies branch has been obtained from the sbt repo by
rerunning the filter-branch command that has been used originally to
spawn zinc repo out of sbt repo:
git filter-branch --index-filter 'git rm --cached -qr -- . && git reset -q $GIT_COMMIT -- compile interface util/classfile util/classpath util/datatype util/relation' --prune-empty
That preserved some merges from the original class-based-dependencies
branch that didn't make sense so I rebased the filtered branch on top of
7e9ccc10af0c53f17ecfe87cead3ef03fd95642f. This commit is the first commit
in zinc's history after filtering. Once the rebase was done and I got a
cleaner history, I've merged 1.0 into my class-based-dependencies branch.
This commit is exactly that merge.
The merge itself is very complicated because very non-trivial changes
happened in both zinc and my class-based-dependencies branch. What you see
here represents the best effort at merging. I expect mistakes to be
discovered and fixed in subsequent commits. I got it as far as to compile
everything and make most of the test pass.
I highlight below some of the least obvious changes I've made:
- Analysis.groupBy operation has been nuked. It's been broken in
class-based-dependencies and it's not clear who needs it
- AnalysisTest is removed as it was mostly testing the Analysis.groupBy
- changes to definition.json and other.json are ports of changes from
definition and other (from the interface subproject). Consult hisotry
of those files to understand those changes
- TestAnalyzingCompiler has been adapted to handle invalidation at class
level instead of source file level
- TestAnalysisCallback has been adapted to changes how apis, products
and dependencies are tracked. A lot of code has been copied over from
AnalysisCallback. I'm not sure if that the best course of action but
I don't understand the role of TestAnalysisCallback exactly to have
a better opinion.
# Conflicts:
# compile/api/src/main/scala/xsbt/api/ShowAPI.scala
# compile/api/src/test/scala/xsbt/api/NameHashingSpecification.scala
# compile/inc/src/main/scala/sbt/inc/Dependency.scala
# compile/integration/src/main/scala/sbt/compiler/AggressiveCompile.scala
# compile/integration/src/main/scala/sbt/compiler/IncrementalCompiler.scala
# compile/interface/src/main/scala/xsbt/Dependency.scala
# compile/persist/src/main/scala/sbt/inc/AnalysisFormats.scala
# compile/persist/src/main/scala/xsbt/api/AnalyzedClassFormat.scala
# compile/persist/src/main/scala/xsbt/api/SourceFormat.scala
# compile/src/test/scala/sbt/compiler/javac/JavaCompilerSpec.scala
# incrementalcompiler/src/main/scala/sbt/internal/inc/MixedAnalyzingCompiler.scala
# incrementalcompiler/src/main/scala/sbt/internal/inc/javac/AnalyzingJavaCompiler.scala
# interface/definition
# interface/other
# interface/src/main/java/xsbti/DependencyContext.java
# interface/src/test/scala/xsbti/TestCallback.scala
# internal/compiler-bridge/src-2.10/main/scala/xsbt/API.scala
# internal/compiler-bridge/src-2.10/main/scala/xsbt/CompilerInterface.scala
# internal/compiler-bridge/src-2.10/main/scala/xsbt/ExtractAPI.scala
# internal/compiler-bridge/src-2.10/main/scala/xsbt/ExtractUsedNames.scala
# internal/compiler-bridge/src/test/scala/xsbt/DependencySpecification.scala
# internal/compiler-bridge/src/test/scala/xsbt/ExtractAPISpecification.scala
# internal/compiler-bridge/src/test/scala/xsbt/ExtractUsedNamesSpecification.scala
# internal/compiler-bridge/src/test/scala/xsbt/ScalaCompilerForUnitTesting.scala
# internal/compiler-interface/src/main/java/xsbti/AnalysisCallback.java
# internal/incrementalcompiler-apiinfo/src/main/scala/xsbt/api/APIUtil.scala
# internal/incrementalcompiler-apiinfo/src/main/scala/xsbt/api/HashAPI.scala
# internal/incrementalcompiler-apiinfo/src/main/scala/xsbt/api/NameHashing.scala
# internal/incrementalcompiler-classfile/src/main/scala/sbt/internal/inc/classfile/Analyze.scala
# internal/incrementalcompiler-core/src/main/scala/sbt/internal/inc/APIDiff.scala
# internal/incrementalcompiler-core/src/main/scala/sbt/internal/inc/APIs.scala
# internal/incrementalcompiler-core/src/main/scala/sbt/internal/inc/Analysis.scala
# internal/incrementalcompiler-core/src/main/scala/sbt/internal/inc/Compile.scala
# internal/incrementalcompiler-core/src/main/scala/sbt/internal/inc/Incremental.scala
# internal/incrementalcompiler-core/src/main/scala/sbt/internal/inc/IncrementalAntStyle.scala
# internal/incrementalcompiler-core/src/main/scala/sbt/internal/inc/IncrementalCommon.scala
# internal/incrementalcompiler-core/src/main/scala/sbt/internal/inc/IncrementalDefaultImpl.scala
# internal/incrementalcompiler-core/src/main/scala/sbt/internal/inc/IncrementalNameHashing.scala
# internal/incrementalcompiler-core/src/main/scala/sbt/internal/inc/MemberRefInvalidator.scala
# internal/incrementalcompiler-core/src/main/scala/sbt/internal/inc/Relations.scala
# internal/incrementalcompiler-core/src/test/scala/sbt/inc/AnalysisTest.scala
# internal/incrementalcompiler-core/src/test/scala/sbt/inc/TestCaseGenerators.scala
# internal/incrementalcompiler-persist/src/main/scala/sbt/internal/inc/TextAnalysisFormat.scala
# internal/incrementalcompiler-persist/src/main/scala/xsbt/api/SourceFormat.scala
# internal/incrementalcompiler-persist/src/test/scala/sbt/inc/TextAnalysisFormatSpecification.scala
Rewritten from sbt/zinc@b0a8a91
The merge in b0a8a91aaa5b6f55714986f7bf389ec8aa091ec0 messed up code in Dependency.scala. This commit just copies over the code of the `traverse` method as-is from class-based-dependencies. This commit makes all tests in compilerBridge to pass. Rewritten from sbt/zinc@49f8786
The fix was to just copy over the main code and adapt it to Scala 2.10 compiler API. Rewritten from sbt/zinc@469a3ac
Rewritten from sbt/zinc@19d8b9d
The b0a8a91aaa5b6f55714986f7bf389ec8aa091ec0 merge commit didn't resolve merge conflicts for AnalyzeSpecification.scala and for ClassToAPISpecifaction.scala. This commit oves them to the right subprojects and adapts them to Scalatest APIs. Both specifications depend on TestCallback so it had to be moved to a subproject commonly referenced. I've moved it to the compiler-interface subproject. As a consequence, I had to add a dependency on scala-library in `test` configuration to compile TestCallback written in Scala. Rewritten from sbt/zinc@ae5d0fe
The `declaredClasses` relation duplicates the data stored and functionality provided by `classes` relation and it's not being used in incremental compiler. Let's remove it. A historical note: the declaredClasses relation has been introduced at the time when classes relation fulfilled a different role. At some point, classes has been refactored to fulfill exactly the same role as declaredClasses relation. Rewritten from sbt/zinc@deac13a
Remove dependency on CallbackGlobal from ExtractAPI that doesn't require it anymore and update the docs. Remove dependency on CallbackGlobal from ClassName and GlobalHelpers that do not need it either. Rewritten from sbt/zinc@f475de2
This commit enables control of whether a compiler instance should be reused between compiling groups of Scala source files. Check comments in the code for why this can be useful to control. Rewritten from sbt/zinc@a184722
Add a pending test that shows a problem with instability of representing self variables. This test covers the bug described in scala#2504. In order to test API representation of a class declared either in source file or unpickled from a class file, ScalaCompilerForUnitTesting has been extended to extract APIs from multiple compiler instances sharing a classpath. Rewritten from sbt/zinc@3f26571
The reason for instability is a bit tricky so let's unpack what the previous code checking if there's self type declared was doing. It would check if `thisSym` of a class is equal to a symbol representing the class. If that's true, we know that there's no self type. If it's false, then `thisSym` represents either a self type or a self variable. The second (type test) was supposed to check whether the type of `thisSym` is different from a type of the class. However, it would always yield false because TypeRef of `thisSym` was compared to ClassInfoType of a class. So if you had a self variable the logic would see a self type (and that's what API representation would give you). Now the tricky bit: `thisSym` is not pickled when it's representing just a self variable because self variable doesn't affect other classes referring to a class. If you looked at a type after unpickling, the symbol equality test would yield true and we would not see self type when just a self variable was declared. The fix is to check equality of type refs on both side of the type equality check. This makes the pending test passing. Also, I added another test that checks if self types are represented in various combinations of declaring a self variable or/and self type. Fixes scala#2504. Rewritten from sbt/zinc@81cbabf
When `B2.scala` replaces `B.scala` in the new test `types-in-used-names-a`, the name hash of `listb` does not change because the signature of `C.listb` is still `List[B]`, however users of `C.listb` have to be recompiled since the subtyping relationships of its type have changed. This commit does this by extending the definition of "used names" to also include the names of the types of trees, even if these types do not appear in the source like `List[B]` in `D.scala` (since `B` has been invalidated, this will force the recompilation of `D.scala`). This commit does not fix every issue with used types as illustrated by the pending test `types-in-used-names-b`, `B.scala` is not recompiled because it uses the type `T` whose hash has not changed, but `T` is bounded by `S` and `S` has changed, so it should be recompiled. This should be fixable by including the type bounds underlying a `TypeRef` in `symbolsInType`. The test `as-seen-from-a` that did not work before shows that we may not have to worry about tracking prefixes in `ExtractAPI` anymore, see the discussion in scala#87 for more information. Rewritten from sbt/zinc@350afa7
The previous approach to value class API (introduced by sbt/sbt#2261 and refined by sbt/sbt#2413 and sbt/sbt#2414) was to store both unerased and erased signatures so that changes to value classes forced recompilations. This is no longer necessary thanks to scala#87: if a class type is used, then it becomes a dependency of the current class and its name is part of the used names of the current class. Since the name hash of a class changes if it stops or start extending AnyVal, this is enough to force recompilation of anything that uses a value class type. If the underlying type of a value class change, its name hash doesn't change, but the name hash of <init> change and since every class uses the name <init>, we don't need to do anything special to trigger recompilations either. Rewritten from sbt/zinc@1e7e99e
Rewritten from sbt/zinc@7769b58
This change refactors definition.json to make `ClassLike` a non-recursive
type. Before the introduction of class-based dependency tracking, an api
of a nested class has been stored as a member of the enclosing class so a
class could have other class as a member. This was expressed by the fact
that `ClassLike` was recursive: its Structure had a collection of
`Definition` to represents the members and `ClassLike` was a subtype of
`Definition`.
With introduction of class-based dependency tracking, each class has
been extracted and stored separately. The inner class would still be stored
as a member of outer class but members of inner classes were skipped.
An empty inner class was stored just to mark the fact that there's a member
with a given name which is important for name hashing correctness when
there's no dependency on a class directly but a rename could introduce one.
Storing an empty class was a hack and this commit fixes the type hierarchy
by introducing the following changes:
- introduce ClassDefinition that is a subtype of Definition; all members
of a class are subtypes of ClassDefinition (ClassLike is not a subtype
of ClassDefinition)
- change Structure to refer to ClassDefinition instead of Definition for
members
- move ClassLike higher up in type hierarchy so its a direct subtype of
Definition
- introduce ClassLikeDef which represents an inner class as a member of
the outer class; ClassLikeDef carries only information about class
declaration itself but not about its members and that is enforced
statically
NameHashing has been simplified because it doesn't have to keep track of
the entire path for definitions it hashes. Hashes of names are tracked
individually per class so location is simply name of the class and it's
type (we want to distinguish between objects and classes).
NameHashingSpecification has been refactored to not rely on nested classes
for testing the desired scenarios. The semantics of tests has been
preserved even if a different API structure is used in tests.
Rewritten from sbt/zinc@1a696ed
traverse(tree: Tree) used to call super.traverse(tree) at the end. sbt/sbt@0f61629 brought the traversing call to inside of the pattern matching. For the case of MacroExpansionOf(original), it amounts to not traveling the macro-expanded code. See sbt/src/sbt-test/source-dependencies/macro-nonarg-dep for the repro. Rewritten from sbt/zinc@acf4fac
For example, when the `--sourcepath` option is provided and the refchecks phase compiles an annotation found on a referenced symbol from the sourcepath. `compileLate` assumes that all non-sentinel compiler phases can be down cast to `GlobalPhase`. This commit changes the two phases in SBT to extend this instead of `Phase`. This has the knock on benefit of simplifying the phases by letting the `GlobalPhase.run` iterator over the list of compilation units and feed them to us one by one. I checked that the test case failed before making each change. Rewritten from sbt/zinc@a27be44
FPORT: Fixes incremental compiler missing member ref from macro expansion Rewritten from sbt/zinc@ddefbb0
FPORT: Avoid CCE when scalac internally uses compileLate. Fixes scala#2452 Rewritten from sbt/zinc@d0d97f4
For an inherited class, ExtractAPI would form a (source) class name by
calling `Symbol.className` on the inherited class. However, that created
a name of a class as seen at declaration site and not at inheritance site.
Let's consider an example:
class A { class AA }
class B extends A
Before this change, ExtractAPI would create an API representation of `AA`
twice: once seen from A, and then the second time seen from B as an
inherited member. However, in both cases it would use `A.AA` as a name.
This commit fixes naming so an inherited representation of `AA` has
a name `B.AA`.
This commit also clarifies how classes declared in package objects
are named. If you have:
package pkg1.pkg2
package object pkg3 {
class Foo
}
then the fully qualified name of the class corresponding to `pkg3` package
object is pkg1.pkg2.pkg3.package. The full name of the `Foo` class is
pkg2.pkg2.pkg3.Foo.
Rewritten from sbt/zinc@459df6b
Use debuglog for logging in the API phase. That method takes care of checking whether debugging is enabled for a given phase with `-Ylog` option. Previously, the verbose log was spammed with details of the API phase execution, making reading verbose log difficult. Rewritten from sbt/zinc@5391d89
Fix API extraction of inherited classes Rewritten from sbt/zinc@1f07510
Rewritten from sbt/zinc@8813305
Rewritten from sbt/zinc@f872f9c
This avoids an NPE when accessing position info in case `sourcePath` or `sourceFile` are `null`. See sbt/sbt#2766 for a stack trace. Rewritten from sbt/zinc@e2a249a
Rewritten from sbt/zinc@3fb6f44
Ref sbt/zinc#187 Rewritten from sbt/zinc@23ed4ef
SethTisue
added
the
prio:hi
|
After more testing, this is looking fine. The binary bridge seems to work as intended in a test project, I can compile, doc, run the Scala repl in the sbt console. We managed to get a code action from the Scala compiler all the way to VS Code and apply it.
There are a couple of changes that still need to happen for code actions to "just work" with 2.13.12:
From these changes, only the last one will require a follow-up in the bridge source code. Even so, this PR is ready for review (only the last 10 commits, the rest is zinc git history). |
|
I added a commit to avoid dropping code actions in problems reported through the With this we can get quick fixes to work reliably in VS code. The remaining changes to make them work out of the box are outside of scala/scala and may need to wait for 2.13.12 to be released. So I think this PR is ready @SethTisue. To get it working:
|
|
Added one more commit to mark The difference was discovered by @tgodzik while updating bloop to use the binary interface. |
|
I am guessing that the details are ok, but I'd like to communicate the tradeoff here. Neither compiler nor Zinc promises API, and both can independently add or break features. Thus far Scala 2.x has relied on Zinc's bridge that is specific to the latest Zinc. By switching to the binary bridge model (like Dotty), Scala 2.x will carry the responsibility of keeping up with the changes + end users will be forced to bump to the latest patch version to take advantage of the latest Zinc features. |
Nothing from me, as far as I can see this should be simple to support in Bloop, The only issues might be the trade offs Eugene mentioned. |
|
@eed3si9n thanks @lrytz we're comfortable with that, yes? After all, we remain committed to pumping out 2.13.x releases indefinitely. (...from now until the heat death of the universe, under our "VLTS" (Very Long Term Support) designation, which we have scrawled on a mirror with lipstick for anyone to consult...) |
|
FWIW the Mill build tool has its own set of pre-compiled bridges that it checks for before falling back to on-demand compilation. It works great. If new Scala versions come with their own bridges then that would save us the hassle of manually publishing a new bridge every Scala release com-lihaoyi/mill#2424 |
|
A cool thing you can explore doing once the compiler is responsible for calling into zinc is to start recording dependencies during typechecking, We've just started doing this in Scala 3 to keep track of mirrors, but it could be used for macros and other areas where we tend to under-compile (though for macros especially we might need something like @lihaoyi's bytecode-level target invalidation). In any case, I strongly recommend porting the zinc scripted tests) along with the compiler bridge itself (for reference, in dotty we keep them in https://github.com/lampepfl/dotty/tree/main/sbt-test). This is useful for tracking new regressions of course, but it seems that zinc itself only runs its scripted tests on Scala 2.12 which has already led to undiscovered regressions in 2.13: sbt/zinc#1229 |
IMO yes 👍
Thanks, will take a look! |
|
(Zinc scripted tests can wait for a separate PR.) |
|
if anybody wants to test a published bridge (please do, if you can, so we know this is going to work for 2.13.12!), there's one here: Once 2.13.12 proper is out, its bridge will be on Maven Central, of course. |
SethTisue
removed
the
prio:hi
|
discussion thread for tooling authors: https://contributors.scala-lang.org/t/scala-2-13-12-in-source-sbt-compiler-bridge-clarifications/6289 |
This was referenced Sep 11, 2023
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.
Previous attempt: #8531. I think it was just closed because there was no strong reason to do it.
This time there is a good reason: to support "quick fixes" / code actions that the Scala 2.13.12 compiler delivers with warnings and errors, the bridge needs to be changed. This cannot be done in a source-compatible manner, i.e., the bridge sources need to be different for 2.13.11 and 2.13.12. Instead of having multiple versions of the bridge sources for 2.13 compiler versions, releasing the bridge as binary with every Scala release might be easier. Details at sbt/zinc#1214.
This PR includes the git history of the compiler bridge. Documentation how it was done:
The bridge tests from the zinc repo were also migrated (without history).
Advantages of in-sourcing the bridge
A new
scala2-sbt-bridgeartifact will be published with every Scala release.New sbt releases can use the binary bridge instead of compiling it from source.
The binary bridge can be used with existing sbt versions by setting the
scalaCompilerBridgeBinaryJar. Here's how I tried it - probably there are more elegant solutions:I haven't managed yet to make an end-to-end test for quick fixes with metals in VS code, WIP / discussion here sbt/zinc#1214 (comment)