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

Worksheet mode in Dotty IDE #5102

Merged
merged 38 commits into from Oct 9, 2018

Conversation

Projects
None yet
4 participants
@Duhemm
Copy link
Contributor

Duhemm commented Sep 13, 2018

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

Still missing:

  • Cancel execution
  • Show execution progress
  • Tests on Dotty IDE side
  • Tests on VSCode side
  • Figuring out some details with syntax highlighting

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].

ezgif-5-6bcb5f85eb

@Duhemm Duhemm force-pushed the dotty-staging:topic/worksheets branch 2 times, most recently from 812ba2a to 11f96fa Sep 14, 2018

@Duhemm

This comment has been minimized.

Copy link
Contributor

Duhemm commented Sep 14, 2018

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

This comment has been minimized.

Copy link
Member

smarter commented Sep 14, 2018

I can have a look at what configureIDE is doing

@smarter

This comment has been minimized.

Copy link
Member

smarter commented Sep 15, 2018

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.


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

This comment has been minimized.

@smarter

smarter Sep 15, 2018

Member

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

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

This comment has been minimized.

@smarter

smarter Sep 15, 2018

Member

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

}

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

This comment has been minimized.

@MasseGuillaume

MasseGuillaume Sep 18, 2018

Contributor

NonFatal

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

This comment has been minimized.

@MasseGuillaume

MasseGuillaume Sep 18, 2018

Contributor

this will throw a CancellationException as well.

}
}

try task.get(0, TimeUnit.SECONDS)

This comment has been minimized.

@MasseGuillaume

MasseGuillaume Sep 18, 2018

Contributor

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


class WorksheetTest {

@Test def evaluateExpression: Unit = {

This comment has been minimized.

@MasseGuillaume

MasseGuillaume Sep 18, 2018

Contributor

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

This comment has been minimized.

@smarter

smarter Oct 8, 2018

Member

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

This comment has been minimized.

@Duhemm Duhemm force-pushed the dotty-staging:topic/worksheets branch from 64affd8 to 921d329 Sep 27, 2018

@@ -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

This comment has been minimized.

@smarter

smarter Sep 27, 2018

Member

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

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

This comment has been minimized.

@smarter

smarter Sep 27, 2018

Member

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.

This comment has been minimized.

@Duhemm

Duhemm Sep 28, 2018

Contributor

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.

@Duhemm Duhemm force-pushed the dotty-staging:topic/worksheets branch from 921d329 to 4155daf Sep 27, 2018

@@ -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")

This comment has been minimized.

@smarter

smarter Sep 27, 2018

Member

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 comment has been minimized.

@Duhemm

Duhemm Sep 28, 2018

Contributor

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

@Duhemm Duhemm force-pushed the dotty-staging:topic/worksheets branch from 149d2cd to ea33432 Sep 28, 2018

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

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

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

override def didSave(params: DidSaveTextDocumentParams): Unit = {

This comment has been minimized.

@smarter

smarter Oct 1, 2018

Member

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

private object Evaluator {

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

This comment has been minimized.

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

This comment has been minimized.

@smarter

smarter Oct 1, 2018

Member

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

@Duhemm Duhemm force-pushed the dotty-staging:topic/worksheets branch from e79158a to 6b68c3d Oct 1, 2018

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

This comment has been minimized.

@smarter

smarter Oct 1, 2018

Member

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

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[]>()

This comment has been minimized.

@smarter

smarter Oct 1, 2018

Member

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

import java.net.URI

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

This comment has been minimized.

@smarter

smarter Oct 2, 2018

Member

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+

Duhemm added some commits Sep 27, 2018

IDE: Remove unused substring reference search
`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.
Fix synchronization issue with worksheets
Because the process running the worksheets is shared between worksheets
(when possible), we need to synchronize worksheet evaluation.
vscode: Add `Worksheet` class
It is used to represent the worksheets managed by vscode, instead of
having several global variables.
Add `worksheet/exec` and `worksheet/publishOutput`
Instead of relying on `textDocument/didSave` and `window/logMessage`
like we did in the past.
Add empty secondary ctors to worksheet messages
They are used when deserializing from JSON.
Filter pure expression warning in worksheets
We now hide the warning the pure expression in statement position when
the pure expression is an immediate children of the worksheet wrapper.

@Duhemm Duhemm force-pushed the dotty-staging:topic/worksheets branch from 237739e to 9a19df7 Oct 9, 2018

@Duhemm

This comment has been minimized.

Copy link
Contributor

Duhemm commented Oct 9, 2018

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

@smarter smarter merged commit f693e04 into lampepfl:master Oct 9, 2018

2 checks passed

CLA User signed CLA
Details
continuous-integration/drone/pr the build was successful
Details

@allanrenucci allanrenucci deleted the dotty-staging:topic/worksheets branch Oct 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment