From 00f2a4e18dc762360aacf2369923678c0f4c1cbb Mon Sep 17 00:00:00 2001 From: krassowski <5832902+krassowski@users.noreply.github.com> Date: Fri, 31 Dec 2021 17:32:43 +0000 Subject: [PATCH] Implement jump to definitions and basic target selector --- packages/jupyterlab-lsp/src/connection.ts | 4 +- .../src/editor_integration/codemirror.ts | 12 + packages/jupyterlab-lsp/src/features/hover.ts | 32 +- .../jupyterlab-lsp/src/features/jump_to.ts | 335 +++++++++++++----- packages/jupyterlab-lsp/src/positioning.ts | 17 + .../lsp-ws-connection/src/ws-connection.ts | 17 + 6 files changed, 306 insertions(+), 111 deletions(-) diff --git a/packages/jupyterlab-lsp/src/connection.ts b/packages/jupyterlab-lsp/src/connection.ts index 7e7609284..ef17d576e 100644 --- a/packages/jupyterlab-lsp/src/connection.ts +++ b/packages/jupyterlab-lsp/src/connection.ts @@ -129,10 +129,10 @@ export interface IClientResult { [Method.ClientRequest.DEFINITION]: AnyLocation; [Method.ClientRequest.DOCUMENT_HIGHLIGHT]: lsp.DocumentHighlight[]; [Method.ClientRequest.DOCUMENT_SYMBOL]: lsp.DocumentSymbol[]; - [Method.ClientRequest.HOVER]: lsp.Hover; + [Method.ClientRequest.HOVER]: lsp.Hover | null; [Method.ClientRequest.IMPLEMENTATION]: AnyLocation; [Method.ClientRequest.INITIALIZE]: lsp.InitializeResult; - [Method.ClientRequest.REFERENCES]: Location[]; + [Method.ClientRequest.REFERENCES]: lsp.Location[] | null; [Method.ClientRequest.RENAME]: lsp.WorkspaceEdit; [Method.ClientRequest.SIGNATURE_HELP]: lsp.SignatureHelp; [Method.ClientRequest.TYPE_DEFINITION]: AnyLocation; diff --git a/packages/jupyterlab-lsp/src/editor_integration/codemirror.ts b/packages/jupyterlab-lsp/src/editor_integration/codemirror.ts index 5b209bc1b..26123f55c 100644 --- a/packages/jupyterlab-lsp/src/editor_integration/codemirror.ts +++ b/packages/jupyterlab-lsp/src/editor_integration/codemirror.ts @@ -98,6 +98,7 @@ export abstract class CodeMirrorIntegration protected virtual_document: VirtualDocument; protected connection: LSPConnection; + /** @deprecated: use `setStatusMessage()` instead */ protected status_message: StatusMessage; protected adapter: WidgetAdapter; protected console: ILSPLogConsole; @@ -128,6 +129,17 @@ export abstract class CodeMirrorIntegration this.is_registered = false; } + /** + * Set the text message and (optionally) the timeout to remove it. + * @param message + * @param timeout - number of ms to until the message is cleaned; + * -1 if the message should stay up indefinitely; + * defaults to 3000ms (3 seconds) + */ + setStatusMessage(message: string, timeout?: number): void { + this.status_message.set(message, timeout); + } + register(): void { // register editor handlers for (let [event_name, handler] of this.editor_handlers) { diff --git a/packages/jupyterlab-lsp/src/features/hover.ts b/packages/jupyterlab-lsp/src/features/hover.ts index 769303fcb..a9acdbaea 100644 --- a/packages/jupyterlab-lsp/src/features/hover.ts +++ b/packages/jupyterlab-lsp/src/features/hover.ts @@ -23,7 +23,12 @@ import { IEditorIntegrationOptions, IFeatureLabIntegration } from '../feature'; -import { IRootPosition, IVirtualPosition, is_equal } from '../positioning'; +import { + IRootPosition, + IVirtualPosition, + ProtocolCoordinates, + is_equal +} from '../positioning'; import { ILSPFeatureManager, PLUGIN_ID } from '../tokens'; import { getModifierState } from '../utils'; import { VirtualDocument } from '../virtual/document'; @@ -122,10 +127,12 @@ export class HoverCM extends CodeMirrorIntegration { private virtual_position: IVirtualPosition; protected cache: ResponseCache; - private debounced_get_hover: Throttler>; + private debounced_get_hover: Throttler< + Promise + >; private tooltip: FreeTooltip; private _previousHoverRequest: Promise< - Promise + Promise > | null = null; constructor(options: IEditorIntegrationOptions) { @@ -154,13 +161,7 @@ export class HoverCM extends CodeMirrorIntegration { return false; } let range = cache_item.response.range!; - return ( - line >= range.start.line && - line <= range.end.line && - // need to be non-overlapping see https://github.com/jupyter-lsp/jupyterlab-lsp/issues/628 - (line != range.start.line || ch > range.start.character) && - (line != range.end.line || ch <= range.end.character) - ); + return ProtocolCoordinates.isWithinRange({ line, character: ch }, range); }); if (matching_items.length > 1) { this.console.warn( @@ -250,10 +251,13 @@ export class HoverCM extends CodeMirrorIntegration { } protected create_throttler() { - return new Throttler>(this.on_hover, { - limit: this.settings.composite.throttlerDelay, - edge: 'trailing' - }); + return new Throttler>( + this.on_hover, + { + limit: this.settings.composite.throttlerDelay, + edge: 'trailing' + } + ); } afterChange(change: IEditorChange, root_position: IRootPosition) { diff --git a/packages/jupyterlab-lsp/src/features/jump_to.ts b/packages/jupyterlab-lsp/src/features/jump_to.ts index 47d3799ba..261ab2ff4 100644 --- a/packages/jupyterlab-lsp/src/features/jump_to.ts +++ b/packages/jupyterlab-lsp/src/features/jump_to.ts @@ -2,6 +2,7 @@ import { JupyterFrontEnd, JupyterFrontEndPlugin } from '@jupyterlab/application'; +import { InputDialog } from '@jupyterlab/apputils'; import { CodeMirrorEditor } from '@jupyterlab/codemirror'; import { URLExt } from '@jupyterlab/coreutils'; import { IDocumentManager } from '@jupyterlab/docmanager'; @@ -16,6 +17,7 @@ import { NotebookJumper } from '@krassowski/code-jumpers'; import { AnyLocation } from 'lsp-ws-connection/lib/types'; +import type * as lsp from 'vscode-languageserver-protocol'; import jumpToSvg from '../../style/icons/jump-to.svg'; import { CodeJump as LSPJumpSettings, ModifierKey } from '../_jump_to'; @@ -27,9 +29,10 @@ import { IFeatureCommand, IFeatureLabIntegration } from '../feature'; -import { IVirtualPosition } from '../positioning'; +import { IVirtualPosition, ProtocolCoordinates } from '../positioning'; import { ILSPAdapterManager, ILSPFeatureManager, PLUGIN_ID } from '../tokens'; import { getModifierState, uri_to_contents_path, uris_equal } from '../utils'; +import { CodeMirrorVirtualEditor } from '../virtual/codemirror_editor'; export const jumpToIcon = new LabIcon({ name: 'lsp:jump-to', @@ -45,6 +48,15 @@ const FEATURE_ID = PLUGIN_ID + ':jump_to'; let trans: TranslationBundle; +const enum JumpResult { + NoTargetsFound = 1, + PositioningFailure = 2, + PathResolutionFailure = 3, + AssumeSuccess = 4, + UnspecifiedFailure = 5, + AlreadyAtTarget = 6 +} + export class CMJumpToDefinition extends CodeMirrorIntegration { get jumper() { return (this.feature.labIntegration as JumperLabIntegration).jumper; @@ -61,94 +73,173 @@ export class CMJumpToDefinition extends CodeMirrorIntegration { register() { this.editor_handlers.set( 'mousedown', - (virtual_editor, event: MouseEvent) => { - const { button } = event; - if (button === 0 && getModifierState(event, this.modifierKey)) { - let root_position = this.position_from_mouse(event); - if (root_position == null) { - this.console.warn( - 'Could not retrieve root position from mouse event to jump to definition' - ); - return; - } - let document = - virtual_editor.document_at_root_position(root_position); - let virtual_position = - virtual_editor.root_position_to_virtual_position(root_position); - - this.connection - .getDefinition(virtual_position, document.document_info, false) - .then(targets => { - this.handle_jump(targets, document.document_info.uri).catch( - this.console.warn - ); - }) - .catch(this.console.warn); - event.preventDefault(); - event.stopPropagation(); - } - } + this._jumpToDefinitionOrRefernce.bind(this) ); super.register(); } - get_uri_and_range(location_or_locations: AnyLocation) { - if (location_or_locations == null) { - return undefined; + private _jumpToDefinitionOrRefernce( + virtual_editor: CodeMirrorVirtualEditor, + event: MouseEvent + ) { + const { button } = event; + const shouldJump = + button === 0 && getModifierState(event, this.modifierKey); + if (!shouldJump) { + return; } - // some language servers appear to return a single object - const locations = Array.isArray(location_or_locations) - ? location_or_locations - : [location_or_locations]; - - // TODO: implement selector for multiple locations - // (like when there are multiple definitions or usages) - // could use the showHints() or completion frontend as a reference - if (locations.length === 0) { - return undefined; + let root_position = this.position_from_mouse(event); + if (root_position == null) { + this.console.warn( + 'Could not retrieve root position from mouse event to jump to definition/reference' + ); + return; } + let document = virtual_editor.document_at_root_position(root_position); + let virtual_position = + virtual_editor.root_position_to_virtual_position(root_position); + + const positionParams: lsp.TextDocumentPositionParams = { + textDocument: { + uri: document.document_info.uri + }, + position: { + line: virtual_position.line, + character: virtual_position.ch + } + }; + + this.connection.clientRequests['textDocument/definition'] + .request(positionParams) + .then(targets => { + this.handleJump(targets, positionParams) + .then((result: JumpResult | undefined) => { + if ( + result === JumpResult.NoTargetsFound || + result === JumpResult.AlreadyAtTarget + ) { + // definition was not found, or we are in definition already, suggest references + this.connection.clientRequests['textDocument/references'] + .request({ + ...positionParams, + context: { includeDeclaration: false } + }) + .then(targets => + // TODO: explain that we are now presenting references? + this.handleJump(targets, positionParams) + ) + .catch(this.console.warn); + } + }) + .catch(this.console.warn); + }) + .catch(this.console.warn); - this.console.log( - 'Will jump to the first of suggested locations:', - locations - ); + event.preventDefault(); + event.stopPropagation(); + } - const location_or_link = locations[0]; + private _harmonizeLocations(locationData: AnyLocation): lsp.Location[] { + if (locationData == null) { + return []; + } - if ('targetUri' in location_or_link) { - return { - uri: location_or_link.targetUri, - range: location_or_link.targetRange - }; - } else if ('uri' in location_or_link) { - return { - uri: location_or_link.uri, - range: location_or_link.range + const locationsList = Array.isArray(locationData) + ? locationData + : [locationData]; + + return (locationsList as (lsp.Location | lsp.LocationLink)[]) + .map((locationOrLink): lsp.Location | undefined => { + if ('targetUri' in locationOrLink) { + return { + uri: locationOrLink.targetUri, + range: locationOrLink.targetRange + }; + } else if ('uri' in locationOrLink) { + return { + uri: locationOrLink.uri, + range: locationOrLink.range + }; + } else { + this.console.warn( + 'Returned jump location is incorrect (no uri or targetUri):', + locationOrLink + ); + return undefined; + } + }) + .filter((location): location is lsp.Location => location != null); + } + + private async _chooseTarget(locations: lsp.Location[]) { + if (locations.length > 1) { + const choices = locations.map(location => { + // TODO: extract the line, the line above and below, and show it + const path = this._resolvePath(location.uri) || location.uri; + return path + ', line: ' + location.range.start.line; + }); + + // TODO: use selector with preview, basically needes the ui-component + // from jupyterlab-citation-manager; let's try to move it to JupyterLab core + // (and re-implement command palette with it) + // the preview should use this.jumper.document_manager.services.contents + + let getItemOptions = { + title: trans.__('Choose the jump target'), + okLabel: trans.__('Jump'), + items: choices }; - } else { - this.console.warn( - 'Returned jump location is incorrect (no uri or targetUri):', - location_or_link + // TODO: use showHints() or completion-like widget instead? + const choice = await InputDialog.getItem(getItemOptions).catch( + this.console.warn ); - return undefined; + if (!choice || choice.value == null) { + this.console.warn('No choice selected for jump location selection'); + return; + } + const choiceIndex = choices.indexOf(choice.value); + if (choiceIndex === -1) { + this.console.error( + 'Choice selection error: please report this as a bug:', + choices, + choice + ); + return; + } + return locations[choiceIndex]; + } else { + return locations[0]; } } - async handle_jump(location_or_locations: AnyLocation, document_uri: string) { - const target_info = this.get_uri_and_range(location_or_locations); + private _resolvePath(uri: string): string | null { + let contentsPath = uri_to_contents_path(uri); - if (!target_info) { - this.status_message.set(trans.__('No jump targets found'), 2 * 1000); - return; + if (contentsPath == null && uri.startsWith('file://')) { + contentsPath = decodeURI(uri.slice(7)); } + return contentsPath; + } - let { uri, range } = target_info; + async handleJump( + locationData: AnyLocation, + positionParams: lsp.TextDocumentPositionParams + ) { + const locations = this._harmonizeLocations(locationData); + const targetInfo = await this._chooseTarget(locations); + + if (!targetInfo) { + this.setStatusMessage(trans.__('No jump targets found'), 2 * 1000); + return JumpResult.NoTargetsFound; + } + + let { uri, range } = targetInfo; let virtual_position = PositionConverter.lsp_to_cm( range.start ) as IVirtualPosition; - if (uris_equal(uri, document_uri)) { + if (uris_equal(uri, positionParams.textDocument.uri)) { let editor_index = this.adapter.get_editor_index_at(virtual_position); // if in current file, transform from the position within virtual document to the editor position: let editor_position = @@ -158,21 +249,32 @@ export class CMJumpToDefinition extends CodeMirrorIntegration { 'Could not jump: conversion from virtual position to editor position failed', virtual_position ); - return; + return JumpResult.PositioningFailure; } let editor_position_ce = PositionConverter.cm_to_ce(editor_position); this.console.log(`Jumping to ${editor_index}th editor of ${uri}`); this.console.log('Jump target within editor:', editor_position_ce); - let contents_path = this.adapter.widget.context.path; + let contentsPath = this.adapter.widget.context.path; + + const didUserChooseThis = locations.length > 1; + + // note: we already know that URIs are equal, so just check the position range + if ( + !didUserChooseThis && + ProtocolCoordinates.isWithinRange(positionParams.position, range) + ) { + return JumpResult.AlreadyAtTarget; + } this.jumper.global_jump({ line: editor_position_ce.line, column: editor_position.ch, editor_index: editor_index, is_symlink: false, - contents_path: contents_path + contents_path: contentsPath }); + return JumpResult.AssumeSuccess; } else { // otherwise there is no virtual document and we expect the returned position to be source position: let source_position_ce = PositionConverter.cm_to_ce(virtual_position); @@ -190,39 +292,33 @@ export class CMJumpToDefinition extends CodeMirrorIntegration { // with different OSes but also with JupyterHub and other platforms. // can it be resolved vs our guessed server root? - let contents_path = uri_to_contents_path(uri); - - if (contents_path == null && uri.startsWith('file://')) { - contents_path = decodeURI(uri.slice(7)); - } + const contentsPath = this._resolvePath(uri); - if (contents_path === null) { + if (contentsPath === null) { this.console.warn('contents_path could not be resolved'); - return; + return JumpResult.PathResolutionFailure; } try { - await this.jumper.document_manager.services.contents.get( - contents_path, - { - content: false - } - ); + await this.jumper.document_manager.services.contents.get(contentsPath, { + content: false + }); this.jumper.global_jump({ - contents_path, + contents_path: contentsPath, ...jump_data, is_symlink: false }); - return; + return JumpResult.AssumeSuccess; } catch (err) { this.console.warn(err); } this.jumper.global_jump({ - contents_path: URLExt.join('.lsp_symlink', contents_path), + contents_path: URLExt.join('.lsp_symlink', contentsPath), ...jump_data, is_symlink: true }); + return JumpResult.AssumeSuccess; } } } @@ -271,19 +367,65 @@ const COMMANDS = (trans: TranslationBundle): IFeatureCommand[] => [ { id: 'jump-to-definition', execute: async ({ connection, virtual_position, document, features }) => { - const jump_feature = features.get(FEATURE_ID) as CMJumpToDefinition; - const targets = await connection?.getDefinition( - virtual_position, - document.document_info, - false - ); - await jump_feature.handle_jump(targets, document.document_info.uri); + const jumpFeature = features.get(FEATURE_ID) as CMJumpToDefinition; + if (!connection) { + jumpFeature.setStatusMessage( + trans.__('Connection not found for jump'), + 2 * 1000 + ); + return; + } + + const positionParams: lsp.TextDocumentPositionParams = { + textDocument: { + uri: document.document_info.uri + }, + position: { + line: virtual_position.line, + character: virtual_position.ch + } + }; + const targets = await connection.clientRequests[ + 'textDocument/definition' + ].request(positionParams); + await jumpFeature.handleJump(targets, positionParams); }, is_enabled: ({ connection }) => - connection ? connection.isDefinitionSupported() : false, + connection ? connection.provides('definitionProvider') : false, label: trans.__('Jump to definition'), icon: jumpToIcon }, + { + id: 'jump-to-reference', + execute: async ({ connection, virtual_position, document, features }) => { + const jumpFeature = features.get(FEATURE_ID) as CMJumpToDefinition; + if (!connection) { + jumpFeature.setStatusMessage( + trans.__('Connection not found for jump'), + 2 * 1000 + ); + return; + } + + const positionParams: lsp.TextDocumentPositionParams = { + textDocument: { + uri: document.document_info.uri + }, + position: { + line: virtual_position.line, + character: virtual_position.ch + } + }; + const targets = await connection.clientRequests[ + 'textDocument/references' + ].request({ ...positionParams, context: { includeDeclaration: false } }); + await jumpFeature.handleJump(targets, positionParams); + }, + is_enabled: ({ connection }) => + connection ? connection.provides('referencesProvider') : false, + label: trans.__('Jump to references'), + icon: jumpToIcon + }, { id: 'jump-back', execute: async ({ connection, virtual_position, document, features }) => { @@ -291,7 +433,10 @@ const COMMANDS = (trans: TranslationBundle): IFeatureCommand[] => [ jump_feature.jumper.global_jump_back(); }, is_enabled: ({ connection }) => - connection ? connection.isDefinitionSupported() : false, + connection + ? connection.provides('definitionProvider') || + connection.provides('referencesProvider') + : false, label: trans.__('Jump back'), icon: jumpBackIcon, // do not attach to any of the context menus diff --git a/packages/jupyterlab-lsp/src/positioning.ts b/packages/jupyterlab-lsp/src/positioning.ts index cdbc36964..c54d55679 100644 --- a/packages/jupyterlab-lsp/src/positioning.ts +++ b/packages/jupyterlab-lsp/src/positioning.ts @@ -1,5 +1,6 @@ import { CodeEditor } from '@jupyterlab/codeeditor'; import type * as CodeMirror from 'codemirror'; +import type * as lsp from 'vscode-languageserver-protocol'; /** * is_* attributes are there only to enforce strict interface type checking @@ -68,3 +69,19 @@ export function offset_at_position( export class PositionError extends Error { // no-op } + +export namespace ProtocolCoordinates { + export function isWithinRange( + position: lsp.Position, + range: lsp.Range + ): boolean { + const { line, character } = position; + return ( + line >= range.start.line && + line <= range.end.line && + // need to be non-overlapping see https://github.com/jupyter-lsp/jupyterlab-lsp/issues/628 + (line != range.start.line || character > range.start.character) && + (line != range.end.line || character <= range.end.character) + ); + } +} diff --git a/packages/lsp-ws-connection/src/ws-connection.ts b/packages/lsp-ws-connection/src/ws-connection.ts index 9876a14f1..19eac3eb4 100644 --- a/packages/lsp-ws-connection/src/ws-connection.ts +++ b/packages/lsp-ws-connection/src/ws-connection.ts @@ -247,6 +247,9 @@ export class LspWsConnection ); } + /** + * @deprecated + */ public async getHoverTooltip( location: IPosition, documentInfo: IDocumentInfo, @@ -278,6 +281,9 @@ export class LspWsConnection return hover; } + /** + * @deprecated + */ public async getCompletion( location: IPosition, token: ITokenInfo, @@ -378,6 +384,7 @@ export class LspWsConnection /** * Request the locations of all matching document symbols + * @deprecated */ public async getDocumentHighlights( location: IPosition, @@ -410,6 +417,7 @@ export class LspWsConnection /** * Request a link to the definition of the current symbol. The results will not be displayed * unless they are within the same file URI + * @deprecated */ public async getDefinition( location: IPosition, @@ -445,6 +453,7 @@ export class LspWsConnection /** * Request a link to the type definition of the current symbol. The results will not be displayed * unless they are within the same file URI + * @deprecated */ public async getTypeDefinition( location: IPosition, @@ -480,6 +489,7 @@ export class LspWsConnection /** * Request a link to the implementation of the current symbol. The results will not be displayed * unless they are within the same file URI + * @deprecated */ public getImplementation(location: IPosition, documentInfo: IDocumentInfo) { if (!this.isReady || !this.isImplementationSupported()) { @@ -507,6 +517,7 @@ export class LspWsConnection /** * Request a link to all references to the current symbol. The results will not be displayed * unless they are within the same file URI + * @deprecated */ public async getReferences( location: IPosition, @@ -544,6 +555,7 @@ export class LspWsConnection /** * The characters that trigger completion automatically. + * @deprecated */ public getLanguageCompletionCharacters(): string[] { return this.serverCapabilities?.completionProvider?.triggerCharacters || []; @@ -551,6 +563,7 @@ export class LspWsConnection /** * The characters that trigger signature help automatically. + * @deprecated */ public getLanguageSignatureCharacters(): string[] { return ( @@ -560,6 +573,7 @@ export class LspWsConnection /** * Does the server support go to definition? + * @deprecated */ public isDefinitionSupported() { return !!this.serverCapabilities?.definitionProvider; @@ -567,6 +581,7 @@ export class LspWsConnection /** * Does the server support go to type definition? + * @deprecated */ public isTypeDefinitionSupported() { return !!this.serverCapabilities?.typeDefinitionProvider; @@ -574,6 +589,7 @@ export class LspWsConnection /** * Does the server support go to implementation? + * @deprecated */ public isImplementationSupported() { return !!this.serverCapabilities?.implementationProvider; @@ -581,6 +597,7 @@ export class LspWsConnection /** * Does the server support find all references? + * @deprecated */ public isReferencesSupported() { return !!this.serverCapabilities?.referencesProvider;