diff --git a/src-node/lsp-client.js b/src-node/lsp-client.js index f8929ec9f2..02800d35d4 100644 --- a/src-node/lsp-client.js +++ b/src-node/lsp-client.js @@ -218,6 +218,12 @@ exports.startServer = async function startServer(params) { if (code) { console.error(`[lsp-client][${serverId}] exited code=${code} signal=${signal || 'none'}`); } + // Fail every in-flight request immediately - the dead process can never answer them, and + // leaving them pending stalls callers until the per-request timeout (2 minutes). + for (const { reject: rejectPending } of serverState.pending.values()) { + rejectPending(new Error(`Server ${serverId} exited with pending request`)); + } + serverState.pending.clear(); nodeConnector.triggerPeer('serverExit', { serverId, code, signal, stderr }); if (!hasResolved) { hasResolved = true; diff --git a/src/extensions/default/TypeScriptSupport/unittests.js b/src/extensions/default/TypeScriptSupport/unittests.js index 35b2b20d7a..28a78e896b 100644 --- a/src/extensions/default/TypeScriptSupport/unittests.js +++ b/src/extensions/default/TypeScriptSupport/unittests.js @@ -329,6 +329,43 @@ define(function (require, exports, module) { expect(content("jsconfig.json", { checkJs: true }, false).compilerOptions.checkJs).toBe(false); }); + // LSP quickfixes: diagnostics and fixes are separate channels - after diagnostics land, the + // LintingProvider idle-fetches textDocument/codeAction quickfixes, decorates the cached + // errors, and re-runs inspection so the Fix All button appears. `consol.log(1)` produces + // TS 2552 ("Cannot find name 'consol'") whose quickfix is a single same-file text edit + // ("Change spelling to 'console'") - exactly the admitted shape. Fix application goes + // through Editor.replaceMultipleRanges, so no editor focus is needed (unlike the code-hint + // menu), making this runnable in an unfocused test window. + it("wires LSP quickfixes into the problems panel Fix All workflow", async function () { + const FileSystem = testWindow.brackets.test.FileSystem; + const projectPath = await SpecRunnerUtils.getTempTestDirectory(testRootSpec + "ts"); + await jsPromise(SpecRunnerUtils.createTextFile( + path.join(projectPath, "fixable.ts"), "consol.log(1);\n", FileSystem)); + await SpecRunnerUtils.loadProjectInTestWindow(projectPath); + await awaitsForDone(SpecRunnerUtils.openProjectFiles(["fixable.ts"]), "open fixable.ts"); + + await awaitsFor(function () { + return $("#problems-panel").text().indexOf("Cannot find name 'consol'") !== -1; + }, "the spelling diagnostic to appear in the problems panel", 30000); + + // The idle codeAction fetch (~800ms after diagnostics settle) attaches the fix and + // nudges a re-run - the Fix All button becoming visible proves the whole chain. + await awaitsFor(function () { + const $btn = $("#problems-panel .problems-fix-all-btn"); + return $btn.length && !$btn.hasClass("forced-hidden"); + }, "the Fix All button to appear once quickfixes are fetched", 30000); + + const editor = EditorManager.getCurrentFullEditor(); + $("#problems-panel .problems-fix-all-btn").click(); + await awaitsFor(function () { + return editor.document.getText().indexOf("console.log(1);") !== -1; + }, "the quickfix to change consol -> console", 10000); + + await awaitsForDone(CommandManager.execute(Commands.FILE_CLOSE, { _forceClose: true }), + "close fixable.ts"); + await SpecRunnerUtils.removeTempDirectory(); + }, 90000); + // ----- hover quick-actions (Go to Definition / Find Usages) ------------------------------- // Query the hover popover at a position the same way QuickViewManager does internally. diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index 0cc3db380a..8ca27655ba 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -912,7 +912,7 @@ define(function (require, exports, module) { * htmlMessage:string, * type:?Type , * fix: { // an optional fix, if present will show the fix button - * replace: "text to replace the offset given below", + * replaceText: "text to replace the offset given below", * rangeOffset: { * start: number, * end: number @@ -929,7 +929,7 @@ define(function (require, exports, module) { * @property {string} htmlMessage - The error message to be displayed as HTML. * @property {?Type} type - The type of the error. Defaults to `Type.WARNING` if unspecified. * @property {?Object} fix - An optional fix object. - * @property {string} fix.replace - The text to replace the error with. + * @property {string} fix.replaceText - The text to replace the error with. * @property {Object} fix.rangeOffset - The range within the text to replace. * @property {number} fix.rangeOffset.start - The start offset of the range. * @property {number} fix.rangeOffset.end - The end offset of the range. diff --git a/src/languageTools/DefaultProviders.js b/src/languageTools/DefaultProviders.js index 749fe8b124..f8b0f1ee28 100644 --- a/src/languageTools/DefaultProviders.js +++ b/src/languageTools/DefaultProviders.js @@ -814,11 +814,19 @@ define(function (require, exports, module) { .done(function () { setJumpPosition(startCurPos); $deferredHints.resolve(); + }) + .fail(function () { + $deferredHints.reject(); }); } else { //definition is in current document setJumpPosition(startCurPos); $deferredHints.resolve(); } + } 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 + // the NAVIGATE_JUMPTO_DEFINITION command promise pending forever. + $deferredHints.reject(); } }).fail(function () { $deferredHints.reject(); @@ -832,6 +840,19 @@ define(function (require, exports, module) { this._promiseMap = new Map(); this._lastSignature = new Map(); this._validateOnType = false; + // --- quickfix (textDocument/codeAction) state --- + this._quickFixClient = null; // LanguageClient injected by LSPClient._registerProviders + this._rawDiagnostics = new Map(); // filePath -> raw LSP diagnostics (setInspectionResults flattens) + this._quickFixState = new Map(); // filePath -> {timestamp, signature} of the last fetch + this._quickFixTimer = null; // idle-fetch timer (cleared on every newer publish) + // Diagnostics can arrive while their file sits in a background pane (fetches only run for + // the active file) - when such a file becomes active, schedule the fetch it missed. + const self = this; + EditorManager.on("activeEditorChange", function (evt, current) { + if (current && current.document) { + self._scheduleQuickFixFetch(current.document.file._path); + } + }); } LintingProvider.prototype.setClient = setClient; @@ -843,11 +864,19 @@ define(function (require, exports, module) { this._results.delete(filePath); this._promiseMap.delete(filePath); this._lastSignature.delete(filePath); + this._rawDiagnostics.delete(filePath); + this._quickFixState.delete(filePath); } else { //clear all results this._results.clear(); this._promiseMap.clear(); this._lastSignature.clear(); + this._rawDiagnostics.clear(); + this._quickFixState.clear(); + } + if (this._quickFixTimer) { + clearTimeout(this._quickFixTimer); + this._quickFixTimer = null; } }; @@ -866,18 +895,15 @@ define(function (require, exports, module) { line: obj.range.start.line, ch: obj.range.start.character }, + endPos: { + line: obj.range.end.line, + ch: obj.range.end.character + }, message: obj.message, type: (obj.severity === 1 ? CodeInspection.Type.ERROR : (obj.severity === 2 ? CodeInspection.Type.WARNING : CodeInspection.Type.META)) }; }); - this._results.set(filePath, { - errors: errors - }); - if(this._promiseMap.get(filePath)) { - this._promiseMap.get(filePath).resolve(this._results.get(filePath)); - this._promiseMap.delete(filePath); - } // Language servers re-publish diagnostics in waves (e.g. syntax then semantic passes) and // again on every edit - frequently with identical content. Re-running inspection only to // render the same problems rebuilds the Problems panel for nothing, which both wastes work @@ -890,6 +916,19 @@ define(function (require, exports, module) { previous = this._lastSignature.has(filePath) ? this._lastSignature.get(filePath) : "[]", changed = previous !== signature; this._lastSignature.set(filePath, signature); + + // Keep the EXISTING errors array when content is unchanged: the quickfix layer decorates the + // cached errors with `fix` after an async codeAction fetch, and replacing the array on an + // identical re-publish would silently wipe those fixes without triggering a re-fetch. + if (changed || !this._results.has(filePath)) { + this._results.set(filePath, { + errors: errors + }); + } + if(this._promiseMap.get(filePath)) { + this._promiseMap.get(filePath).resolve(this._results.get(filePath)); + this._promiseMap.delete(filePath); + } if (this._validateOnType && changed) { var editor = EditorManager.getActiveEditor(), docPath = editor ? editor.document.file._path : ""; @@ -905,6 +944,193 @@ define(function (require, exports, module) { } }; + // ----- quickfixes (textDocument/codeAction) ------------------------------------------------ + // Diagnostics and their fixes are separate LSP channels: fixes must be REQUESTED per range with + // the diagnostics as context. CodeInspection needs fixes ON the errors at scan time, so the flow + // is: retain the raw diagnostics -> idle-fetch code actions (never in the typing hot path) -> + // decorate the cached errors with { replaceText, rangeOffset } -> requestRun() so CodeInspection + // re-pulls and registers them (fix icons + Fix All appear). Server-agnostic: gated purely on the + // server's codeActionProvider capability via LanguageClient.requestCodeActions. + + // Fetch this long after diagnostics settle. Typing produces a new publish per change wave, which + // reschedules - so no codeAction traffic while the user is actively typing, and a fetched fix is + // never dead-on-arrival (CodeInspection drops fixes whose document changed anyway). + var QUICKFIX_IDLE_MS = 800; + // Per-diagnostic fallback cap, for servers that don't answer a whole-file batched request. + var MAX_QUICKFIX_REQUESTS = 20; + + /** + * Record the raw diagnostics for a file and schedule an idle quickfix fetch. Called by the + * publishDiagnostics handler right after setInspectionResults. + * @param {string} filePath + * @param {Array} rawDiagnostics - unflattened LSP diagnostics (range/code/data intact) + */ + LintingProvider.prototype.updateQuickFixes = function (filePath, rawDiagnostics) { + this._rawDiagnostics.set(filePath, rawDiagnostics || []); + this._scheduleQuickFixFetch(filePath); + }; + + LintingProvider.prototype._scheduleQuickFixFetch = function (filePath) { + var self = this; + if (!this._quickFixClient || !this._rawDiagnostics.has(filePath)) { + return; + } + var editor = EditorManager.getActiveEditor(); + if (!editor || editor.document.file._path !== filePath || + !this._quickFixClient.servesDocument(editor)) { + return; // fixes are only fetched for the active, server-synced document + } + var state = this._quickFixState.get(filePath); + if (state && state.timestamp === editor.document.lastChangeTimestamp && + state.signature === this._lastSignature.get(filePath)) { + return; // already fetched for this exact document + diagnostics content + } + if (this._quickFixTimer) { + clearTimeout(this._quickFixTimer); + } + this._quickFixTimer = setTimeout(function () { + self._quickFixTimer = null; + self._fetchQuickFixes(filePath); + }, QUICKFIX_IDLE_MS); + }; + + /** + * @private + * The single admitted fix of a code action, or null. Admitted: a literal quickfix CodeAction, + * not disabled, whose WorkspaceEdit touches EXACTLY this file with EXACTLY one TextEdit (the + * CodeInspection fix contract is one contiguous same-document replace). Legacy bare Commands + * (no edit to apply client-side) are skipped. + * @return {?{replaceText:string, rangeOffset:{start:number, end:number}}} + */ + LintingProvider.prototype._fixFromAction = function (action, editor, filePath) { + if (!action || !action.title || action.disabled) { + return null; + } + if (!action.kind || action.kind.indexOf("quickfix") !== 0) { + return null; // not a quickfix (servers may ignore context.only), or a bare Command + } + var edit = action.edit; + if (!edit) { + return null; + } + var uri = null, edits = null; + if (edit.documentChanges && edit.documentChanges.length === 1 && + edit.documentChanges[0].textDocument) { + uri = edit.documentChanges[0].textDocument.uri; + edits = edit.documentChanges[0].edits; + } else if (edit.changes) { + var uris = Object.keys(edit.changes); + if (uris.length === 1) { + uri = uris[0]; + edits = edit.changes[uri]; + } + } + if (!uri || !edits || edits.length !== 1 || typeof edits[0].newText !== "string") { + return null; // multi-file / multi-edit / create-rename ops - beyond the one-replace contract + } + // Compare as decoded platform paths so URI encoding differences can't break the match. + var ownUri = this._quickFixClient.uriForPath(filePath); + if (PathConverters.uriToPath(uri) !== PathConverters.uriToPath(ownUri)) { + return null; // edit lands in a different file + } + var range = edits[0].range; + return { + replaceText: edits[0].newText, + rangeOffset: { + start: editor.indexFromPos({ line: range.start.line, ch: range.start.character }), + end: editor.indexFromPos({ line: range.end.line, ch: range.end.character }) + } + }; + }; + + // Attach an action's fix to the cached error matching one of the action's diagnostics. + // `forDiagnostic` pins the target in per-diagnostic fallback mode, where the request itself + // identifies the diagnostic even when the server omits action.diagnostics. + LintingProvider.prototype._attachFix = function (errors, action, fix, forDiagnostic) { + var diags = (action.diagnostics && action.diagnostics.length) ? action.diagnostics + : (forDiagnostic ? [forDiagnostic] : []); + for (var d = 0; d < diags.length; d++) { + var diag = diags[d]; + for (var i = 0; i < errors.length; i++) { + var err = errors[i]; + if (err.pos.line === diag.range.start.line && err.pos.ch === diag.range.start.character && + err.message === diag.message && (!err.fix || action.isPreferred)) { + err.fix = fix; + return true; + } + } + } + return false; + }; + + LintingProvider.prototype._fetchQuickFixes = async function (filePath) { + var self = this; + var client = this._quickFixClient; + var editor = EditorManager.getActiveEditor(); + var diagnostics = this._rawDiagnostics.get(filePath) || []; + var cached = this._results.get(filePath); + if (!client || !editor || editor.document.file._path !== filePath || + !client.servesDocument(editor) || !diagnostics.length || !cached) { + return; + } + var fetchedAt = editor.document.lastChangeTimestamp; + // Record the fetch attempt up front so identical re-publishes don't re-fetch, even when the + // server turns out to have no fixes for these diagnostics. + this._quickFixState.set(filePath, { + timestamp: fetchedAt, + signature: this._lastSignature.get(filePath) + }); + + try { + // One batched whole-file request first (spec-legal, one round trip for every fix)... + var lastLine = editor.lineCount() - 1; + var wholeFile = { + start: { line: 0, character: 0 }, + end: { line: lastLine, character: editor.document.getLine(lastLine).length } + }; + var actions = await client.requestCodeActions({ + filePath: filePath, range: wholeFile, diagnostics: diagnostics + }); + var attached = 0; + if (actions && actions.length) { + for (var i = 0; i < actions.length; i++) { + var resolved = await client.resolveCodeAction(actions[i]); // no-op unless deferred + var fix = this._fixFromAction(resolved, editor, filePath); + if (fix && this._attachFix(cached.errors, resolved, fix, null)) { + attached++; + } + } + } else { + // ...falling back to capped per-diagnostic requests for servers that don't answer + // the batched form. + var capped = diagnostics.slice(0, MAX_QUICKFIX_REQUESTS); + for (var d = 0; d < capped.length; d++) { + var acts = await client.requestCodeActions({ + filePath: filePath, range: capped[d].range, diagnostics: [capped[d]] + }); + for (var a = 0; acts && a < acts.length; a++) { + var res = await client.resolveCodeAction(acts[a]); + var f = this._fixFromAction(res, editor, filePath); + if (f && this._attachFix(cached.errors, res, f, capped[d])) { + attached++; + break; // one fix per diagnostic + } + } + } + } + // Re-render only when something changed AND the document didn't move under us (a moved + // document gets fresh diagnostics -> a fresh fetch; its fixes would be dropped anyway). + var activeEditor = EditorManager.getActiveEditor(); + if (attached && activeEditor && activeEditor.document.file._path === filePath && + activeEditor.document.lastChangeTimestamp === fetchedAt && + self._isRegisteredInspector(filePath)) { + CodeInspection.requestRun(); + } + } catch (err) { + console.warn("[LSP] quickfix fetch failed:", err && (err.message || err)); + } + }; + /** * @private * @param {string} filePath - active document path diff --git a/src/languageTools/LSPClient.js b/src/languageTools/LSPClient.js index ab6b964783..6b286db83e 100644 --- a/src/languageTools/LSPClient.js +++ b/src/languageTools/LSPClient.js @@ -213,6 +213,10 @@ define(function (require, exports, module) { uri: vfsUri, diagnostics: diagnostics }); + // Hand the RAW diagnostics (setInspectionResults flattens them, losing range/code/data) + // to the quickfix layer, which idle-fetches textDocument/codeAction fixes for them and + // decorates the cached results - see LintingProvider.updateQuickFixes. + client.lintingProvider.updateQuickFixes(vfsPath, diagnostics); } } @@ -531,6 +535,62 @@ define(function (require, exports, module) { return deferred.promise(); }; + /** + * Quickfix code actions for the given range's diagnostics (LSP textDocument/codeAction). + * Server-agnostic: gated on the server's codeActionProvider capability (boolean or object). + * @param {{filePath:string, range:Object, diagnostics:Array}} params - `range` is an LSP + * range ({start:{line,character}, end:{...}}) and `diagnostics` the raw LSP diagnostics to + * pass as context (servers key their fixes off these). + * @return {jQuery.Promise} resolves the server's (CodeAction|Command)[] or [] + */ + LanguageClient.prototype.requestCodeActions = function (params) { + const self = this; + const deferred = $.Deferred(); + if (!this.capabilities || !this.capabilities.codeActionProvider) { + return deferred.resolve([]).promise(); // server offers no code actions + } + (async function () { + try { + await DocumentSync.flush(self, params.filePath); + const result = await self._request("textDocument/codeAction", { + textDocument: { uri: self.uriForPath(params.filePath) }, + range: params.range, + context: { + diagnostics: params.diagnostics || [], + only: ["quickfix"] + } + }); + deferred.resolve(Array.isArray(result) ? result : []); + } catch (err) { + console.warn("[LSP] request failed:", err && (err.message || err)); + deferred.reject(err); + } + }()); + return deferred.promise(); + }; + + /** + * Fill in a CodeAction's deferred properties (typically `edit`) via codeAction/resolve. We don't + * advertise resolveSupport, so conformant servers inline edits and this is never needed - it + * exists for servers that defer anyway, gated on their codeActionProvider.resolveProvider. + * Mirrors resolveCompletion: always resolves, falling back to the unresolved action. + * @param {Object} action - the CodeAction as returned by requestCodeActions + * @return {jQuery.Promise} + */ + LanguageClient.prototype.resolveCodeAction = function (action) { + const deferred = $.Deferred(); + const provider = this.capabilities && this.capabilities.codeActionProvider; + if (!provider || !provider.resolveProvider) { + return deferred.resolve(action).promise(); + } + this._request("codeAction/resolve", action).then(function (resolved) { + deferred.resolve(resolved || action); + }, function () { + deferred.resolve(action); + }); + return deferred.promise(); + }; + // ------------------------------------------------------------------------------------------ // Server lifecycle + provider registration // ------------------------------------------------------------------------------------------ @@ -567,7 +627,17 @@ define(function (require, exports, module) { }, definition: { dynamicRegistration: false }, references: { dynamicRegistration: false }, - publishDiagnostics: { relatedInformation: false } + publishDiagnostics: { relatedInformation: false }, + codeAction: { + dynamicRegistration: false, + // Literal CodeAction support (title/kind/edit) - without this servers degrade to + // bare Commands, which carry no WorkspaceEdit we could apply. resolveSupport is + // deliberately NOT advertised: conformant servers then inline `edit` in the + // codeAction response, sparing a codeAction/resolve round-trip per fix. + codeActionLiteralSupport: { + codeActionKind: { valueSet: ["quickfix"] } + } + } }, workspace: { workspaceFolders: true, configuration: false } }; @@ -634,6 +704,9 @@ define(function (require, exports, module) { // recorded so the provider can tell whether it is still a participating inspector before nudging a // re-run on async diagnostics. client.lintingProvider._inspectionProviderName = client.serverId; + // The quickfix layer requests textDocument/codeAction through this client (capability-gated + // inside requestCodeActions), keeping the provider itself transport-agnostic. + client.lintingProvider._quickFixClient = client; client.hover = new HoverProvider.HoverProvider(client); CodeHintManager.registerHintProvider(client.codeHints, langs, DEFAULT_PRIORITY); diff --git a/src/styles/brackets.less b/src/styles/brackets.less index 2b8e8945c3..1eee29d54b 100644 --- a/src/styles/brackets.less +++ b/src/styles/brackets.less @@ -2221,6 +2221,11 @@ a, img { .lsp-hover-quickview { text-align: left; + // The QuickView popover centers + shrink-wraps each provider's content (.quick-view-item is a + // flex column with align-items:center - right for image/color previews). When another provider + // (e.g. a problem row with its Fix button) makes the popover wider, our content would float + // centered in the leftover space - stretch to the available width instead. + align-self: stretch; max-width: 520px; padding: 2px 4px; font-size: 12.5px; diff --git a/test/spec/EditorCommandHandlers-integ-test.js b/test/spec/EditorCommandHandlers-integ-test.js index 4b33190445..7a0489db0b 100644 --- a/test/spec/EditorCommandHandlers-integ-test.js +++ b/test/spec/EditorCommandHandlers-integ-test.js @@ -115,7 +115,27 @@ define(function (require, exports, module) { EditorManager = testWindow.brackets.test.EditorManager; await SpecRunnerUtils.loadProjectInTestWindow(testPath); - }, 30000); + + if (testWindow.Phoenix.isNativeApp) { + // Warm up the TypeScript language server here, where the one-time cold start (spawn + // vtsls + tsserver loading the TypeScript library and project) belongs - so the + // jump-to-definition spec below only needs a short wait for its own request instead + // of budgeting for a cold server (its very first request could otherwise pend longer + // than any reasonable spec timeout on slow/loaded CI runners). + await awaitsForDone( + CommandManager.execute(Commands.CMD_ADD_TO_WORKINGSET_AND_OPEN, + {fullPath: testPath + "/test.js"}), + "warm-up: open test.js"); + const LSPClient = await new Promise(function (resolve) { + testWindow.brackets.getModule(["languageTools/LSPClient"], resolve); + }); + await awaitsFor(function () { + return LSPClient.isLintingProviderActive("javascript"); + }, "the TypeScript language server to finish its cold start", 90000); + await awaitsForDone(CommandManager.execute(Commands.FILE_CLOSE_ALL, { _forceClose: true }), + "close warm-up file"); + } + }, 120000); afterAll(async function () { await closeTestWindow(); @@ -202,17 +222,34 @@ define(function (require, exports, module) { // JavaScript at a higher priority than the built-in Tern provider. vtsls returns // testMe's full declaration range and we jump to its start (the `function` // keyword), giving a collapsed cursor at {0,0} - whereas Tern selects the - // identifier name. The server can take a moment to prime after the file opens, so - // retry the jump until the LSP result lands rather than using a fixed wait (which - // flakes on slow CI: before vtsls is ready, Tern answers with {0,9}-{0,15}). + // identifier name. The server was already warmed up in beforeAll, so 15s is + // plenty here. Each attempt is time-boxed: a slow or unanswered request counts + // as a retry instead of pinning the whole wait (awaitsFor races each pollFn + // invocation against the FULL timeout, so one hung attempt would otherwise eat + // the entire budget), and a rejected jump ("no definition yet") also retries - + // awaitsFor aborts outright on a pollFn exception, so the promise is absorbed. await awaitsFor(async function () { myEditor.setCursorPos({line: 5, ch: 8}); - await awaitsForDone(CommandManager.execute(Commands.NAVIGATE_JUMPTO_DEFINITION), - "Jump To Definition"); + var landed = await new Promise(function (resolve) { + var settled = false; + function settle(val) { + if (!settled) { + settled = true; + resolve(val); + } + } + CommandManager.execute(Commands.NAVIGATE_JUMPTO_DEFINITION) + .done(function () { settle(true); }) + .fail(function () { settle(false); }); + setTimeout(function () { settle(false); }, 3000); + }); + if (!landed) { + return false; + } var sel = myEditor.getSelection(); return sel.start.line === 0 && sel.start.ch === 0 && sel.end.line === 0 && sel.end.ch === 0; - }, "LSP jump-to-definition to land on the testMe declaration", 30000); + }, "LSP jump-to-definition to land on the testMe declaration", 15000, 500); selection = myEditor.getSelection(); expect(fixSel(selection)).toEql(fixSel({ start: {line: 0, ch: 0},