diff --git a/frontend/src/lib/components/Editor.svelte b/frontend/src/lib/components/Editor.svelte index 6f1e2c222c..f050c8d1f3 100644 --- a/frontend/src/lib/components/Editor.svelte +++ b/frontend/src/lib/components/Editor.svelte @@ -107,7 +107,7 @@ } from './lint/typescriptExtraLibs' import { createWindmillAta, genAtaRoot } from './lint/typescriptAta' import { readModelMarkers } from './lint/markers' - import { buildDenoImportMap, lspServersFor } from './lint/lspLanguageConfig' + import { buildDenoImportMap, hasLanguageServers, lspServersFor } from './lint/lspLanguageConfig' import { aiChatManager } from './copilot/chat/AIChatManager.svelte' import type { Selection } from 'monaco-editor' import { canHavePreprocessor, getPreprocessorModuleCode } from '$lib/script_helpers' @@ -980,15 +980,7 @@ export async function reloadWebsocket() { await closeWebsockets() - if ( - !useWebsockets || - !( - (lang == 'typescript' && scriptLang === 'deno') || - lang == 'python' || - lang == 'go' || - lang == 'shell' - ) - ) { + if (!useWebsockets || !hasLanguageServers(lang, scriptLang)) { return } console.log('reloadWebsocket') @@ -1146,11 +1138,7 @@ denoImportMap = buildDenoImportMap(await genAtaRoot($workspaceStore ?? ''), filePath) } - const servers = lspServersFor({ editorLang: lang, scriptLang, denoImportMap }) - if (servers.length === 0) { - closeWebsockets() - } - for (const server of servers) { + for (const server of lspServersFor({ editorLang: lang, scriptLang, denoImportMap })) { await connectToLanguageServer( server.url, server.name, @@ -1359,6 +1347,12 @@ if (!nmodel) { throw err } + // Headless linting creates models at this same canonical URI, so the existing + // one can hold whatever content was last linted. Show what this editor was + // given, not that. + if (nmodel.getValue() !== (code ?? '')) { + nmodel.setValue(code ?? '') + } model = nmodel } model.updateOptions(lang == 'python' ? { tabSize: 4, insertSpaces: true } : updateOptions) @@ -1620,8 +1614,8 @@ modelUri: uri, absolutePathExtraLibs, isCancelled: () => destroyed, + registerLocalModels: () => editor != undefined, onLocalFileRegistered: () => { - if (!editor) return try { model?.setValue(model.getValue()) } catch (e) { diff --git a/frontend/src/lib/components/copilot/chat/global/core.ts b/frontend/src/lib/components/copilot/chat/global/core.ts index 71f4b5bc20..2b371b4b82 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.ts @@ -3832,7 +3832,7 @@ async function getLintErrors(args: LintTargetArgs, ctx: WriteDraftCtx): Promise< let response = formatScriptLintResult(result) if (result.unavailableServers?.length) { - response += `\n\nNote: the ${result.unavailableServers.join(' and ')} language server${result.unavailableServers.length > 1 ? 's were' : ' was'} unavailable, so some problems may not be listed. This usually means an editor is already open on a ${target.language} item.` + response += `\n\nNote: the ${result.unavailableServers.join(' and ')} language server${result.unavailableServers.length > 1 ? 's did' : ' did'} not respond, so some problems may not be listed. Treat a clean result as inconclusive.` } if (result.contentMismatch) { response += `\n\nNote: an editor is currently open on this code and its buffer differs from the draft. The results above are for what that editor shows.` diff --git a/frontend/src/lib/components/lint/headlessLint.ts b/frontend/src/lib/components/lint/headlessLint.ts index f9be64fd3e..6e7eb4ef35 100644 --- a/frontend/src/lib/components/lint/headlessLint.ts +++ b/frontend/src/lib/components/lint/headlessLint.ts @@ -47,8 +47,7 @@ export interface HeadlessLintOutcome extends ScriptLintResult { unavailableServers?: string[] } -// Models we created ourselves, oldest first. An editor may later adopt one by URI, so -// eviction must never dispose a model that is attached to an editor. +// Models we created ourselves, least recently used first. const ownedModels: string[] = [] const MAX_OWNED_MODELS = 4 @@ -56,11 +55,29 @@ const absolutePathExtraLibs = new Map void }>() const ataByKey = new Map Promise>() const pendingByUri = new Map>() +function withDeadline(promise: Promise, ms: number): Promise { + return Promise.race([promise, new Promise((resolve) => setTimeout(resolve, ms))]) +} + +function touchOwnedModel(uriString: string) { + const at = ownedModels.indexOf(uriString) + if (at >= 0) ownedModels.splice(at, 1) + ownedModels.push(uriString) +} + function evictOwnedModels(keepUri: string) { while (ownedModels.length > MAX_OWNED_MODELS) { const candidate = ownedModels.shift() - if (!candidate || candidate === keepUri) continue + if (!candidate) continue + if (candidate === keepUri) { + // Still in use: keep tracking it, or a later lint would mistake it for a model + // owned by an editor and refuse to update its content. + ownedModels.push(candidate) + continue + } const model = meditor.getModel(Uri.parse(candidate)) + // An editor may have adopted it in the meantime; disposing it would close the + // user's buffer. if (model && !model.isAttachedToEditor()) { model.dispose() } @@ -127,32 +144,41 @@ async function lintOne( let model = meditor.getModel(uri) let contentMismatch = false if (model) { - const isOurs = ownedModels.includes(uriString) - if (isOurs) { - if (model.getValue() !== req.content) model.setValue(req.content) - } else { - // An editor owns this model: its content is what the user sees and what they would - // see the markers for. Lint it as-is rather than overwriting their buffer. + // Attachment decides, not who created it: an editor can adopt a model we made, and + // its buffer is what the user sees and would save. Never write to it. + if (model.isAttachedToEditor() || !ownedModels.includes(uriString)) { contentMismatch = model.getValue() !== req.content + } else if (model.getValue() !== req.content) { + model.setValue(req.content) + // Markers from the previous content survive until revalidation finishes, and a + // matching marker count would let the wait below settle on them. + meditor.setModelMarkers(model, editorLang, []) + touchOwnedModel(uriString) } } else { model = meditor.createModel(req.content, editorLang, uri) - ownedModels.push(uriString) + touchOwnedModel(uriString) evictOwnedModels(uriString) } + // Bounded: these fetch over the network, and lints of one URI are serialized behind + // each other, so one hung request would wedge the tool for that item indefinitely. if (editorLang === 'typescript') { - await Promise.all([ - ensureResourceTypeNamespace(req.workspace, req.scriptLang).catch((e) => - console.error('headlessLint: resource types unavailable', e) - ), - ensureCustomWmillTypes(req.workspace).catch(() => undefined) - ]) + await withDeadline( + Promise.all([ + ensureResourceTypeNamespace(req.workspace, req.scriptLang), + ensureCustomWmillTypes(req.workspace) + ]).catch((e) => console.error('headlessLint: extra libs unavailable', e)), + timeoutMs + ) } if (req.scriptLang === 'bun' || req.scriptLang === 'bunnative') { - await acquireTypes(req, uriString).catch((e) => - console.error('headlessLint: type acquisition failed', e) + await withDeadline( + acquireTypes(req, uriString).catch((e) => + console.error('headlessLint: type acquisition failed', e) + ), + timeoutMs ) } @@ -164,6 +190,11 @@ async function acquireTypes(req: HeadlessLintRequest, uriString: string): Promis const key = `${req.workspace}:${uriString}` let ata = ataByKey.get(key) if (!ata) { + while (ataByKey.size >= MAX_OWNED_MODELS) { + const oldest = ataByKey.keys().next().value + if (oldest === undefined) break + ataByKey.delete(oldest) + } ata = await createWindmillAta({ root: await genAtaRoot(req.workspace), scriptPath: req.path, @@ -214,15 +245,16 @@ async function countExpectedMarkers(uri: Uri, editorLang: string): Promise setTimeout(r, 50)) } } diff --git a/frontend/src/lib/components/lint/headlessLsp.ts b/frontend/src/lib/components/lint/headlessLsp.ts index 2e06da1c12..3e709b4140 100644 --- a/frontend/src/lib/components/lint/headlessLsp.ts +++ b/frontend/src/lib/components/lint/headlessLsp.ts @@ -57,12 +57,20 @@ function startClient( return new Promise((resolve) => { let settled = false let webSocket: WebSocket + let client: MonacoLanguageClient | undefined const finish = (v: StartedClient | undefined) => { - if (settled) return + if (settled) { + // The deadline already gave up on this client. Stop it, or it keeps holding + // the global command names it registered and every later lint of this + // language reports the server as unavailable. + if (v) void stopClient(v) + return + } settled = true clearTimeout(timer) if (!v) { + if (client) void stopClient({ server, client, webSocket, pushedDiagnostics: Promise.resolve() }) try { webSocket?.close() } catch {} @@ -93,7 +101,7 @@ function startClient( let resolvePushed: () => void = () => {} const pushedDiagnostics = new Promise((r) => (resolvePushed = r)) - const client = new MonacoLanguageClient({ + client = new MonacoLanguageClient({ name: `headless-${server.name}`, messageTransports: { reader, writer }, clientOptions: { @@ -226,17 +234,16 @@ export async function lintWithLsp(req: { }): Promise { await initializeVscode('headlessLsp') - const uriString = computeModelUri( - computeModelPath(undefined, req.scriptLang), - req.scriptLang, - req.editorLang - ) + const modelPath = computeModelPath(undefined, req.scriptLang) + const uriString = computeModelUri(modelPath, req.scriptLang, req.editorLang) const uri = Uri.parse(uriString) const model = meditor.createModel(req.content, req.editorLang, uri) + // Built from where the document actually lives, not the workspace path: the map's + // relative entries are derived from the document's own depth. const denoImportMap = req.scriptLang === 'deno' - ? buildDenoImportMap(await genAtaRoot(req.workspace), req.path) + ? buildDenoImportMap(await genAtaRoot(req.workspace), modelPath) : undefined const servers = lspServersFor({ editorLang: req.editorLang, @@ -257,29 +264,40 @@ export async function lintWithLsp(req: { ) } - // A server is done when it has answered the diagnostic request. If it answers with - // nothing it may be a publisher instead, so give its push a bounded chance rather - // than concluding the document is clean. - const pulled = await withDeadline( - Promise.all( - started.map(async (s) => { - const items = await pullDiagnostics(s, uri) - if (items.length === 0) { - await withDeadline(s.pushedDiagnostics, PUSH_FALLBACK_MS, undefined) - } - return items - }) - ).then((r) => r.flat()), - timeoutMs, - [] as meditor.IMarker[] + // Each server is awaited on its own deadline: one that stalls must not discard + // what the others already reported, and must be named rather than counted as + // having found nothing. + const stalled: string[] = [] + const perServer = await Promise.all( + started.map(async (s) => { + const items = await withDeadline( + (async () => { + const pulled = await pullDiagnostics(s, uri) + // A server that answers with nothing may be a publisher instead, so give + // its push a bounded chance before concluding the document is clean. + if (pulled.length === 0) { + await withDeadline(s.pushedDiagnostics, PUSH_FALLBACK_MS, undefined) + } + return pulled + })(), + timeoutMs, + undefined + ) + if (items === undefined) { + stalled.push(s.server.name) + return [] + } + return items + }) ) // Pushed diagnostics have already been turned into markers on the model. const pushedMarkers = meditor.getModelMarkers({ resource: uri }) - const unavailableServers = servers - .filter((s) => !started.some((st) => st.server.name === s.name)) - .map((s) => s.name) - return { ...dedupe([...pushedMarkers, ...pulled]), unavailableServers } + const unavailableServers = [ + ...servers.filter((s) => !started.some((st) => st.server.name === s.name)).map((s) => s.name), + ...stalled + ] + return { ...dedupe([...pushedMarkers, ...perServer.flat()]), unavailableServers } } finally { await Promise.all(started.map(stopClient)) model.dispose() diff --git a/frontend/src/lib/components/lint/lspLanguageConfig.ts b/frontend/src/lib/components/lint/lspLanguageConfig.ts index 23fccc09be..138efd3999 100644 --- a/frontend/src/lib/components/lint/lspLanguageConfig.ts +++ b/frontend/src/lib/components/lint/lspLanguageConfig.ts @@ -11,7 +11,7 @@ export interface LspServerConfig { middleware?: (params: any, token: any, next: any) => any } -export function lspLanguagesFor(editorLang: string, scriptLang: string | undefined): boolean { +export function hasLanguageServers(editorLang: string, scriptLang: string | undefined): boolean { return ( (editorLang === 'typescript' && scriptLang === 'deno') || editorLang === 'python' || diff --git a/frontend/src/lib/components/lint/typescriptAta.ts b/frontend/src/lib/components/lint/typescriptAta.ts index a868ac5608..078ec9922b 100644 --- a/frontend/src/lib/components/lint/typescriptAta.ts +++ b/frontend/src/lib/components/lint/typescriptAta.ts @@ -36,6 +36,8 @@ export interface WindmillAtaOptions { modelUri: string absolutePathExtraLibs: Map void }> isCancelled?: () => boolean + /** Whether relative-import files should be materialized as Monaco models. */ + registerLocalModels?: () => boolean /** Called after a relative-import model is registered, to nudge revalidation. */ onLocalFileRegistered?: () => void } @@ -67,13 +69,15 @@ export async function createWindmillAta( opts.absolutePathExtraLibs.get(_path)?.dispose() opts.absolutePathExtraLibs.set(_path, typescriptDefaults.addExtraLib(code, _path)) } - const localModel = meditor.getModel(nuri) - if (localModel) { - localModel.setValue(code) - } else { - meditor.createModel(code, 'typescript', nuri) + if (opts.registerLocalModels?.() ?? true) { + const localModel = meditor.getModel(nuri) + if (localModel) { + localModel.setValue(code) + } else { + meditor.createModel(code, 'typescript', nuri) + } + opts.onLocalFileRegistered?.() } - opts.onLocalFileRegistered?.() } await initWasmTs() diff --git a/frontend/src/lib/components/lint/typescriptExtraLibs.ts b/frontend/src/lib/components/lint/typescriptExtraLibs.ts index 0eea42dc8c..6f348d68de 100644 --- a/frontend/src/lib/components/lint/typescriptExtraLibs.ts +++ b/frontend/src/lib/components/lint/typescriptExtraLibs.ts @@ -59,6 +59,6 @@ export function applyCustomWmillTypes(data: CustomWmillTypesData): () => void { } } -export async function ensureCustomWmillTypes(workspace: string): Promise<() => void> { - return applyCustomWmillTypes(await fetchCustomWmillTypesData(workspace)) +export async function ensureCustomWmillTypes(workspace: string): Promise { + applyCustomWmillTypes(await fetchCustomWmillTypesData(workspace)) }