From 858e7463332d1ca31b31b66c86a68f4b098295f7 Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Mon, 14 Sep 2026 17:26:11 -0700 Subject: [PATCH] feat: offer inline-script env setup as an unresolved-import quick fix The PEP 723 inline-script feature's only discovery surface is a CodeLens, and provideCodeLenses returns [] while document.isDirty (codeLens.ts:51). That hides it at the one moment a user most needs it: right after typing `import requests` and seeing the squiggle. Add a CodeActionProvider that offers "Set up this script's Python environment" on an unresolved-import diagnostic in a .py file that declares a `# /// script` block and has no environment yet. It parses the in-memory buffer, so it works on an unsaved edit. The action makes no promise it cannot keep: the title says what it does rather than that the squiggle will clear, and `diagnostics`/`isPreferred` are both left unset so VS Code is not told the action resolves anything. Setup reads the block from disk (envManager.ts:308), so the handler now saves a dirty document first and seeds routing metadata from the saved bytes, closing a race where a just-typed block would be misread as a mid-setup edit and have its association silently skipped. Adds an `inlineScript.setupInvoked` telemetry event with a low-cardinality `trigger` (codelens | codeaction | bulk) to measure whether the new surface actually improves adoption. Everything stays behind `python-envs.inlineScripts.enabled`, so none of this is user-visible yet. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/common/localize.ts | 13 + src/common/telemetry/constants.ts | 32 +++ src/features/inlineScript/codeLens.ts | 4 +- src/features/inlineScript/setupCodeAction.ts | 170 ++++++++++++ src/features/inlineScript/setupEnvironment.ts | 133 +++++++-- .../inlineScript/codeLens.unit.test.ts | 2 +- .../inlineScript/setupCodeAction.unit.test.ts | 257 ++++++++++++++++++ .../setupEnvironment.unit.test.ts | 174 ++++++++++++ 8 files changed, 765 insertions(+), 20 deletions(-) create mode 100644 src/features/inlineScript/setupCodeAction.ts create mode 100644 src/test/features/inlineScript/setupCodeAction.unit.test.ts diff --git a/src/common/localize.ts b/src/common/localize.ts index a6a6a3ce4..a23766eb8 100644 --- a/src/common/localize.ts +++ b/src/common/localize.ts @@ -26,6 +26,19 @@ export namespace WorkbenchStrings { export namespace InlineScriptStrings { export const updateExtension = l10n.t('Update Extension'); + /** + * Quick fix title offered on an unresolved import in a PEP 723 script. + * + * Deliberately describes what the action *does* (set the environment up) rather than promising + * to fix the import: setup installs the block's declared `dependencies` verbatim, which may not + * include the module that is actually unresolved. + */ + export const setUpScriptEnvironment = l10n.t("Set up this script's Python environment"); + + export const saveFailedBeforeSetup = l10n.t( + 'Could not save this script, so its environment was not set up. Save the file and try again.', + ); + export const updatePythonExtension = l10n.t( 'The environment for this script was created. Update the Python extension for the full inline script experience.', ); diff --git a/src/common/telemetry/constants.ts b/src/common/telemetry/constants.ts index 5e12d5f38..b3f7e015c 100644 --- a/src/common/telemetry/constants.ts +++ b/src/common/telemetry/constants.ts @@ -253,8 +253,29 @@ export enum EventNames { * - duration: number (ms between the detection and the first edit) */ INLINE_SCRIPT_EDITED = 'inlineScript.edited', + /** + * Telemetry event fired once per user-initiated inline-script environment + * setup, recording which surface started it. This is the adoption measure + * for the unresolved-import quick fix: the CodeLens is hidden while a + * document is dirty, so `codeaction` counts setups that the CodeLens alone + * could not have produced. + * Properties: + * - trigger: 'codelens' | 'codeaction' | 'bulk' (which surface invoked setup) + * - outcome: 'created' | 'notCreated' | 'error' + * + * `notCreated` covers every benign or reported non-creation (cancelled, + * skipped, no compatible Python, ...); the failure taxonomy itself already + * ships on `inlineScript.envError` and is not duplicated here. + */ + INLINE_SCRIPT_SETUP_INVOKED = 'inlineScript.setupInvoked', } +/** Surface that started an inline-script environment setup. */ +export type InlineScriptSetupTrigger = 'codelens' | 'codeaction' | 'bulk'; + +/** Result of one inline-script environment setup attempt, as seen by the invoking surface. */ +export type InlineScriptSetupOutcomeKind = 'created' | 'notCreated' | 'error'; + export type InlineScriptEnvErrorCategory = | 'compatible-python-declined' | 'discovery-failure' @@ -778,4 +799,15 @@ export interface IEventNamePropertyMapping { } */ [EventNames.INLINE_SCRIPT_EDITED]: never | undefined; + + /* __GDPR__ + "inlineScript.setupInvoked": { + "trigger": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "owner": "StellaHuang95" }, + "outcome": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "owner": "StellaHuang95" } + } + */ + [EventNames.INLINE_SCRIPT_SETUP_INVOKED]: { + trigger: InlineScriptSetupTrigger; + outcome: InlineScriptSetupOutcomeKind; + }; } diff --git a/src/features/inlineScript/codeLens.ts b/src/features/inlineScript/codeLens.ts index f605134cd..4c9565487 100644 --- a/src/features/inlineScript/codeLens.ts +++ b/src/features/inlineScript/codeLens.ts @@ -13,6 +13,7 @@ import { TextDocument, } from 'vscode'; import { InlineScriptRoutingRegistry } from '../../common/inlineScript/routingRegistry'; +import { InlineScriptSetupTrigger } from '../../common/telemetry/constants'; /** * Shows a single "Set up environment for this script" CodeLens above a `.py` file's PEP 723 @@ -67,11 +68,12 @@ export class InlineScriptCodeLensProvider implements CodeLensProvider, Disposabl const offset = metadata.sourceRange?.start ?? metadata.range.start; const position = document.positionAt(offset); const range = new Range(position, position); + const trigger: InlineScriptSetupTrigger = 'codelens'; return [ new CodeLens(range, { title: l10n.t('Set up environment for this script'), command: this.setupCommand, - arguments: [uri], + arguments: [uri, trigger], }), ]; } diff --git a/src/features/inlineScript/setupCodeAction.ts b/src/features/inlineScript/setupCodeAction.ts new file mode 100644 index 000000000..a54da2882 --- /dev/null +++ b/src/features/inlineScript/setupCodeAction.ts @@ -0,0 +1,170 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { + CancellationToken, + CodeAction, + CodeActionContext, + CodeActionKind, + CodeActionProvider, + Diagnostic, + Disposable, + languages, + Range, + TextDocument, +} from 'vscode'; +import { MAX_HEADER_BYTES, readInlineScriptMetadata } from '../../common/inlineScript/metadata'; +import { getInlineScriptRoutingKey, InlineScriptRoutingRegistry } from '../../common/inlineScript/routingRegistry'; +import { InlineScriptStrings } from '../../common/localize'; +import { InlineScriptSetupTrigger } from '../../common/telemetry/constants'; +import { isInlineScriptsFeatureEnabled } from '../../helpers'; + +/** + * Diagnostic codes that mean "this import did not resolve", across every type checker a user of this + * extension is likely to have enabled. Stored lowercased; compare with {@link normalizeDiagnosticCode}. + * + * Matching is on `code` only, never on `source`: Pyrefly-backed Pylance reports its source as the + * literal string `pylance + pyrefly`, so any source allow-list would be wrong somewhere. + * + * `reportMissingModuleSource` is included deliberately, diverging from Pylance's own + * `isMissingImportDiagnostic`, which excludes it because a stub-resolved module is correctly spelled + * and so has nothing for a "change spelling" fix to suggest. For us the meaning is the opposite kind + * of useful: a stub was found but the source was not, i.e. the package is not installed — which is + * exactly what setting the script's environment up addresses. + */ +const UNRESOLVED_IMPORT_DIAGNOSTIC_CODES: ReadonlySet = new Set([ + // Pyright / Pylance / basedpyright. + 'reportmissingimports', + 'reportmissingmodulesource', + // Ty (kebab-case), mapped to the two rules above by Pylance's TyDiagnosticCodeMapper. + 'unresolved-import', + 'possibly-missing-import', + // Pyrefly (kebab-case `ErrorKind` names), mapped by Pylance's PyreflyDiagnosticCodeMapper. + 'missing-import', + 'missing-source', + 'missing-source-for-stubs', + // mypy, via the separate ms-python.mypy-type-checker extension. + 'import-not-found', + 'import-untyped', +]); + +/** + * Reduce `Diagnostic.code` — which is `string | number | { value: string | number; target: Uri }` — + * to a lowercased string, or `undefined` when the diagnostic carries no code. + */ +function normalizeDiagnosticCode(code: Diagnostic['code']): string | undefined { + if (code === undefined || code === null) { + return undefined; + } + const value = typeof code === 'object' ? code.value : code; + return typeof value === 'string' || typeof value === 'number' ? String(value).toLowerCase() : undefined; +} + +/** + * Whether `diagnostic` reports an import that could not be resolved. See + * {@link UNRESOLVED_IMPORT_DIAGNOSTIC_CODES} for the dialects covered and for why `source` is ignored. + */ +export function isUnresolvedImportDiagnostic(diagnostic: Diagnostic): boolean { + const code = normalizeDiagnosticCode(diagnostic.code); + return code !== undefined && UNRESOLVED_IMPORT_DIAGNOSTIC_CODES.has(code); +} + +/** + * The head of `document`'s in-memory text, bounded to the same byte budget that + * `readInlineScriptMetadataFromFile` reads from disk. + * + * Bounding it matters twice over: it keeps this provider's work constant regardless of file size, + * and it keeps what the quick fix can see identical to what setup will later parse off disk, so the + * action is never offered for a block that setup would not find. + */ +function getInlineScriptHeaderText(document: TextDocument): string { + const text = document.getText(); + if (Buffer.byteLength(text, 'utf-8') <= MAX_HEADER_BYTES) { + return text; + } + // Truncating on a byte boundary can split a multi-byte character; the disk reader's bounded + // `read` has exactly the same behaviour, so the two stay in agreement. + return Buffer.from(text, 'utf-8').subarray(0, MAX_HEADER_BYTES).toString('utf-8'); +} + +/** + * Offers "Set up this script's Python environment" as a quick fix on an unresolved import in a `.py` + * file that declares a PEP 723 `# /// script` block and has no inline-script environment yet. + * + * This exists because the CodeLens is the feature's only other entry point and `provideCodeLenses` + * returns nothing while `document.isDirty` — so it is absent at the one moment a user most needs it, + * right after typing `import requests` and seeing the squiggle. This provider parses the in-memory + * buffer instead, so it works on an unsaved edit. + * + * Deliberate non-promises, both in wording and in mechanics: + * - the title says what the action does, not that the squiggle will clear — the unresolved module + * may be undeclared in the block, or declared under a different distribution name (`PIL` vs + * `pillow`); + * - `diagnostics` is left unset, because populating it would tell VS Code this action *resolves* + * those diagnostics and would opt it into fix-all affordances; + * - `isPreferred` is left unset, so it never pre-empts a real import fix such as "add import". + * + * The action disappears once the script is set up (`shouldRoute`) and comes back by itself if the + * user later edits the metadata block, because a changed metadata identity resets the registry's + * validated association. + */ +export class InlineScriptSetupCodeActionProvider implements CodeActionProvider { + constructor( + private readonly routing: InlineScriptRoutingRegistry, + private readonly setupCommand: string, + ) {} + + /** + * Gates are ordered cheapest-first because VS Code may call this on every cursor move. The + * `context.diagnostics` test comes before any parsing: the common case is a file with no + * unresolved import at the cursor, and that case must cost nothing but a short array scan. + */ + public provideCodeActions( + document: TextDocument, + _range: Range, + context: CodeActionContext, + _token: CancellationToken, + ): CodeAction[] { + if (!isInlineScriptsFeatureEnabled()) { + return []; + } + if (!context.diagnostics.some(isUnresolvedImportDiagnostic)) { + return []; + } + const uri = document.uri; + if (!getInlineScriptRoutingKey(uri)) { + // Not a local `.py` file, so it can never carry an inline-script environment. + return []; + } + if (this.routing.shouldRoute(uri)) { + // A validated inline-script environment matching the current metadata already exists. + return []; + } + if (!readInlineScriptMetadata(getInlineScriptHeaderText(document), uri.fsPath)) { + return []; + } + const action = new CodeAction(InlineScriptStrings.setUpScriptEnvironment, CodeActionKind.QuickFix); + const trigger: InlineScriptSetupTrigger = 'codeaction'; + action.command = { + title: InlineScriptStrings.setUpScriptEnvironment, + command: this.setupCommand, + arguments: [uri, trigger], + }; + return [action]; + } +} + +/** + * Register the inline-script quick fix for local `.py` files. Only called when the PEP 723 + * inline-script feature flag is enabled, so it is a no-op for everyone else. + */ +export function registerInlineScriptSetupCodeAction( + routing: InlineScriptRoutingRegistry, + setupCommand: string, +): Disposable { + return languages.registerCodeActionsProvider( + { scheme: 'file', language: 'python' }, + new InlineScriptSetupCodeActionProvider(routing, setupCommand), + { providedCodeActionKinds: [CodeActionKind.QuickFix] }, + ); +} diff --git a/src/features/inlineScript/setupEnvironment.ts b/src/features/inlineScript/setupEnvironment.ts index f604fdde2..3954dee44 100644 --- a/src/features/inlineScript/setupEnvironment.ts +++ b/src/features/inlineScript/setupEnvironment.ts @@ -1,12 +1,19 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -import { commands, Disposable, l10n, QuickPickItem, Uri, window } from 'vscode'; +import { commands, Disposable, l10n, QuickPickItem, TextDocument, Uri, window } from 'vscode'; import { PythonEnvironment } from '../../api'; import { INLINE_SCRIPT_MANAGER_ID } from '../../common/constants'; import { readInlineScriptMetadataFromFile } from '../../common/inlineScript/metadata'; import { InlineScriptRoutingRegistry } from '../../common/inlineScript/routingRegistry'; -import { traceError, traceInfo } from '../../common/logging'; +import { InlineScriptStrings } from '../../common/localize'; +import { traceError, traceInfo, traceVerbose } from '../../common/logging'; +import { + EventNames, + InlineScriptSetupOutcomeKind, + InlineScriptSetupTrigger, +} from '../../common/telemetry/constants'; +import { sendTelemetryEvent } from '../../common/telemetry/sender'; import { normalizePath } from '../../common/utils/pathUtils'; import { showErrorMessage, @@ -18,6 +25,7 @@ import { asRelativePath, findFiles, getOpenTextDocuments } from '../../common/wo import { EnvironmentManagers } from '../../internal.api'; import { registerInlineScriptCodeLens } from './codeLens'; import { promptUpdateExtensionsForInlineScripts } from './extensionVersionCheck'; +import { registerInlineScriptSetupCodeAction } from './setupCodeAction'; /** * Hidden command invoked by the inline-script CodeLens to set up the environment for one script. @@ -86,47 +94,131 @@ async function seedRoutingMetadataForClosedScript(scriptUri: Uri, routing: Inlin if (routing.getMetadata(scriptUri)) { return; } + if (findOpenDocument(scriptUri)) { + return; + } + const metadata = await readInlineScriptMetadataFromFile(scriptUri); + if (metadata && !routing.getMetadata(scriptUri)) { + routing.setMetadata(scriptUri, metadata); + } +} + +/** The open text document backing `scriptUri`, if the user has it open. */ +function findOpenDocument(scriptUri: Uri): TextDocument | undefined { const scriptPath = normalizePath(scriptUri.fsPath); - const isOpen = getOpenTextDocuments().some( + return getOpenTextDocuments().find( (document) => document.uri.scheme === 'file' && normalizePath(document.uri.fsPath) === scriptPath, ); - if (isOpen) { - return; +} + +/** Record one user-initiated setup attempt and which surface started it. */ +function sendInlineScriptSetupInvokedTelemetry( + trigger: InlineScriptSetupTrigger, + outcome: InlineScriptSetupOutcomeKind, +): void { + sendTelemetryEvent(EventNames.INLINE_SCRIPT_SETUP_INVOKED, undefined, { trigger, outcome }); +} + +const INLINE_SCRIPT_SETUP_TRIGGERS: ReadonlySet = new Set([ + 'codelens', + 'codeaction', + 'bulk', +]); + +/** + * Coerce the trigger a command caller supplied to one of the known values, so the telemetry property + * stays low-cardinality no matter what the command is invoked with. Defaults to `codelens`, the only + * surface that predates the argument. + */ +function normalizeSetupTrigger(trigger: unknown): InlineScriptSetupTrigger { + return typeof trigger === 'string' && INLINE_SCRIPT_SETUP_TRIGGERS.has(trigger) + ? (trigger as InlineScriptSetupTrigger) + : 'codelens'; +} + +/** + * Save `scriptUri` if it is open with unsaved changes, so setup reads what the user actually sees. + * + * Setup resolves the PEP 723 block from disk (`InlineScriptEnvManager.create` → + * `readInlineScriptMetadataFromFile`), so an unsaved buffer would be set up against stale metadata — + * or against none at all, for a block the user just typed. The quick fix is offered in exactly that + * situation, since unlike the CodeLens it stays available while the document is dirty, so the save + * belongs here rather than at one call site. + * + * Returns `false` when the document could not be saved; setup must not run in that case. + */ +async function saveScriptBeforeSetup(scriptUri: Uri, routing: InlineScriptRoutingRegistry): Promise { + const document = findOpenDocument(scriptUri); + if (!document?.isDirty) { + return true; } + if (!(await document.save())) { + traceError(`Could not save ${scriptUri.fsPath} before setting up its inline-script environment.`); + return false; + } + traceVerbose(`Saved ${scriptUri.fsPath} before setting up its inline-script environment.`); + // Seed routing metadata from the file we just wrote. The lazy detector does this too, from its + // own save handler, but that runs asynchronously with respect to `save()` — and + // `setUpInlineScriptEnvironment` compares the metadata identity before and after `create` to + // detect an edit made mid-setup. Without seeding, a block the user only just typed would go from + // `undefined` to an identity during setup, be misread as a concurrent edit, and have its + // association silently skipped. Reading the same bytes the detector will read keeps the identity + // stable, and an unchanged identity preserves any existing validated association. const metadata = await readInlineScriptMetadataFromFile(scriptUri); - if (metadata && !routing.getMetadata(scriptUri)) { + if (metadata) { routing.setMetadata(scriptUri, metadata); } + return true; } -function setupInlineScriptEnvironmentHandler( +/** + * Handler for the single-file setup command, shared by the CodeLens and the unresolved-import quick + * fix. `trigger` records which of them invoked it; see {@link normalizeSetupTrigger}. + */ +export function setupInlineScriptEnvironmentHandler( em: EnvironmentManagers, routing: InlineScriptRoutingRegistry, -): (scriptUri?: Uri) => Promise { - return async (scriptUri?: Uri): Promise => { +): (scriptUri?: Uri, trigger?: InlineScriptSetupTrigger) => Promise { + return async (scriptUri?: Uri, rawTrigger?: InlineScriptSetupTrigger): Promise => { const uri = scriptUri ?? window.activeTextEditor?.document.uri; if (!uri || uri.scheme !== 'file') { return; } + const trigger = normalizeSetupTrigger(rawTrigger); if (!em.getEnvironmentManager(INLINE_SCRIPT_MANAGER_ID)) { + sendInlineScriptSetupInvokedTelemetry(trigger, 'error'); showErrorMessage(l10n.t('The inline script environment manager is not available yet. Try again shortly.')); return; } + if (!(await saveScriptBeforeSetup(uri, routing))) { + sendInlineScriptSetupInvokedTelemetry(trigger, 'error'); + showErrorMessage(InlineScriptStrings.saveFailedBeforeSetup); + return; + } + let environment: PythonEnvironment | undefined; try { - const environment = await setUpInlineScriptEnvironment(uri, em, routing); - if (!environment) { - notifyInlineScriptSetupOutcome(uri, routing); - return; - } - await promptUpdateExtensionsForInlineScripts(); + environment = await setUpInlineScriptEnvironment(uri, em, routing); } catch (error) { + sendInlineScriptSetupInvokedTelemetry(trigger, 'error'); traceError(`Failed to set up the inline-script environment for ${uri.fsPath}:`, error); showErrorMessage( l10n.t( 'Failed to set up the environment for this script. See the Python Environments output for details.', ), ); + return; } + sendInlineScriptSetupInvokedTelemetry(trigger, environment ? 'created' : 'notCreated'); + if (!environment) { + notifyInlineScriptSetupOutcome(uri, routing); + return; + } + // The companion-extension prompt is a follow-up nicety, and the environment is already set + // up by this point: a failure there must not be reported to the user as a setup failure, nor + // counted as a second setup attempt. The bulk path treats it the same way. + await promptUpdateExtensionsForInlineScripts().catch((error) => + traceError('Failed to check companion extension versions for inline scripts:', error), + ); }; } @@ -241,8 +333,10 @@ export async function setUpInlineScriptEnvironmentsInWorkspace( try { if (await setUpInlineScriptEnvironment(pick.uri, em, routing)) { succeeded += 1; + sendInlineScriptSetupInvokedTelemetry('bulk', 'created'); continue; } + sendInlineScriptSetupInvokedTelemetry('bulk', 'notCreated'); const outcome = routing.getSetupOutcome(pick.uri); if (outcome?.kind === 'cancelled') { // Cancelling one script's installer stops the whole run rather than immediately @@ -255,6 +349,7 @@ export async function setUpInlineScriptEnvironmentsInWorkspace( } } catch (error) { failed += 1; + sendInlineScriptSetupInvokedTelemetry('bulk', 'error'); traceError(`Failed to set up the inline-script environment for ${pick.uri.fsPath}:`, error); } } @@ -314,13 +409,15 @@ async function filterInlineScriptFiles(files: readonly Uri[]): Promise { } /** - * Register the inline-script user-facing surfaces (the CodeLens and its setup commands). Only called - * when the PEP 723 inline-script feature flag is enabled. The single-file setup command is invoked by - * the CodeLens and stays out of `package.json`; the bulk command is palette-gated behind the flag. + * Register the inline-script user-facing surfaces (the CodeLens, the unresolved-import quick fix, + * and their setup commands). Only called when the PEP 723 inline-script feature flag is enabled. + * The single-file setup command is invoked by both UI surfaces and stays out of `package.json`; the + * bulk command is palette-gated behind the flag. */ export function registerInlineScriptUx(em: EnvironmentManagers, routing: InlineScriptRoutingRegistry): Disposable[] { return [ registerInlineScriptCodeLens(routing, SETUP_INLINE_SCRIPT_ENV_COMMAND), + registerInlineScriptSetupCodeAction(routing, SETUP_INLINE_SCRIPT_ENV_COMMAND), commands.registerCommand(SETUP_INLINE_SCRIPT_ENV_COMMAND, setupInlineScriptEnvironmentHandler(em, routing)), commands.registerCommand(SETUP_INLINE_SCRIPT_ENVS_COMMAND, () => setUpInlineScriptEnvironmentsInWorkspace(em, routing), diff --git a/src/test/features/inlineScript/codeLens.unit.test.ts b/src/test/features/inlineScript/codeLens.unit.test.ts index 935af095b..05731fd4c 100644 --- a/src/test/features/inlineScript/codeLens.unit.test.ts +++ b/src/test/features/inlineScript/codeLens.unit.test.ts @@ -52,7 +52,7 @@ suite('Inline script CodeLens provider', () => { assert.strictEqual(lenses.length, 1); assert.strictEqual(lenses[0].command?.command, SETUP_COMMAND); - assert.deepStrictEqual(lenses[0].command?.arguments, [scriptUri]); + assert.deepStrictEqual(lenses[0].command?.arguments, [scriptUri, 'codelens']); }); test('shows no CodeLens while the document has unsaved changes', () => { diff --git a/src/test/features/inlineScript/setupCodeAction.unit.test.ts b/src/test/features/inlineScript/setupCodeAction.unit.test.ts new file mode 100644 index 000000000..d2e6df33a --- /dev/null +++ b/src/test/features/inlineScript/setupCodeAction.unit.test.ts @@ -0,0 +1,257 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import assert from 'assert'; +import * as sinon from 'sinon'; +import { CodeActionContext, Diagnostic, Position, Range, TextDocument, Uri } from 'vscode'; +import { InlineScriptMetadata, MAX_HEADER_BYTES } from '../../../common/inlineScript/metadata'; +import { InlineScriptRoutingRegistry } from '../../../common/inlineScript/routingRegistry'; +import { InlineScriptStrings } from '../../../common/localize'; +import { + InlineScriptSetupCodeActionProvider, + isUnresolvedImportDiagnostic, +} from '../../../features/inlineScript/setupCodeAction'; +import * as helpers from '../../../helpers'; + +const SETUP_COMMAND = 'python-envs.setupInlineScriptEnv'; + +const SCRIPT_WITH_METADATA = [ + '# /// script', + '# dependencies = ["requests"]', + '# ///', + '', + 'import requests', + '', +].join('\n'); + +const SCRIPT_WITHOUT_METADATA = ['import requests', '', 'print(requests)', ''].join('\n'); + +function makeMetadata(): InlineScriptMetadata { + return { + dependencies: ['requests'], + range: { start: 0, end: 47 }, + sourceRange: { start: 0, end: 47 }, + }; +} + +function makeDocument(uri: Uri, text = SCRIPT_WITH_METADATA): TextDocument { + return { + uri, + languageId: 'python', + getText: () => text, + } as unknown as TextDocument; +} + +function makeDiagnostic(code: Diagnostic['code'], source = 'Pylance'): Diagnostic { + const range = new Range(new Position(4, 7), new Position(4, 15)); + const diagnostic = new Diagnostic(range, 'Import "requests" could not be resolved'); + diagnostic.code = code; + diagnostic.source = source; + return diagnostic; +} + +function makeContext(diagnostics: Diagnostic[]): CodeActionContext { + return { diagnostics, only: undefined, triggerKind: 1 } as unknown as CodeActionContext; +} + +const INVOCATION_RANGE = new Range(new Position(4, 7), new Position(4, 7)); + +suite('Inline script setup code action', () => { + const scriptUri = Uri.file('/workspace/app.py'); + let routing: InlineScriptRoutingRegistry; + let provider: InlineScriptSetupCodeActionProvider; + let featureEnabledStub: sinon.SinonStub; + + setup(() => { + featureEnabledStub = sinon.stub(helpers, 'isInlineScriptsFeatureEnabled').returns(true); + routing = new InlineScriptRoutingRegistry(); + provider = new InlineScriptSetupCodeActionProvider(routing, SETUP_COMMAND); + }); + + teardown(() => { + routing.dispose(); + sinon.restore(); + }); + + function provide(document: TextDocument, diagnostics: Diagnostic[]) { + return provider.provideCodeActions(document, INVOCATION_RANGE, makeContext(diagnostics), {} as never); + } + + suite('isUnresolvedImportDiagnostic', () => { + const matching: Array<[string, Diagnostic['code']]> = [ + ['Pyright/Pylance reportMissingImports', 'reportMissingImports'], + ['Pyright/Pylance reportMissingModuleSource', 'reportMissingModuleSource'], + ['Ty unresolved-import', 'unresolved-import'], + ['Ty possibly-missing-import', 'possibly-missing-import'], + ['Pyrefly missing-import', 'missing-import'], + ['Pyrefly missing-source', 'missing-source'], + ['Pyrefly missing-source-for-stubs', 'missing-source-for-stubs'], + ['mypy import-not-found', 'import-not-found'], + ['mypy import-untyped', 'import-untyped'], + ]; + + for (const [label, code] of matching) { + test(`matches ${label}`, () => { + assert.strictEqual(isUnresolvedImportDiagnostic(makeDiagnostic(code)), true); + }); + } + + test('matches the {value, target} object form of Diagnostic.code', () => { + const code = { value: 'reportMissingImports', target: Uri.parse('https://example.invalid/rule') }; + assert.strictEqual(isUnresolvedImportDiagnostic(makeDiagnostic(code)), true); + }); + + test('matches regardless of case', () => { + assert.strictEqual(isUnresolvedImportDiagnostic(makeDiagnostic('ReportMissingImports')), true); + }); + + test('ignores the diagnostic source, which is "pylance + pyrefly" for Pyrefly', () => { + assert.strictEqual( + isUnresolvedImportDiagnostic(makeDiagnostic('missing-import', 'pylance + pyrefly')), + true, + ); + }); + + test('does not match unrelated diagnostic codes', () => { + assert.strictEqual(isUnresolvedImportDiagnostic(makeDiagnostic('reportUndefinedVariable')), false); + assert.strictEqual(isUnresolvedImportDiagnostic(makeDiagnostic('unresolved-reference')), false); + }); + + test('does not match a diagnostic with no code', () => { + assert.strictEqual(isUnresolvedImportDiagnostic(makeDiagnostic(undefined)), false); + }); + + test('does not match a numeric code that is not an import rule', () => { + assert.strictEqual(isUnresolvedImportDiagnostic(makeDiagnostic(42)), false); + }); + }); + + suite('provideCodeActions', () => { + test('offers setup for an unresolved import in a not-yet-configured inline script', () => { + const actions = provide(makeDocument(scriptUri), [makeDiagnostic('reportMissingImports')]); + + assert.strictEqual(actions.length, 1); + assert.strictEqual(actions[0].title, InlineScriptStrings.setUpScriptEnvironment); + assert.strictEqual(actions[0].command?.command, SETUP_COMMAND); + assert.deepStrictEqual(actions[0].command?.arguments, [scriptUri, 'codeaction']); + }); + + test('leaves diagnostics unset so VS Code is not told the action resolves them', () => { + const actions = provide(makeDocument(scriptUri), [makeDiagnostic('reportMissingImports')]); + + assert.strictEqual(actions.length, 1); + assert.strictEqual( + actions[0].diagnostics, + undefined, + 'setting diagnostics would claim the action fixes them and opt it into fix-all', + ); + }); + + test('leaves isPreferred unset so it never pre-empts a real import fix', () => { + const actions = provide(makeDocument(scriptUri), [makeDiagnostic('reportMissingImports')]); + + assert.strictEqual(actions.length, 1); + assert.strictEqual(actions[0].isPreferred, undefined); + }); + + test('offers nothing when no diagnostic reports an unresolved import', () => { + const actions = provide(makeDocument(scriptUri), [makeDiagnostic('reportUndefinedVariable')]); + + assert.strictEqual(actions.length, 0); + }); + + test('offers nothing when there are no diagnostics at the invocation range', () => { + const actions = provide(makeDocument(scriptUri), []); + + assert.strictEqual(actions.length, 0); + }); + + test('offers nothing when the file has no PEP 723 block', () => { + const actions = provide(makeDocument(scriptUri, SCRIPT_WITHOUT_METADATA), [ + makeDiagnostic('reportMissingImports'), + ]); + + assert.strictEqual(actions.length, 0); + }); + + test('offers nothing when the PEP 723 block is malformed', () => { + const malformed = ['# /// script', '# dependencies = [', '# ///', '', 'import requests', ''].join('\n'); + + const actions = provide(makeDocument(scriptUri, malformed), [makeDiagnostic('reportMissingImports')]); + + assert.strictEqual(actions.length, 0); + }); + + test('offers nothing once the script is already set up', () => { + routing.setMetadata(scriptUri, makeMetadata()); + routing.setValidatedAssociation(scriptUri, true); + assert.strictEqual(routing.shouldRoute(scriptUri), true); + + const actions = provide(makeDocument(scriptUri), [makeDiagnostic('reportMissingImports')]); + + assert.strictEqual(actions.length, 0); + }); + + test('offers nothing when the inline-scripts feature flag is off', () => { + featureEnabledStub.returns(false); + + const actions = provide(makeDocument(scriptUri), [makeDiagnostic('reportMissingImports')]); + + assert.strictEqual(actions.length, 0); + }); + + test('offers nothing for a file that cannot carry an inline-script environment', () => { + const notebookUri = Uri.parse('vscode-notebook-cell:/workspace/app.py#ch0'); + + const actions = provide(makeDocument(notebookUri), [makeDiagnostic('reportMissingImports')]); + + assert.strictEqual(actions.length, 0); + }); + + test('offers setup again after the metadata block is edited to add the missing dependency', () => { + routing.setMetadata(scriptUri, makeMetadata()); + routing.setValidatedAssociation(scriptUri, true); + assert.strictEqual( + provide(makeDocument(scriptUri), [makeDiagnostic('reportMissingImports')]).length, + 0, + 'precondition: hidden while the script is set up', + ); + + // The user adds the module they were missing to the block; a changed metadata identity + // resets the validated association, so the script needs setting up again. + routing.setMetadata(scriptUri, { ...makeMetadata(), dependencies: ['requests', 'rich'] }); + assert.strictEqual(routing.shouldRoute(scriptUri), false); + + const edited = ['# /// script', '# dependencies = ["requests", "rich"]', '# ///', '', 'import rich', ''].join( + '\n', + ); + const actions = provide(makeDocument(scriptUri, edited), [makeDiagnostic('reportMissingImports')]); + + assert.strictEqual(actions.length, 1); + assert.strictEqual(actions[0].command?.command, SETUP_COMMAND); + }); + + test('parses the in-memory buffer, so it works on an unsaved edit the CodeLens cannot see', () => { + // Nothing has been saved, so the routing registry knows nothing about this file — the + // state in which `provideCodeLenses` returns no lens. + assert.strictEqual(routing.getMetadata(scriptUri), undefined); + + const actions = provide(makeDocument(scriptUri), [makeDiagnostic('reportMissingImports')]); + + assert.strictEqual(actions.length, 1); + }); + + test('ignores a metadata block that sits past the header byte budget setup reads', () => { + const padding = `${'# padding comment\n'.repeat(Math.ceil(MAX_HEADER_BYTES / 18) + 10)}`; + const document = makeDocument(scriptUri, padding + SCRIPT_WITH_METADATA); + + const actions = provide(document, [makeDiagnostic('reportMissingImports')]); + + assert.strictEqual( + actions.length, + 0, + 'setup reads only the first MAX_HEADER_BYTES from disk, so the block must be invisible here too', + ); + }); + }); +}); diff --git a/src/test/features/inlineScript/setupEnvironment.unit.test.ts b/src/test/features/inlineScript/setupEnvironment.unit.test.ts index 9f3ff1a8a..3acea0d23 100644 --- a/src/test/features/inlineScript/setupEnvironment.unit.test.ts +++ b/src/test/features/inlineScript/setupEnvironment.unit.test.ts @@ -10,13 +10,17 @@ import { INLINE_SCRIPT_MANAGER_ID } from '../../../common/constants'; import { InlineScriptMetadata } from '../../../common/inlineScript/metadata'; import * as metadataApi from '../../../common/inlineScript/metadata'; import { InlineScriptRoutingRegistry } from '../../../common/inlineScript/routingRegistry'; +import { EventNames } from '../../../common/telemetry/constants'; +import * as telemetrySender from '../../../common/telemetry/sender'; import * as winapi from '../../../common/window.apis'; import * as wapi from '../../../common/workspace.apis'; import { notifyInlineScriptSetupOutcome, setUpInlineScriptEnvironment, setUpInlineScriptEnvironmentsInWorkspace, + setupInlineScriptEnvironmentHandler, } from '../../../features/inlineScript/setupEnvironment'; +import * as extensionVersionCheck from '../../../features/inlineScript/extensionVersionCheck'; import { EnvironmentManagers, InternalEnvironmentManager } from '../../../internal.api'; function makeEnv(): PythonEnvironment { @@ -313,3 +317,173 @@ suite('notifyInlineScriptSetupOutcome', () => { sinon.assert.notCalled(errorStub); }); }); + +suite('setupInlineScriptEnvironmentHandler', () => { + const scriptUri = Uri.file('/workspace/app.py'); + let em: typemoq.IMock; + let manager: typemoq.IMock; + let routing: InlineScriptRoutingRegistry; + let readMetadataStub: sinon.SinonStub; + let openDocumentsStub: sinon.SinonStub; + let sendTelemetryStub: sinon.SinonStub; + let errorStub: sinon.SinonStub; + let saveStub: sinon.SinonStub; + let promptStub: sinon.SinonStub; + + setup(() => { + em = typemoq.Mock.ofType(); + manager = typemoq.Mock.ofType(); + routing = new InlineScriptRoutingRegistry(); + em.setup((m) => m.getEnvironmentManager(INLINE_SCRIPT_MANAGER_ID)).returns(() => manager.object); + readMetadataStub = sinon.stub(metadataApi, 'readInlineScriptMetadataFromFile').resolves(undefined); + openDocumentsStub = sinon.stub(wapi, 'getOpenTextDocuments').returns([]); + sendTelemetryStub = sinon.stub(telemetrySender, 'sendTelemetryEvent'); + errorStub = sinon.stub(winapi, 'showErrorMessage').resolves(undefined); + sinon.stub(winapi, 'showInformationMessage').resolves(undefined); + sinon.stub(winapi, 'showWarningMessage').resolves(undefined); + promptStub = sinon.stub(extensionVersionCheck, 'promptUpdateExtensionsForInlineScripts').resolves(); + saveStub = sinon.stub().resolves(true); + }); + + teardown(() => { + routing.dispose(); + sinon.restore(); + }); + + function openDirtyDocument(isDirty = true): void { + openDocumentsStub.returns([{ uri: scriptUri, isDirty, save: saveStub }]); + } + + function setupInvokedCalls(): sinon.SinonSpyCall[] { + return sendTelemetryStub + .getCalls() + .filter((call) => call.args[0] === EventNames.INLINE_SCRIPT_SETUP_INVOKED); + } + + function expectEnvironmentCreated(): PythonEnvironment { + const env = makeEnv(); + manager.setup((m) => m.create(scriptUri, undefined)).returns(() => Promise.resolve(env)); + em.setup((m) => m.setEnvironment(scriptUri, env)).returns(() => Promise.resolve()); + return env; + } + + test('saves a dirty document before setup, because setup reads the block from disk', async () => { + openDirtyDocument(); + expectEnvironmentCreated(); + + await setupInlineScriptEnvironmentHandler(em.object, routing)(scriptUri, 'codeaction'); + + sinon.assert.calledOnce(saveStub); + em.verify((m) => m.setEnvironment(scriptUri, typemoq.It.isAny()), typemoq.Times.once()); + }); + + test('seeds routing metadata from the saved file so setup does not misread it as a mid-setup edit', async () => { + // The detector's own save handler runs asynchronously relative to `save()`. Without this + // seeding, a block the user just typed goes `undefined` -> identity while `create` runs, + // which `setUpInlineScriptEnvironment` treats as a concurrent edit and silently skips. + openDirtyDocument(); + const metadata = makeMetadata(['requests']); + readMetadataStub.resolves(metadata); + const env = makeEnv(); + manager + .setup((m) => m.create(scriptUri, undefined)) + .returns(async () => { + // The detector catches up mid-setup and republishes the same saved metadata. + routing.setMetadata(scriptUri, makeMetadata(['requests'])); + return env; + }); + em.setup((m) => m.setEnvironment(scriptUri, env)).returns(() => Promise.resolve()); + + await setupInlineScriptEnvironmentHandler(em.object, routing)(scriptUri, 'codeaction'); + + assert.deepStrictEqual(routing.getMetadata(scriptUri), metadata); + em.verify((m) => m.setEnvironment(scriptUri, env), typemoq.Times.once()); + }); + + test('does not save a document that has no unsaved changes', async () => { + openDirtyDocument(false); + expectEnvironmentCreated(); + + await setupInlineScriptEnvironmentHandler(em.object, routing)(scriptUri, 'codelens'); + + sinon.assert.notCalled(saveStub); + }); + + test('does not run setup when the document could not be saved', async () => { + openDirtyDocument(); + saveStub.resolves(false); + + await setupInlineScriptEnvironmentHandler(em.object, routing)(scriptUri, 'codeaction'); + + em.verify((m) => m.setEnvironment(typemoq.It.isAny(), typemoq.It.isAny()), typemoq.Times.never()); + sinon.assert.calledOnce(errorStub); + assert.deepStrictEqual(setupInvokedCalls()[0].args[2], { trigger: 'codeaction', outcome: 'error' }); + }); + + test('records the invoking surface and a created outcome', async () => { + expectEnvironmentCreated(); + + await setupInlineScriptEnvironmentHandler(em.object, routing)(scriptUri, 'codeaction'); + + assert.strictEqual(setupInvokedCalls().length, 1); + assert.deepStrictEqual(setupInvokedCalls()[0].args[2], { trigger: 'codeaction', outcome: 'created' }); + }); + + test('records the CodeLens surface separately', async () => { + expectEnvironmentCreated(); + + await setupInlineScriptEnvironmentHandler(em.object, routing)(scriptUri, 'codelens'); + + assert.deepStrictEqual(setupInvokedCalls()[0].args[2], { trigger: 'codelens', outcome: 'created' }); + }); + + test('records a notCreated outcome when setup produces no environment', async () => { + manager.setup((m) => m.create(scriptUri, undefined)).returns(() => Promise.resolve(undefined)); + + await setupInlineScriptEnvironmentHandler(em.object, routing)(scriptUri, 'codeaction'); + + assert.deepStrictEqual(setupInvokedCalls()[0].args[2], { trigger: 'codeaction', outcome: 'notCreated' }); + }); + + test('records an error outcome when setup throws', async () => { + manager.setup((m) => m.create(scriptUri, undefined)).returns(() => Promise.reject(new Error('boom'))); + + await setupInlineScriptEnvironmentHandler(em.object, routing)(scriptUri, 'codeaction'); + + assert.deepStrictEqual(setupInvokedCalls()[0].args[2], { trigger: 'codeaction', outcome: 'error' }); + sinon.assert.calledOnce(errorStub); + }); + + test('coerces an unknown trigger so the telemetry property stays low-cardinality', async () => { + expectEnvironmentCreated(); + + await setupInlineScriptEnvironmentHandler(em.object, routing)( + scriptUri, + 'something-else' as never, + ); + + assert.deepStrictEqual(setupInvokedCalls()[0].args[2], { trigger: 'codelens', outcome: 'created' }); + }); + + test('emits exactly one setup event per invocation', async () => { + expectEnvironmentCreated(); + + await setupInlineScriptEnvironmentHandler(em.object, routing)(scriptUri, 'codeaction'); + + assert.strictEqual(setupInvokedCalls().length, 1); + }); + + test('does not report a failing companion-extension prompt as a setup failure', async () => { + // The environment is already set up by then, so a failure in the follow-up version check + // must not surface an error or count as a second attempt. + expectEnvironmentCreated(); + promptStub.rejects(new Error('boom')); + + await setupInlineScriptEnvironmentHandler(em.object, routing)(scriptUri, 'codeaction'); + + assert.deepStrictEqual(setupInvokedCalls().map((call) => call.args[2]), [ + { trigger: 'codeaction', outcome: 'created' }, + ]); + sinon.assert.notCalled(errorStub); + }); +});