diff --git a/src/features/QuickViewManager.js b/src/features/QuickViewManager.js index 17a47f7adf..2970f9fd48 100644 --- a/src/features/QuickViewManager.js +++ b/src/features/QuickViewManager.js @@ -241,6 +241,23 @@ define(function (require, exports, module) { animationRequest, quickViewLocked = false; + // True while some other menu-like UI is open - the top menu bar, a context menu (including the + // editor's right-click menu), the autocomplete Code Hints list, or an InlineMenu picker like + // jump-to-definition's multi-target picker (see languageTools/DefaultProviders.js) or Extract + // to Variable/Function. QuickView shouldn't pop up while any of those has the user's attention. + // All of them share the same underlying convention - a `
" + _.escape(excerpt) + ""; + } + } + _showDocPopup(inlineMenu.$menu.find("ul.dropdown-menu"), titleHtml + codeHtml); + }).fail(function () { + if (myToken === hoverToken) { + _hideDocPopup(); + } + }); + } + + inlineMenu.onHover(function (id) { + // The very first selection (index 0) is set synchronously inside InlineMenu.open(), + // before $menu has been appended to the DOM - _showDocPopup's positioning needs a + // connected element, so defer just that one call by a tick. + if (!inlineMenu.$menu.closest("body").length) { + setTimeout(function () { showExcerptFor(id); }, 0); + } else { + showExcerptFor(id); + } + }); + + inlineMenu.onSelect(function (id) { + _hideDocPopup(); + inlineMenu.close(); + jumpToLocation(locations[id]); + }); + + inlineMenu.onClose(function () { + _hideDocPopup(); + inlineMenu.close(); + $deferredHints.reject(); + }); + + // Dismiss a quickview that happened to already be showing (e.g. the user pressed + // Ctrl+J while hovering something). QuickViewManager itself won't show a new one while + // this picker's .inlinemenu-menu.open is in the DOM - checked live, not via a + // suppress/close pairing, so it can't get stuck if the picker closes via a path other + // than onSelect/onClose (e.g. clicking elsewhere - InlineMenu doesn't notify on that). + QuickViewManager.hideQuickView(); + inlineMenu.open(items); + } + + // A single definition location is frequently just the base/interface declaration for a + // polymorphic call (obj.method() where obj's type has several concrete overrides) - LSP's + // "go to definition" intentionally resolves to one canonical declaration for that case. + // When that happens, ask the server for implementations too: if it has more than one, + // that's the real "which override did you mean" answer the user is after. See #3093. + function fallBackToImplementations(singleLocation) { + var serverCapabilities = client.getServerCapabilities(); + if (!serverCapabilities || !serverCapabilities.implementationProvider) { + jumpToLocation(singleLocation); + return; + } + + client.gotoImplementation({ + filePath: docPath, + cursorPos: pos + }).done(function (implResult) { + var implLocations = Array.isArray(implResult) ? implResult : (implResult ? [implResult] : []), + merged = mergeLocations(singleLocation, implLocations); + if (merged.length > 1) { + Metrics.countEvent(Metrics.EVENT_TYPE.LSP, "def", "Impl." + metricLabel); + showJumpTargetPicker(merged, 1); // merged[0] is always the declaration slot + } else { + jumpToLocation(merged[0]); + } + }).fail(function () { + jumpToLocation(singleLocation); + }); + } + + client.gotoDefinition({ filePath: docPath, cursorPos: pos }).done(function (msgObj) { - //For Older servers + if (Array.isArray(msgObj) && msgObj.length > 1) { + showJumpTargetPicker(msgObj); + return; + } + + //For Older servers that always return an array if (Array.isArray(msgObj)) { msgObj = msgObj[msgObj.length - 1]; } if (msgObj && msgObj.range) { - var docUri = msgObj.uri, - startCurPos = {}; - startCurPos.line = msgObj.range.start.line; - startCurPos.ch = msgObj.range.start.character; - - if (docUri !== docPathUri) { - let documentPath = PathConverters.uriToPath(docUri); - CommandManager.execute(Commands.FILE_OPEN, { - fullPath: documentPath - }) - .done(function () { - setJumpPosition(startCurPos); - $deferredHints.resolve(); - }) - .fail(function () { - $deferredHints.reject(); - }); - } else { //definition is in current document - setJumpPosition(startCurPos); - $deferredHints.resolve(); - } + fallBackToImplementations(msgObj); } else { // No definition at this position (servers answer null/[] - e.g. tsserver while it // is still loading the project). MUST settle: an unresolved deferred here leaves @@ -1342,4 +1703,6 @@ define(function (require, exports, module) { exports.showHintDocPopup = _showDocPopup; exports.hideHintDocPopup = _hideDocPopup; exports._docPopupHtml = _docPopupHtml; // exposed for unit tests + exports._buildJumpToDefExcerpt = buildExcerpt; // exposed for unit tests + exports._mergeJumpToDefLocations = mergeLocations; // exposed for unit tests }); diff --git a/src/languageTools/LSPClient.js b/src/languageTools/LSPClient.js index d8141183e8..f2c630cc10 100644 --- a/src/languageTools/LSPClient.js +++ b/src/languageTools/LSPClient.js @@ -341,6 +341,7 @@ define(function (require, exports, module) { "textDocument/signatureHelp": "Sig", "textDocument/hover": "Hover", "textDocument/definition": "Def", + "textDocument/implementation": "Impl", "textDocument/references": "Ref", "textDocument/codeAction": "Fix", "completionItem/resolve": "Res", @@ -570,14 +571,15 @@ define(function (require, exports, module) { return deferred.promise(); }; - LanguageClient.prototype.gotoDefinition = function (params) { - const self = this; + // Shared by gotoDefinition/gotoImplementation - both are "resolve a position to one or more + // {uri, range} locations" requests that only differ in LSP method name. + function _requestLocations(client, params, method) { const deferred = $.Deferred(); (async function () { try { - await DocumentSync.flush(self, params.filePath); - const result = await self._request("textDocument/definition", { - textDocument: { uri: self.uriForPath(params.filePath) }, + await DocumentSync.flush(client, params.filePath); + const result = await client._request(method, { + textDocument: { uri: client.uriForPath(params.filePath) }, position: _positionOf(params.cursorPos) }); if (!result || (Array.isArray(result) && !result.length)) { @@ -600,6 +602,18 @@ define(function (require, exports, module) { } }()); return deferred.promise(); + } + + LanguageClient.prototype.gotoDefinition = function (params) { + return _requestLocations(this, params, "textDocument/definition"); + }; + + // Finds concrete implementations of the symbol at a position. Used as a fallback when + // gotoDefinition resolves to a single canonical declaration (e.g. a base class/interface + // method) for what is actually a polymorphic call site - see DefaultProviders.js doJumpToDef + // and https://github.com/phcode-dev/phoenix/issues/3093. + LanguageClient.prototype.gotoImplementation = function (params) { + return _requestLocations(this, params, "textDocument/implementation"); }; LanguageClient.prototype.findReferences = function (params) { diff --git a/src/nls/root/strings.js b/src/nls/root/strings.js index 36a2a2806f..704cad2a44 100644 --- a/src/nls/root/strings.js +++ b/src/nls/root/strings.js @@ -1577,6 +1577,9 @@ define({ // extensions/default/JavaScriptCodeHints "CMD_JUMPTO_DEFINITION": "Go to Definition", + "JUMPTO_DEFINITION_SELECT_TARGET": "Select a definition", + "JUMPTO_DEFINITION_LINE_LABEL": "Line {0}", + "JUMPTO_DEFINITION_IMPLEMENTATION_BADGE": "Implementation", "CMD_SHOW_PARAMETER_HINT": "Show Parameter Hint", "NO_ARGUMENTS": "
, not inside it
+ return $(".lsp-hint-doc-popup").text();
+ }
+
+ // One jump attempt capped at 3s (same pattern/reasoning as
+ // EditorCommandHandlers-integ-test.js's attemptJumpToDefinition): a hung or
+ // not-yet-ready request just counts as a failed attempt so awaitsFor can retry
+ // instead of the whole budget being pinned on one slow request. Stashes the
+ // command's own promise on lastJumpPromise so a successful attempt's picker can be
+ // interacted with afterward without re-invoking the command (which would open a
+ // second picker on top of the one already showing).
+ var lastJumpPromise = null;
+ function attemptOpenPicker(editor, line, ch) {
+ return new Promise(function (resolve) {
+ editor.setCursorPos(line, ch);
+ lastJumpPromise = CommandManager.execute(Commands.NAVIGATE_JUMPTO_DEFINITION);
+ setTimeout(function () { resolve(getOpenMenuItems().length); }, 3000);
+ });
+ }
+
+ it("should show the multi-target picker and correct excerpts for a real click " +
+ "on obj.sayHello()", async function () {
+ await awaitsForDone(
+ CommandManager.execute(Commands.CMD_ADD_TO_WORKINGSET_AND_OPEN,
+ { fullPath: testPath + "/" + REAL_FILE }),
+ "open " + REAL_FILE
+ );
+ var editor = EditorManager.getCurrentFullEditor(),
+ callLine = editor.document.getLine(REAL_CALL_LINE),
+ callCh = callLine.indexOf("sayHello") + 2;
+
+ await awaitsFor(async function () {
+ return (await attemptOpenPicker(editor, REAL_CALL_LINE, callCh)) === 3;
+ }, "vtsls to resolve obj.sayHello() to John/Jane/Alice's 3 implementations", 20000, 500);
+
+ var johnIdx = itemIndexContaining("17:3"), // JohnClass.sayHello, 1-based
+ janeIdx = itemIndexContaining("23:3"), // JaneClass.sayHello, 1-based
+ aliceIdx = itemIndexContaining("13:3"); // AliceClass.sayHello, 1-based
+ expect(johnIdx).toBeGreaterThan(-1);
+ expect(janeIdx).toBeGreaterThan(-1);
+ expect(aliceIdx).toBeGreaterThan(-1);
+
+ // Same-file candidates (John/Jane) show REAL_FILE's own name, not the other
+ // file's - and John/Jane sayHello is each class's only member, directly
+ // adjacent to the declaration, so no "..." should be implied where nothing
+ // was hidden.
+ var johnItemText = getOpenMenuItems().eq(johnIdx).text();
+ expect(johnItemText).toContain(REAL_FILE);
+ expect(johnItemText).not.toContain(REAL_ALICE_FILE);
+ var johnExcerpt = await excerptTextForItem(johnIdx);
+ expect(johnExcerpt).toContain(REAL_FILE);
+ expect(johnExcerpt).not.toContain(REAL_ALICE_FILE);
+ expect(johnExcerpt).toContain("class JohnClass extends MyBaseClass {");
+ expect(johnExcerpt).toContain("Hello, John!");
+ expect(johnExcerpt).not.toContain("...");
+
+ var janeExcerpt = await excerptTextForItem(janeIdx);
+ expect(janeExcerpt).toContain("class JaneClass extends MyBaseClass {");
+ expect(janeExcerpt).toContain("Hello, Jane!");
+ expect(janeExcerpt).not.toContain("...");
+
+ // Alice lives in a different file (REAL_ALICE_FILE) - the item/excerpt must
+ // say so using the real server-provided URI, not REAL_FILE's name. yellow()
+ // sits between the declaration and sayHello (top "..."), and sayHello's own
+ // body is 21 lines long (bottom "...") - both collapse markers should fire in
+ // the same excerpt, yellow() itself never shown.
+ var aliceItemText = getOpenMenuItems().eq(aliceIdx).text();
+ expect(aliceItemText).toContain(REAL_ALICE_FILE);
+ var aliceExcerpt = await excerptTextForItem(aliceIdx);
+ expect(aliceExcerpt).toContain(REAL_ALICE_FILE);
+ expect(aliceExcerpt).toContain("class AliceClass extends MyBaseClass {");
+ expect(aliceExcerpt).not.toContain("yellow");
+ expect(aliceExcerpt).not.toContain("not so soon");
+ expect((aliceExcerpt.match(/\.\.\./g) || []).length).toBe(2);
+ var helloAliceCount = (aliceExcerpt.match(/Hello, Alice!/g) || []).length;
+ expect(helloAliceCount).toBeGreaterThan(0);
+ expect(helloAliceCount).toBeLessThan(21);
+
+ // Click Alice's candidate (on the picker already open from the last
+ // successful attempt above) and confirm the jump actually switches to
+ // REAL_ALICE_FILE and lands on the right line there.
+ getOpenMenuItems().eq(aliceIdx).trigger("click");
+ await awaitsForDone(lastJumpPromise, "jump to AliceClass.sayHello");
+
+ expect(getOpenMenuItems().length).toBe(0);
+ var aliceEditor = EditorManager.getCurrentFullEditor();
+ expect(aliceEditor.document.file.name).toBe(REAL_ALICE_FILE);
+ var pos = aliceEditor.getCursorPos();
+ expect(pos.line).toBe(12); // " sayHello() {" in AliceClass, 0-based
+ }, 35000);
+ });
+ }
+
+ it("should no-op without a picker when no definition is found", async function () {
+ registerMockProvider([]);
+ var editor = await openPolymorphismFile();
+ var posBefore = editor.getCursorPos();
+
+ await awaitsForFail(CommandManager.execute(Commands.NAVIGATE_JUMPTO_DEFINITION),
+ "no definition found rejects instead of hanging");
+
+ expect(getOpenMenuItems().length).toBe(0);
+ var posAfter = editor.getCursorPos();
+ expect(posAfter.line).toBe(posBefore.line);
+ expect(posAfter.ch).toBe(posBefore.ch);
+ });
+ });
+});
diff --git a/test/spec/JumpToDefinitionMultiTarget-test-files/aliceClass.js b/test/spec/JumpToDefinitionMultiTarget-test-files/aliceClass.js
new file mode 100644
index 0000000000..2591fe2fd6
--- /dev/null
+++ b/test/spec/JumpToDefinitionMultiTarget-test-files/aliceClass.js
@@ -0,0 +1,11 @@
+// A third override living in its own file, used to exercise the "jump target is in a
+// different document" branch of doJumpToDef's picker (see JumpToDefinitionMultiTarget-integ-test.js).
+const { MyBaseClass } = require("./polymorphism");
+
+class AliceClass extends MyBaseClass {
+ sayHello() {
+ console.log("Hello from AliceClass");
+ }
+}
+
+module.exports = { AliceClass };
diff --git a/test/spec/JumpToDefinitionMultiTarget-test-files/polymorphism.js b/test/spec/JumpToDefinitionMultiTarget-test-files/polymorphism.js
new file mode 100644
index 0000000000..185a6c7605
--- /dev/null
+++ b/test/spec/JumpToDefinitionMultiTarget-test-files/polymorphism.js
@@ -0,0 +1,25 @@
+// Fixture modeled on the https://github.com/phcode-dev/phoenix/issues/3093 repro: a base class
+// whose method is overridden by several subclasses, called polymorphically.
+class MyBaseClass {
+ sayHello() {
+ console.log("Hello from MyBaseClass");
+ }
+}
+
+class JohnClass extends MyBaseClass {
+ sayHello() {
+ console.log("Hello from JohnClass");
+ }
+}
+
+class JaneClass extends MyBaseClass {
+ sayHello() {
+ console.log("Hello from JaneClass");
+ }
+}
+
+function greet(person) {
+ person.sayHello();
+}
+
+module.exports = { MyBaseClass, JohnClass, JaneClass, greet };
diff --git a/test/spec/JumpToDefinitionMultiTarget-test-files/realLspAliceClass.js b/test/spec/JumpToDefinitionMultiTarget-test-files/realLspAliceClass.js
new file mode 100644
index 0000000000..f6bab32f34
--- /dev/null
+++ b/test/spec/JumpToDefinitionMultiTarget-test-files/realLspAliceClass.js
@@ -0,0 +1,38 @@
+// AliceClass lives here, separate from realLspPolymorphism.js, so the real-vtsls spec in
+// JumpToDefinitionMultiTarget-integ-test.js can verify the picker/excerpt correctly show and open
+// a candidate in a different file (real server-provided URI, not a hand-built mock one). It has an
+// extra sibling method (yellow) and a long sayHello body so the same real jump also exercises both
+// excerpt collapse cases (a member between the declaration and target, and a target body too long
+// to show in full) alongside John/Jane's plain adjacent-declaration case in the other file.
+const { MyBaseClass } = require("./realLspPolymorphism");
+
+class AliceClass extends MyBaseClass {
+ yellow() {
+ console.log("not so soon");
+ }
+ sayHello() {
+ console.log("Hello, Alice!");
+ console.log("Hello, Alice!");
+ console.log("Hello, Alice!");
+ console.log("Hello, Alice!");
+ console.log("Hello, Alice!");
+ console.log("Hello, Alice!");
+ console.log("Hello, Alice!");
+ console.log("Hello, Alice!");
+ console.log("Hello, Alice!");
+ console.log("Hello, Alice!");
+ console.log("Hello, Alice!");
+ console.log("Hello, Alice!");
+ console.log("Hello, Alice!");
+ console.log("Hello, Alice!");
+ console.log("Hello, Alice!");
+ console.log("Hello, Alice!");
+ console.log("Hello, Alice!");
+ console.log("Hello, Alice!");
+ console.log("Hello, Alice!");
+ console.log("Hello, Alice!");
+ console.log("Hello, Alice!");
+ }
+}
+
+module.exports = { AliceClass };
diff --git a/test/spec/JumpToDefinitionMultiTarget-test-files/realLspPolymorphism.js b/test/spec/JumpToDefinitionMultiTarget-test-files/realLspPolymorphism.js
new file mode 100644
index 0000000000..ca1e359c7e
--- /dev/null
+++ b/test/spec/JumpToDefinitionMultiTarget-test-files/realLspPolymorphism.js
@@ -0,0 +1,38 @@
+// Real-LSP repro fixture (see #3093) - used only by the native-app "real vtsls" spec in
+// JumpToDefinitionMultiTarget-integ-test.js, deliberately kept separate from the mocked-provider
+// fixtures (polymorphism.js / aliceClass.js) used by the rest of that file's tests.
+//
+// AliceClass lives in realLspAliceClass.js (imported below) rather than here, same split as the
+// mocked fixtures - so the picker's cross-file candidate (a different filename shown/opened) is
+// exercised against a real server-provided URI too, not just the hand-built mock ones.
+const { AliceClass } = require("./realLspAliceClass");
+
+class MyBaseClass {
+ sayHello() {
+ throw new Error("Method not implemented");
+ }
+}
+
+class JohnClass extends MyBaseClass {
+ sayHello() {
+ console.log("Hello, John!");
+ }
+}
+
+class JaneClass extends MyBaseClass {
+ sayHello() {
+ console.log("Hello, Jane!");
+ }
+}
+
+const myArray = [
+ new JohnClass(),
+ new JaneClass(),
+ new AliceClass()
+];
+
+for (const obj of myArray) {
+ obj.sayHello();
+}
+
+module.exports = { MyBaseClass, JohnClass, JaneClass };