From 1f3d6f629492fef456fcf24b40bab146daea0a40 Mon Sep 17 00:00:00 2001 From: Matt Hillsdon Date: Thu, 5 Dec 2024 14:37:19 +0000 Subject: [PATCH 1/2] Signature help: restructure async work sendRequest isn't an async function as it can synchronously throw errors so we need something to prevent reentrant updates in the error case. Otherwise though we don't needed it if we move more work into the state. --- .../language-server/signatureHelp.ts | 134 ++++++++++-------- 1 file changed, 77 insertions(+), 57 deletions(-) diff --git a/src/editor/codemirror/language-server/signatureHelp.ts b/src/editor/codemirror/language-server/signatureHelp.ts index 9903cdf79..05cb9d77f 100644 --- a/src/editor/codemirror/language-server/signatureHelp.ts +++ b/src/editor/codemirror/language-server/signatureHelp.ts @@ -7,7 +7,7 @@ * * SPDX-License-Identifier: MIT */ -import { EditorState, StateEffect, StateField } from "@codemirror/state"; +import { Facet, StateEffect, StateField } from "@codemirror/state"; import { Command, EditorView, @@ -23,7 +23,6 @@ import { IntlShape } from "react-intl"; import { MarkupContent, SignatureHelp, - SignatureHelpParams, SignatureHelpRequest, } from "vscode-languageserver-protocol"; import { ApiReferenceMap } from "../../../documentation/mapping/content"; @@ -38,6 +37,10 @@ import { import { nameFromSignature, removeFullyQualifiedName } from "./names"; import { offsetToPosition } from "./positions"; +export const automaticFacet = Facet.define({ + combine: (values) => values[values.length - 1] ?? true, +}); + export const setSignatureHelpRequestPosition = StateEffect.define({}); export const setSignatureHelpResult = StateEffect.define( @@ -49,6 +52,7 @@ class SignatureHelpState { * -1 for no signature help requested. */ pos: number; + /** * The latest result we want to display. * @@ -77,44 +81,12 @@ const signatureHelpToolTipBaseTheme = EditorView.baseTheme({ }, }); -const triggerSignatureHelpRequest = async ( - view: EditorView, - state: EditorState -): Promise => { - const uri = state.facet(uriFacet)!; - const client = state.facet(clientFacet)!; - const pos = state.selection.main.from; - const params: SignatureHelpParams = { - textDocument: { uri }, - position: offsetToPosition(state.doc, pos), - }; - try { - // Must happen before other event handling that might dispatch more - // changes that invalidate our position. - queueMicrotask(() => { - view.dispatch({ - effects: [setSignatureHelpRequestPosition.of(pos)], - }); - }); - const result = await client.connection.sendRequest( - SignatureHelpRequest.type, - params - ); - view.dispatch({ - effects: [setSignatureHelpResult.of(result)], - }); - } catch (e) { - if (!isErrorDueToDispose(e)) { - logException(state, e, "signature-help"); - } - view.dispatch({ - effects: [setSignatureHelpResult.of(null)], - }); - } -}; - const openSignatureHelp: Command = (view: EditorView) => { - triggerSignatureHelpRequest(view, view.state); + view.dispatch({ + effects: [ + setSignatureHelpRequestPosition.of(view.state.selection.main.from), + ], + }); return true; }; @@ -138,12 +110,35 @@ export const signatureHelp = ( } } } + // Even if we just got a result, if the position has been cleared we don't want it. if (pos === -1) { result = null; } + // By default map the previous position forward pos = pos === -1 ? -1 : tr.changes.mapPos(pos); + + // Did the selection moved while open? We'll re-request but keep the old result for now. + if (pos !== -1 && tr.selection) { + pos = tr.selection.main.from; + } + + // Automatic triggering cases + const automatic = tr.state.facet(automaticFacet).valueOf(); + if ( + automatic && + ((tr.docChanged && tr.isUserEvent("input")) || + tr.isUserEvent("dnd.drop.call")) + ) { + tr.changes.iterChanges((_fromA, _toA, _fromB, _toB, inserted) => { + if (inserted.sliceString(0).trim().endsWith("()")) { + // Triggered + pos = tr.newSelection.main.from; + } + }); + } + if (state.pos === pos && state.result === result) { // Avoid pointless tooltip updates. If nothing else it makes e2e tests hard. return state; @@ -191,30 +186,54 @@ export const signatureHelp = ( extends BaseLanguageServerView implements PluginValue { - constructor(view: EditorView, private automatic: boolean) { + private destroyed = false; + private lastPos = -1; + + constructor(view: EditorView) { super(view); } update(update: ViewUpdate) { - if ( - (update.docChanged || update.selectionSet) && - this.view.state.field(signatureHelpTooltipField).pos !== -1 - ) { - triggerSignatureHelpRequest(this.view, update.state); - } else if (this.automatic && update.docChanged) { - const last = update.transactions[update.transactions.length - 1]; - - // This needs to trigger for autocomplete adding function parens - // as well as normal user input with `closebrackets` inserting - // the closing bracket. - if (last.isUserEvent("input") || last.isUserEvent("dnd.drop.call")) { - last.changes.iterChanges((_fromA, _toA, _fromB, _toB, inserted) => { - if (inserted.sliceString(0).trim().endsWith("()")) { - triggerSignatureHelpRequest(this.view, update.state); + const { view, state } = update; + const uri = state.facet(uriFacet)!; + const client = state.facet(clientFacet)!; + const { pos } = update.state.field(signatureHelpTooltipField); + if (this.lastPos !== pos) { + this.lastPos = pos; + if (this.lastPos !== -1) { + (async () => { + try { + const result = await client.connection.sendRequest( + SignatureHelpRequest.type, + { + textDocument: { uri }, + position: offsetToPosition(state.doc, this.lastPos), + } + ); + if (!this.destroyed) { + view.dispatch({ + effects: [setSignatureHelpResult.of(result)], + }); + } + } catch (e) { + if (!isErrorDueToDispose(e)) { + logException(state, e, "signature-help"); + } + // The sendRequest call can fail synchronously when disposed so we need to ensure our clean-up doesn't happen inside the CM update call. + queueMicrotask(() => { + if (!this.destroyed) { + view.dispatch({ + effects: [setSignatureHelpResult.of(null)], + }); + } + }); } - }); + })(); } } } + destroy(): void { + this.destroyed = true; + } } const formatSignatureHelp = ( @@ -306,10 +325,11 @@ export const signatureHelp = ( return [ // View only handles automatic triggering. - ViewPlugin.define((view) => new SignatureHelpView(view, automatic)), + ViewPlugin.define((view) => new SignatureHelpView(view)), signatureHelpTooltipField, signatureHelpToolTipBaseTheme, keymap.of(signatureHelpKeymap), + automaticFacet.of(automatic), EditorView.domEventHandlers({ blur(event, view) { // Close signature help as it interacts badly with drag and drop if From b7d566753a0fac97b6519c036e6cd6e23de118b7 Mon Sep 17 00:00:00 2001 From: Matt Hillsdon Date: Thu, 5 Dec 2024 16:52:25 +0000 Subject: [PATCH 2/2] Calculate line/column immediately I'm not sure this is necessary but it feels obviously correct / straightforward which is a win in this code. --- .../language-server/signatureHelp.ts | 46 +++++++++++++++---- 1 file changed, 36 insertions(+), 10 deletions(-) diff --git a/src/editor/codemirror/language-server/signatureHelp.ts b/src/editor/codemirror/language-server/signatureHelp.ts index 05cb9d77f..b14789ddb 100644 --- a/src/editor/codemirror/language-server/signatureHelp.ts +++ b/src/editor/codemirror/language-server/signatureHelp.ts @@ -22,6 +22,7 @@ import { import { IntlShape } from "react-intl"; import { MarkupContent, + Position, SignatureHelp, SignatureHelpRequest, } from "vscode-languageserver-protocol"; @@ -53,6 +54,11 @@ class SignatureHelpState { */ pos: number; + /** + * The LSP position for pos. + */ + position: Position | null; + /** * The latest result we want to display. * @@ -61,11 +67,16 @@ class SignatureHelpState { */ result: SignatureHelp | null; - constructor(pos: number, result: SignatureHelp | null) { + constructor( + pos: number, + position: Position | null, + result: SignatureHelp | null + ) { if (result && pos === -1) { throw new Error("Invalid state"); } this.pos = pos; + this.position = position; this.result = result; } } @@ -81,6 +92,16 @@ const signatureHelpToolTipBaseTheme = EditorView.baseTheme({ }, }); +const positionEq = (a: Position | null, b: Position | null): boolean => { + if (a === null) { + return b === null; + } + if (b === null) { + return a === null; + } + return a.character === b.character && a.line === b.line; +}; + const openSignatureHelp: Command = (view: EditorView) => { view.dispatch({ effects: [ @@ -96,7 +117,7 @@ export const signatureHelp = ( apiReferenceMap: ApiReferenceMap ) => { const signatureHelpTooltipField = StateField.define({ - create: () => new SignatureHelpState(-1, null), + create: () => new SignatureHelpState(-1, null, null), update(state, tr) { let { pos, result } = state; for (const effect of tr.effects) { @@ -139,11 +160,16 @@ export const signatureHelp = ( }); } - if (state.pos === pos && state.result === result) { + const position = pos === -1 ? null : offsetToPosition(tr.state.doc, pos); + if ( + state.pos === pos && + state.result === result && + positionEq(state.position, position) + ) { // Avoid pointless tooltip updates. If nothing else it makes e2e tests hard. return state; } - return new SignatureHelpState(pos, result); + return new SignatureHelpState(pos, position, result); }, provide: (f) => showTooltip.from(f, (val) => { @@ -187,7 +213,7 @@ export const signatureHelp = ( implements PluginValue { private destroyed = false; - private lastPos = -1; + private lastPosition: Position | null = null; constructor(view: EditorView) { super(view); @@ -196,17 +222,17 @@ export const signatureHelp = ( const { view, state } = update; const uri = state.facet(uriFacet)!; const client = state.facet(clientFacet)!; - const { pos } = update.state.field(signatureHelpTooltipField); - if (this.lastPos !== pos) { - this.lastPos = pos; - if (this.lastPos !== -1) { + const { position } = update.state.field(signatureHelpTooltipField); + if (!positionEq(this.lastPosition, position)) { + this.lastPosition = position; + if (position !== null) { (async () => { try { const result = await client.connection.sendRequest( SignatureHelpRequest.type, { textDocument: { uri }, - position: offsetToPosition(state.doc, this.lastPos), + position, } ); if (!this.destroyed) {