diff --git a/lsp/.vscode-test.mjs b/lsp/.vscode-test.mjs index 0bfcf916c8..9da0c6d994 100644 --- a/lsp/.vscode-test.mjs +++ b/lsp/.vscode-test.mjs @@ -6,6 +6,7 @@ */ import { defineConfig } from '@vscode/test-cli'; +import { makeStartupSettingsWorkspaces } from './src/test/startup-settings/workspaces.mjs'; // One configuration per window shape. The multi-root tests need two folders // open; the failed-start tests open a workspace whose settings break the first @@ -18,6 +19,8 @@ import { defineConfig } from '@vscode/test-cli'; // Mocha's 2s default is not enough for that on the slower CI runners. const mocha = {timeout: 20000}; +const startup = makeStartupSettingsWorkspaces(['instant', 'delay', 'drop']); + export default defineConfig([ { label: 'single-root', @@ -51,5 +54,17 @@ export default defineConfig([ // first `client.start()` is made to fail. workspaceFolder: "./src/test/fixtures/failed-start", launchArgs: ['--disable-workspace-trust'] - } + }, + // The startup-settings tests run the real Python extension, because the + // extension asks it for the interpreter while it answers the server's + // configuration request. One window per mode of `lsp-shim.js`, because each + // mode decides how the server's startup goes. + ...['instant', 'delay', 'drop'].map(mode => ({ + label: `startup-settings-${mode}`, + files: `dist/test/startup-settings/${mode}/*.test.js`, + workspaceFolder: startup[mode], + installExtensions: ['ms-python.python'], + mocha, + launchArgs: ['--disable-workspace-trust'] + })) ]); diff --git a/lsp/src/test/startup-settings/common.ts b/lsp/src/test/startup-settings/common.ts new file mode 100644 index 0000000000..3205973784 --- /dev/null +++ b/lsp/src/test/startup-settings/common.ts @@ -0,0 +1,193 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import * as assert from 'assert'; +import {promises as fs} from 'fs'; +import {join} from 'path'; +import * as vscode from 'vscode'; +import {PythonEnvironments} from '@vscode/python-environments'; +import {PythonExtension} from '@vscode/python-extension'; + +export const fixtureFolder = (): vscode.Uri => + vscode.workspace.workspaceFolders![0].uri; + +export const mainPy = (): vscode.Uri => + vscode.Uri.joinPath(fixtureFolder(), 'main.py'); + +/** The client messages in the order that `lsp-shim.js` forwarded them. */ +export async function shimLog(): Promise[]> { + const text = await fs.readFile( + join(fixtureFolder().fsPath, '.vscode', 'lsp-shim.jsonl'), + 'utf8', + ); + return text + .split('\n') + .filter(line => line !== '') + .map(line => JSON.parse(line)); +} + +/** + * The interpreter that the extension would send for the fixture folder. This + * asks the same APIs in the same order as `PythonEnvironment`: the Python + * Environments extension first, then the Python extension. + */ +async function selectedInterpreter(): Promise { + try { + const api = await PythonEnvironments.api(); + return (await api.getEnvironment(fixtureFolder()))?.execInfo?.run + ?.executable; + } catch { + const api = await PythonExtension.api(); + return (await api.environments.getActiveEnvironmentPath(fixtureFolder())) + .path; + } +} + +/** + * Wait until the Python extensions select `python.defaultInterpreterPath`. + * Right after they start, they can select the machine's interpreter instead, + * and then the client's settings do not name the virtual environment. + * The paths are compared as `fsPath`s, because on Windows the Python + * extensions report the drive letter in lower case. + */ +export async function waitForConfiguredInterpreter(): Promise { + const expected = vscode.Uri.file( + vscode.workspace + .getConfiguration('python', fixtureFolder()) + .get('defaultInterpreterPath')!, + ).fsPath; + let actual: string | undefined; + await waitFor( + 'the Python extensions to select python.defaultInterpreterPath', + async () => { + actual = await selectedInterpreter(); + return actual !== undefined && vscode.Uri.file(actual).fsPath === expected; + }, + 30000, + ).catch(error => { + throw new Error(`${error.message}; they selected ${actual}`); + }); +} + +/** + * Activate the extension and open `main.py`. The extension starts the server + * on activation, so the shim's mode decides whether the editor's requests + * reach the server before the client's settings do. + */ +export async function openMainPy(): Promise { + await vscode.extensions.getExtension('meta.pyrefly')!.activate(); + const document = await vscode.workspace.openTextDocument(mainPy()); + await vscode.window.showTextDocument(document); + return document; +} + +export async function definition( + uri: vscode.Uri, + position: vscode.Position, +): Promise { + const locations = await vscode.commands.executeCommand< + (vscode.Location | vscode.LocationLink)[] + >('vscode.executeDefinitionProvider', uri, position); + return locations.map(location => + 'targetUri' in location ? location.targetUri : location.uri, + ); +} + +/** Every set of Pyrefly diagnostics that VS Code reported for `uri`. */ +export function recordDiagnostics(uri: vscode.Uri): { + sets: vscode.Diagnostic[][]; + dispose: () => void; +} { + const sets: vscode.Diagnostic[][] = []; + const subscription = vscode.languages.onDidChangeDiagnostics(event => { + if (event.uris.some(changed => changed.toString() === uri.toString())) { + sets.push( + vscode.languages + .getDiagnostics(uri) + .filter(diagnostic => diagnostic.source === 'Pyrefly'), + ); + } + }); + return {sets, dispose: () => subscription.dispose()}; +} + +export function diagnosticCode(diagnostic: vscode.Diagnostic): string { + const code = diagnostic.code; + return String(typeof code === 'object' ? code.value : code); +} + +export async function waitFor( + description: string, + condition: () => boolean | Promise, + timeoutMs: number, +): Promise { + const deadline = Date.now() + timeoutMs; + while (!(await condition())) { + if (Date.now() > deadline) { + throw new Error(`timed out waiting for ${description}`); + } + await new Promise(resolve => setTimeout(resolve, 100)); + } +} + +/** + * Open `main.py` and check that startup go-to-definition and every diagnostic + * set that the editor shows use the workspace settings. Only the interpreter + * from the Python extension can import `fakepkg`, and only the workspace + * settings select the preset that reports `bad-assignment`. Returns the client + * messages in the order that the shim forwarded them. + */ +export async function expectWorkspaceSettingsAtStartup(): Promise< + (string | undefined)[] +> { + const diagnostics = recordDiagnostics(mainPy()); + try { + await waitForConfiguredInterpreter(); + await openMainPy(); + const targets = await definition(mainPy(), new vscode.Position(0, 8)); + + let log: (string | undefined)[] = []; + await waitFor( + 'the shim to forward the configuration response', + async () => { + log = (await shimLog()).map(entry => entry.method); + return log.includes('workspace/configuration response'); + }, + 30000, + ); + assert.deepStrictEqual( + targets.map(target => target.path.split('/').slice(-2).join('/')), + ['fakepkg/__init__.py'], + ); + + await waitFor( + 'the bad-assignment error in main.py', + () => + diagnostics.sets.some(set => + set.some(d => diagnosticCode(d) === 'bad-assignment'), + ), + 30000, + ).catch(error => { + throw new Error( + `${error.message}; the editor showed ${JSON.stringify( + diagnostics.sets.map(set => set.map(diagnosticCode)), + )}`, + ); + }); + // Each set is what the editor showed at one moment. Default settings use + // the `basic` preset, which hides `bad-assignment`, and do not name the + // interpreter that can import `fakepkg`. A set from them is empty or + // reports `missing-import`. + assert.deepStrictEqual( + diagnostics.sets.map(set => set.map(diagnosticCode)), + diagnostics.sets.map(() => ['bad-assignment']), + ); + return log; + } finally { + diagnostics.dispose(); + } +} diff --git a/lsp/src/test/startup-settings/delay/delay.test.ts b/lsp/src/test/startup-settings/delay/delay.test.ts new file mode 100644 index 0000000000..38b980801b --- /dev/null +++ b/lsp/src/test/startup-settings/delay/delay.test.ts @@ -0,0 +1,32 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import * as assert from 'assert'; +import * as vscode from 'vscode'; +import {expectWorkspaceSettingsAtStartup} from '../common'; + +/** + * The shim holds the client's configuration responses for 3 seconds, as a slow + * Python extension does, so the editor's startup requests reach the server + * before the interpreter does. The server must hold those requests until the + * interpreter arrives. + */ +suite('startup with a late initial configuration response', () => { + test('the Python extension is installed', () => { + assert.ok(vscode.extensions.getExtension('ms-python.python')); + }); + + test('startup requests and diagnostics use the interpreter from the Python extension', async function () { + this.timeout(60000); + const log = await expectWorkspaceSettingsAtStartup(); + assert.ok( + log.indexOf('textDocument/definition') < + log.indexOf('workspace/configuration response'), + `the definition request must reach the server before the settings: ${log}`, + ); + }); +}); diff --git a/lsp/src/test/startup-settings/drop/drop.test.ts b/lsp/src/test/startup-settings/drop/drop.test.ts new file mode 100644 index 0000000000..362b57a0a7 --- /dev/null +++ b/lsp/src/test/startup-settings/drop/drop.test.ts @@ -0,0 +1,33 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import * as assert from 'assert'; +import * as vscode from 'vscode'; +import {definition, mainPy, openMainPy, shimLog, waitFor} from '../common'; + +/** + * The shim never forwards the client's first configuration response. The + * server must still answer the editor's startup requests, with default + * settings, instead of holding them forever. + */ +suite('startup without an initial configuration response', () => { + test('a startup request is answered with default settings', async function () { + this.timeout(60000); + await openMainPy(); + const targets = await definition(mainPy(), new vscode.Position(1, 17)); + + assert.deepStrictEqual( + targets.map(target => target.path.split('/').pop()), + ['bar.py'], + ); + await waitFor( + 'the shim to drop the first configuration response', + async () => (await shimLog()).some(entry => entry.dropped !== undefined), + 30000, + ); + }); +}); diff --git a/lsp/src/test/startup-settings/instant/instant.test.ts b/lsp/src/test/startup-settings/instant/instant.test.ts new file mode 100644 index 0000000000..3f6dcfaeea --- /dev/null +++ b/lsp/src/test/startup-settings/instant/instant.test.ts @@ -0,0 +1,27 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import * as assert from 'assert'; +import * as vscode from 'vscode'; +import {expectWorkspaceSettingsAtStartup} from '../common'; + +/** + * The shim forwards the client's configuration responses at once, as when the + * Python extension has the interpreter ready. The server must still answer + * startup requests with the workspace settings and never show diagnostics from + * default settings. + */ +suite('startup with an instant initial configuration response', () => { + test('the Python extension is installed', () => { + assert.ok(vscode.extensions.getExtension('ms-python.python')); + }); + + test('startup requests and diagnostics use the interpreter from the Python extension', async function () { + this.timeout(60000); + await expectWorkspaceSettingsAtStartup(); + }); +}); diff --git a/lsp/src/test/startup-settings/lsp-shim.js b/lsp/src/test/startup-settings/lsp-shim.js new file mode 100644 index 0000000000..1555b2f45b --- /dev/null +++ b/lsp/src/test/startup-settings/lsp-shim.js @@ -0,0 +1,123 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +// Sits between VS Code and the language server and can hold back the client's +// responses to the server's `workspace/configuration` requests. Every other +// message passes through in order, so the requests that VS Code sends while a +// response is held reach the server before the settings do. +// +// Usage: node lsp-shim.js [server args...] +// +// `instant` forwards every configuration response as soon as the client sends +// it. `delay` copies a slow Python extension, which the client waits on before it +// answers any configuration request: the shim holds every configuration +// response from the first one until HOLD_MS later, then forwards them in +// order. `drop` copies a client that never answers the first configuration +// request: the shim discards that response and forwards the rest. +// +// The log is one JSON object per line, naming each client message in the order +// that the shim forwarded it to the server, so a test can check that the race +// it relies on really happened. + +const {spawn} = require('child_process'); +const fs = require('fs'); + +const HOLD_MS = 3000; +const [server, mode, logPath, ...serverArgs] = process.argv.slice(2); +if (!['instant', 'delay', 'drop'].includes(mode)) { + throw new Error(`unknown mode: ${mode}`); +} +fs.writeFileSync(logPath, ''); + +function log(entry) { + fs.appendFileSync(logPath, JSON.stringify(entry) + '\n'); +} + +const child = spawn(server, serverArgs, {stdio: ['pipe', 'pipe', 'inherit']}); +child.on('exit', code => process.exit(code ?? 1)); +for (const signal of ['SIGINT', 'SIGTERM']) { + process.on(signal, () => child.kill(signal)); +} + +/** Call `onMessage` with each complete LSP frame read from `stream`. */ +function readFrames(stream, onMessage) { + let buffer = Buffer.alloc(0); + stream.on('data', chunk => { + buffer = Buffer.concat([buffer, chunk]); + for (;;) { + const headerEnd = buffer.indexOf('\r\n\r\n'); + if (headerEnd < 0) { + return; + } + const length = Number( + /Content-Length: *(\d+)/i.exec(buffer.subarray(0, headerEnd).toString())[1], + ); + const bodyStart = headerEnd + 4; + if (buffer.length < bodyStart + length) { + return; + } + const frame = buffer.subarray(0, bodyStart + length); + const message = JSON.parse(buffer.subarray(bodyStart, bodyStart + length)); + buffer = buffer.subarray(bodyStart + length); + onMessage(frame, message); + } + }); +} + +/** The ids of the server's configuration requests, in the order sent. */ +const configurationIds = []; + +readFrames(child.stdout, (frame, message) => { + if (message.method === 'workspace/configuration') { + configurationIds.push(message.id); + } + process.stdout.write(frame); +}); + +function forwardConfigurationResponse(frame) { + log({method: 'workspace/configuration response'}); + child.stdin.write(frame); +} + +/** In `delay` mode, the responses held until the hold ends; `undefined` after. */ +let held; +let holdStarted = false; + +readFrames(process.stdin, (frame, message) => { + const isConfigurationResponse = + message.method === undefined && configurationIds.includes(message.id); + if (!isConfigurationResponse) { + log({method: message.method ?? 'response'}); + child.stdin.write(frame); + } else if (mode === 'instant') { + forwardConfigurationResponse(frame); + } else if (mode === 'drop') { + if (message.id === configurationIds[0]) { + log({dropped: 'workspace/configuration response'}); + } else { + forwardConfigurationResponse(frame); + } + } else if (!holdStarted) { + holdStarted = true; + log({held: 'workspace/configuration response'}); + held = [frame]; + setTimeout(() => { + held.forEach(forwardConfigurationResponse); + held = undefined; + }, HOLD_MS); + } else if (held !== undefined) { + held.push(frame); + } else { + forwardConfigurationResponse(frame); + } +}); +// The editor closing the pipe ends the session, even when the server still +// holds a request that never completes. +process.stdin.on('end', () => { + child.stdin.end(); + setTimeout(() => child.kill(), 1000).unref(); +}); diff --git a/lsp/src/test/startup-settings/workspaces.mjs b/lsp/src/test/startup-settings/workspaces.mjs new file mode 100644 index 0000000000..7516c72e33 --- /dev/null +++ b/lsp/src/test/startup-settings/workspaces.mjs @@ -0,0 +1,79 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +// Builds the workspaces for the startup-settings tests. They live outside the +// repository for two reasons: the settings need absolute paths, and Pyrefly +// finds a virtual environment near a project on its own, which would let the +// default settings resolve the same imports as the client's settings. + +import {execFileSync} from 'child_process'; +import {mkdirSync, mkdtempSync, writeFileSync} from 'fs'; +import {tmpdir} from 'os'; +import {dirname, join} from 'path'; +import {fileURLToPath} from 'url'; + +const here = dirname(fileURLToPath(import.meta.url)); +const windows = process.platform === 'win32'; + +function write(path, text) { + mkdirSync(dirname(path), {recursive: true}); + writeFileSync(path, text); +} + +/** + * Make one workspace per mode of `lsp-shim.js`, sharing one virtual + * environment that only `python.defaultInterpreterPath` points at. + * Returns the workspace folder for each mode. + */ +export function makeStartupSettingsWorkspaces(modes) { + const root = mkdtempSync(join(tmpdir(), 'pyrefly-startup-settings-')); + // Pyrefly also finds a virtual environment beside a project's ancestors, so + // the environment gets a temporary directory of its own. + const venv = join(mkdtempSync(join(tmpdir(), 'pyrefly-startup-venv-')), 'venv'); + execFileSync(windows ? 'python' : 'python3', ['-m', 'venv', '--without-pip', venv]); + const python = join(venv, windows ? 'Scripts/python.exe' : 'bin/python'); + const purelib = execFileSync(python, [ + '-c', + 'import sysconfig; print(sysconfig.get_paths()["purelib"])', + ]) + .toString() + .trim(); + write(join(purelib, 'fakepkg', '__init__.py'), 'VALUE: int = 1\n'); + + const server = join(here, '../../../bin', windows ? 'pyrefly.exe' : 'pyrefly'); + const folders = {}; + for (const mode of modes) { + const folder = join(root, mode); + write( + join(folder, 'main.py'), + 'import fakepkg\nfrom bar import helper\n\ny: str = fakepkg.VALUE\nhelper()\n', + ); + write(join(folder, 'bar.py'), 'def helper() -> None: ...\n'); + write( + join(folder, '.vscode', 'settings.json'), + JSON.stringify( + { + 'python.defaultInterpreterPath': python, + 'python.languageServer': 'None', + 'python.pyrefly.typeCheckingMode': 'default', + 'pyrefly.lspPath': process.execPath, + 'pyrefly.lspArguments': [ + join(here, 'lsp-shim.js'), + server, + mode, + join(folder, '.vscode', 'lsp-shim.jsonl'), + 'lsp', + ], + }, + null, + 4, + ), + ); + folders[mode] = folder; + } + return folders; +}