From 9ed9357c6c3d1e2f8701506ba9cad0a455e79db0 Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Tue, 16 Oct 2018 18:31:17 +0200 Subject: [PATCH 1/4] Use a CodeLens to run/cancel the worksheet This makes it possible to run the worksheet without saving the document or manually running the dotty.worksheet.run command. The new UI is also less distracting since you don't get a notification pop-up every time the worksheet is being run. --- vscode-dotty/package.json | 2 +- vscode-dotty/src/worksheet.ts | 175 +++++++++++++++++++++++++++------- 2 files changed, 144 insertions(+), 33 deletions(-) diff --git a/vscode-dotty/package.json b/vscode-dotty/package.json index 68923c8c3127..3d76a617f466 100644 --- a/vscode-dotty/package.json +++ b/vscode-dotty/package.json @@ -59,7 +59,7 @@ }, { "command": "dotty.worksheet.cancel", - "title": "Cancel worksheet evaluation", + "title": "Cancel running worksheet", "category": "Scala" } ], diff --git a/vscode-dotty/src/worksheet.ts b/vscode-dotty/src/worksheet.ts index 3e2177b2e33f..240853d72ba0 100644 --- a/vscode-dotty/src/worksheet.ts +++ b/vscode-dotty/src/worksheet.ts @@ -1,5 +1,8 @@ import * as vscode from 'vscode' -import { TextEdit } from 'vscode' +import { + CancellationToken, CancellationTokenSource, CodeLens, CodeLensProvider, Command, + Event, EventEmitter, ProgressLocation, Range, TextDocument, TextEdit +} from 'vscode' import { asWorksheetRunParams, WorksheetRunRequest, WorksheetRunParams, WorksheetRunResult, @@ -14,11 +17,14 @@ import { Disposable } from 'vscode-jsonrpc' */ export const worksheetRunKey = "dotty.worksheet.run" -/** A worksheet managed by vscode */ -class Worksheet { +/** + * The command key for cancelling a running worksheet. Exposed to users as + * `Cancel running worksheet`. + */ +export const worksheetCancelKey = "dotty.worksheet.cancel" - constructor(readonly document: vscode.TextDocument, readonly client: BaseLanguageClient) { - } +/** A worksheet managed by vscode */ +class Worksheet implements Disposable { /** All decorations that have been added so far */ private decorationTypes: vscode.TextEditorDecorationType[] = [] @@ -32,7 +38,29 @@ class Worksheet { /** The minimum margin to add so that the decoration is shown after all text. */ private margin: number = 0 - /** Remove all decorations and resets this worksheet. */ + private readonly _onDidStateChange: EventEmitter = new EventEmitter() + /** This event is fired when the worksheet starts or stops running. */ + readonly onDidStateChange: Event = this._onDidStateChange.event + + /** + * If this is not null, this can be used to signal cancellation of the + * currently running worksheet. + */ + private canceller?: CancellationTokenSource = undefined + + constructor(readonly document: vscode.TextDocument, readonly client: BaseLanguageClient) { + } + + dispose() { + this.reset() + if (this.canceller) { + this.canceller.dispose() + this.canceller = undefined + } + this._onDidStateChange.dispose() + } + + /** Remove all decorations, and resets this worksheet. */ private reset(): void { this.decorationTypes.forEach(decoration => decoration.dispose()) this.insertedLines = 0 @@ -51,26 +79,58 @@ class Worksheet { return edits } + /** If this worksheet is currently being run, cancel the run. */ + cancel(): void { + if (this.canceller) { + this.canceller.cancel() + this.canceller = undefined + + this._onDidStateChange.fire() + } + } + + /** Is this worksheet currently being run ? */ + isRunning(): boolean { + return this.canceller != undefined + } + /** - * Run the worksheet in `document`, display a progress bar during the run. + * Run the worksheet in `document`, if a previous run is in progress, it is + * cancelled first. */ run(): Promise { - return new Promise((resolve, reject) => { + this.cancel() + const canceller = new CancellationTokenSource() + const token = canceller.token + // This ensures that isRunning() returns true. + this.canceller = canceller + + this._onDidStateChange.fire() + + return new Promise(resolve => { const textEdits = this.prepareRun() const edit = new vscode.WorkspaceEdit() edit.set(this.document.uri, textEdits) vscode.workspace.applyEdit(edit).then(editSucceeded => { - if (editSucceeded) { - return resolve(vscode.window.withProgress({ - location: vscode.ProgressLocation.Notification, - title: "Run the worksheet", - cancellable: true - }, (_, token) => this.client.sendRequest( + if (editSucceeded && !token.isCancellationRequested) + resolve(vscode.window.withProgress({ + location: ProgressLocation.Window, + title: "Running worksheet" + }, () => this.client.sendRequest( WorksheetRunRequest.type, asWorksheetRunParams(this.document), token ))) - } else - reject() + else + resolve({ success: false }) }) + }).then(result => { + canceller.dispose() + if (this.canceller === canceller) { // If false, a new run has already started + // This ensures that isRunning() returns false. + this.canceller = undefined + + this._onDidStateChange.fire() + } + return result }) } @@ -210,13 +270,20 @@ class Worksheet { } export class WorksheetProvider implements Disposable { - private disposables: Disposable[] = [] private worksheets: Map = new Map() + private readonly _onDidWorksheetStateChange: EventEmitter = new EventEmitter() + /** This event is fired when a worksheet starts or stops running. */ + readonly onDidWorksheetStateChange: Event = this._onDidWorksheetStateChange.event + + private disposables: Disposable[] = [ this._onDidWorksheetStateChange ] constructor( readonly client: BaseLanguageClient, - readonly documentSelectors: vscode.DocumentSelector[]) { + readonly documentSelector: vscode.DocumentSelector) { + const codeLensProvider = new WorksheetCodeLensProvider(this) this.disposables.push( + codeLensProvider, + vscode.languages.registerCodeLensProvider(documentSelector, codeLensProvider), vscode.workspace.onWillSaveTextDocument(event => { const worksheet = this.worksheetFor(event.document) if (worksheet) { @@ -231,12 +298,17 @@ export class WorksheetProvider implements Disposable { } }), vscode.workspace.onDidCloseTextDocument(document => { - if (this.isWorksheet(document)) { + const worksheet = this.worksheetFor(document) + if (worksheet) { + worksheet.dispose() this.worksheets.delete(document) } }), vscode.commands.registerCommand(worksheetRunKey, () => { - this.runWorksheetCommand() + this.callOnActiveWorksheet(w => w.run()) + }), + vscode.commands.registerCommand(worksheetCancelKey, () => { + this.callOnActiveWorksheet(w => w.cancel()) }) ) client.onNotification(WorksheetPublishOutputNotification.type, params => { @@ -245,17 +317,19 @@ export class WorksheetProvider implements Disposable { } dispose() { - this.disposables.forEach(d => d.dispose()); - this.disposables = []; + this.worksheets.forEach(d => d.dispose()) + this.worksheets.clear() + this.disposables.forEach(d => d.dispose()) + this.disposables = [] } /** Is this document a worksheet? */ private isWorksheet(document: vscode.TextDocument): boolean { - return this.documentSelectors.some(sel => vscode.languages.match(sel, document) > 0) + return vscode.languages.match(this.documentSelector, document) > 0 } /** If `document` is a worksheet, create a new worksheet for it, or return the existing one. */ - private worksheetFor(document: vscode.TextDocument): Worksheet | undefined { + worksheetFor(document: vscode.TextDocument): Worksheet | undefined { if (!this.isWorksheet(document)) return else { const existing = this.worksheets.get(document) @@ -264,20 +338,21 @@ export class WorksheetProvider implements Disposable { } else { const newWorksheet = new Worksheet(document, this.client) this.worksheets.set(document, newWorksheet) + this.disposables.push( + newWorksheet.onDidStateChange(() => this._onDidWorksheetStateChange.fire(newWorksheet)) + ) return newWorksheet } } } - /** - * The VSCode command executed when the user select `Run worksheet`. - */ - private runWorksheetCommand() { - const editor = vscode.window.activeTextEditor - if (editor) { - const worksheet = this.worksheetFor(editor.document) + /** If the active text editor contains a worksheet, apply `f` to it. */ + private callOnActiveWorksheet(f: (_: Worksheet) => void) { + let document = vscode.window.activeTextEditor && vscode.window.activeTextEditor.document + if (document) { + const worksheet = this.worksheetFor(document) if (worksheet) { - worksheet.run() + f(worksheet) } } } @@ -302,3 +377,39 @@ export class WorksheetProvider implements Disposable { } } } + +class WorksheetCodeLensProvider implements CodeLensProvider, Disposable { + private readonly _onDidChangeCodeLenses: EventEmitter = new EventEmitter() + readonly onDidChangeCodeLenses: Event = this._onDidChangeCodeLenses.event + + private disposables: Disposable[] = [ this._onDidChangeCodeLenses ] + + constructor(readonly worksheetProvider: WorksheetProvider) { + this.disposables.push( + worksheetProvider.onDidWorksheetStateChange(() => this._onDidChangeCodeLenses.fire()) + ) + } + + dispose() { + this.disposables.forEach(d => d.dispose()) + this.disposables = [] + } + + private readonly runCommand: Command = { + command: worksheetRunKey, + title: "Run this worksheet" + } + + private readonly cancelCommand: Command = { + command: worksheetCancelKey, + title: "Worksheet running, click to cancel" + } + + provideCodeLenses(document: TextDocument, token: CancellationToken) { + const worksheet = this.worksheetProvider.worksheetFor(document) + if (worksheet) { + const cmd = worksheet.isRunning() ? this.cancelCommand : this.runCommand + return [ new CodeLens(new Range(0, 0, 0, 0), cmd) ] + } + } +} From 0ec624a7ca07aa3ec0a2c67dcc73596e5b9219ff Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Wed, 17 Oct 2018 15:48:17 +0200 Subject: [PATCH 2/4] Remove worksheet cancellation on document change This is unreliable because we change the document by adding newlines when we receive a multi-line result for a given line of the worksheet --- .../src/dotty/tools/languageserver/DottyLanguageServer.scala | 4 ---- 1 file changed, 4 deletions(-) diff --git a/language-server/src/dotty/tools/languageserver/DottyLanguageServer.scala b/language-server/src/dotty/tools/languageserver/DottyLanguageServer.scala index 1fcfe3e1d3de..9fb8976a3f44 100644 --- a/language-server/src/dotty/tools/languageserver/DottyLanguageServer.scala +++ b/language-server/src/dotty/tools/languageserver/DottyLanguageServer.scala @@ -202,10 +202,6 @@ class DottyLanguageServer extends LanguageServer val uri = new URI(document.getUri) val worksheetMode = isWorksheet(uri) - if (worksheetMode) { - Option(worksheets.get(uri)).foreach(_.cancel(true)) - } - thisServer.synchronized { checkMemory() From 99ac0609ff72aa296b8d62859c4f05fec8d99168 Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Wed, 17 Oct 2018 16:37:29 +0200 Subject: [PATCH 3/4] Use a color that is theme-specific for the worksheet output This way we can be sure that our output is readable in all themes. --- vscode-dotty/src/worksheet.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/vscode-dotty/src/worksheet.ts b/vscode-dotty/src/worksheet.ts index 240853d72ba0..ec8e6c5a5152 100644 --- a/vscode-dotty/src/worksheet.ts +++ b/vscode-dotty/src/worksheet.ts @@ -193,7 +193,10 @@ class Worksheet implements Disposable { contentText: text, margin: `0px 0px 0px ${margin}ch`, fontStyle: "italic", - color: "light gray", + // It would make more sense to use the colors of commments in the + // current theme, but there's no API to access this currently + // (https://github.com/Microsoft/vscode/issues/32813). + color: new vscode.ThemeColor("terminal.ansiGreen"), } }) } From 8c3319c3d5aa18d8b9b90042851d62e0b0f1dd53 Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Wed, 17 Oct 2018 18:18:33 +0200 Subject: [PATCH 4/4] Run worksheets asynchronously A worksheet in an infinite loop shouldn't block the whole language server from responding to queries. We just need to be careful to lock the language server when operating on trees since these operations may not be thread-safe (for example, when a symbol gets forced). We could also allow running multiple worksheets at the same time but this commit doesn't implement that. --- .../languageserver/DottyLanguageServer.scala | 10 ++- .../src/dotty/tools/languageserver/Main.scala | 2 +- .../languageserver/worksheet/Evaluator.scala | 2 +- .../languageserver/worksheet/Worksheet.scala | 65 ++++++++++--------- .../worksheet/WorksheetService.scala | 47 ++++++-------- 5 files changed, 65 insertions(+), 61 deletions(-) diff --git a/language-server/src/dotty/tools/languageserver/DottyLanguageServer.scala b/language-server/src/dotty/tools/languageserver/DottyLanguageServer.scala index 9fb8976a3f44..88aab52053ae 100644 --- a/language-server/src/dotty/tools/languageserver/DottyLanguageServer.scala +++ b/language-server/src/dotty/tools/languageserver/DottyLanguageServer.scala @@ -107,7 +107,7 @@ class DottyLanguageServer extends LanguageServer if (Memory.isCritical()) CompletableFutures.computeAsync { _ => restart() } /** The driver instance responsible for compiling `uri` */ - def driverFor(uri: URI): InteractiveDriver = { + def driverFor(uri: URI): InteractiveDriver = thisServer.synchronized { val matchingConfig = drivers.keys.find(config => config.sourceDirectories.exists(sourceDir => new File(uri.getPath).getCanonicalPath.startsWith(sourceDir.getCanonicalPath))) @@ -133,10 +133,10 @@ class DottyLanguageServer extends LanguageServer CompletableFuture.completedFuture(new Object) } - def computeAsync[R](fun: CancelChecker => R): CompletableFuture[R] = + def computeAsync[R](fun: CancelChecker => R, synchronize: Boolean = true): CompletableFuture[R] = CompletableFutures.computeAsync { cancelToken => // We do not support any concurrent use of the compiler currently. - thisServer.synchronized { + def computation(): R = { cancelToken.checkCanceled() checkMemory() try { @@ -147,6 +147,10 @@ class DottyLanguageServer extends LanguageServer throw ex } } + if (synchronize) + thisServer.synchronized { computation() } + else + computation() } override def initialize(params: InitializeParams) = computeAsync { cancelToken => diff --git a/language-server/src/dotty/tools/languageserver/Main.scala b/language-server/src/dotty/tools/languageserver/Main.scala index 5223f0d2c63b..6b509ce35160 100644 --- a/language-server/src/dotty/tools/languageserver/Main.scala +++ b/language-server/src/dotty/tools/languageserver/Main.scala @@ -73,7 +73,7 @@ object Main { .setInput(in) .setOutput(out) // For debugging JSON messages: - // .traceMessages(new java.io.PrintWriter(System.err, true)) + //.traceMessages(new java.io.PrintWriter(System.err, true)) .create(); val client = launcher.getRemoteProxy() diff --git a/language-server/src/dotty/tools/languageserver/worksheet/Evaluator.scala b/language-server/src/dotty/tools/languageserver/worksheet/Evaluator.scala index c8c346b7c695..afa303d4b8a7 100644 --- a/language-server/src/dotty/tools/languageserver/worksheet/Evaluator.scala +++ b/language-server/src/dotty/tools/languageserver/worksheet/Evaluator.scala @@ -28,7 +28,7 @@ private object Evaluator { * @param cancelChecker The token that indicates whether evaluation has been cancelled. * @return A JVM running the REPL. */ - def get(cancelChecker: CancelChecker)(implicit ctx: Context): Option[Evaluator] = { + def get(cancelChecker: CancelChecker)(implicit ctx: Context): Option[Evaluator] = synchronized { val classpath = ctx.settings.classpath.value previousEvaluator match { case Some(cp, evaluator) if evaluator.isAlive() && cp == classpath => diff --git a/language-server/src/dotty/tools/languageserver/worksheet/Worksheet.scala b/language-server/src/dotty/tools/languageserver/worksheet/Worksheet.scala index 4fcbbc86c398..60e10688a924 100644 --- a/language-server/src/dotty/tools/languageserver/worksheet/Worksheet.scala +++ b/language-server/src/dotty/tools/languageserver/worksheet/Worksheet.scala @@ -10,60 +10,65 @@ import dotty.tools.dotc.core.Flags.Synthetic import org.eclipse.lsp4j.jsonrpc.CancelChecker -import java.util.concurrent.CancellationException - object Worksheet { /** * Run `tree` as a worksheet using the REPL. * * @param tree The top level object wrapping the worksheet. + * @param treeLock Object on which to lock when doing operations on trees. * @param sendMessage A mean of communicating the results of evaluation back. * @param cancelChecker A token to check whether execution should be cancelled. */ def run(tree: SourceTree, + treeLock: Object, sendMessage: (Int, String) => Unit, cancelChecker: CancelChecker)( - implicit ctx: Context): Unit = synchronized { - - Evaluator.get(cancelChecker) match { - case None => - sendMessage(1, "Couldn't start JVM.") - case Some(evaluator) => - tree.tree match { - case td @ TypeDef(_, template: Template) => - val executed = collection.mutable.Set.empty[(Int, Int)] - - template.body.foreach { - case statement: DefTree if statement.symbol.is(Synthetic) => - () + implicit ctx: Context): Unit = { + // For now, don't try to run multiple evaluators in parallel, this would require + // changes to the logic of Evaluator.get among other things. + Evaluator.synchronized { + Evaluator.get(cancelChecker) match { + case None => + sendMessage(1, "Couldn't start the JVM.") + case Some(evaluator) => + val queries = treeLock.synchronized { + tree.tree match { + case td @ TypeDef(_, template: Template) => + val seen = collection.mutable.Set.empty[(Int, Int)] - case statement if evaluator.isAlive() && executed.add(bounds(statement.pos)) => - try { - cancelChecker.checkCanceled() - val (line, result) = execute(evaluator, statement, tree.source) - if (result.nonEmpty) sendMessage(line, result) - } catch { case _: CancellationException => () } - - case _ => - () + template.body.flatMap { + case statement: DefTree if statement.symbol.is(Synthetic) => + None + case statement if seen.add(bounds(statement.pos)) => + Some(query(statement, tree.source)) + case _ => + None + } } - } + } + queries.foreach { (line, code) => + cancelChecker.checkCanceled() + val res = evaluator.eval(code).getOrElse("") + cancelChecker.checkCanceled() + if (res.nonEmpty) + sendMessage(line, res) + } + } } } /** - * Extract `tree` from the source and evaluate it in the REPL. + * Extract the line number and source code corresponding to this tree * * @param evaluator The JVM that runs the REPL. * @param tree The compiled tree to evaluate. * @param sourcefile The sourcefile of the worksheet. - * @return The line in the sourcefile that corresponds to `tree`, and the result. */ - private def execute(evaluator: Evaluator, tree: Tree, sourcefile: SourceFile): (Int, String) = { - val source = sourcefile.content.slice(tree.pos.start, tree.pos.end).mkString + private def query(tree: Tree, sourcefile: SourceFile): (Int, String) = { val line = sourcefile.offsetToLine(tree.pos.end) - (line, evaluator.eval(source).getOrElse("")) + val source = sourcefile.content.slice(tree.pos.start, tree.pos.end).mkString + (line, source) } private def bounds(pos: Position): (Int, Int) = (pos.start, pos.end) diff --git a/language-server/src/dotty/tools/languageserver/worksheet/WorksheetService.scala b/language-server/src/dotty/tools/languageserver/worksheet/WorksheetService.scala index 3bc3876cc089..a5a273003e7f 100644 --- a/language-server/src/dotty/tools/languageserver/worksheet/WorksheetService.scala +++ b/language-server/src/dotty/tools/languageserver/worksheet/WorksheetService.scala @@ -13,28 +13,23 @@ import java.util.concurrent.{CompletableFuture, ConcurrentHashMap} @JsonSegment("worksheet") trait WorksheetService { thisServer: DottyLanguageServer => - val worksheets: ConcurrentHashMap[URI, CompletableFuture[_]] = new ConcurrentHashMap() - @JsonRequest - def run(params: WorksheetRunParams): CompletableFuture[WorksheetRunResult] = thisServer.synchronized { - val uri = new URI(params.textDocument.getUri) - val future = - computeAsync { cancelChecker => - try { - val driver = driverFor(uri) - val sendMessage = (line: Int, msg: String) => client.publishOutput(WorksheetRunOutput(params.textDocument, line, msg)) - runWorksheet(driver, uri, sendMessage, cancelChecker)(driver.currentCtx) - WorksheetRunResult(success = true) - } catch { - case _: Throwable => - WorksheetRunResult(success = false) - } finally { - worksheets.remove(uri) - } + def run(params: WorksheetRunParams): CompletableFuture[WorksheetRunResult] = + computeAsync(synchronize = false, fun = { cancelChecker => + val uri = new URI(params.textDocument.getUri) + try { + val driver = driverFor(uri) + val sendMessage = + (line: Int, msg: String) => client.publishOutput(WorksheetRunOutput(params.textDocument, line, msg)) + + runWorksheet(driver, uri, sendMessage, cancelChecker)(driver.currentCtx) + cancelChecker.checkCanceled() + WorksheetRunResult(success = true) + } catch { + case _: Throwable => + WorksheetRunResult(success = false) } - worksheets.put(uri, future) - future - } + }) /** * Run the worksheet at `uri`. @@ -45,13 +40,13 @@ trait WorksheetService { thisServer: DottyLanguageServer => * @param cancelChecker Token to check whether evaluation was cancelled */ private def runWorksheet(driver: InteractiveDriver, - uri: URI, - sendMessage: (Int, String) => Unit, - cancelChecker: CancelChecker)( + uri: URI, + sendMessage: (Int, String) => Unit, + cancelChecker: CancelChecker)( implicit ctx: Context): Unit = { - val trees = driver.openedTrees(uri) - trees.headOption.foreach { tree => - Worksheet.run(tree, sendMessage, cancelChecker) + val treeOpt = thisServer.synchronized { + driver.openedTrees(uri).headOption } + treeOpt.foreach(tree => Worksheet.run(tree, thisServer, sendMessage, cancelChecker)) } }