From ddaf1689f1ad32039211f62fd0d651f373f67729 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonatan=20K=C5=82osko?= Date: Fri, 28 Jan 2022 19:01:44 +0100 Subject: [PATCH] Highlight code errors on formatting and evaluation --- assets/js/cell/index.js | 7 ++ assets/js/cell/live_editor.js | 94 ++++++++++++++++++--------- lib/livebook/evaluator.ex | 31 +++++++-- lib/livebook/intellisense.ex | 13 ++-- lib/livebook/runtime.ex | 7 +- lib/livebook_web/live/session_live.ex | 6 +- test/livebook/evaluator_test.exs | 45 ++++++++++++- test/livebook/intellisense_test.exs | 16 +++++ 8 files changed, 170 insertions(+), 49 deletions(-) diff --git a/assets/js/cell/index.js b/assets/js/cell/index.js index 37a0370b674..c81be85d6d6 100644 --- a/assets/js/cell/index.js +++ b/assets/js/cell/index.js @@ -108,6 +108,13 @@ const Cell = { this.state.liveEditor.onChange((newSource) => { updateChangeIndicator(); }); + + this.handleEvent( + `evaluation_finished:${this.props.cellId}`, + ({ code_error }) => { + this.state.liveEditor.setCodeErrorMarker(code_error); + } + ); } // Setup markdown updates diff --git a/assets/js/cell/live_editor.js b/assets/js/cell/live_editor.js index 4b4fb702afe..187fb9d32f0 100644 --- a/assets/js/cell/live_editor.js +++ b/assets/js/cell/live_editor.js @@ -140,6 +140,34 @@ class LiveEditor { } } + /** + * Adds an underline marker for the given syntax error. + * + * To clear an existing marker `null` error is also supported. + */ + setCodeErrorMarker(error) { + const owner = "elixir.error.syntax"; + + if (error) { + const line = this.editor.getModel().getLineContent(error.line); + const [, leadingWhitespace, trailingWhitespace] = + line.match(/^(\s*).*?(\s*)$/); + + monaco.editor.setModelMarkers(this.editor.getModel(), owner, [ + { + startLineNumber: error.line, + startColumn: leadingWhitespace.length + 1, + endLineNumber: error.line, + endColumn: line.length + 1 - trailingWhitespace.length, + message: error.description, + severity: monaco.MarkerSeverity.Error, + }, + ]); + } else { + monaco.editor.setModelMarkers(this.editor.getModel(), owner, []); + } + } + __mountEditor() { const settings = settingsStore.get(); @@ -347,37 +375,43 @@ class LiveEditor { return this.__asyncIntellisenseRequest("format", { code: content }) .then((response) => { - /** - * We use a single edit replacing the whole editor content, - * but the editor itself optimises this into a list of edits - * that produce minimal diff using the Myers string difference. - * - * References: - * * https://github.com/microsoft/vscode/blob/628b4d46357f2420f1dbfcea499f8ff59ee2c251/src/vs/editor/contrib/format/format.ts#L324 - * * https://github.com/microsoft/vscode/blob/628b4d46357f2420f1dbfcea499f8ff59ee2c251/src/vs/editor/common/services/editorSimpleWorker.ts#L489 - * * https://github.com/microsoft/vscode/blob/628b4d46357f2420f1dbfcea499f8ff59ee2c251/src/vs/base/common/diff/diff.ts#L227-L231 - * - * Eventually the editor will received the optimised list of edits, - * which we then convert to Delta and send to the server. - * Consequently, the Delta carries only the minimal formatting diff. - * - * Also, if edits are applied to the editor, either by typing - * or receiving remote changes, the formatting is cancelled. - * In other words the formatting changes are actually applied - * only if the editor stays intact. - * - * References: - * * https://github.com/microsoft/vscode/blob/628b4d46357f2420f1dbfcea499f8ff59ee2c251/src/vs/editor/contrib/format/format.ts#L313 - * * https://github.com/microsoft/vscode/blob/628b4d46357f2420f1dbfcea499f8ff59ee2c251/src/vs/editor/browser/core/editorState.ts#L137 - * * https://github.com/microsoft/vscode/blob/628b4d46357f2420f1dbfcea499f8ff59ee2c251/src/vs/editor/contrib/format/format.ts#L326 - */ - - const replaceEdit = { - range: model.getFullModelRange(), - text: response.code, - }; + this.setCodeErrorMarker(response.code_error); + + if (response.code) { + /** + * We use a single edit replacing the whole editor content, + * but the editor itself optimises this into a list of edits + * that produce minimal diff using the Myers string difference. + * + * References: + * * https://github.com/microsoft/vscode/blob/628b4d46357f2420f1dbfcea499f8ff59ee2c251/src/vs/editor/contrib/format/format.ts#L324 + * * https://github.com/microsoft/vscode/blob/628b4d46357f2420f1dbfcea499f8ff59ee2c251/src/vs/editor/common/services/editorSimpleWorker.ts#L489 + * * https://github.com/microsoft/vscode/blob/628b4d46357f2420f1dbfcea499f8ff59ee2c251/src/vs/base/common/diff/diff.ts#L227-L231 + * + * Eventually the editor will received the optimised list of edits, + * which we then convert to Delta and send to the server. + * Consequently, the Delta carries only the minimal formatting diff. + * + * Also, if edits are applied to the editor, either by typing + * or receiving remote changes, the formatting is cancelled. + * In other words the formatting changes are actually applied + * only if the editor stays intact. + * + * References: + * * https://github.com/microsoft/vscode/blob/628b4d46357f2420f1dbfcea499f8ff59ee2c251/src/vs/editor/contrib/format/format.ts#L313 + * * https://github.com/microsoft/vscode/blob/628b4d46357f2420f1dbfcea499f8ff59ee2c251/src/vs/editor/browser/core/editorState.ts#L137 + * * https://github.com/microsoft/vscode/blob/628b4d46357f2420f1dbfcea499f8ff59ee2c251/src/vs/editor/contrib/format/format.ts#L326 + */ + + const replaceEdit = { + range: model.getFullModelRange(), + text: response.code, + }; - return [replaceEdit]; + return [replaceEdit]; + } else { + return []; + } }) .catch(() => null); }; diff --git a/lib/livebook/evaluator.ex b/lib/livebook/evaluator.ex index 87fccfb64e6..93f5af5bc6b 100644 --- a/lib/livebook/evaluator.ex +++ b/lib/livebook/evaluator.ex @@ -289,16 +289,16 @@ defmodule Livebook.Evaluator do context = put_in(context.env.file, file) start_time = System.monotonic_time() - {result_context, response} = + {result_context, response, code_error} = case eval(code, context.binding, context.env) do {:ok, result, binding, env} -> result_context = %{binding: binding, env: env, id: random_id()} response = {:ok, result} - {result_context, response} + {result_context, response, nil} - {:error, kind, error, stacktrace} -> + {:error, kind, error, stacktrace, code_error} -> response = {:error, kind, error, stacktrace} - {context, response} + {context, response, code_error} end evaluation_time_ms = get_execution_time_delta(start_time) @@ -309,7 +309,13 @@ defmodule Livebook.Evaluator do Evaluator.IOProxy.clear_input_cache(state.io_proxy) output = state.formatter.format_response(response) - metadata = %{evaluation_time_ms: evaluation_time_ms, memory_usage: memory()} + + metadata = %{ + evaluation_time_ms: evaluation_time_ms, + memory_usage: memory(), + code_error: code_error + } + send(send_to, {:evaluation_response, ref, output, metadata}) :erlang.garbage_collect(self()) @@ -391,10 +397,23 @@ defmodule Livebook.Evaluator do catch kind, error -> stacktrace = prune_stacktrace(__STACKTRACE__) - {:error, kind, error, stacktrace} + + code_error = + if code_error?(error) and (error.file == env.file and error.file != "nofile") do + %{line: error.line, description: error.description} + else + nil + end + + {:error, kind, error, stacktrace, code_error} end end + defp code_error?(%SyntaxError{}), do: true + defp code_error?(%TokenMissingError{}), do: true + defp code_error?(%CompileError{}), do: true + defp code_error?(_error), do: false + # Adapted from https://github.com/elixir-lang/elixir/blob/1c1654c88adfdbef38ff07fc30f6fbd34a542c07/lib/iex/lib/iex/evaluator.ex#L355-L372 @elixir_internals [:elixir, :elixir_expand, :elixir_compiler, :elixir_module] ++ diff --git a/lib/livebook/intellisense.ex b/lib/livebook/intellisense.ex index 7c1ecc7ad80..bb1c3dc4f77 100644 --- a/lib/livebook/intellisense.ex +++ b/lib/livebook/intellisense.ex @@ -51,16 +51,13 @@ defmodule Livebook.Intellisense do end def handle_request({:format, code}, _context) do - case format_code(code) do - {:ok, code} -> %{code: code} - :error -> nil - end + format_code(code) end @doc """ Formats Elixir code. """ - @spec format_code(String.t()) :: {:ok, String.t()} | :error + @spec format_code(String.t()) :: Runtime.format_response() def format_code(code) do try do formatted = @@ -68,9 +65,11 @@ defmodule Livebook.Intellisense do |> Code.format_string!() |> IO.iodata_to_binary() - {:ok, formatted} + %{code: formatted, code_error: nil} rescue - _ -> :error + error -> + code_error = %{line: error.line, description: error.description} + %{code: nil, code_error: code_error} end end diff --git a/lib/livebook/runtime.ex b/lib/livebook/runtime.ex index 0a420861f9e..6fd93c7e629 100644 --- a/lib/livebook/runtime.ex +++ b/lib/livebook/runtime.ex @@ -112,9 +112,12 @@ defprotocol Livebook.Runtime do @type format_request :: {:format, code :: String.t()} @type format_response :: %{ - code: String.t() + code: String.t() | nil, + code_error: code_error() | nil } + @type code_error :: %{line: pos_integer(), description: String.t()} + @typedoc """ The runtime memory usage for each type in bytes. @@ -173,7 +176,7 @@ defprotocol Livebook.Runtime do * `{:evaluation_response, ref, output, metadata}` - final result of the evaluation. Recognised metadata entries - are: `evaluation_time_ms` and `memory_usage` + are: `code_error`, `evaluation_time_ms` and `memory_usage` The output may include input fields. The evaluation may then request the current value of a previously rendered input by diff --git a/lib/livebook_web/live/session_live.ex b/lib/livebook_web/live/session_live.ex index 8f44bc13a00..3c3d98b5101 100644 --- a/lib/livebook_web/live/session_live.ex +++ b/lib/livebook_web/live/session_live.ex @@ -1212,9 +1212,11 @@ defmodule LivebookWeb.SessionLive do defp after_operation( socket, _prev_socket, - {:add_cell_evaluation_response, _client_pid, _id, _output, _metadata} + {:add_cell_evaluation_response, _client_pid, cell_id, _output, metadata} ) do - prune_outputs(socket) + socket + |> prune_outputs() + |> push_event("evaluation_finished:#{cell_id}", %{code_error: metadata.code_error}) end defp after_operation(socket, _prev_socket, _operation), do: socket diff --git a/test/livebook/evaluator_test.exs b/test/livebook/evaluator_test.exs index 65686e45ff6..63892dddb81 100644 --- a/test/livebook/evaluator_test.exs +++ b/test/livebook/evaluator_test.exs @@ -11,7 +11,7 @@ defmodule Livebook.EvaluatorTest do defmacrop metadata do quote do - %{evaluation_time_ms: _, memory_usage: %{}} + %{evaluation_time_ms: _, memory_usage: %{}, code_error: _} end end @@ -91,13 +91,54 @@ defmodule Livebook.EvaluatorTest do List.first(%{}) """ - Evaluator.evaluate_code(evaluator, self(), code, :code_1) + Evaluator.evaluate_code(evaluator, self(), code, :code_1, nil, file: "file.ex") assert_receive {:evaluation_response, :code_1, {:error, :error, :function_clause, [{List, :first, _arity, _location}]}, metadata()} end + test "returns additional metadata when there is a syntax error", %{evaluator: evaluator} do + code = "1+" + + Evaluator.evaluate_code(evaluator, self(), code, :code_1, nil, file: "file.ex") + + assert_receive {:evaluation_response, :code_1, {:error, :error, %TokenMissingError{}, []}, + %{ + code_error: %{ + line: 1, + description: "syntax error: expression is incomplete" + } + }} + end + + test "returns additional metadata when there is a compilation error", %{evaluator: evaluator} do + code = "x" + + Evaluator.evaluate_code(evaluator, self(), code, :code_1, nil, file: "file.ex") + + assert_receive {:evaluation_response, :code_1, {:error, :error, %CompileError{}, []}, + %{ + code_error: %{ + line: 1, + description: "undefined function x/0 (there is no such import)" + } + }} + end + + test "ignores code errors when they happen in the actual evaluation", %{evaluator: evaluator} do + code = """ + Code.eval_string("x") + """ + + Evaluator.evaluate_code(evaluator, self(), code, :code_1, nil, file: "file.ex") + + expected_stacktrace = [{Code, :validated_eval_string, 3, [file: 'lib/code.ex', line: 404]}] + + assert_receive {:evaluation_response, :code_1, + {:error, :error, %CompileError{}, ^expected_stacktrace}, %{code_error: nil}} + end + test "in case of an error returns only the relevant part of stacktrace", %{evaluator: evaluator} do code = """ diff --git a/test/livebook/intellisense_test.exs b/test/livebook/intellisense_test.exs index 4ce0689cfb7..d50533268f9 100644 --- a/test/livebook/intellisense_test.exs +++ b/test/livebook/intellisense_test.exs @@ -30,6 +30,22 @@ defmodule Livebook.IntellisenseTest do alias Livebook.Intellisense + describe "format_code/1" do + test "formats valid code" do + assert %{code: "1 + 1", code_error: nil} = Intellisense.format_code("1+1") + end + + test "returns a syntax error when invalid code is given" do + assert %{ + code: nil, + code_error: %{ + line: 1, + description: "syntax error: expression is incomplete" + } + } = Intellisense.format_code("1+") + end + end + describe "get_completion_items/3" do test "completion when no hint given" do context = eval(do: nil)