Worksheet mode in Dotty IDE #5102

Duhemm · 2018-09-13T08:44:40Z

This is a work in progress to support worksheets in Dotty IDE. The worksheets are backed by the REPL.

Still missing:

Some details:

The worksheets use the REPL
Evaluation is triggered when the worksheet is saved
Results are brought back to VSCode using window/logMessage. They are encoded as:
[URI][line in source that produced the output]:[output].

Duhemm · 2018-09-14T17:08:38Z

Rebased and fixed syntax highlighting.

There's an issue with creating a worksheet and letting vscode configure the IDE: When creating a worksheet in an empty, unconfigured project, vscode will ask the user whether it should configure the IDE. After selecting yes, sbt will set the scalaVersion and run configureIDE, which will produce a json file containing only [ ], and the language server will later crash.

Everything works fine when the language server is configured by creating a .scala file.

smarter · 2018-09-14T17:18:55Z

I can have a look at what configureIDE is doing

smarter · 2018-09-15T15:39:13Z

Should be fixed. Also I've noticed that every line with an expression in the worksheet gets a warning a pure expression does nothing in statement position, we need some way to silence this.

smarter · 2018-09-15T16:35:04Z

language-server/src/dotty/tools/languageserver/DottyLanguageServer.scala

+
+    val (text, positionMapper) =
+      if (worksheetMode) (wrapWorksheet(document.getText), worksheetPositionMapper _)
+      else (document.getText, identity[SourcePosition] _)


I would rather make this parameter an Option[Position => Position] so we can pass None here.

smarter · 2018-09-15T16:38:54Z

vscode-dotty/package-lock.json

@@ -92,7 +92,7 @@
      "integrity": "sha1-mjRBDk9OPaI96jdb5b5w8kd47Dk=",
      "dev": true,
      "requires": {
-        "array-uniq": "^1.0.1"
+        "array-uniq": "1.0.3"


I think your npm is doing something weird because mine is reverting these changes. Try upgrading to the latest npm (6.4.1)

MasseGuillaume · 2018-09-18T14:26:14Z

language-server/src/dotty/tools/languageserver/Worksheet.scala

+            }
+
+            try task.get(0, TimeUnit.SECONDS)
+            catch { case _: Throwable => state }


MasseGuillaume · 2018-09-18T14:51:31Z

language-server/src/dotty/tools/languageserver/Worksheet.scala

+                Thread.sleep(checkCancelledDelayMs)
+              } catch {
+                case _: CancellationException =>
+                  task.cancel(true)


this will throw a CancellationException as well.

MasseGuillaume · 2018-09-18T14:55:21Z

language-server/src/dotty/tools/languageserver/Worksheet.scala

+              }
+            }
+
+            try task.get(0, TimeUnit.SECONDS)


Maybe make this configurable, the default can be infinite. Otherwise bound it to the configured timeout.

MasseGuillaume · 2018-09-18T15:22:08Z

language-server/test/dotty/tools/languageserver/WorksheetTest.scala

+
+class WorksheetTest {
+
+  @Test def evaluateExpression: Unit = {


I would add a bunch of test from Scastie: https://github.com/scalacenter/scastie/blob/master/sbt-runner/src/test/scala/com.olegych.scastie.sbt/SbtActorTest.scala

multiple while(true) { }

exceptions 1 / 0

utf-8 encoding

System.err

initialization issues

looks like we're still missing test cases with while(true)

We have it in worksheetCancel:
https://github.com/lampepfl/dotty/pull/5102/files#diff-99d60ad8fb95377d0449fe3f60e1e87bR196

smarter · 2018-09-27T14:01:17Z

language-server/src/dotty/tools/languageserver/DottyLanguageServer.scala

@@ -382,7 +382,7 @@ class DottyLanguageServer extends LanguageServer
    else {
      val refs = Interactive.namedTrees(uriTrees, Include.references | Include.overriding, sym)
      (for {
-        ref <- refs
+        ref <- refs if !ref.tree.symbol.isConstructor


primary constructors are synthetic, but secondary constructors like def this() = this(1) are not

smarter · 2018-09-27T14:04:01Z

compiler/src/dotty/tools/dotc/interactive/Interactive.scala

-    namedTrees(trees, includeReferences, _.show.toString.contains(nameSubstring))
+   (implicit ctx: Context): List[SourceTree] = {
+    val predicate: NameTree => Boolean =
+      if (includeReferences) _.show.contains(nameSubstring)


I think the code was broken before, but I don't understand why now we still have to print the whole tree in some situations. This does not match the documentation comment above, and I don't see why we would ever want to do this.

I removed includeReferences from this function as it was never set. I have another fix that collects the names from the tree and check whether there exists one that contains the substring if you prefer to keep it.

smarter · 2018-09-27T14:11:46Z

language-server/src/dotty/tools/languageserver/Worksheet.scala

@@ -31,59 +37,59 @@ object Worksheet {
               sendMessage: String => Unit,
               cancelChecker: CancelChecker)(
      implicit ctx: Context): Unit = {
+
+    val replMain = dotty.tools.repl.WorksheetMain.getClass.getName.init
+    val classpath = sys.props("java.class.path")


Shouldn't the classpath used for the worksheet be the same classpath that the language server uses for the InteractiveDriver for the current directory ?

This one is the JVM classpath that runs the REPL, the REPL receives ctx.settings.classpath.value as classpath.

smarter · 2018-10-01T08:56:34Z

language-server/src/dotty/tools/languageserver/DottyLanguageServer.scala

-  override def didSave(params: DidSaveTextDocumentParams): Unit =
-    /*thisServer.synchronized*/ {}
-
+  override def didSave(params: DidSaveTextDocumentParams): Unit = {


Put thisServer.synchronized around since this isn't thread-safe.

smarter · 2018-10-01T09:06:23Z

language-server/src/dotty/tools/languageserver/Worksheet.scala

+private object Evaluator {
+
+  private val javaExec: Option[String] = {
+    val isWindows = sys.props("os.name").toLowerCase().indexOf("win") >= 0


https://www.scala-lang.org/api/2.12.7/scala/util/Properties$.html has isWin and javaHome

smarter · 2018-10-01T09:09:39Z

language-server/src/dotty/tools/languageserver/Worksheet.scala

+ *
+ * @param stream The stream of messages coming out of the REPL.
+ */
+private class ReplReader(stream: InputStream) extends Thread {


That's a lot of classes in one file, maybe make a worksheet/ subdirectory instead?

smarter · 2018-10-01T09:41:14Z

vscode-dotty/src/worksheet.ts

@@ -0,0 +1,346 @@
+'use strict'


This isn't actually necessary with recent versions of typescript (it's automatically inserted in the generated js)

smarter · 2018-10-01T09:43:01Z

vscode-dotty/src/worksheet.ts

+import { LanguageClient } from 'vscode-languageclient'
+
+/** All decorations that have been added so far */
+let worksheetDecorationTypes: Map<vscode.TextDocument, vscode.TextEditorDecorationType[]> = new Map<vscode.TextDocument, vscode.TextEditorDecorationType[]>()


I would make a class with fields instead of using global variables in this file.

smarter · 2018-10-02T18:06:14Z

language-server/src/dotty/tools/languageserver/worksheet/WorksheetMessages.scala

+import java.net.URI
+
+/** The parameter for the `worksheet/exec` request. */
+case class WorksheetExecParams(uri: URI)


We should add empty secondary constructors that just pass null/0 to every param like def this() = this(null) here, otherwise when Gson deserializes from json, it will need to create the class instance using sun.misc.Unsafe which seems to work but could be problematic on Java 9+

`Interactive.scala` offered an API to find all references to names given substrings of the names. This feature was unused. This worked by pretty printing the tree and looking whether it contained the substring. This is broken, because the substring may match parts that are not references (what if we look for `if`?) It's gone now.

Because the process running the worksheets is shared between worksheets (when possible), we need to synchronize worksheet evaluation.

It is used to represent the worksheets managed by vscode, instead of having several global variables.

Instead of relying on `textDocument/didSave` and `window/logMessage` like we did in the past.

They are used when deserializing from JSON.

We now hide the warning the pure expression in statement position when the pure expression is an immediate children of the worksheet wrapper.

Duhemm · 2018-10-09T08:39:58Z

Looks like there's been a regression in the way worksheet output is display, if I write:

def foo(x: Int): Int = x
in a worksheet then the decoration is truncated: Int): Int = 1

Thanks, I fixed it. It would be cool to have tests for VSCode at some point.................

Comments have been addressed

Duhemm added stat:wip area:ide labels Sep 13, 2018

Duhemm force-pushed the topic/worksheets branch 2 times, most recently from 812ba2a to 11f96fa Compare September 14, 2018 16:53

smarter reviewed Sep 15, 2018

View reviewed changes

MasseGuillaume reviewed Sep 18, 2018

View reviewed changes

Duhemm force-pushed the topic/worksheets branch from 64affd8 to 921d329 Compare September 27, 2018 13:58

smarter reviewed Sep 27, 2018

View reviewed changes

Duhemm force-pushed the topic/worksheets branch from 921d329 to 4155daf Compare September 27, 2018 14:08

smarter reviewed Sep 27, 2018

View reviewed changes

Duhemm force-pushed the topic/worksheets branch from 149d2cd to ea33432 Compare September 28, 2018 09:42

Duhemm added stat:needs review and removed stat:wip labels Sep 28, 2018

Duhemm changed the title ~~(WIP) worksheet mode in Dotty IDE~~ Worksheet mode in Dotty IDE Sep 28, 2018

smarter reviewed Oct 1, 2018

View reviewed changes

Duhemm force-pushed the topic/worksheets branch from e79158a to 6b68c3d Compare October 1, 2018 09:37

smarter reviewed Oct 1, 2018

View reviewed changes

OlivierBlanvillain assigned smarter Oct 2, 2018

smarter reviewed Oct 2, 2018

View reviewed changes

Duhemm added 24 commits October 9, 2018 09:58

Support go to def, rename, etc. in worksheets

185e765

Add worksheet cancellation test

79860e0

Test exception in worksheet evaluation

c3d1367

Exclude only primary constructors

4223ecb

Respect -color:never with typedef in REPL

ae10994

Support System.exit in worksheet

ced2868

Check that worksheets capture stderr

f4cbbca

Don't wait for messages after stream is closed

0eb7fc9

Fix synchronization issue with worksheets

c58b314

Because the process running the worksheets is shared between worksheets (when possible), we need to synchronize worksheet evaluation.

Split worksheet implementation

ce04cc8

Check if evaluator runs on Windows

e6b3abd

Fix worksheet tests for Windows

471a059

vscode: Add Worksheet class

701e8ea

It is used to represent the worksheets managed by vscode, instead of having several global variables.

Remove worksheets when closed

a3bdde1

Add worksheet/exec and worksheet/publishOutput

de5c28e

Instead of relying on `textDocument/didSave` and `window/logMessage` like we did in the past.

Add empty secondary ctors to worksheet messages

0689ef3

They are used when deserializing from JSON.

Address review comments

9ed6971

Remove only lines added to fit output

9666d23

Cancel worksheet execution on didChange

a3efd48

Error message for pure expr in stat position

2b7e8cf

Filter pure expression warning in worksheets

dba854d

We now hide the warning the pure expression in statement position when the pure expression is an immediate children of the worksheet wrapper.

Factor WorksheetWrapper name to StdNames

a2569c2

Remove unnecessary cast

9a19df7

Duhemm force-pushed the topic/worksheets branch from 237739e to 9a19df7 Compare October 9, 2018 08:38

smarter merged commit f693e04 into scala:master Oct 9, 2018

allanrenucci deleted the topic/worksheets branch October 9, 2018 12:05

smarter mentioned this pull request Nov 3, 2018

Document the worksheet mode #5380

Closed

Worksheet mode in Dotty IDE #5102

Worksheet mode in Dotty IDE #5102

Uh oh!

Conversation

Duhemm commented Sep 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Duhemm commented Sep 14, 2018

Uh oh!

smarter commented Sep 14, 2018

Uh oh!

smarter commented Sep 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Duhemm commented Oct 9, 2018

Uh oh!

Uh oh!

Duhemm commented Sep 13, 2018 •

edited

Loading