Skip to content

Commit 9fbea91

Browse files
committed
fix(lsp): only reuse cached completions when provably a candidate superset
The completion cache was keyed only by word context (file|line|word start), so a list fetched at a bare position was refiltered client-side for every later keystroke on the line. intelephense answers a bare position with an empty list (cold index) or an arbitrary grab-bag of symbols marked complete (warm index) - neither is a superset, so typing is_ after a blank-line Ctrl-Space showed no completions, or is_int vanished from its own hint list. The cache now stores the query it was fetched with and is reused only when the current query extends it, and the fetched query was non-empty or anchored to a trigger char (member lists after . / -> / :: stay cheaply cacheable; bare-position answers are never reused). Empty lists are never cached. Adds a PHP LSP regression spec (blank query then prefix on the same line must surface is_int) and a test-only _getClient hook in PHPSupport.
1 parent 66ab0c8 commit 9fbea91

3 files changed

Lines changed: 92 additions & 14 deletions

File tree

src/extensions/default/PHPSupport/main.js

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ define(function (require, exports, module) {
6262

6363
let lspClientPromise = null;
6464
let registered = false;
65+
let _client = null;
6566
let starting = false;
6667
let pendingRepoint = false;
6768
let initErrorReported = false;
@@ -123,6 +124,7 @@ define(function (require, exports, module) {
123124
});
124125
if (client) {
125126
registered = true;
127+
_client = client;
126128
}
127129
}
128130

@@ -251,4 +253,11 @@ define(function (require, exports, module) {
251253
// for tests
252254
exports._ensureServerForActiveEditor = _ensureServerForActiveEditor;
253255
exports.SERVER_ID = SERVER_ID;
256+
if (Phoenix.isTestWindow) {
257+
// the registered LanguageClient (null until the server has started) - lets integration
258+
// tests drive the LSP providers/requests directly
259+
exports._getClient = function () {
260+
return _client;
261+
};
262+
}
254263
});

src/extensions/default/PHPSupport/unittests.js

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,53 @@ define(function (require, exports, module) {
135135
}, "cursor to land on the definition", 30000);
136136
}, 45000);
137137

138+
it("should offer completions after typing a prefix where a blank-position query was empty", async function () {
139+
// Regression: intelephense returns ZERO completions at a bare blank position but a full
140+
// list once a prefix exists. The empty result used to be cached under the word-context
141+
// key (file|line|text-before-word-start), which is IDENTICAL for the blank position and
142+
// the typed prefix - so every later Ctrl-Space on the line replayed "no completions".
143+
const phpMain = await _phpModule("main");
144+
const client = phpMain._getClient();
145+
expect(client).toBeTruthy();
146+
await _openFile("funcs.php");
147+
const editor = EditorManager.getActiveEditor();
148+
const lines = editor.document.getText().split("\n");
149+
const defLine = lines.findIndex(function (l) { return l.indexOf("function computeTotal") !== -1; });
150+
// fresh blank line inside the <?php block, right above the function
151+
editor.document.replaceRange(" \n", { line: defLine, ch: 0 });
152+
const blankLine = defLine;
153+
const filePath = editor.document.file._path;
154+
155+
function _hintLabels(cursorPos) {
156+
return new Promise(function (resolve) {
157+
client.requestHints({ filePath: filePath, cursorPos: cursorPos })
158+
.done(function (r) {
159+
resolve(r.items.map(function (i) { return i.label; }));
160+
})
161+
.fail(function () { resolve(null); });
162+
});
163+
}
164+
165+
// 1. query the bare blank position - primes the completion cache with whatever the
166+
// server answers there (an empty list, for intelephense)
167+
await _hintLabels({ line: blankLine, ch: 4 });
168+
// 2. type a prefix on the SAME line (same context key) and query again - the server's
169+
// real list must come through, not a stale cached answer
170+
editor.document.replaceRange("is_", { line: blankLine, ch: 4 });
171+
editor.setCursorPos(blankLine, 7);
172+
let labels = null;
173+
await awaitsFor(function () {
174+
return _hintLabels({ line: blankLine, ch: 7 }).then(function (result) {
175+
labels = result;
176+
return !!(labels && labels.length);
177+
});
178+
}, "completions for the is_ prefix on the previously-blank line", 30000);
179+
expect(labels).toContain("is_int");
180+
181+
await awaitsForDone(CommandManager.execute(Commands.FILE_CLOSE,
182+
{ fullPath: testFolder + "/funcs.php", _forceClose: true }));
183+
}, 45000);
184+
138185
it("should keep Tern serving embedded <script> JS inside php files", async function () {
139186
await _openFile("embedded.php");
140187
const editor = EditorManager.getActiveEditor();

src/languageTools/LSPClient.js

Lines changed: 36 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -364,12 +364,13 @@ define(function (require, exports, module) {
364364
return { line: cursorPos.line, character: cursorPos.ch };
365365
}
366366

367-
// Build a cache key identifying the current completion "context": the file, line, the column
368-
// where the word under the cursor starts, and the text on the line before that word. While the
369-
// user types/moves within the same word, this key stays constant, so we can reuse the server's
370-
// (complete) result and filter client-side instead of re-querying. That avoids slow late
371-
// responses rebuilding the list mid-navigation.
372-
function _completionContextKey(filePath, pos) {
367+
// Describe the completion "context" at a position:
368+
// - key: identifies the word being typed - file, line, and the text on the line before the
369+
// column where that word starts. Stays constant while typing/moving within one word.
370+
// - query: the part of the word already typed before the cursor.
371+
// - anchored: whether the word start sits directly after a trigger-ish non-space character
372+
// (".", "->", "::") rather than after whitespace/line start.
373+
function _completionContext(filePath, pos) {
373374
const doc = DocumentManager.getOpenDocumentForPath(filePath);
374375
if (!doc) {
375376
return null;
@@ -379,19 +380,35 @@ define(function (require, exports, module) {
379380
while (start > 0 && /[\w$]/.test(lineText.charAt(start - 1))) {
380381
start--;
381382
}
382-
return filePath + "|" + pos.line + "|" + lineText.substring(0, start);
383+
return {
384+
key: filePath + "|" + pos.line + "|" + lineText.substring(0, start),
385+
query: lineText.substring(start, pos.ch),
386+
anchored: start > 0 && /\S/.test(lineText.charAt(start - 1))
387+
};
383388
}
384389

385390
LanguageClient.prototype.requestHints = function (params) {
386391
const self = this;
387392
const deferred = $.Deferred();
388393
(async function () {
389394
try {
390-
// Reuse the cached (complete) completion list while still in the same completion
391-
// context, so typing/cursor-moves within a word don't re-hit the server.
392-
const ctxKey = _completionContextKey(params.filePath, params.cursorPos);
393-
if (ctxKey && self._completionCache && self._completionCache.key === ctxKey) {
394-
deferred.resolve({ items: self._completionCache.items });
395+
// Reuse the cached completion list while typing forward within one word, so every
396+
// keystroke doesn't re-hit the server. Reuse is only sound when the cached list is
397+
// a candidate SUPERSET of what the current query would return, so it needs ALL of:
398+
// - the same context key (same file/line/word-start),
399+
// - the current query extending the query the list was fetched with, and
400+
// - that fetched query being non-empty OR anchored to a trigger char: a member
401+
// list after "." / "->" is a closed set, but what a server answers at a BARE
402+
// position is an arbitrary relevance selection, NOT a superset. intelephense
403+
// answers a blank line with a grab-bag of symbols (marked complete!) which,
404+
// if reused, swallows every later keystroke on that line (e.g. typing is_int
405+
// showed no is_int because a stale blank-line list was being refiltered).
406+
const ctx = _completionContext(params.filePath, params.cursorPos);
407+
const cached = self._completionCache;
408+
if (ctx && cached && cached.key === ctx.key &&
409+
ctx.query.startsWith(cached.query) &&
410+
(cached.query || cached.anchored)) {
411+
deferred.resolve({ items: cached.items });
395412
return;
396413
}
397414
await DocumentSync.flush(self, params.filePath);
@@ -406,8 +423,13 @@ define(function (require, exports, module) {
406423
// just coerce documentation to a string for inline display.
407424
item.documentation = _markupToString(item.documentation);
408425
});
409-
// Only cache a complete list (an incomplete one must be re-queried as the user types).
410-
self._completionCache = (ctxKey && !isIncomplete) ? { key: ctxKey, items: items } : null;
426+
// Only cache a complete, NON-EMPTY list (an incomplete one must be re-queried as
427+
// the user types; an empty one has nothing to refilter - and some servers answer
428+
// empty at a bare position yet answer fully once a prefix exists). The fetched
429+
// query + anchoring are stored so the reuse check above can prove superset-ness.
430+
self._completionCache = (ctx && !isIncomplete && items.length)
431+
? { key: ctx.key, query: ctx.query, anchored: ctx.anchored, items: items }
432+
: null;
411433
deferred.resolve({ items: items });
412434
} catch (err) {
413435
console.warn("[LSP] request failed:", err && (err.message || err));

0 commit comments

Comments
 (0)