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

Implement basic worksheet mode using mdoc and text decorations #1041

Merged
merged 18 commits into from Nov 10, 2019

Conversation

@olafurpg
Copy link
Member

olafurpg commented Nov 3, 2019

Previously, there was no way for users to interactively explore APIs and evaluate small programs when coding with Metals. This commit adds a new "worksheet mode" for files with the extension .worksheet.sc. Programs with this file extension are evaluated on file save and with evaluated results are displayed inline alongside the code using a new "Decoration Protocol".

Snappy feedback

Simple code usually compiles and evaluates pretty fast, even if you do a bit of I/O

2019-11-03 17 33 09

Completions, type at point, parameter hints work as expected

Robust against slow programs

Slow running programs can be cancelled with a single click, even infinite loops.

2019-11-03 20 06 54

Compile errors work as expected

2019-11-03 18 13 34

Infinite stream are evaluated lazily

Screenshot 2019-11-03 at 19 23 23

Thanks to http://www.lihaoyi.com/PPrint/

Runtime crashes are reported with red squiggles

A full stack trace is included in the error message.

Screenshot 2019-11-03 at 18 19 29

Hover message for large values

Large values are truncated to a single line and a restricted column width. Hover over the decoration to see the full evaluated value.
2019-11-03 20 22 35
Screenshot 2019-11-03 at 19 24 23

Acknowledgements

Kudos to @Duhemm for pioneering Scala worksheets in the Dotty Language Server (http://dotty.epfl.ch/docs/usage/worksheet-mode-implementation-details.html). The implementation in this PR is heavily inspired by worksheets in the Dotty Language Server.

Big thanks to @gabro for exploring decorations in the VS Code API and giving lots of useful feedback.

Fixes #1040.

Previously, there as no way for users to interactively explore APIs and
evaluate small programs. This commit adds a new "worksheet mode" for
files with the extension ".worksheet.sc". These files allow toplevel
statements and the statements are evaluated on file save, with evaluated
results displayed inline alongside the code using a new "Decoration
Protocol", a LSP extension.

Fixes #1040.
@olafurpg olafurpg changed the title Implement basic worksheet mode using mdoc and decorations. Implement basic worksheet mode using mdoc and text decorations Nov 3, 2019
@olafurpg

This comment has been minimized.

Copy link
Member Author

olafurpg commented Nov 3, 2019

There are several features that are not implemented in this PR but would be super cool to support in the future

  • evaluate parts of the document (e.g., notebook cells) to avoid repeating slow computation. I have some ideas how to support this with mdoc while ensuring that cells always evaluate in the right order.
  • displaying rich media objects (plots, charts, progress bars, etc). This might be blocked by microsoft/vscode#3220. We could maybe build on top of the Notebook support for Python in VS Code 🤔
  • writing documentation in the script. I believe it would be quite straightforward to extend the mdoc-based implementation to support "markdown worksheets" in a similar fashion as R Markdown https://rmarkdown.rstudio.com/
@olafurpg olafurpg requested review from tgodzik and gabro Nov 3, 2019
olafurpg added a commit to olafurpg/metals-vscode that referenced this pull request Nov 3, 2019
This commit enables Metals to support worksheets.
See scalameta/metals#1041
olafurpg added a commit to olafurpg/metals-vscode that referenced this pull request Nov 3, 2019
This commit enables Metals to support worksheets.
See scalameta/metals#1041
@olafurpg

This comment has been minimized.

Copy link
Member Author

olafurpg commented Nov 3, 2019

Accompanying changes to the VS Code extension scalameta/metals-vscode#159

docs/editors/new-editor.md Outdated Show resolved Hide resolved
@ckipp01 ckipp01 mentioned this pull request Nov 4, 2019
Copy link
Member

gabro left a comment

Pretty cool! I'm excited to have this in Metals 🚀
I've left a few comments here and there

docs/editors/decoration-protocol.md Outdated Show resolved Hide resolved
docs/editors/new-editor.md Outdated Show resolved Hide resolved
docs/editors/decoration-protocol.md Show resolved Hide resolved
ThemableDecorationInstanceRenderOptions(
after = ThemableDecorationAttachmentRenderOptions(
contentText,
color = "green",

This comment has been minimized.

Copy link
@gabro

gabro Nov 4, 2019

Member

It's fine for this first iteration, but I wonder if we can let the editors decide on how to render these.

It feels strange to decide on styling details server-side.

This comment has been minimized.

Copy link
@olafurpg

olafurpg Nov 6, 2019

Author Member

I agree, but my understanding is that there's no way for us to access the colors for things like comments microsoft/vscode#32813

This comment has been minimized.

Copy link
@olafurpg

olafurpg Nov 6, 2019

Author Member

We could hardcode this value in the client but I'm not sure that's any better 🤔 Or maybe it is better to send some enum to the client

object Colors {
  val comment = 1
  val identifier = 2
}

and then have the VS Code client translate that into colors

This comment has been minimized.

Copy link
@gabro

gabro Nov 6, 2019

Member

My first idea would be to use tmLanguage classes, but then I don’t know how easy is for the client to use them

_ = {
if (!isSupported) {
scribe.warn(
s"worksheet: unsupported Scala version '${scalaVersion}', to fix this problem use Scala version '${BuildInfo.scala212}' instead."

This comment has been minimized.

Copy link
@gabro

gabro Nov 4, 2019

Member

do worksheets only work with Scala 2.12? If not, shouldn't we recommend 2.13 instead?

This comment has been minimized.

Copy link
@olafurpg

olafurpg Nov 6, 2019

Author Member

Worksheets currently only work with 2.12.10. To support 2.11/2.13 I would prefer to move some of the logic here into mdoc and publish a stable mdoc-interfaces Java library. I felt it would be better to leave that for a separate PR

@@ -384,7 +385,11 @@ class MetalsGlobal(
case _ => code
}
val unit = newCompilationUnit(codeWithCursor, filename)
val richUnit = new RichCompilationUnit(unit.source)
val source =
if (filename.endsWith(".sc"))

This comment has been minimized.

Copy link
@gabro

gabro Nov 4, 2019

Member

filename.isScalaScript for consistency?

This comment has been minimized.

Copy link
@olafurpg

olafurpg Nov 6, 2019

Author Member

Done.

olafurpg added 4 commits Nov 4, 2019
Previously, worksheets wouldn't pick up the latest classpath in the
build. Now, we reset the worksheet classpath when build targets finish
compilation.
Previously, we had custom queue for evaluating worksheets. Now,
worksheets are evaluated as part of the normal compilation.
* Display correct timer in slowTask (start at 4 seconds)
* Use custom execution context for stopping threads to
  avoid blocking other features like status bar.
Copy link
Collaborator

tgodzik left a comment

This looks amazing, just a few questions about the code. The rendering logic doesn't seem very maintainable and wasn't able to properly review that.

I would also look into how we can provide worksheet experience for other clients than VS Code out of the box. I was thinking about code lenses, but that might not be the best idea after talking with @olafurpg . What about using workspaceEdit with adding the results as comments? This is also not ideal, but I worry about adding LSP extensions that there will be no one to actually implement those for anything else.

We could default to mechanism like:

  • save file
  • compiles and produces comments as workspace edits
  • on save checks whether we need to recompile or only the added comments are saved

The problem is that workspaceEdit will not save the file, so somehow we need to make the experience better for users.

@@ -1610,6 +1640,31 @@ class MetalsLanguageServer(
)
}

private def onWorksheetChangedUnbatched(

This comment has been minimized.

Copy link
@tgodzik

tgodzik Nov 4, 2019

Collaborator

Can we move this inside the WorksheetProvider ?

This comment has been minimized.

Copy link
@olafurpg

olafurpg Nov 6, 2019

Author Member

I simplified this implementation a bit. For consistency with onBuildChanged I'd like to keep it in this file for now

)
}

private def evaluateWorksheet(

This comment has been minimized.

Copy link
@tgodzik

tgodzik Nov 4, 2019

Collaborator

This method looks a bit complex - can we move some functionalities to separate methods? It's a bit hard to review it, since a lot of the logic seems rendering specific.

This comment has been minimized.

Copy link
@olafurpg

olafurpg Nov 6, 2019

Author Member

I agree, I should probably have opened a "draft PR". I had already refactored this method into smaller methods before seeing your review

olafurpg added 3 commits Nov 4, 2019
Previously, a syntax error in `*.sc` files would invalidate the
published type errors. Now, type errors are mixed together with syntax
errors julist like for `*.scala` files.
Makes the code more readable.
Copy link
Member Author

olafurpg left a comment

Thank you so much for the review!

docs/editors/decoration-protocol.md Outdated Show resolved Hide resolved
docs/editors/decoration-protocol.md Show resolved Hide resolved
docs/editors/new-editor.md Outdated Show resolved Hide resolved
ThemableDecorationInstanceRenderOptions(
after = ThemableDecorationAttachmentRenderOptions(
contentText,
color = "green",

This comment has been minimized.

Copy link
@olafurpg

olafurpg Nov 6, 2019

Author Member

We could hardcode this value in the client but I'm not sure that's any better 🤔 Or maybe it is better to send some enum to the client

object Colors {
  val comment = 1
  val identifier = 2
}

and then have the VS Code client translate that into colors

_ = {
if (!isSupported) {
scribe.warn(
s"worksheet: unsupported Scala version '${scalaVersion}', to fix this problem use Scala version '${BuildInfo.scala212}' instead."

This comment has been minimized.

Copy link
@olafurpg

olafurpg Nov 6, 2019

Author Member

Worksheets currently only work with 2.12.10. To support 2.11/2.13 I would prefer to move some of the logic here into mdoc and publish a stable mdoc-interfaces Java library. I felt it would be better to leave that for a separate PR

@@ -384,7 +385,11 @@ class MetalsGlobal(
case _ => code
}
val unit = newCompilationUnit(codeWithCursor, filename)
val richUnit = new RichCompilationUnit(unit.source)
val source =
if (filename.endsWith(".sc"))

This comment has been minimized.

Copy link
@olafurpg

olafurpg Nov 6, 2019

Author Member

Done.

@olafurpg olafurpg requested a review from gabro Nov 6, 2019
@olafurpg olafurpg requested a review from tgodzik Nov 6, 2019
@olafurpg

This comment has been minimized.

Copy link
Member Author

olafurpg commented Nov 7, 2019

What about using workspaceEdit with adding the results as comments? This is also not ideal, but I worry about adding LSP extensions that there will be no one to actually implement those for anything else.

@tgodzik I think this approach is worth exploring as a fallback for other editors. We might be able to include a magic marker for the inserted comments so it's easy to tell which comments are generated by worksheets and which comments are written by the user.

Copy link
Collaborator

tgodzik left a comment

Just a few more comments. Thanks for splitting the rendering function! Looks much more readable now. 😍

Copy link
Member

gabro left a comment

Overall LGTM 👍 I only have a couple more questions

docs/editors/new-editor.md Outdated Show resolved Hide resolved
Copy link
Member Author

olafurpg left a comment

@gabro @tgodzik Thank you for the great feedback!

@olafurpg olafurpg force-pushed the olafurpg:worksheets branch from cbe02c9 to 337d770 Nov 9, 2019
@tgodzik
tgodzik approved these changes Nov 9, 2019
Copy link
Collaborator

tgodzik left a comment

LGTM! Pretty exciting to have this in Metals! 🎉

@gabro

This comment has been minimized.

This comment has been minimized.

Copy link
Member Author

olafurpg replied Nov 9, 2019

fixed!

@gabro
gabro approved these changes Nov 9, 2019
Copy link
Member

gabro left a comment

🕺💃

@olafurpg

This comment has been minimized.

Copy link
Member Author

olafurpg commented Nov 9, 2019

I would normally ignore the flaky test failure (it was passing before with the same code) but the failing test case is from WorksheetLspSuite

 X tests.worksheets.WorksheetLspSuite.completion 4510ms 
No more data in the server stdin, exiting...
  tests.TestFailedException: failed assertion at D:\a\metals\metals\tests\unit\src\test\scala\tests\worksheets\WorksheetLspSuite.scala:30
failed assertion at D:\a\metals\metals\tests\mtest\src\main\scala\tests\DiffAssertions.scala:24
Obtained empty output!
  tests.DiffAssertions$.colored(DiffAssertions.scala:107)
  tests.BaseSuite.assertNoDiff(BaseSuite.scala:100)
  tests.worksheets.WorksheetLspSuite$.$anonfun$new$4(WorksheetLspSuite.scala:30)
  scala.util.Success.$anonfun$map$1(Try.scala:255)
  scala.util.Success.map(Try.scala:213)
  scala.concurrent.Future.$anonfun$map$1(Future.scala:292)

I'll try to investigate...

Maybe the worksheet will get evaluated more reliably with a `didSave`
@olafurpg

This comment has been minimized.

Copy link
Member Author

olafurpg commented Nov 10, 2019

Ignoring unrelated flaky test failure

X tests.TreeViewLspSuite.no-op 2280ms 
  tests.TestFailedException: failed assertion at /home/runner/work/metals/metals/tests/unit/src/test/scala/tests/TreeViewLspSuite.scala:487
===========
=> Obtained
===========
    """|b/ +


=======
=> Diff
=======
--- obtained
+++ expected
@@ -1,1 +1,1 @@
-b/ +
+
@olafurpg olafurpg merged commit 90bc490 into scalameta:master Nov 10, 2019
8 of 9 checks passed
8 of 9 checks passed
Windows unit tests
Details
Linux unit tests Linux unit tests
Details
Sbt integration
Details
Maven integration
Details
Gradle integration
Details
Mill integration
Details
Slow tests
Details
Scala cross tests
Details
Scalafmt/Scalacheck/Docs
Details
@olafurpg olafurpg deleted the olafurpg:worksheets branch Nov 10, 2019
@olafurpg

This comment has been minimized.

Copy link
Member Author

olafurpg commented Nov 10, 2019

For people who want to try out the new worksheets

  • Upgrade VS Code extension, may require a restart to pick up the new release
  • Set "Server version" setting to 0.7.6+208-90bc4909-SNAPSHOT

Feedback welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.