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

Add support for xsbti.compile.CompileProgress #18739

Merged
merged 13 commits into from
Oct 31, 2023

Conversation

bishabosha
Copy link
Member

@bishabosha bishabosha commented Oct 20, 2023

so far only the progress tracking, I intend to add in support for cancellation as well
Edit: now supports cancellation and progress tracking.

I am testing cancellation in a synchronous way, should I test in another way (thread interruptions)?

fixes #13082

Release Notes

Scala 3.4 restores support for progress tracking (available in Scala 2), this also provides an interface for cooperative interruption of compilation (e.g. cancellation from within an editor). Metals IDE users will notice that compilation progress is no longer frozen at 0% when using sbt or bloop as the BSP server. IntelliJ will also correctly report progress for BSP and sbt based projects

@@ -400,7 +460,101 @@ class Run(comp: Compiler, ictx: Context) extends ImplicitRunInfo with Constraint
}

object Run {

class SubPhases(val phase: Phase):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason this exists is because TyperPhase is really like three phases, it might be good to allow cancellation in the middle of the phase

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any reason not to split TyperPhase into three phases?

@bishabosha bishabosha marked this pull request as ready for review October 23, 2023 16:07
@bishabosha
Copy link
Member Author

updated with cancellation support

@bishabosha bishabosha assigned sjrd and unassigned bishabosha Oct 23, 2023
@bishabosha bishabosha requested a review from sjrd October 23, 2023 16:10
@bishabosha
Copy link
Member Author

bishabosha commented Oct 24, 2023

If you want to see this working in action, check out https://github.com/bishabosha/spire-scala3, set the scalaVersion to 3.4.0-RC1-bin-SNAPSHOT after running scala3-bootstrapped/publishLocalBin on this branch, open it in vscode with metals and import the project with bloop

compiler/src/dotty/tools/dotc/core/Contexts.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/Run.scala Show resolved Hide resolved
compiler/src/dotty/tools/dotc/Run.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/parsing/ParserPhase.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/typer/TyperPhase.scala Outdated Show resolved Hide resolved
Comment on lines 61 to 62
/** Keep synchronised with `monitor` subcalls */
override def subPhases: List[String] = List("indexing", "typechecking", "checking java")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want that "keep synchronized" comment to be actually upheld, you should also put a similar comments at all the places that actually do call monitor. Otherwise someone changing the call sites will never see this comment and hence will never update subPhases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made the strings into context parameters, and refactored the way we tick the subphase so its actually more declarative

compiler/src/dotty/tools/dotc/core/Phases.scala Outdated Show resolved Hide resolved
@bishabosha bishabosha requested a review from sjrd October 25, 2023 19:34
@sjrd sjrd merged commit 58810fd into scala:main Oct 31, 2023
18 checks passed
@sjrd sjrd deleted the add-compile-progress branch October 31, 2023 15:45
@tgodzik
Copy link
Contributor

tgodzik commented Nov 1, 2023

This is great! Should we backport this to LTS?

@bishabosha bishabosha added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Nov 1, 2023
@bishabosha
Copy link
Member Author

bishabosha commented Nov 1, 2023

This is great! Should we backport this to LTS?

this also depends on #18137 being backported Edit: actually I think it does not depend - as we are already using a wrapper interface

Edit 2: we should backport, to clear confusion - note we do change the sbt-bridge so IntelliJ will need to be notified

@bishabosha bishabosha added the release-notes Should be mentioned in the release notes label Nov 8, 2023
@Kordyjan Kordyjan added this to the 3.4.0 milestone Dec 20, 2023
@bishabosha
Copy link
Member Author

@Kordyjan since you backported #18137 there's probably no barrier to this one

WojciechMazur added a commit that referenced this pull request Jun 23, 2024
Backports #18739 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. release-notes Should be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

report compilation progress via xsbti.compile.CompileProgress
4 participants