From af87c123aad0597f81fac9db8bfe28d535cc5047 Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Mon, 11 May 2026 16:36:33 -0700 Subject: [PATCH 1/9] implement parser --- src/common/inlineScriptMetadata.ts | 398 +++++++++++++++ .../common/inlineScriptMetadata.unit.test.ts | 477 ++++++++++++++++++ 2 files changed, 875 insertions(+) create mode 100644 src/common/inlineScriptMetadata.ts create mode 100644 src/test/common/inlineScriptMetadata.unit.test.ts diff --git a/src/common/inlineScriptMetadata.ts b/src/common/inlineScriptMetadata.ts new file mode 100644 index 00000000..28c0f841 --- /dev/null +++ b/src/common/inlineScriptMetadata.ts @@ -0,0 +1,398 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import * as tomljs from '@iarna/toml'; +import * as fs from 'fs/promises'; +import { Uri } from 'vscode'; +import { traceVerbose, traceWarn } from './logging'; + +/** + * Parsed and validated PEP 723 `script` metadata block. + * + * See: https://packaging.python.org/en/latest/specifications/inline-script-metadata/ + */ +export interface InlineScriptMetadata { + /** Parsed value of `requires-python`, if present. */ + readonly requiresPython?: string; + /** Parsed value of `dependencies`, if present. */ + readonly dependencies?: readonly string[]; + /** Parsed `[tool]` table, opaque to this parser. */ + readonly tool?: tomljs.JsonMap; + /** + * Character offsets of the `# /// script` … `# ///` block in the + * (normalized — see notes on BOM and CRLF handling below) text that + * was parsed: inclusive start of the `# /// script` line, exclusive + * end immediately after the closing `# ///` line's terminating + * newline (or end of string if there is no trailing newline). + */ + readonly range: { readonly start: number; readonly end: number }; +} + +/** + * Maximum bytes read from the head of a file when looking for inline + * script metadata. PEP 723 blocks live at the top of files, so reading + * the first chunk is sufficient. Callers that need to handle scripts + * with very large leading shebang / comment blocks should know that + * anything past this byte boundary is invisible to the detector. + */ +export const MAX_HEADER_BYTES = 8 * 1024; + +/** + * Canonical block regex from the PEP 723 spec, translated to JavaScript + * (Python's `(?P...)` becomes `(?...)` in JS). The flag + * combination `gm` is required so `^` / `$` anchor on line boundaries + * and so the engine can iterate multiple candidate blocks. + * + * Important: this regex assumes line endings have already been + * normalized to `\n`. In Python's `re` module `.` matches `\r`, but in + * JavaScript it does not, so a literal CRLF file would behave + * inconsistently against this pattern. `readInlineScriptMetadata` + * normalizes line endings before applying the regex. + */ +const BLOCK_RE = /^# \/\/\/ (?[a-zA-Z0-9-]+)$\s(?(^#(| .*)$\s)+)^# \/\/\/$/gm; + +/** + * Parse PEP 723 `script` metadata from script source text. + * + * Returns: + * - the parsed metadata if the text contains exactly one well-formed + * `script` block; + * - `undefined` if there is no `script` block, if there are multiple + * `script` blocks (per spec this MUST error), or if the block's + * TOML payload is malformed. + * + * Encoding: input is processed as UTF-8 text. The `# -*- coding: ... -*-` + * declaration is not honored (the spec permits but does not require it). + */ +export function readInlineScriptMetadata(scriptText: string): InlineScriptMetadata | undefined { + if (!scriptText) { + return undefined; + } + + // Strip a single leading UTF-8 BOM (\uFEFF). Files saved as + // "UTF-8 with BOM" on Windows have this; without stripping it the + // first line becomes "\uFEFF# /// script" and the regex fails to + // match. + let text = scriptText.charCodeAt(0) === 0xfeff ? scriptText.slice(1) : scriptText; + + // Normalize CRLF and lone CR to LF so the canonical regex (which + // was authored assuming `.` matches `\r`, true in Python's re but + // not in JavaScript) behaves consistently. The offsets in `range` + // refer to this normalized text. + text = text.replace(/\r\n?/g, '\n'); + + // Collect ALL matches first so we can detect the "multiple script + // blocks" error case the spec requires us to surface. + BLOCK_RE.lastIndex = 0; + const scriptMatches: RegExpExecArray[] = []; + let m: RegExpExecArray | null; + while ((m = BLOCK_RE.exec(text)) !== null) { + // Per spec, tools MUST NOT read non-standardized block types. + // The only standardized type today is `script`. + if (m.groups?.type === 'script') { + scriptMatches.push(m); + } + // Defensive: zero-width match would cause an infinite loop. + if (m.index === BLOCK_RE.lastIndex) { + BLOCK_RE.lastIndex += 1; + } + } + + if (scriptMatches.length === 0) { + traceVerbose('inline script metadata: no `# /// script` block found'); + return undefined; + } + if (scriptMatches.length > 1) { + traceWarn( + `inline script metadata: ${scriptMatches.length} \`# /// script\` blocks found; per PEP 723 multiple blocks of the same type MUST be an error.`, + ); + return undefined; + } + + const match = scriptMatches[0]; + const rawContent = match.groups!.content; + + // Validate each content line and reconstruct the TOML payload, + // applying the spec's content-extraction rule: + // if line[1] === ' ' drop 2 chars, else drop 1 char (the leading '#'). + // The canonical regex already restricts content lines to '#' or + // '# ', but we walk the lines explicitly here both for + // safety against regex-engine quirks and to keep the + // reconstruction logic obvious. + const reconstructed: string[] = []; + const contentLines = rawContent.split('\n'); + for (const line of contentLines) { + if (line.length === 0) { + // Final element after splitting on the trailing '\n' that + // belongs to the last content line. Not a real line. + continue; + } + if (line[0] !== '#') { + traceWarn(`inline script metadata: invalid content line (must start with '#'): ${JSON.stringify(line)}`); + return undefined; + } + if (line.length === 1) { + // Bare '#': a blank content line within the block. + reconstructed.push(''); + continue; + } + if (line[1] !== ' ') { + // Per spec, content lines are exactly '#' or '# '. + // '##foo', '#\tfoo', '#foo' are not valid. + traceWarn(`inline script metadata: invalid content line (expected '#' or '# '): ${JSON.stringify(line)}`); + return undefined; + } + reconstructed.push(line.slice(2)); + } + + let parsed: tomljs.JsonMap; + try { + parsed = tomljs.parse(reconstructed.join('\n')); + } catch (err) { + traceWarn('inline script metadata: failed to parse TOML in `# /// script` block:', err); + return undefined; + } + + // Validate the small set of known fields. Unknown top-level keys + // are tolerated — the spec reserves room for future tool tables + // and we don't want to be brittle. + let requiresPython: string | undefined; + if (parsed['requires-python'] !== undefined) { + if (typeof parsed['requires-python'] !== 'string') { + traceWarn( + `inline script metadata: 'requires-python' must be a string, got ${typeof parsed['requires-python']}`, + ); + return undefined; + } + requiresPython = parsed['requires-python']; + } + + let dependencies: readonly string[] | undefined; + if (parsed.dependencies !== undefined) { + if (!Array.isArray(parsed.dependencies)) { + traceWarn('inline script metadata: `dependencies` must be an array of strings'); + return undefined; + } + for (const dep of parsed.dependencies) { + if (typeof dep !== 'string') { + traceWarn('inline script metadata: each entry in `dependencies` must be a string'); + return undefined; + } + } + // Defensive copy + freeze so consumers can't mutate the cached + // parse result. + dependencies = Object.freeze((parsed.dependencies as string[]).slice()); + } + + let tool: tomljs.JsonMap | undefined; + if (parsed.tool !== undefined) { + if (typeof parsed.tool !== 'object' || Array.isArray(parsed.tool) || parsed.tool === null) { + traceWarn('inline script metadata: `tool` must be a table'); + return undefined; + } + tool = parsed.tool as tomljs.JsonMap; + } + + // Range end: position immediately AFTER the closing `# ///` line's + // newline. The regex's `$` anchor stops before the newline, so we + // step over it explicitly when present. + let end = match.index + match[0].length; + if (text.charAt(end) === '\n') { + end += 1; + } + + return { + requiresPython, + dependencies, + tool, + range: { start: match.index, end }, + }; +} + +/** + * Read PEP 723 metadata from a file. Reads only the first + * `MAX_HEADER_BYTES` bytes of the file — PEP 723 blocks live at the + * top of files, so reading the whole file would be wasteful when this + * is invoked across many candidate `.py` files. + * + * Returns `undefined` for: + * - any URI scheme other than `file:` (notebook cells, untitled + * buffers, git: revisions, etc. are out of scope); + * - any I/O error (logged at `traceVerbose`); + * - any of the malformed-metadata cases handled by + * `readInlineScriptMetadata`. + */ +export async function readInlineScriptMetadataFromFile(uri: Uri): Promise { + if (uri.scheme !== 'file') { + traceVerbose(`inline script metadata: skipping non-file URI scheme '${uri.scheme}'`); + return undefined; + } + + let text: string; + try { + const handle = await fs.open(uri.fsPath, 'r'); + try { + const buf = Buffer.alloc(MAX_HEADER_BYTES); + const { bytesRead } = await handle.read(buf, 0, MAX_HEADER_BYTES, 0); + text = buf.toString('utf-8', 0, bytesRead); + } finally { + await handle.close(); + } + } catch (err) { + traceVerbose(`inline script metadata: failed to read ${uri.fsPath}:`, err); + return undefined; + } + + return readInlineScriptMetadata(text); +} + +/** + * Test whether a Python `version` (e.g. "3.12.4") satisfies a PEP 440 + * version specifier (e.g. ">=3.11"). Implements the subset of PEP 440 + * needed by `requires-python`: + * + * - operators `==`, `!=`, `>=`, `<=`, `>`, `<`, `~=`, `===`; + * - comma-separated clauses are AND-ed; + * - wildcard `==X.Y.*` (and the negated `!=X.Y.*`) is supported; + * - pre-release / dev / post / local version semantics are NOT + * modeled (script `requires-python` is almost always a simple + * lower bound; suffixes on the input version are truncated to + * the release segments). + * + * Returns `false` (and logs a `traceWarn`) on an unparseable + * specifier — safer than defaulting to "any version goes". + */ +export function matchesPythonVersion(requiresPython: string, version: string): boolean { + if (!requiresPython || !version) { + return false; + } + const clauses = requiresPython + .split(',') + .map((c) => c.trim()) + .filter((c) => c.length > 0); + if (clauses.length === 0) { + return false; + } + for (const clause of clauses) { + if (!matchSingleClause(clause, version)) { + return false; + } + } + return true; +} + +// Longest-match-first order matters: `===` must beat `==`, `~=` and +// `>=` / `<=` / `!=` must beat the single-char operators. +const SPECIFIER_RE = /^(===|~=|==|!=|>=|<=|>|<)\s*(.+)$/; + +function matchSingleClause(clause: string, version: string): boolean { + const m = clause.match(SPECIFIER_RE); + if (!m) { + traceWarn(`inline script metadata: unrecognized requires-python clause: ${JSON.stringify(clause)}`); + return false; + } + const op = m[1]; + const specVersion = m[2].trim(); + + if (op === '===') { + // Arbitrary-equality: exact string comparison after stripping + // a leading 'v' (which PEP 440 permits). + const normSpec = specVersion.replace(/^v/i, ''); + const normVer = version.replace(/^v/i, ''); + return normSpec === normVer; + } + + if (specVersion.endsWith('.*')) { + if (op !== '==' && op !== '!=') { + traceWarn( + `inline script metadata: wildcard versions are only valid with '==' or '!=': ${JSON.stringify(clause)}`, + ); + return false; + } + const prefix = parseRelease(specVersion.slice(0, -2)); + const ver = parseRelease(version); + if (prefix === undefined || ver === undefined) { + traceWarn(`inline script metadata: cannot parse version for clause ${JSON.stringify(clause)}`); + return false; + } + const isPrefixMatch = ver.length >= prefix.length && prefix.every((seg, i) => ver[i] === seg); + return op === '==' ? isPrefixMatch : !isPrefixMatch; + } + + const specSegs = parseRelease(specVersion); + const verSegs = parseRelease(version); + if (specSegs === undefined || verSegs === undefined) { + traceWarn(`inline script metadata: cannot parse version for clause ${JSON.stringify(clause)}`); + return false; + } + + const cmp = compareReleases(verSegs, specSegs); + switch (op) { + case '==': + return cmp === 0; + case '!=': + return cmp !== 0; + case '>=': + return cmp >= 0; + case '<=': + return cmp <= 0; + case '>': + return cmp > 0; + case '<': + return cmp < 0; + case '~=': { + // Compatible release. `~=X.Y` is equivalent to + // `>= X.Y, == X.*`; `~=X.Y.Z` is `>= X.Y.Z, == X.Y.*`. + // PEP 440 requires at least two release segments here. + if (specSegs.length < 2) { + traceWarn( + `inline script metadata: '~=' requires at least two release segments: ${JSON.stringify(clause)}`, + ); + return false; + } + if (cmp < 0) { + return false; + } + const prefix = specSegs.slice(0, -1); + if (verSegs.length < prefix.length) { + return false; + } + return prefix.every((seg, i) => verSegs[i] === seg); + } + default: + // Unreachable — SPECIFIER_RE only matches the operators above. + return false; + } +} + +function parseRelease(v: string): number[] | undefined { + let s = v.trim().replace(/^v/i, ''); + // Strip optional epoch prefix `N!`. + const epoch = s.match(/^(\d+)!(.*)$/); + if (epoch) { + s = epoch[2]; + } + // Take only the leading dotted-integer segments; PEP 440 release + // segments must be integers. Pre/post/dev/local suffixes are + // dropped, which is sufficient for `requires-python` matching. + const m = s.match(/^(\d+(?:\.\d+)*)/); + if (!m) { + return undefined; + } + return m[1].split('.').map((x) => parseInt(x, 10)); +} + +function compareReleases(a: readonly number[], b: readonly number[]): number { + const n = Math.max(a.length, b.length); + for (let i = 0; i < n; i++) { + const av = a[i] ?? 0; + const bv = b[i] ?? 0; + if (av < bv) { + return -1; + } + if (av > bv) { + return 1; + } + } + return 0; +} diff --git a/src/test/common/inlineScriptMetadata.unit.test.ts b/src/test/common/inlineScriptMetadata.unit.test.ts new file mode 100644 index 00000000..9b98c502 --- /dev/null +++ b/src/test/common/inlineScriptMetadata.unit.test.ts @@ -0,0 +1,477 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import assert from 'assert'; +import * as fs from 'fs-extra'; +import * as os from 'os'; +import path from 'path'; +import * as sinon from 'sinon'; +import { Uri } from 'vscode'; +import { + InlineScriptMetadata, + MAX_HEADER_BYTES, + matchesPythonVersion, + readInlineScriptMetadata, + readInlineScriptMetadataFromFile, +} from '../../common/inlineScriptMetadata'; +import * as logging from '../../common/logging'; + +// Helper to assemble a script body. Lines are joined with '\n' so each +// test controls line endings explicitly via `joiner` when needed. +function script(lines: string[], joiner = '\n'): string { + return lines.join(joiner); +} + +suite('inlineScriptMetadata', () => { + let traceWarnStub: sinon.SinonStub; + let traceVerboseStub: sinon.SinonStub; + + setup(() => { + traceWarnStub = sinon.stub(logging, 'traceWarn'); + traceVerboseStub = sinon.stub(logging, 'traceVerbose'); + }); + + teardown(() => { + sinon.restore(); + }); + + suite('readInlineScriptMetadata', () => { + test('empty input returns undefined', () => { + assert.strictEqual(readInlineScriptMetadata(''), undefined); + }); + + test('file without any block returns undefined', () => { + const text = script(['#!/usr/bin/env python3', 'import sys', 'print("hello")']); + assert.strictEqual(readInlineScriptMetadata(text), undefined); + }); + + test('minimal valid block (single blank content line) parses with empty metadata', () => { + // The canonical PEP 723 regex requires AT LEAST ONE content + // line between the open and close markers (the `+` quantifier + // in `(^#(| .*)$\s)+`). A bare `#` line counts as a blank + // content line and is the minimal accepted form. + const text = script(['# /// script', '#', '# ///', '', 'print("hi")']); + const md = readInlineScriptMetadata(text); + assert.ok(md, 'expected metadata to be defined'); + assert.strictEqual(md.requiresPython, undefined); + assert.strictEqual(md.dependencies, undefined); + assert.strictEqual(md.tool, undefined); + assert.strictEqual(md.range.start, 0); + const expectedBlock = '# /// script\n#\n# ///\n'; + assert.strictEqual(md.range.end, expectedBlock.length); + }); + + test('valid block with requires-python and dependencies', () => { + const text = script([ + '# /// script', + '# requires-python = ">=3.11"', + '# dependencies = [', + '# "requests<3",', + '# "rich",', + '# ]', + '# ///', + 'import requests', + ]); + const md = readInlineScriptMetadata(text); + assert.ok(md); + assert.strictEqual(md.requiresPython, '>=3.11'); + assert.deepStrictEqual([...(md.dependencies ?? [])], ['requests<3', 'rich']); + assert.strictEqual(md.tool, undefined); + }); + + test('valid block with [tool] table is opaque', () => { + const text = script([ + '# /// script', + '# dependencies = ["x"]', + '# [tool.mybuild]', + '# extra = "thing"', + '# ///', + ]); + const md = readInlineScriptMetadata(text); + assert.ok(md); + assert.ok(md.tool, 'tool table should be populated'); + assert.deepStrictEqual(md.tool, { mybuild: { extra: 'thing' } }); + }); + + test('multiple `script` blocks returns undefined and logs a warning', () => { + const text = script([ + '# /// script', + '# dependencies = ["a"]', + '# ///', + '', + '# /// script', + '# dependencies = ["b"]', + '# ///', + ]); + assert.strictEqual(readInlineScriptMetadata(text), undefined); + assert.ok(traceWarnStub.called, 'expected a traceWarn for multiple script blocks'); + }); + + test('unclosed block returns undefined', () => { + const text = script(['# /// script', '# dependencies = ["a"]', 'import x']); + assert.strictEqual(readInlineScriptMetadata(text), undefined); + }); + + test('content lines may be bare `#` (blank in metadata)', () => { + const text = script([ + '# /// script', + '# requires-python = ">=3.10"', + '#', + '# dependencies = ["a"]', + '# ///', + ]); + const md = readInlineScriptMetadata(text); + assert.ok(md); + assert.strictEqual(md.requiresPython, '>=3.10'); + assert.deepStrictEqual([...(md.dependencies ?? [])], ['a']); + }); + + test('CRLF line endings parse identically to LF', () => { + const text = script(['# /// script', '# dependencies = ["a"]', '# ///', ''], '\r\n'); + const md = readInlineScriptMetadata(text); + assert.ok(md); + assert.deepStrictEqual([...(md.dependencies ?? [])], ['a']); + }); + + test('lone-CR line endings parse identically to LF', () => { + const text = script(['# /// script', '# dependencies = ["a"]', '# ///', ''], '\r'); + const md = readInlineScriptMetadata(text); + assert.ok(md); + assert.deepStrictEqual([...(md.dependencies ?? [])], ['a']); + }); + + test('content with `##` line is rejected', () => { + const text = script(['# /// script', '## not a valid content line', '# ///']); + assert.strictEqual(readInlineScriptMetadata(text), undefined); + }); + + test('content with `#\\t` line is rejected', () => { + const text = script(['# /// script', '#\tnot a valid content line', '# ///']); + assert.strictEqual(readInlineScriptMetadata(text), undefined); + }); + + test('leading UTF-8 BOM is stripped before matching', () => { + const text = '\uFEFF' + script(['# /// script', '# dependencies = ["a"]', '# ///']); + const md = readInlineScriptMetadata(text); + assert.ok(md); + assert.deepStrictEqual([...(md.dependencies ?? [])], ['a']); + }); + + test('shebang before block does not block detection', () => { + const text = script(['#!/usr/bin/env python3', '# /// script', '# dependencies = ["a"]', '# ///']); + const md = readInlineScriptMetadata(text); + assert.ok(md); + assert.deepStrictEqual([...(md.dependencies ?? [])], ['a']); + }); + + test('encoding declaration before block does not block detection', () => { + const text = script(['# -*- coding: utf-8 -*-', '# /// script', '# dependencies = ["a"]', '# ///']); + const md = readInlineScriptMetadata(text); + assert.ok(md); + assert.deepStrictEqual([...(md.dependencies ?? [])], ['a']); + }); + + test('shebang AND encoding declaration before block', () => { + const text = script([ + '#!/usr/bin/env python3', + '# -*- coding: utf-8 -*-', + '# /// script', + '# dependencies = ["a"]', + '# ///', + ]); + const md = readInlineScriptMetadata(text); + assert.ok(md); + assert.deepStrictEqual([...(md.dependencies ?? [])], ['a']); + }); + + test('closing `# ///` with trailing whitespace is rejected', () => { + const text = script(['# /// script', '# dependencies = ["a"]', '# /// ']); + assert.strictEqual(readInlineScriptMetadata(text), undefined); + }); + + test('opening `# /// script ` with trailing whitespace is rejected', () => { + const text = script(['# /// script ', '# dependencies = ["a"]', '# ///']); + assert.strictEqual(readInlineScriptMetadata(text), undefined); + }); + + test('malformed TOML returns undefined and logs warn', () => { + const text = script(['# /// script', '# this is = not valid = toml', '# ///']); + assert.strictEqual(readInlineScriptMetadata(text), undefined); + assert.ok(traceWarnStub.called, 'expected a traceWarn for malformed TOML'); + }); + + test('dependencies is not a list returns undefined', () => { + const text = script(['# /// script', '# dependencies = "not a list"', '# ///']); + assert.strictEqual(readInlineScriptMetadata(text), undefined); + }); + + test('dependencies contains non-string returns undefined', () => { + // @iarna/toml will accept a mixed array; we validate downstream. + const text = script(['# /// script', '# dependencies = ["ok", 42]', '# ///']); + assert.strictEqual(readInlineScriptMetadata(text), undefined); + }); + + test('dependencies array with an empty string is passed through', () => { + const text = script(['# /// script', '# dependencies = [""]', '# ///']); + const md = readInlineScriptMetadata(text); + assert.ok(md); + assert.deepStrictEqual([...(md.dependencies ?? [])], ['']); + }); + + test('requires-python that is not a string returns undefined', () => { + const text = script(['# /// script', '# requires-python = 311', '# ///']); + assert.strictEqual(readInlineScriptMetadata(text), undefined); + }); + + test('non-`script` block TYPE is ignored', () => { + const text = script(['# /// pyproject', '# foo = "bar"', '# ///']); + assert.strictEqual(readInlineScriptMetadata(text), undefined); + }); + + test('non-`script` block followed by valid script block', () => { + const text = script([ + '# /// pyproject', + '# foo = "bar"', + '# ///', + '', + '# /// script', + '# dependencies = ["a"]', + '# ///', + ]); + const md = readInlineScriptMetadata(text); + assert.ok(md); + assert.deepStrictEqual([...(md.dependencies ?? [])], ['a']); + }); + + test('range refers to the normalized text', () => { + // Canonical regex requires at least one content line; use a + // bare `#` so the block is the minimal accepted form. + const text = script(['# /// script', '#', '# ///', 'rest']); + const md = readInlineScriptMetadata(text); + assert.ok(md); + assert.strictEqual(md.range.start, 0); + // start of `# /// script` line through and including the + // closing `# ///`'s terminating newline. + assert.strictEqual(text.slice(md.range.start, md.range.end), '# /// script\n#\n# ///\n'); + }); + + test('dependencies array is frozen (defensive copy)', () => { + const text = script(['# /// script', '# dependencies = ["a"]', '# ///']); + const md = readInlineScriptMetadata(text); + assert.ok(md?.dependencies); + assert.throws(() => { + (md.dependencies as string[]).push('b'); + }); + }); + + test('block beyond MAX_HEADER_BYTES boundary is invisible to file-reader (known limitation)', () => { + // The string parser sees the whole text, so it WILL find a + // block past the byte boundary. This test documents that + // the boundary is only enforced by the file reader. + const padding = 'a'.repeat(MAX_HEADER_BYTES + 100); + const text = padding + '\n' + script(['# /// script', '# dependencies = ["a"]', '# ///']); + const md = readInlineScriptMetadata(text); + assert.ok(md, 'parser ignores byte caps; only the file reader enforces them'); + assert.deepStrictEqual([...(md.dependencies ?? [])], ['a']); + }); + + // The plan calls out a "nested-looking line" test case (e.g. a + // `# ///` that appears inside what is morally a TOML multi-line + // string). The canonical regex is greedy with backtracking, so + // the LAST `# ///` line wins as the closing marker. We assert + // that the well-formed text round-trips. + test('content containing a `# ///` mid-block: last `# ///` wins as closer', () => { + // The "mid-block" `# ///` is a real `# ///` line inside the + // block. Per the regex's greedy backtracking, the parser + // treats it as content and the trailing `# ///` as the + // close. + const text = script(['# /// script', '# dependencies = ["a"]', '# # /// inside as comment', '# ///']); + const md = readInlineScriptMetadata(text); + assert.ok(md); + assert.deepStrictEqual([...(md.dependencies ?? [])], ['a']); + }); + + // ensure traceVerboseStub silences "no block" log noise — also + // a sanity check that the verbose log fires on the negative path. + test('no-block path logs only at verbose level', () => { + readInlineScriptMetadata('print("hi")'); + assert.strictEqual(traceWarnStub.called, false); + assert.ok(traceVerboseStub.called); + }); + }); + + suite('readInlineScriptMetadataFromFile', () => { + let tmpDir: string; + + setup(async () => { + tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'ism-test-')); + }); + + teardown(async () => { + await fs.remove(tmpDir); + }); + + test('returns metadata for a real on-disk .py file', async () => { + const filePath = path.join(tmpDir, 'script.py'); + await fs.writeFile( + filePath, + script(['# /// script', '# requires-python = ">=3.11"', '# dependencies = ["a"]', '# ///', 'print(1)']), + ); + const md = await readInlineScriptMetadataFromFile(Uri.file(filePath)); + assert.ok(md); + assert.strictEqual(md.requiresPython, '>=3.11'); + }); + + test('returns undefined for a file without a block', async () => { + const filePath = path.join(tmpDir, 'plain.py'); + await fs.writeFile(filePath, 'print("hi")\n'); + const md = await readInlineScriptMetadataFromFile(Uri.file(filePath)); + assert.strictEqual(md, undefined); + }); + + test('returns undefined for a non-file URI scheme', async () => { + const md = await readInlineScriptMetadataFromFile(Uri.parse('untitled:foo.py')); + assert.strictEqual(md, undefined); + }); + + test('returns undefined when the file does not exist', async () => { + const md = await readInlineScriptMetadataFromFile(Uri.file(path.join(tmpDir, 'does-not-exist.py'))); + assert.strictEqual(md, undefined); + }); + + test('block past MAX_HEADER_BYTES boundary is NOT found (cap is enforced)', async () => { + const filePath = path.join(tmpDir, 'big.py'); + // Pad with a comment that fills more than MAX_HEADER_BYTES, + // then put a valid block AFTER the cap. The reader should + // see only the padding and return undefined. + const padding = '# ' + 'a'.repeat(MAX_HEADER_BYTES); + const body = script([padding, '# /// script', '# dependencies = ["a"]', '# ///']); + await fs.writeFile(filePath, body); + const md = await readInlineScriptMetadataFromFile(Uri.file(filePath)); + assert.strictEqual(md, undefined); + }); + + test('block at top of a >MAX_HEADER_BYTES file is still found', async () => { + const filePath = path.join(tmpDir, 'big-top.py'); + const trailing = 'x'.repeat(MAX_HEADER_BYTES * 2); + const body = script(['# /// script', '# dependencies = ["a"]', '# ///', trailing]); + await fs.writeFile(filePath, body); + const md = await readInlineScriptMetadataFromFile(Uri.file(filePath)); + assert.ok(md); + assert.deepStrictEqual([...(md.dependencies ?? [])], ['a']); + }); + + test('reader returns a parsed block for a minimal on-disk file', async () => { + // The negative case above ("block past MAX_HEADER_BYTES + // boundary is NOT found") proves the byte cap is enforced. + // This test just sanity-checks the happy path on a minimal + // file: the reader returns the parsed block. + const filePath = path.join(tmpDir, 'spy.py'); + await fs.writeFile(filePath, script(['# /// script', '# dependencies = ["a"]', '# ///'])); + const md = await readInlineScriptMetadataFromFile(Uri.file(filePath)); + assert.ok(md); + assert.deepStrictEqual([...(md.dependencies ?? [])], ['a']); + }); + }); + + suite('matchesPythonVersion', () => { + test('>=3.11 vs 3.10/3.11/3.12', () => { + assert.strictEqual(matchesPythonVersion('>=3.11', '3.10'), false); + assert.strictEqual(matchesPythonVersion('>=3.11', '3.11'), true); + assert.strictEqual(matchesPythonVersion('>=3.11', '3.11.4'), true); + assert.strictEqual(matchesPythonVersion('>=3.11', '3.12'), true); + }); + + test('==3.12.* wildcard prefix match', () => { + assert.strictEqual(matchesPythonVersion('==3.12.*', '3.12'), true); + assert.strictEqual(matchesPythonVersion('==3.12.*', '3.12.4'), true); + assert.strictEqual(matchesPythonVersion('==3.12.*', '3.11.4'), false); + assert.strictEqual(matchesPythonVersion('==3.12.*', '3.13.0'), false); + }); + + test('!=3.12.* wildcard prefix anti-match', () => { + assert.strictEqual(matchesPythonVersion('!=3.12.*', '3.12.4'), false); + assert.strictEqual(matchesPythonVersion('!=3.12.*', '3.11.4'), true); + assert.strictEqual(matchesPythonVersion('!=3.12.*', '3.13.0'), true); + }); + + test('multi-clause >=3.10,<3.13 is AND-ed', () => { + assert.strictEqual(matchesPythonVersion('>=3.10,<3.13', '3.9.0'), false); + assert.strictEqual(matchesPythonVersion('>=3.10,<3.13', '3.10'), true); + assert.strictEqual(matchesPythonVersion('>=3.10,<3.13', '3.12.4'), true); + assert.strictEqual(matchesPythonVersion('>=3.10,<3.13', '3.13'), false); + }); + + test('~=3.11 (compatible release at minor level)', () => { + assert.strictEqual(matchesPythonVersion('~=3.11', '3.10.0'), false); + assert.strictEqual(matchesPythonVersion('~=3.11', '3.11.0'), true); + assert.strictEqual(matchesPythonVersion('~=3.11', '3.12.4'), true); + assert.strictEqual(matchesPythonVersion('~=3.11', '4.0.0'), false); + }); + + test('~=3.11.2 (compatible release at patch level)', () => { + assert.strictEqual(matchesPythonVersion('~=3.11.2', '3.11.1'), false); + assert.strictEqual(matchesPythonVersion('~=3.11.2', '3.11.2'), true); + assert.strictEqual(matchesPythonVersion('~=3.11.2', '3.11.10'), true); + assert.strictEqual(matchesPythonVersion('~=3.11.2', '3.12.0'), false); + }); + + test('== exact match (PEP 440 release-segment equality with zero padding)', () => { + // Per PEP 440 §"Version matching": when comparing release + // segments of different lengths, the shorter is padded + // with zeros. So `==3.11` matches both `3.11` and `3.11.0`. + // Users who want strict-shape equality use `===`. + assert.strictEqual(matchesPythonVersion('==3.11', '3.11'), true); + assert.strictEqual(matchesPythonVersion('==3.11', '3.11.0'), true); + assert.strictEqual(matchesPythonVersion('==3.11.0', '3.11.0'), true); + assert.strictEqual(matchesPythonVersion('==3.11', '3.12'), false); + assert.strictEqual(matchesPythonVersion('==3.11', '3.11.1'), false); + }); + + test('!= inequality', () => { + assert.strictEqual(matchesPythonVersion('!=3.11', '3.11'), false); + assert.strictEqual(matchesPythonVersion('!=3.11', '3.12'), true); + }); + + test('===X (arbitrary equality) is string match', () => { + assert.strictEqual(matchesPythonVersion('===3.11.0', '3.11.0'), true); + assert.strictEqual(matchesPythonVersion('===3.11.0', '3.11'), false); + }); + + test('input version with pre/dev suffix is truncated to release', () => { + assert.strictEqual(matchesPythonVersion('>=3.11', '3.11.0rc1'), true); + assert.strictEqual(matchesPythonVersion('>=3.11', '3.10.0rc1'), false); + }); + + test('invalid specifier returns false and logs warn', () => { + assert.strictEqual(matchesPythonVersion('weird-thing', '3.11'), false); + assert.ok(traceWarnStub.called); + }); + + test('empty specifier or version returns false', () => { + assert.strictEqual(matchesPythonVersion('', '3.11'), false); + assert.strictEqual(matchesPythonVersion('>=3.11', ''), false); + }); + + test('whitespace around clauses is tolerated', () => { + assert.strictEqual(matchesPythonVersion(' >= 3.11 , < 3.13 ', '3.12.4'), true); + }); + + test('wildcard with non-equality operator is invalid', () => { + assert.strictEqual(matchesPythonVersion('>=3.12.*', '3.12.4'), false); + assert.ok(traceWarnStub.called); + }); + }); + + // Type-only assertion: make sure InlineScriptMetadata is exported + // and structurally compatible with downstream use. + test('InlineScriptMetadata interface is exported and structurally correct', () => { + const sample: InlineScriptMetadata = { + requiresPython: '>=3.11', + dependencies: ['x'], + tool: { mykey: 'v' }, + range: { start: 0, end: 10 }, + }; + assert.strictEqual(sample.requiresPython, '>=3.11'); + }); +}); From 257fa6e067ebc969a89a96e59f7a842e498624c4 Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Tue, 12 May 2026 10:12:52 -0700 Subject: [PATCH 2/9] implement setting and project --- package.json | 14 +- package.nls.json | 3 +- src/common/inlineScriptMetadata.ts | 23 +- src/common/localize.ts | 10 + src/common/workspace.apis.ts | 17 ++ src/extension.ts | 12 + src/features/creators/inlineScriptDetector.ts | 190 ++++++++++++ src/features/inlineScriptLazyDetector.ts | 162 ++++++++++ src/internal.api.ts | 13 + src/managers/builtin/pipUtils.ts | 42 ++- .../inlineScriptDetector.unit.test.ts | 236 +++++++++++++++ .../inlineScriptLazyDetector.unit.test.ts | 280 ++++++++++++++++++ .../pipUtils.inlineScript.unit.test.ts | 146 +++++++++ 13 files changed, 1144 insertions(+), 4 deletions(-) create mode 100644 src/features/creators/inlineScriptDetector.ts create mode 100644 src/features/inlineScriptLazyDetector.ts create mode 100644 src/test/features/creators/inlineScriptDetector.unit.test.ts create mode 100644 src/test/features/inlineScriptLazyDetector.unit.test.ts create mode 100644 src/test/managers/builtin/pipUtils.inlineScript.unit.test.ts diff --git a/package.json b/package.json index df79269f..de0630dc 100644 --- a/package.json +++ b/package.json @@ -124,7 +124,10 @@ "python-envs.workspaceSearchPaths": { "type": "array", "description": "%python-envs.workspaceSearchPaths.description%", - "default": [".venv", "*/.venv"], + "default": [ + ".venv", + "*/.venv" + ], "scope": "resource", "items": { "type": "string" @@ -135,6 +138,15 @@ "description": "%python-envs.alwaysUseUv.description%", "default": true, "scope": "machine" + }, + "python-envs.useInlineScriptMetadata": { + "type": "boolean", + "description": "%python-envs.useInlineScriptMetadata.description%", + "default": false, + "scope": "window", + "tags": [ + "experimental" + ] } } }, diff --git a/package.nls.json b/package.nls.json index 3a4ddcec..8f576971 100644 --- a/package.nls.json +++ b/package.nls.json @@ -45,5 +45,6 @@ "python-envs.revealProjectInExplorer.title": "Reveal Project in Explorer", "python-envs.revealEnvInManagerView.title": "Reveal in Environment Managers View", "python-envs.runPetInTerminal.title": "Run Python Environment Tool (PET) in Terminal...", - "python-envs.alwaysUseUv.description": "When set to true, uv will be used to manage all virtual environments if available. When set to false, uv will only manage virtual environments explicitly created by uv." + "python-envs.alwaysUseUv.description": "When set to true, uv will be used to manage all virtual environments if available. When set to false, uv will only manage virtual environments explicitly created by uv.", + "python-envs.useInlineScriptMetadata.description": "Use inline script metadata (PEP 723) to manage environments for single-file Python scripts. When enabled, scripts with a `# /// script` block at the top are detected as projects, environments are created honoring `requires-python` and `dependencies`, and the extension watches for metadata changes. Disabled by default while the feature is in preview." } diff --git a/src/common/inlineScriptMetadata.ts b/src/common/inlineScriptMetadata.ts index 28c0f841..b3263c83 100644 --- a/src/common/inlineScriptMetadata.ts +++ b/src/common/inlineScriptMetadata.ts @@ -3,7 +3,7 @@ import * as tomljs from '@iarna/toml'; import * as fs from 'fs/promises'; -import { Uri } from 'vscode'; +import { ConfigurationScope, Uri, workspace } from 'vscode'; import { traceVerbose, traceWarn } from './logging'; /** @@ -396,3 +396,24 @@ function compareReleases(a: readonly number[], b: readonly number[]): number { } return 0; } + +/** + * Configuration section and key for the single user-facing setting that + * gates the inline-script-metadata feature. + */ +const SETTING_SECTION = 'python-envs'; +const SETTING_KEY = 'useInlineScriptMetadata'; + +/** + * Returns `true` when the inline-script-metadata feature is enabled + * for the given scope. The setting is window-scoped, so callers may + * pass a `Uri` to resolve a workspace-folder-aware view of the + * configuration, or `undefined` to read the window-level value. + * + * Every consumer of inline-script-metadata behavior — detection, env + * creation, watcher — MUST gate its work through this helper so that + * disabling the setting makes the feature invisible. + */ +export function isInlineScriptMetadataEnabled(scope?: ConfigurationScope): boolean { + return workspace.getConfiguration(SETTING_SECTION, scope).get(SETTING_KEY, false); +} diff --git a/src/common/localize.ts b/src/common/localize.ts index d3ce5c10..64f3b8a4 100644 --- a/src/common/localize.ts +++ b/src/common/localize.ts @@ -212,6 +212,16 @@ export namespace ProjectCreatorString { export const noProjectsFound = l10n.t('No projects found'); } +export namespace InlineScriptStrings { + export const detectorDisplayName = l10n.t('Find Inline-Metadata Scripts'); + export const detectorDescription = l10n.t( + 'Find single-file Python scripts that declare their dependencies inline (PEP 723).', + ); + export const selectScripts = l10n.t('Select inline-metadata scripts to add as projects'); + export const noScriptsFound = l10n.t('No Python scripts with inline metadata were found in this workspace.'); + export const installableDescription = l10n.t('Inline script dependency'); +} + export namespace EnvViewStrings { export const selectedGlobalTooltip = l10n.t('This environment is selected for non-workspace files'); export const selectedWorkspaceTooltip = l10n.t('This environment is selected for project files'); diff --git a/src/common/workspace.apis.ts b/src/common/workspace.apis.ts index c009cd6d..199dae30 100644 --- a/src/common/workspace.apis.ts +++ b/src/common/workspace.apis.ts @@ -8,6 +8,7 @@ import { FileRenameEvent, FileSystemWatcher, GlobPattern, + TextDocument, Uri, workspace, WorkspaceConfiguration, @@ -72,3 +73,19 @@ export function onDidRenameFiles( ): Disposable { return workspace.onDidRenameFiles(listener, thisArgs, disposables); } + +export function onDidOpenTextDocument( + listener: (e: TextDocument) => any, + thisArgs?: any, + disposables?: Disposable[], +): Disposable { + return workspace.onDidOpenTextDocument(listener, thisArgs, disposables); +} + +export function onDidSaveTextDocument( + listener: (e: TextDocument) => any, + thisArgs?: any, + disposables?: Disposable[], +): Disposable { + return workspace.onDidSaveTextDocument(listener, thisArgs, disposables); +} diff --git a/src/extension.ts b/src/extension.ts index 3a28d78f..d11f5b33 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -39,6 +39,7 @@ import { getConfiguration, getWorkspaceFolders } from './common/workspace.apis'; import { createManagerReady } from './features/common/managerReady'; import { AutoFindProjects } from './features/creators/autoFindProjects'; import { ExistingProjects } from './features/creators/existingProjects'; +import { InlineScriptDetector } from './features/creators/inlineScriptDetector'; import { NewPackageProject } from './features/creators/newPackageProject'; import { NewScriptProject } from './features/creators/newScriptProject'; import { ProjectCreatorsImpl } from './features/creators/projectCreators'; @@ -64,6 +65,7 @@ import { } from './features/envCommands'; import { PythonEnvironmentManagers } from './features/envManagers'; import { EnvVarManager, PythonEnvVariableManager } from './features/execution/envVariableManager'; +import { InlineScriptLazyDetector } from './features/inlineScriptLazyDetector'; import { applyInitialEnvironmentSelection, registerInterpreterSettingsChangeListener, @@ -203,8 +205,18 @@ export async function activate(context: ExtensionContext): Promise(); diff --git a/src/features/creators/inlineScriptDetector.ts b/src/features/creators/inlineScriptDetector.ts new file mode 100644 index 00000000..3a131aca --- /dev/null +++ b/src/features/creators/inlineScriptDetector.ts @@ -0,0 +1,190 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import * as path from 'path'; +import { Uri } from 'vscode'; +import { PythonProject, PythonProjectCreator, PythonProjectCreatorOptions } from '../../api'; +import { + InlineScriptMetadata, + isInlineScriptMetadataEnabled, + readInlineScriptMetadataFromFile, +} from '../../common/inlineScriptMetadata'; +import { InlineScriptStrings } from '../../common/localize'; +import { traceInfo, traceVerbose } from '../../common/logging'; +import { showErrorMessage, showQuickPickWithButtons } from '../../common/window.apis'; +import { findFiles } from '../../common/workspace.apis'; +import { PythonProjectManager, PythonProjectsImpl } from '../../internal.api'; + +/** + * Hard cap on how many `.py` files the opt-in workspace-scan creator + * will inspect. Per design principle §3.10 this is intentionally a + * code constant rather than a user-facing setting while the feature + * is experimental. + */ +export const MAX_SCRIPTS_TO_SCAN = 500; + +/** + * Glob pattern for the `.py` files we consider. The exclusion glob + * mirrors the existing pip-installable scan in `pipUtils.ts` plus a + * handful of additional caches that tend to contain large numbers of + * `.py` files we never want to inspect (mypy/pytest caches and the + * special `__pypackages__` PEP 582 directory). + */ +const SCRIPT_INCLUDE = '**/*.py'; +const SCRIPT_EXCLUDE = + '**/{.venv*,.git,.nox,.tox,.conda,site-packages,__pypackages__,node_modules,.mypy_cache,.pytest_cache}/**'; + +/** Maximum number of header reads to run in parallel. */ +const SCAN_CONCURRENCY = 8; + +/** + * Opt-in `PythonProjectCreator` that scans the open workspace folders + * for `.py` files which declare inline script metadata (PEP 723) and + * offers any newly-found scripts as Python projects. + * + * This is the SECONDARY discovery path. The primary path is the lazy + * detector in `src/features/inlineScriptLazyDetector.ts`, which picks + * up scripts as the user opens or saves them. This creator is for + * users who want bulk discovery on demand (for example, when they + * first turn the experimental setting on in a workspace that already + * contains many scripts). + */ +export class InlineScriptDetector implements PythonProjectCreator { + public readonly name = 'inlineScriptDetector'; + public readonly displayName = InlineScriptStrings.detectorDisplayName; + public readonly description = InlineScriptStrings.detectorDescription; + + constructor(private readonly pm: PythonProjectManager) {} + + public async create(_options?: PythonProjectCreatorOptions): Promise { + if (!isInlineScriptMetadataEnabled()) { + // The feature is gated behind the experimental setting. + // If the creator is somehow invoked while disabled (e.g. + // toggled mid-session), fail gracefully rather than + // performing the scan. + setImmediate(() => { + showErrorMessage(InlineScriptStrings.noScriptsFound); + }); + return undefined; + } + + const candidates = await findFiles(SCRIPT_INCLUDE, SCRIPT_EXCLUDE, MAX_SCRIPTS_TO_SCAN); + if (!candidates || candidates.length === 0) { + setImmediate(() => { + showErrorMessage(InlineScriptStrings.noScriptsFound); + }); + return undefined; + } + + // Filter out scripts that are already registered as a project + // with the exact same URI. Folder-scoped projects that happen + // to contain this script are intentionally NOT filtered out: + // a `.py` file with inline metadata is its own project and + // sits alongside its enclosing folder project. + const fresh = candidates.filter((uri) => { + const existing = this.pm.get(uri); + if (!existing) { + return true; + } + return path.normalize(existing.uri.fsPath) !== path.normalize(uri.fsPath); + }); + if (fresh.length === 0) { + traceInfo(`InlineScriptDetector: all ${candidates.length} candidate .py files are already registered.`); + setImmediate(() => { + showErrorMessage(InlineScriptStrings.noScriptsFound); + }); + return undefined; + } + + // Read headers in bounded parallel batches. Each read is at + // most `MAX_HEADER_BYTES` (8 KiB) so the total I/O at the cap + // (500 files × 8 KiB ≈ 4 MiB) is small. + const withMetadata = await scanForInlineScripts(fresh); + if (withMetadata.length === 0) { + traceInfo(`InlineScriptDetector: scanned ${fresh.length} .py files, none declared inline metadata.`); + setImmediate(() => { + showErrorMessage(InlineScriptStrings.noScriptsFound); + }); + return undefined; + } + + const items = withMetadata + .map(({ uri }) => ({ + label: path.basename(uri.fsPath), + description: uri.fsPath, + uri, + })) + .sort((a, b) => a.label.localeCompare(b.label)); + + const selected = await showQuickPickWithButtons(items, { + canPickMany: true, + ignoreFocusOut: true, + placeHolder: InlineScriptStrings.selectScripts, + showBackButton: true, + }); + + let chosen: typeof items; + if (Array.isArray(selected)) { + chosen = selected as typeof items; + } else if (selected) { + chosen = [selected as (typeof items)[number]]; + } else { + // User cancelled the picker. + return undefined; + } + if (chosen.length === 0) { + return undefined; + } + + // Re-associate each chosen URI back to its metadata so we can + // cache it on the project. We do this rather than re-reading + // the file so a save between scan and pick can't change what + // the user just confirmed. + const metadataByUri = new Map(withMetadata.map((r) => [r.uri.toString(), r.metadata])); + const projects: PythonProject[] = chosen.map((c) => { + const proj = new PythonProjectsImpl(path.basename(c.uri.fsPath), c.uri); + proj.inlineScriptMetadata = metadataByUri.get(c.uri.toString()); + return proj; + }); + await this.pm.add(projects); + return projects; + } +} + +/** + * Read inline script metadata from each URI in `uris` with bounded + * parallelism, returning only the URIs whose `.py` file declared a + * well-formed `# /// script` block. Exported for unit-test use. + */ +export async function scanForInlineScripts( + uris: readonly Uri[], +): Promise> { + const results: Array<{ uri: Uri; metadata: InlineScriptMetadata }> = []; + let cursor = 0; + + const worker = async (): Promise => { + for (;;) { + const index = cursor; + cursor += 1; + if (index >= uris.length) { + return; + } + const uri = uris[index]; + try { + const metadata = await readInlineScriptMetadataFromFile(uri); + if (metadata !== undefined) { + results.push({ uri, metadata }); + } + } catch (err) { + traceVerbose(`InlineScriptDetector: failed to read ${uri.fsPath}:`, err); + } + } + }; + + const workers: Promise[] = []; + for (let i = 0; i < Math.min(SCAN_CONCURRENCY, uris.length); i++) { + workers.push(worker()); + } + await Promise.all(workers); + return results; +} diff --git a/src/features/inlineScriptLazyDetector.ts b/src/features/inlineScriptLazyDetector.ts new file mode 100644 index 00000000..8d1823f4 --- /dev/null +++ b/src/features/inlineScriptLazyDetector.ts @@ -0,0 +1,162 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import * as path from 'path'; +import { Disposable, TextDocument, Uri } from 'vscode'; +import { + InlineScriptMetadata, + isInlineScriptMetadataEnabled, + readInlineScriptMetadataFromFile, +} from '../common/inlineScriptMetadata'; +import { traceVerbose, traceWarn } from '../common/logging'; +import { getWorkspaceFolder, onDidOpenTextDocument, onDidSaveTextDocument } from '../common/workspace.apis'; +import { PythonProjectManager, PythonProjectsImpl } from '../internal.api'; + +/** + * Lazy on-open / on-save detector for `.py` files that declare inline + * script metadata (PEP 723). This is the PRIMARY detection path: when + * the user opens or saves a `.py` script, we read the head of the + * file, parse any `# /// script` block, and register the script as a + * Python project so its environment and dependencies surface in the + * rest of the extension. + * + * Detection is cheap (≤ 8 KiB read + regex + TOML parse) and runs + * only on files the user has already shown intent in. Workspace-wide + * scanning is the responsibility of the opt-in creator in + * `src/features/creators/inlineScriptDetector.ts`. + */ +export class InlineScriptLazyDetector implements Disposable { + private readonly subscriptions: Disposable[] = []; + // In-flight reads keyed by `uri.toString()` so rapid open+save + // doesn't double-process the same file. + private readonly inFlight = new Map>(); + + constructor(private readonly projectManager: PythonProjectManager) {} + + /** + * Subscribe to workspace text-document events. Safe to call once + * during extension activation. The detector starts working + * immediately; the experimental gate is re-checked on every event + * so toggling the setting takes effect without a reload. + * + * Listeners return the promise from `handleDocument` rather than + * void-ing it. VS Code's event bus does not await listener + * promises (so production behaviour is unchanged — still + * fire-and-forget), but returning the promise lets tests await + * the work triggered by a synthetic open/save event. + */ + public activate(): void { + this.subscriptions.push( + onDidOpenTextDocument((doc) => this.handleDocument(doc, 'open')), + onDidSaveTextDocument((doc) => this.handleDocument(doc, 'save')), + ); + } + + public dispose(): void { + this.subscriptions.forEach((s) => s.dispose()); + this.subscriptions.length = 0; + this.inFlight.clear(); + } + + private async handleDocument(doc: TextDocument, trigger: 'open' | 'save'): Promise { + const uri = doc.uri; + if (!shouldHandleUri(uri)) { + return; + } + if (!isInlineScriptMetadataEnabled(uri)) { + return; + } + const key = uri.toString(); + const existing = this.inFlight.get(key); + if (existing) { + // A previous open/save is still in flight for the same + // URI. Wait for it and skip; that read's result is + // authoritative. + await existing; + return; + } + const work = this.processOnce(uri, trigger).finally(() => { + this.inFlight.delete(key); + }); + this.inFlight.set(key, work); + await work; + } + + private async processOnce(uri: Uri, trigger: 'open' | 'save'): Promise { + let metadata: InlineScriptMetadata | undefined; + try { + metadata = await readInlineScriptMetadataFromFile(uri); + } catch (err) { + // `readInlineScriptMetadataFromFile` already swallows I/O + // errors internally. This catch is a defensive net for + // unexpected synchronous throws (e.g. malformed URI). + traceWarn(`inlineScriptLazyDetector: unexpected error while reading ${uri.fsPath}:`, err); + return; + } + + const existing = this.projectManager.get(uri); + + if (metadata === undefined) { + // No (valid) block in the file. If it was previously + // registered as a script project we keep it — the user + // explicitly added it once, and yanking the project on a + // passing edit would be surprising. We only clear the + // cached metadata so downstream consumers don't act on + // stale data. + if (existing instanceof PythonProjectsImpl && existing.inlineScriptMetadata !== undefined) { + existing.inlineScriptMetadata = undefined; + traceVerbose( + `inlineScriptLazyDetector: cleared cached metadata for ${uri.fsPath} (${trigger}: no block)`, + ); + } + return; + } + + if (existing instanceof PythonProjectsImpl) { + // Already a project — just refresh the cached metadata + // (it may have changed on save; downstream code, e.g. + // `getProjectInstallable`, is also free to re-read). + existing.inlineScriptMetadata = metadata; + traceVerbose( + `inlineScriptLazyDetector: refreshed metadata for ${uri.fsPath} (${trigger}: already a project)`, + ); + return; + } + + if (existing !== undefined) { + // The URI is somehow already registered with a different + // `PythonProject` implementation. Don't replace it. + traceVerbose(`inlineScriptLazyDetector: ${uri.fsPath} is already a project (non-impl); skipping.`); + return; + } + + const project = new PythonProjectsImpl(path.basename(uri.fsPath), uri); + project.inlineScriptMetadata = metadata; + try { + await this.projectManager.add(project); + traceVerbose(`inlineScriptLazyDetector: registered ${uri.fsPath} as a project (${trigger})`); + } catch (err) { + traceWarn(`inlineScriptLazyDetector: failed to register ${uri.fsPath} as a project:`, err); + } + } +} + +/** + * Cheap, side-effect-free gate for which URIs the lazy detector + * should look at. Filters out non-file schemes, non-`.py` + * extensions, and files that are not inside an open workspace + * folder. Exported for test access and for re-use by the opt-in + * workspace-scan creator. + */ +export function shouldHandleUri(uri: Uri): boolean { + if (uri.scheme !== 'file') { + return false; + } + if (path.extname(uri.fsPath).toLowerCase() !== '.py') { + return false; + } + if (getWorkspaceFolder(uri) === undefined) { + return false; + } + return true; +} diff --git a/src/internal.api.ts b/src/internal.api.ts index 3d896c0c..a72a305d 100644 --- a/src/internal.api.ts +++ b/src/internal.api.ts @@ -29,6 +29,7 @@ import { } from './api'; import { ISSUES_URL } from './common/constants'; import { CreateEnvironmentNotSupported, RemoveEnvironmentNotSupported } from './common/errors/NotSupportedError'; +import { InlineScriptMetadata } from './common/inlineScriptMetadata'; import { traceWarn } from './common/logging'; import { StopWatch } from './common/stopWatch'; import { EventNames } from './common/telemetry/constants'; @@ -466,6 +467,18 @@ export class PythonProjectsImpl implements PythonProject { tooltip?: string | MarkdownString; iconPath?: IconPath; + /** + * Parsed inline script metadata (PEP 723) for `.py` script + * projects, cached on the project after the lazy detector has + * read it. `undefined` for folder projects and for `.py` files + * that do not declare a `# /// script` block. + * + * Internal-only: not exposed on the public `PythonProject` + * interface. Consumers access it via an explicit `instanceof + * PythonProjectsImpl` guard so the cast stays in one place. + */ + inlineScriptMetadata?: InlineScriptMetadata; + constructor( name: string, uri: Uri, diff --git a/src/managers/builtin/pipUtils.ts b/src/managers/builtin/pipUtils.ts index 2e7a7f85..a67d21cd 100644 --- a/src/managers/builtin/pipUtils.ts +++ b/src/managers/builtin/pipUtils.ts @@ -4,7 +4,8 @@ import * as path from 'path'; import { l10n, LogOutputChannel, ProgressLocation, QuickInputButtons, QuickPickItem, Uri, window } from 'vscode'; import { PackageManagementOptions, PythonEnvironment, PythonEnvironmentApi, PythonProject } from '../../api'; import { EXTENSION_ROOT_DIR } from '../../common/constants'; -import { PackageManagement, Pickers, VenvManagerStrings } from '../../common/localize'; +import { isInlineScriptMetadataEnabled, readInlineScriptMetadataFromFile } from '../../common/inlineScriptMetadata'; +import { InlineScriptStrings, PackageManagement, Pickers, VenvManagerStrings } from '../../common/localize'; import { traceInfo } from '../../common/logging'; import { showQuickPickWithButtons, withProgress } from '../../common/window.apis'; import { findFiles } from '../../common/workspace.apis'; @@ -348,6 +349,45 @@ export async function getProjectInstallable( } }), ); + + // Inline-script-metadata (PEP 723) branch: any project whose + // `uri` is a `.py` file may carry inline dependency + // declarations at the top of the file. We surface those + // declared deps as `Installable` entries grouped under + // 'Inline metadata' so they appear in the same pre-install + // picker as `pyproject.toml` and `requirements*.txt` + // dependencies. This branch is gated by the experimental + // setting and only runs for projects whose root URI is + // itself a `.py` file — folder projects are never walked + // for `.py` files (that would be an unbounded scan). + await Promise.all( + projects.map(async (project) => { + if (project.uri.scheme !== 'file') { + return; + } + if (path.extname(project.uri.fsPath).toLowerCase() !== '.py') { + return; + } + if (!isInlineScriptMetadataEnabled(project.uri)) { + return; + } + const metadata = await readInlineScriptMetadataFromFile(project.uri); + const deps = metadata?.dependencies; + if (!deps || deps.length === 0) { + return; + } + for (const dep of deps) { + installable.push({ + name: dep, + displayName: dep, + group: 'Inline metadata', + args: [dep], + description: InlineScriptStrings.installableDescription, + uri: project.uri, + }); + } + }), + ); }, ); diff --git a/src/test/features/creators/inlineScriptDetector.unit.test.ts b/src/test/features/creators/inlineScriptDetector.unit.test.ts new file mode 100644 index 00000000..67d15e51 --- /dev/null +++ b/src/test/features/creators/inlineScriptDetector.unit.test.ts @@ -0,0 +1,236 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import assert from 'assert'; +import * as path from 'path'; +import * as sinon from 'sinon'; +import * as typmoq from 'typemoq'; +import { Uri } from 'vscode'; +import { PythonProject } from '../../../api'; +import * as ism from '../../../common/inlineScriptMetadata'; +import * as winapi from '../../../common/window.apis'; +import * as wapi from '../../../common/workspace.apis'; +import { InlineScriptDetector, scanForInlineScripts } from '../../../features/creators/inlineScriptDetector'; +import { PythonProjectManager, PythonProjectsImpl } from '../../../internal.api'; + +const META_A: ism.InlineScriptMetadata = { + requiresPython: '>=3.11', + dependencies: ['requests'], + tool: undefined, + range: { start: 0, end: 40 }, +}; +const META_B: ism.InlineScriptMetadata = { + requiresPython: '>=3.10', + dependencies: ['rich'], + tool: undefined, + range: { start: 0, end: 40 }, +}; + +suite('InlineScriptDetector (creator)', () => { + let findFilesStub: sinon.SinonStub; + let showErrorStub: sinon.SinonStub; + let showQuickPickStub: sinon.SinonStub; + let readMetadataStub: sinon.SinonStub; + let isEnabledStub: sinon.SinonStub; + let pm: typmoq.IMock; + + setup(() => { + findFilesStub = sinon.stub(wapi, 'findFiles'); + findFilesStub.resolves([]); + + showErrorStub = sinon.stub(winapi, 'showErrorMessage'); + showErrorStub.resolves(undefined); + + showQuickPickStub = sinon.stub(winapi, 'showQuickPickWithButtons'); + showQuickPickStub.resolves(undefined); + + readMetadataStub = sinon.stub(ism, 'readInlineScriptMetadataFromFile'); + readMetadataStub.resolves(undefined); + + isEnabledStub = sinon.stub(ism, 'isInlineScriptMetadataEnabled'); + isEnabledStub.returns(true); + + pm = typmoq.Mock.ofType(); + pm.setup((p) => p.get(typmoq.It.isAny())).returns(() => undefined); + pm.setup((p) => p.add(typmoq.It.isAny())).returns(() => Promise.resolve()); + }); + + teardown(() => { + sinon.restore(); + }); + + // Helper to wait for the `setImmediate`-scheduled error toast. + function waitForImmediate(): Promise { + return new Promise((resolve) => setImmediate(resolve)); + } + + test('returns undefined and shows error when feature is disabled', async () => { + isEnabledStub.returns(false); + const detector = new InlineScriptDetector(pm.object); + const result = await detector.create(); + await waitForImmediate(); + assert.strictEqual(result, undefined); + assert.ok(findFilesStub.notCalled, 'should not scan when feature is disabled'); + assert.ok(showErrorStub.calledOnce, 'should show "no scripts found" error'); + }); + + test('returns undefined when findFiles returns no candidates', async () => { + findFilesStub.resolves([]); + const detector = new InlineScriptDetector(pm.object); + const result = await detector.create(); + await waitForImmediate(); + assert.strictEqual(result, undefined); + assert.ok(showErrorStub.calledOnce); + }); + + test('returns undefined when every candidate is already registered with the same URI', async () => { + const uri = Uri.file(path.resolve('/ws/a.py')); + findFilesStub.resolves([uri]); + pm.reset(); + pm.setup((p) => p.get(typmoq.It.isAny())).returns(() => ({ name: 'a.py', uri })); + pm.setup((p) => p.add(typmoq.It.isAny())).returns(() => Promise.resolve()); + + const detector = new InlineScriptDetector(pm.object); + const result = await detector.create(); + await waitForImmediate(); + assert.strictEqual(result, undefined); + assert.ok(readMetadataStub.notCalled, 'should not read metadata when nothing is fresh'); + assert.ok(showErrorStub.calledOnce); + }); + + test('keeps candidate when only a folder project (different URI) contains it', async () => { + const scriptUri = Uri.file(path.resolve('/ws/a.py')); + const folderUri = Uri.file(path.resolve('/ws')); + findFilesStub.resolves([scriptUri]); + pm.reset(); + pm.setup((p) => p.get(typmoq.It.isAny())).returns(() => ({ name: 'ws', uri: folderUri })); + pm.setup((p) => p.add(typmoq.It.isAny())).returns(() => Promise.resolve()); + readMetadataStub.resolves(META_A); + // User cancels picker → undefined, but the scan should have occurred. + showQuickPickStub.resolves(undefined); + + const detector = new InlineScriptDetector(pm.object); + const result = await detector.create(); + assert.strictEqual(result, undefined); + assert.ok(readMetadataStub.calledOnce, 'should still scan the candidate'); + assert.ok(showQuickPickStub.calledOnce, 'should present picker'); + }); + + test('returns undefined and shows error when no candidate has metadata', async () => { + const uri = Uri.file(path.resolve('/ws/plain.py')); + findFilesStub.resolves([uri]); + readMetadataStub.resolves(undefined); + + const detector = new InlineScriptDetector(pm.object); + const result = await detector.create(); + await waitForImmediate(); + assert.strictEqual(result, undefined); + assert.ok(showQuickPickStub.notCalled, 'should not show picker when no metadata found'); + assert.ok(showErrorStub.calledOnce); + }); + + test('returns undefined when the user cancels the picker', async () => { + const uri = Uri.file(path.resolve('/ws/a.py')); + findFilesStub.resolves([uri]); + readMetadataStub.resolves(META_A); + showQuickPickStub.resolves(undefined); + + const detector = new InlineScriptDetector(pm.object); + const result = await detector.create(); + assert.strictEqual(result, undefined); + pm.verify((p) => p.add(typmoq.It.isAny()), typmoq.Times.never()); + }); + + test('registers chosen projects with cached metadata', async () => { + const uriA = Uri.file(path.resolve('/ws/a.py')); + const uriB = Uri.file(path.resolve('/ws/b.py')); + findFilesStub.resolves([uriA, uriB]); + readMetadataStub.withArgs(sinon.match((u: Uri) => u.toString() === uriA.toString())).resolves(META_A); + readMetadataStub.withArgs(sinon.match((u: Uri) => u.toString() === uriB.toString())).resolves(META_B); + + // Simulate user picking BOTH. + showQuickPickStub.callsFake(async (items: Array<{ label: string; uri: Uri }>) => items); + + let captured: PythonProject | PythonProject[] | undefined; + pm.reset(); + pm.setup((p) => p.get(typmoq.It.isAny())).returns(() => undefined); + pm.setup((p) => p.add(typmoq.It.isAny())) + .callback((arg: PythonProject | PythonProject[]) => { + captured = arg; + }) + .returns(() => Promise.resolve()); + + const detector = new InlineScriptDetector(pm.object); + const result = await detector.create(); + + assert.ok(result && result.length === 2, 'should return both projects'); + const sorted = [...result!].sort((a, b) => a.uri.fsPath.localeCompare(b.uri.fsPath)); + const projA = sorted[0] as PythonProjectsImpl; + const projB = sorted[1] as PythonProjectsImpl; + assert.ok(projA instanceof PythonProjectsImpl); + assert.ok(projB instanceof PythonProjectsImpl); + assert.deepStrictEqual(projA.inlineScriptMetadata, META_A); + assert.deepStrictEqual(projB.inlineScriptMetadata, META_B); + pm.verify((p) => p.add(typmoq.It.isAny()), typmoq.Times.once()); + assert.ok(Array.isArray(captured), 'pm.add should receive an array of projects'); + assert.strictEqual((captured as PythonProject[]).length, 2); + }); + + test('handles single-item picker return (non-array) correctly', async () => { + const uri = Uri.file(path.resolve('/ws/only.py')); + findFilesStub.resolves([uri]); + readMetadataStub.resolves(META_A); + // Some VS Code wrappers return a single item rather than an + // array even with `canPickMany: true`. Be tolerant. + showQuickPickStub.callsFake(async (items: Array<{ label: string; uri: Uri }>) => items[0]); + + const detector = new InlineScriptDetector(pm.object); + const result = await detector.create(); + assert.ok(result && result.length === 1); + pm.verify((p) => p.add(typmoq.It.isAny()), typmoq.Times.once()); + }); +}); + +suite('scanForInlineScripts', () => { + let readMetadataStub: sinon.SinonStub; + + setup(() => { + readMetadataStub = sinon.stub(ism, 'readInlineScriptMetadataFromFile'); + }); + + teardown(() => { + sinon.restore(); + }); + + test('returns only URIs whose metadata is defined', async () => { + const uriA = Uri.file(path.resolve('/ws/a.py')); + const uriB = Uri.file(path.resolve('/ws/b.py')); + const uriC = Uri.file(path.resolve('/ws/c.py')); + readMetadataStub.withArgs(sinon.match((u: Uri) => u.toString() === uriA.toString())).resolves(META_A); + readMetadataStub.withArgs(sinon.match((u: Uri) => u.toString() === uriB.toString())).resolves(undefined); + readMetadataStub.withArgs(sinon.match((u: Uri) => u.toString() === uriC.toString())).resolves(META_B); + + const results = await scanForInlineScripts([uriA, uriB, uriC]); + const got = new Map(results.map((r) => [r.uri.toString(), r.metadata])); + assert.strictEqual(got.size, 2); + assert.strictEqual(got.get(uriA.toString()), META_A); + assert.strictEqual(got.get(uriC.toString()), META_B); + }); + + test('swallows per-file read errors and continues', async () => { + const uriA = Uri.file(path.resolve('/ws/a.py')); + const uriB = Uri.file(path.resolve('/ws/b.py')); + readMetadataStub.withArgs(sinon.match((u: Uri) => u.toString() === uriA.toString())).rejects(new Error('boom')); + readMetadataStub.withArgs(sinon.match((u: Uri) => u.toString() === uriB.toString())).resolves(META_A); + + const results = await scanForInlineScripts([uriA, uriB]); + assert.strictEqual(results.length, 1); + assert.strictEqual(results[0].uri.toString(), uriB.toString()); + }); + + test('handles an empty input list', async () => { + const results = await scanForInlineScripts([]); + assert.deepStrictEqual(results, []); + assert.ok(readMetadataStub.notCalled); + }); +}); diff --git a/src/test/features/inlineScriptLazyDetector.unit.test.ts b/src/test/features/inlineScriptLazyDetector.unit.test.ts new file mode 100644 index 00000000..69579872 --- /dev/null +++ b/src/test/features/inlineScriptLazyDetector.unit.test.ts @@ -0,0 +1,280 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import assert from 'assert'; +import * as path from 'path'; +import * as sinon from 'sinon'; +import * as typmoq from 'typemoq'; +import { Disposable, TextDocument, Uri } from 'vscode'; +import { PythonProject } from '../../api'; +import * as ism from '../../common/inlineScriptMetadata'; +import * as wapi from '../../common/workspace.apis'; +import { InlineScriptLazyDetector, shouldHandleUri } from '../../features/inlineScriptLazyDetector'; +import { PythonProjectManager, PythonProjectsImpl } from '../../internal.api'; + +// Build a minimal TextDocument stub. Only the `uri` field is read by +// the detector; the rest exists to satisfy the type. +function makeDoc(uri: Uri): TextDocument { + return { uri } as TextDocument; +} + +const VALID_METADATA: ism.InlineScriptMetadata = { + requiresPython: '>=3.11', + dependencies: ['requests'], + tool: undefined, + range: { start: 0, end: 40 }, +}; + +suite('InlineScriptLazyDetector', () => { + let onDidOpenStub: sinon.SinonStub; + let onDidSaveStub: sinon.SinonStub; + let getWorkspaceFolderStub: sinon.SinonStub; + let readMetadataStub: sinon.SinonStub; + let isEnabledStub: sinon.SinonStub; + let openListener: ((doc: TextDocument) => unknown) | undefined; + let saveListener: ((doc: TextDocument) => unknown) | undefined; + let projectManager: typmoq.IMock; + + setup(() => { + openListener = undefined; + saveListener = undefined; + + onDidOpenStub = sinon.stub(wapi, 'onDidOpenTextDocument'); + onDidOpenStub.callsFake((listener: (doc: TextDocument) => unknown) => { + openListener = listener; + return new Disposable(() => { + openListener = undefined; + }); + }); + + onDidSaveStub = sinon.stub(wapi, 'onDidSaveTextDocument'); + onDidSaveStub.callsFake((listener: (doc: TextDocument) => unknown) => { + saveListener = listener; + return new Disposable(() => { + saveListener = undefined; + }); + }); + + getWorkspaceFolderStub = sinon.stub(wapi, 'getWorkspaceFolder'); + // By default, every URI is treated as being inside a workspace + // folder. Tests that want to exercise the "not in workspace" + // branch override this. + getWorkspaceFolderStub.callsFake((uri: Uri) => ({ + uri: Uri.file(path.dirname(uri.fsPath)), + name: 'mockWorkspace', + index: 0, + })); + + readMetadataStub = sinon.stub(ism, 'readInlineScriptMetadataFromFile'); + readMetadataStub.resolves(undefined); + + isEnabledStub = sinon.stub(ism, 'isInlineScriptMetadataEnabled'); + isEnabledStub.returns(true); + + projectManager = typmoq.Mock.ofType(); + projectManager.setup((pm) => pm.add(typmoq.It.isAny())).returns(() => Promise.resolve()); + }); + + teardown(() => { + sinon.restore(); + }); + + async function fireOpen(uri: Uri): Promise { + assert.ok(openListener, 'open listener should be registered after activate()'); + await openListener!(makeDoc(uri)); + } + + async function fireSave(uri: Uri): Promise { + assert.ok(saveListener, 'save listener should be registered after activate()'); + await saveListener!(makeDoc(uri)); + } + + test('activate() subscribes to onDidOpen and onDidSave', () => { + const detector = new InlineScriptLazyDetector(projectManager.object); + detector.activate(); + assert.ok(onDidOpenStub.calledOnce, 'should subscribe to onDidOpenTextDocument'); + assert.ok(onDidSaveStub.calledOnce, 'should subscribe to onDidSaveTextDocument'); + detector.dispose(); + }); + + test('skips non-file URI schemes', async () => { + const detector = new InlineScriptLazyDetector(projectManager.object); + detector.activate(); + await fireOpen(Uri.parse('untitled:foo.py')); + assert.ok(readMetadataStub.notCalled, 'should not read metadata for non-file URI'); + detector.dispose(); + }); + + test('skips non-.py files', async () => { + const detector = new InlineScriptLazyDetector(projectManager.object); + detector.activate(); + await fireOpen(Uri.file(path.resolve('/ws/foo.txt'))); + assert.ok(readMetadataStub.notCalled, 'should not read metadata for non-.py files'); + detector.dispose(); + }); + + test('skips files outside any workspace folder', async () => { + getWorkspaceFolderStub.returns(undefined); + const detector = new InlineScriptLazyDetector(projectManager.object); + detector.activate(); + await fireOpen(Uri.file(path.resolve('/elsewhere/foo.py'))); + assert.ok(readMetadataStub.notCalled, 'should not read metadata for out-of-workspace files'); + detector.dispose(); + }); + + test('no-op when feature is disabled', async () => { + isEnabledStub.returns(false); + const detector = new InlineScriptLazyDetector(projectManager.object); + detector.activate(); + await fireOpen(Uri.file(path.resolve('/ws/foo.py'))); + assert.ok(readMetadataStub.notCalled, 'should not read metadata when setting is disabled'); + detector.dispose(); + }); + + test('registers a new project when a .py file with metadata is opened', async () => { + const uri = Uri.file(path.resolve('/ws/foo.py')); + readMetadataStub.resolves(VALID_METADATA); + projectManager.reset(); + projectManager.setup((pm) => pm.get(typmoq.It.isAny())).returns(() => undefined); + let captured: PythonProject | PythonProject[] | undefined; + projectManager + .setup((pm) => pm.add(typmoq.It.isAny())) + .callback((arg: PythonProject | PythonProject[]) => { + captured = arg; + }) + .returns(() => Promise.resolve()); + + const detector = new InlineScriptLazyDetector(projectManager.object); + detector.activate(); + await fireOpen(uri); + + projectManager.verify((pm) => pm.add(typmoq.It.isAny()), typmoq.Times.once()); + assert.ok(captured, 'pm.add should have been called with a project'); + assert.ok(!Array.isArray(captured), 'expected a single project, not an array'); + const project = captured as PythonProjectsImpl; + assert.ok(project instanceof PythonProjectsImpl, 'project should be a PythonProjectsImpl'); + assert.strictEqual(project.uri.toString(), uri.toString()); + assert.deepStrictEqual(project.inlineScriptMetadata, VALID_METADATA); + detector.dispose(); + }); + + test('save event also registers a new project', async () => { + const uri = Uri.file(path.resolve('/ws/bar.py')); + readMetadataStub.resolves(VALID_METADATA); + projectManager.setup((pm) => pm.get(typmoq.It.isAny())).returns(() => undefined); + + const detector = new InlineScriptLazyDetector(projectManager.object); + detector.activate(); + await fireSave(uri); + + projectManager.verify((pm) => pm.add(typmoq.It.isAny()), typmoq.Times.once()); + detector.dispose(); + }); + + test('does not register a project when there is no metadata', async () => { + readMetadataStub.resolves(undefined); + projectManager.setup((pm) => pm.get(typmoq.It.isAny())).returns(() => undefined); + + const detector = new InlineScriptLazyDetector(projectManager.object); + detector.activate(); + await fireOpen(Uri.file(path.resolve('/ws/plain.py'))); + + projectManager.verify((pm) => pm.add(typmoq.It.isAny()), typmoq.Times.never()); + detector.dispose(); + }); + + test('refreshes metadata on save when the project is already registered', async () => { + const uri = Uri.file(path.resolve('/ws/already.py')); + const existing = new PythonProjectsImpl('already.py', uri); + existing.inlineScriptMetadata = { + requiresPython: '>=3.10', + dependencies: ['old'], + tool: undefined, + range: { start: 0, end: 20 }, + }; + readMetadataStub.resolves(VALID_METADATA); + projectManager.setup((pm) => pm.get(typmoq.It.isAny())).returns(() => existing); + + const detector = new InlineScriptLazyDetector(projectManager.object); + detector.activate(); + await fireSave(uri); + + // No add() call: the project is already registered. + projectManager.verify((pm) => pm.add(typmoq.It.isAny()), typmoq.Times.never()); + assert.deepStrictEqual(existing.inlineScriptMetadata, VALID_METADATA, 'metadata should be refreshed'); + detector.dispose(); + }); + + test('clears cached metadata when a save removes the block from a known project', async () => { + const uri = Uri.file(path.resolve('/ws/wasScript.py')); + const existing = new PythonProjectsImpl('wasScript.py', uri); + existing.inlineScriptMetadata = VALID_METADATA; + readMetadataStub.resolves(undefined); + projectManager.setup((pm) => pm.get(typmoq.It.isAny())).returns(() => existing); + + const detector = new InlineScriptLazyDetector(projectManager.object); + detector.activate(); + await fireSave(uri); + + projectManager.verify((pm) => pm.add(typmoq.It.isAny()), typmoq.Times.never()); + assert.strictEqual(existing.inlineScriptMetadata, undefined, 'metadata cache should be cleared'); + detector.dispose(); + }); + + test('coalesces concurrent open + save for the same URI', async () => { + const uri = Uri.file(path.resolve('/ws/race.py')); + // First call resolves with metadata, second would too if it + // were made — but we expect coalescing to avoid the second + // call entirely. + readMetadataStub.resolves(VALID_METADATA); + projectManager.setup((pm) => pm.get(typmoq.It.isAny())).returns(() => undefined); + + const detector = new InlineScriptLazyDetector(projectManager.object); + detector.activate(); + await Promise.all([fireOpen(uri), fireSave(uri)]); + + // The read may still be invoked once for the leading event; + // the second is coalesced. + assert.ok(readMetadataStub.callCount === 1, `expected exactly one read, got ${readMetadataStub.callCount}`); + projectManager.verify((pm) => pm.add(typmoq.It.isAny()), typmoq.Times.once()); + detector.dispose(); + }); +}); + +suite('shouldHandleUri', () => { + let getWorkspaceFolderStub: sinon.SinonStub; + + setup(() => { + getWorkspaceFolderStub = sinon.stub(wapi, 'getWorkspaceFolder'); + getWorkspaceFolderStub.callsFake((uri: Uri) => ({ + uri: Uri.file(path.dirname(uri.fsPath)), + name: 'ws', + index: 0, + })); + }); + + teardown(() => { + sinon.restore(); + }); + + test('accepts .py file in workspace folder', () => { + assert.strictEqual(shouldHandleUri(Uri.file(path.resolve('/ws/a.py'))), true); + }); + + test('accepts .PY (uppercase) file', () => { + assert.strictEqual(shouldHandleUri(Uri.file(path.resolve('/ws/A.PY'))), true); + }); + + test('rejects non-.py extension', () => { + assert.strictEqual(shouldHandleUri(Uri.file(path.resolve('/ws/a.txt'))), false); + }); + + test('rejects non-file scheme', () => { + assert.strictEqual(shouldHandleUri(Uri.parse('untitled:a.py')), false); + }); + + test('rejects file outside any workspace folder', () => { + getWorkspaceFolderStub.returns(undefined); + assert.strictEqual(shouldHandleUri(Uri.file(path.resolve('/elsewhere/a.py'))), false); + }); +}); diff --git a/src/test/managers/builtin/pipUtils.inlineScript.unit.test.ts b/src/test/managers/builtin/pipUtils.inlineScript.unit.test.ts new file mode 100644 index 00000000..954a0edd --- /dev/null +++ b/src/test/managers/builtin/pipUtils.inlineScript.unit.test.ts @@ -0,0 +1,146 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import assert from 'assert'; +import * as path from 'path'; +import * as sinon from 'sinon'; +import { CancellationToken, Progress, ProgressOptions, Uri } from 'vscode'; +import { PythonEnvironmentApi, PythonProject } from '../../../api'; +import * as ism from '../../../common/inlineScriptMetadata'; +import * as winapi from '../../../common/window.apis'; +import * as wapi from '../../../common/workspace.apis'; +import { getProjectInstallable } from '../../../managers/builtin/pipUtils'; + +suite('Pip Utils - getProjectInstallable (inline script metadata)', () => { + let findFilesStub: sinon.SinonStub; + let withProgressStub: sinon.SinonStub; + let readMetadataStub: sinon.SinonStub; + let isEnabledStub: sinon.SinonStub; + let mockApi: { getPythonProject: (uri: Uri) => PythonProject | undefined }; + + setup(() => { + findFilesStub = sinon.stub(wapi, 'findFiles'); + // Default: no requirements/pyproject files anywhere. + findFilesStub.resolves([]); + + withProgressStub = sinon.stub(winapi, 'withProgress'); + withProgressStub.callsFake( + async ( + _options: ProgressOptions, + callback: ( + progress: Progress<{ message?: string; increment?: number }>, + token: CancellationToken, + ) => Thenable, + ) => { + return await callback( + {} as Progress<{ message?: string; increment?: number }>, + { isCancellationRequested: false } as CancellationToken, + ); + }, + ); + + readMetadataStub = sinon.stub(ism, 'readInlineScriptMetadataFromFile'); + readMetadataStub.resolves(undefined); + + isEnabledStub = sinon.stub(ism, 'isInlineScriptMetadataEnabled'); + isEnabledStub.returns(true); + + const workspacePath = Uri.file(path.resolve('/test/path/root')).fsPath; + mockApi = { + getPythonProject: (uri: Uri) => { + if (uri.fsPath.startsWith(workspacePath)) { + return { name: 'workspace', uri: Uri.file(workspacePath) }; + } + return undefined; + }, + }; + }); + + teardown(() => { + sinon.restore(); + }); + + test('includes inline-metadata deps for a .py script project', async () => { + const scriptUri = Uri.file(path.resolve('/test/path/root/script.py')); + readMetadataStub.withArgs(sinon.match((u: Uri) => u.toString() === scriptUri.toString())).resolves({ + requiresPython: '>=3.11', + dependencies: ['requests<3', 'rich'], + tool: undefined, + range: { start: 0, end: 80 }, + }); + + const result = ( + await getProjectInstallable(mockApi as PythonEnvironmentApi, [{ name: 'script.py', uri: scriptUri }]) + ).installables; + + assert.strictEqual(result.length, 2, 'should produce one installable per dependency'); + const names = result.map((r) => r.name).sort(); + assert.deepStrictEqual(names, ['requests<3', 'rich']); + result.forEach((item) => { + assert.strictEqual(item.group, 'Inline metadata'); + assert.deepStrictEqual(item.args, [item.name]); + assert.ok(item.uri && item.uri.toString() === scriptUri.toString()); + }); + }); + + test('ignores non-.py projects (folder projects are not walked)', async () => { + const folderProject: PythonProject = { + name: 'workspace', + uri: Uri.file(path.resolve('/test/path/root')), + }; + const result = (await getProjectInstallable(mockApi as PythonEnvironmentApi, [folderProject])).installables; + + assert.ok(readMetadataStub.notCalled, 'should not read metadata for folder projects'); + assert.strictEqual(result.length, 0); + }); + + test('no installables when the .py script has no inline metadata', async () => { + const scriptUri = Uri.file(path.resolve('/test/path/root/plain.py')); + readMetadataStub.resolves(undefined); + + const result = ( + await getProjectInstallable(mockApi as PythonEnvironmentApi, [{ name: 'plain.py', uri: scriptUri }]) + ).installables; + + assert.strictEqual(result.length, 0); + }); + + test('no installables when the .py script has zero declared dependencies', async () => { + const scriptUri = Uri.file(path.resolve('/test/path/root/empty.py')); + readMetadataStub.resolves({ + requiresPython: '>=3.11', + dependencies: [], + tool: undefined, + range: { start: 0, end: 40 }, + }); + + const result = ( + await getProjectInstallable(mockApi as PythonEnvironmentApi, [{ name: 'empty.py', uri: scriptUri }]) + ).installables; + + assert.strictEqual(result.length, 0); + }); + + test('skips metadata read when the experimental setting is off', async () => { + isEnabledStub.returns(false); + const scriptUri = Uri.file(path.resolve('/test/path/root/skip.py')); + + const result = ( + await getProjectInstallable(mockApi as PythonEnvironmentApi, [{ name: 'skip.py', uri: scriptUri }]) + ).installables; + + assert.ok(readMetadataStub.notCalled, 'should not read metadata when feature is disabled'); + assert.strictEqual(result.length, 0); + }); + + test('ignores .py projects with non-file URI schemes', async () => { + const result = ( + await getProjectInstallable(mockApi as PythonEnvironmentApi, [ + { name: 'untitled.py', uri: Uri.parse('untitled:untitled.py') }, + ]) + ).installables; + + assert.ok(readMetadataStub.notCalled, 'should not read metadata for non-file URI'); + assert.strictEqual(result.length, 0); + }); +}); From 20d1e02fe5ef5b2bf6b56175c289869ac1790ff5 Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Tue, 12 May 2026 15:24:49 -0700 Subject: [PATCH 3/9] fix bugs --- src/common/workspace.apis.ts | 9 +++ src/features/inlineScriptLazyDetector.ts | 88 +++++++++++++++++++++--- 2 files changed, 87 insertions(+), 10 deletions(-) diff --git a/src/common/workspace.apis.ts b/src/common/workspace.apis.ts index 199dae30..77f0c3cc 100644 --- a/src/common/workspace.apis.ts +++ b/src/common/workspace.apis.ts @@ -89,3 +89,12 @@ export function onDidSaveTextDocument( ): Disposable { return workspace.onDidSaveTextDocument(listener, thisArgs, disposables); } + +/** + * Snapshot of the text documents VS Code has already opened. Useful + * for extensions activated by `onLanguage:*` events, which miss the + * `onDidOpenTextDocument` fired for the activating document. + */ +export function getOpenTextDocuments(): readonly TextDocument[] { + return workspace.textDocuments; +} diff --git a/src/features/inlineScriptLazyDetector.ts b/src/features/inlineScriptLazyDetector.ts index 8d1823f4..5034c49c 100644 --- a/src/features/inlineScriptLazyDetector.ts +++ b/src/features/inlineScriptLazyDetector.ts @@ -8,8 +8,13 @@ import { isInlineScriptMetadataEnabled, readInlineScriptMetadataFromFile, } from '../common/inlineScriptMetadata'; -import { traceVerbose, traceWarn } from '../common/logging'; -import { getWorkspaceFolder, onDidOpenTextDocument, onDidSaveTextDocument } from '../common/workspace.apis'; +import { traceInfo, traceVerbose, traceWarn } from '../common/logging'; +import { + getOpenTextDocuments, + getWorkspaceFolder, + onDidOpenTextDocument, + onDidSaveTextDocument, +} from '../common/workspace.apis'; import { PythonProjectManager, PythonProjectsImpl } from '../internal.api'; /** @@ -44,12 +49,57 @@ export class InlineScriptLazyDetector implements Disposable { * promises (so production behaviour is unchanged — still * fire-and-forget), but returning the promise lets tests await * the work triggered by a synthetic open/save event. + * + * After subscribing we also replay every document already open at + * activation time. Our `onLanguage:python` activation event fires + * AFTER VS Code has already opened any restored editors, so the + * `onDidOpenTextDocument` for the file that triggered activation + * (the most common case) is gone by the time we subscribe. The + * replay closes that gap; the per-URI dedup in `handleDocument` + * keeps it idempotent if a live event happens to arrive too. + */ + /** + * Subscribe to workspace text-document events. Safe to call once + * during extension activation. The detector starts working + * immediately; the experimental gate is re-checked on every event + * so toggling the setting takes effect without a reload. + * + * Listeners return the promise from `handleDocument` rather than + * void-ing it. VS Code's event bus does not await listener + * promises (so production behaviour is unchanged — still + * fire-and-forget), but returning the promise lets tests await + * the work triggered by a synthetic open/save event. + * + * After subscribing we also replay every document already open at + * activation time. Our `onLanguage:python` activation event fires + * AFTER VS Code has already opened any restored editors, so the + * `onDidOpenTextDocument` for the file that triggered activation + * (the most common case) is gone by the time we subscribe. The + * replay is deferred via `setImmediate` so VS Code finishes any + * in-flight document registration first; the per-URI dedup in + * `handleDocument` keeps it idempotent if a live event happens to + * arrive too. */ public activate(): void { this.subscriptions.push( onDidOpenTextDocument((doc) => this.handleDocument(doc, 'open')), onDidSaveTextDocument((doc) => this.handleDocument(doc, 'save')), ); + // Defer the catch-up pass so we observe `workspace.textDocuments` + // AFTER VS Code finishes registering the document that triggered + // our activation. Running the loop synchronously here can race + // against VS Code's own initialization on `onLanguage:*` activation. + const handle = setImmediate(() => { + const openDocs = getOpenTextDocuments(); + traceInfo( + `inlineScriptLazyDetector: activate() saw ${openDocs.length} open document(s): ` + + openDocs.map((d) => `[${d.languageId}:${d.uri.scheme}]${d.uri.toString()}`).join(', '), + ); + for (const doc of openDocs) { + void this.handleDocument(doc, 'open'); + } + }); + this.subscriptions.push(new Disposable(() => clearImmediate(handle))); } public dispose(): void { @@ -60,12 +110,26 @@ export class InlineScriptLazyDetector implements Disposable { private async handleDocument(doc: TextDocument, trigger: 'open' | 'save'): Promise { const uri = doc.uri; + // Diagnostic: trace every event entering the detector so we + // can see, at the default `Info` channel log level, whether + // open/save events are reaching us at all. + traceInfo(`inlineScriptLazyDetector: event received (${trigger}) ${uri.toString()}`); if (!shouldHandleUri(uri)) { + traceInfo( + `inlineScriptLazyDetector: skipped (${trigger}) ${uri.toString()} ` + + `(scheme='${uri.scheme}', extname='${path.extname(uri.fsPath).toLowerCase()}', ` + + `inWorkspace=${getWorkspaceFolder(uri) !== undefined})`, + ); return; } if (!isInlineScriptMetadataEnabled(uri)) { + traceInfo( + `inlineScriptLazyDetector: skipped (${trigger}) ${uri.fsPath} ` + + `(setting 'python-envs.useInlineScriptMetadata' is false)`, + ); return; } + traceInfo(`inlineScriptLazyDetector: processing (${trigger}) ${uri.fsPath}`); const key = uri.toString(); const existing = this.inFlight.get(key); if (existing) { @@ -94,7 +158,14 @@ export class InlineScriptLazyDetector implements Disposable { return; } - const existing = this.projectManager.get(uri); + // `projectManager.get()` does CONTAINMENT matching — for a + // script inside a workspace folder it returns the folder + // project, not undefined. That is the wrong answer here: a + // script project is distinct from the folder that contains + // it. Filter the result down to an exact-URI match before + // deciding whether this file is already registered. + const candidate = this.projectManager.get(uri); + const existing = candidate !== undefined && candidate.uri.toString() === uri.toString() ? candidate : undefined; if (metadata === undefined) { // No (valid) block in the file. If it was previously @@ -103,11 +174,10 @@ export class InlineScriptLazyDetector implements Disposable { // passing edit would be surprising. We only clear the // cached metadata so downstream consumers don't act on // stale data. + traceInfo(`inlineScriptLazyDetector: no metadata block in ${uri.fsPath} (${trigger})`); if (existing instanceof PythonProjectsImpl && existing.inlineScriptMetadata !== undefined) { existing.inlineScriptMetadata = undefined; - traceVerbose( - `inlineScriptLazyDetector: cleared cached metadata for ${uri.fsPath} (${trigger}: no block)`, - ); + traceInfo(`inlineScriptLazyDetector: cleared cached metadata for ${uri.fsPath} (${trigger}: no block)`); } return; } @@ -117,9 +187,7 @@ export class InlineScriptLazyDetector implements Disposable { // (it may have changed on save; downstream code, e.g. // `getProjectInstallable`, is also free to re-read). existing.inlineScriptMetadata = metadata; - traceVerbose( - `inlineScriptLazyDetector: refreshed metadata for ${uri.fsPath} (${trigger}: already a project)`, - ); + traceInfo(`inlineScriptLazyDetector: refreshed metadata for ${uri.fsPath} (${trigger}: already a project)`); return; } @@ -134,7 +202,7 @@ export class InlineScriptLazyDetector implements Disposable { project.inlineScriptMetadata = metadata; try { await this.projectManager.add(project); - traceVerbose(`inlineScriptLazyDetector: registered ${uri.fsPath} as a project (${trigger})`); + traceInfo(`inlineScriptLazyDetector: registered ${uri.fsPath} as a project (${trigger})`); } catch (err) { traceWarn(`inlineScriptLazyDetector: failed to register ${uri.fsPath} as a project:`, err); } From ba357d0d4b8030c4011a33c69e99fc3238fc8d0a Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Tue, 12 May 2026 15:58:32 -0700 Subject: [PATCH 4/9] change setting scope --- package.json | 2 +- src/common/inlineScriptMetadata.ts | 11 +-- src/features/creators/inlineScriptDetector.ts | 51 ++++++++++---- .../inlineScriptDetector.unit.test.ts | 69 +++++++++++++++++++ 4 files changed, 116 insertions(+), 17 deletions(-) diff --git a/package.json b/package.json index de0630dc..abe642a5 100644 --- a/package.json +++ b/package.json @@ -143,7 +143,7 @@ "type": "boolean", "description": "%python-envs.useInlineScriptMetadata.description%", "default": false, - "scope": "window", + "scope": "resource", "tags": [ "experimental" ] diff --git a/src/common/inlineScriptMetadata.ts b/src/common/inlineScriptMetadata.ts index b3263c83..08e2c2ab 100644 --- a/src/common/inlineScriptMetadata.ts +++ b/src/common/inlineScriptMetadata.ts @@ -406,13 +406,16 @@ const SETTING_KEY = 'useInlineScriptMetadata'; /** * Returns `true` when the inline-script-metadata feature is enabled - * for the given scope. The setting is window-scoped, so callers may - * pass a `Uri` to resolve a workspace-folder-aware view of the - * configuration, or `undefined` to read the window-level value. + * for the given scope. The setting is `resource`-scoped, so callers + * SHOULD pass a `Uri` (typically the script's URI or a workspace + * folder URI) to get a workspace-folder-aware view of the + * configuration. Passing `undefined` falls back to the workspace / + * user value with no folder context. * * Every consumer of inline-script-metadata behavior — detection, env * creation, watcher — MUST gate its work through this helper so that - * disabling the setting makes the feature invisible. + * disabling the setting (globally or in a specific folder) makes the + * feature invisible for that scope. */ export function isInlineScriptMetadataEnabled(scope?: ConfigurationScope): boolean { return workspace.getConfiguration(SETTING_SECTION, scope).get(SETTING_KEY, false); diff --git a/src/features/creators/inlineScriptDetector.ts b/src/features/creators/inlineScriptDetector.ts index 3a131aca..fdc5acce 100644 --- a/src/features/creators/inlineScriptDetector.ts +++ b/src/features/creators/inlineScriptDetector.ts @@ -12,7 +12,7 @@ import { import { InlineScriptStrings } from '../../common/localize'; import { traceInfo, traceVerbose } from '../../common/logging'; import { showErrorMessage, showQuickPickWithButtons } from '../../common/window.apis'; -import { findFiles } from '../../common/workspace.apis'; +import { findFiles, getWorkspaceFolders } from '../../common/workspace.apis'; import { PythonProjectManager, PythonProjectsImpl } from '../../internal.api'; /** @@ -57,11 +57,13 @@ export class InlineScriptDetector implements PythonProjectCreator { constructor(private readonly pm: PythonProjectManager) {} public async create(_options?: PythonProjectCreatorOptions): Promise { - if (!isInlineScriptMetadataEnabled()) { - // The feature is gated behind the experimental setting. - // If the creator is somehow invoked while disabled (e.g. - // toggled mid-session), fail gracefully rather than - // performing the scan. + // The feature is gated behind the experimental setting, which + // is `resource`-scoped — so in a multi-root workspace it may + // be on in some folders and off in others. Bail early only if + // it is off everywhere (no folder has it enabled, and the + // window-level fallback is also off). Per-folder filtering of + // discovered candidates happens further down. + if (!isAnyFolderEnabled()) { setImmediate(() => { showErrorMessage(InlineScriptStrings.noScriptsFound); }); @@ -76,12 +78,19 @@ export class InlineScriptDetector implements PythonProjectCreator { return undefined; } - // Filter out scripts that are already registered as a project - // with the exact same URI. Folder-scoped projects that happen - // to contain this script are intentionally NOT filtered out: - // a `.py` file with inline metadata is its own project and - // sits alongside its enclosing folder project. + // Filter out: + // (a) candidates inside a workspace folder where the user has + // disabled the feature (resource-scoped setting), and + // (b) scripts that are already registered as a project with + // the exact same URI. Folder-scoped projects that happen + // to contain this script are intentionally NOT filtered + // out: a `.py` file with inline metadata is its own + // project and sits alongside its enclosing folder + // project. const fresh = candidates.filter((uri) => { + if (!isInlineScriptMetadataEnabled(uri)) { + return false; + } const existing = this.pm.get(uri); if (!existing) { return true; @@ -89,7 +98,10 @@ export class InlineScriptDetector implements PythonProjectCreator { return path.normalize(existing.uri.fsPath) !== path.normalize(uri.fsPath); }); if (fresh.length === 0) { - traceInfo(`InlineScriptDetector: all ${candidates.length} candidate .py files are already registered.`); + traceInfo( + `InlineScriptDetector: no fresh candidates after filtering ${candidates.length} .py file(s) ` + + `(disabled folders or already-registered).`, + ); setImmediate(() => { showErrorMessage(InlineScriptStrings.noScriptsFound); }); @@ -151,6 +163,21 @@ export class InlineScriptDetector implements PythonProjectCreator { } } +/** + * Returns `true` when the inline-script-metadata feature is enabled + * in at least one open workspace folder (or, if no folders are open, + * at the window / user level). The setting is `resource`-scoped, so a + * window-level read alone would miss folder-only opt-ins in a + * multi-root workspace. + */ +function isAnyFolderEnabled(): boolean { + const folders = getWorkspaceFolders() ?? []; + if (folders.length === 0) { + return isInlineScriptMetadataEnabled(); + } + return folders.some((f) => isInlineScriptMetadataEnabled(f.uri)); +} + /** * Read inline script metadata from each URI in `uris` with bounded * parallelism, returning only the URIs whose `.py` file declared a diff --git a/src/test/features/creators/inlineScriptDetector.unit.test.ts b/src/test/features/creators/inlineScriptDetector.unit.test.ts index 67d15e51..0101e7f7 100644 --- a/src/test/features/creators/inlineScriptDetector.unit.test.ts +++ b/src/test/features/creators/inlineScriptDetector.unit.test.ts @@ -28,6 +28,7 @@ const META_B: ism.InlineScriptMetadata = { suite('InlineScriptDetector (creator)', () => { let findFilesStub: sinon.SinonStub; + let getWorkspaceFoldersStub: sinon.SinonStub; let showErrorStub: sinon.SinonStub; let showQuickPickStub: sinon.SinonStub; let readMetadataStub: sinon.SinonStub; @@ -38,6 +39,11 @@ suite('InlineScriptDetector (creator)', () => { findFilesStub = sinon.stub(wapi, 'findFiles'); findFilesStub.resolves([]); + getWorkspaceFoldersStub = sinon.stub(wapi, 'getWorkspaceFolders'); + // Default: no workspace folders open. Multi-root tests + // override this. + getWorkspaceFoldersStub.returns(undefined); + showErrorStub = sinon.stub(winapi, 'showErrorMessage'); showErrorStub.resolves(undefined); @@ -189,6 +195,69 @@ suite('InlineScriptDetector (creator)', () => { assert.ok(result && result.length === 1); pm.verify((p) => p.add(typmoq.It.isAny()), typmoq.Times.once()); }); + + test('multi-root: bails early when feature is disabled in every open folder', async () => { + const folderA = Uri.file(path.resolve('/wsA')); + const folderB = Uri.file(path.resolve('/wsB')); + getWorkspaceFoldersStub.returns([ + { uri: folderA, name: 'wsA', index: 0 }, + { uri: folderB, name: 'wsB', index: 1 }, + ]); + // Disabled in every folder AND at the no-scope level. + isEnabledStub.returns(false); + + const detector = new InlineScriptDetector(pm.object); + const result = await detector.create(); + await waitForImmediate(); + assert.strictEqual(result, undefined); + assert.ok(findFilesStub.notCalled, 'should not scan when no folder has the feature enabled'); + assert.ok(showErrorStub.calledOnce); + }); + + test('multi-root: scans when feature is enabled in at least one folder, filters candidates by per-folder setting', async () => { + const folderA = Uri.file(path.resolve('/wsA')); + const folderB = Uri.file(path.resolve('/wsB')); + const scriptA = Uri.file(path.resolve('/wsA/a.py')); + const scriptB = Uri.file(path.resolve('/wsB/b.py')); + getWorkspaceFoldersStub.returns([ + { uri: folderA, name: 'wsA', index: 0 }, + { uri: folderB, name: 'wsB', index: 1 }, + ]); + + // Per-folder setting: enabled for wsA, disabled for wsB and + // the no-scope read (window-level). + isEnabledStub.callsFake((scope?: Uri) => { + if (!scope) { + return false; + } + return scope.fsPath.startsWith(folderA.fsPath); + }); + + // findFiles returns one candidate from each folder. + findFilesStub.resolves([scriptA, scriptB]); + readMetadataStub.withArgs(sinon.match((u: Uri) => u.toString() === scriptA.toString())).resolves(META_A); + readMetadataStub.withArgs(sinon.match((u: Uri) => u.toString() === scriptB.toString())).resolves(META_B); + + // Capture what the picker is shown. + let pickerItems: Array<{ uri: Uri }> = []; + showQuickPickStub.callsFake(async (items: Array<{ label: string; uri: Uri }>) => { + pickerItems = items; + return items; + }); + + const detector = new InlineScriptDetector(pm.object); + const result = await detector.create(); + + assert.ok(result && result.length === 1, 'only the enabled-folder script should be offered'); + assert.strictEqual(result![0].uri.toString(), scriptA.toString()); + assert.strictEqual(pickerItems.length, 1, 'picker should only contain the enabled-folder script'); + assert.strictEqual(pickerItems[0].uri.toString(), scriptA.toString()); + // The disabled-folder candidate must not have been read. + assert.ok( + readMetadataStub.neverCalledWith(sinon.match((u: Uri) => u.toString() === scriptB.toString())), + 'should not read metadata for files in disabled folders', + ); + }); }); suite('scanForInlineScripts', () => { From bfd8f76b89358dff6643eca0dd2c889bc72531e4 Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Wed, 13 May 2026 09:56:54 -0700 Subject: [PATCH 5/9] fix bugs --- package.json | 2 +- src/common/inlineScriptMetadata.ts | 15 +- src/features/inlineScriptLazyDetector.ts | 109 +++++++----- src/managers/builtin/pipUtils.ts | 6 +- .../common/inlineScriptMetadata.unit.test.ts | 20 +++ .../inlineScriptLazyDetector.unit.test.ts | 166 +++++++++++++++++- .../pipUtils.inlineScript.unit.test.ts | 2 +- 7 files changed, 272 insertions(+), 48 deletions(-) diff --git a/package.json b/package.json index abe642a5..4d0893e9 100644 --- a/package.json +++ b/package.json @@ -141,7 +141,7 @@ }, "python-envs.useInlineScriptMetadata": { "type": "boolean", - "description": "%python-envs.useInlineScriptMetadata.description%", + "markdownDescription": "%python-envs.useInlineScriptMetadata.description%", "default": false, "scope": "resource", "tags": [ diff --git a/src/common/inlineScriptMetadata.ts b/src/common/inlineScriptMetadata.ts index 08e2c2ab..a66071d7 100644 --- a/src/common/inlineScriptMetadata.ts +++ b/src/common/inlineScriptMetadata.ts @@ -3,8 +3,9 @@ import * as tomljs from '@iarna/toml'; import * as fs from 'fs/promises'; -import { ConfigurationScope, Uri, workspace } from 'vscode'; +import { ConfigurationScope, Uri } from 'vscode'; import { traceVerbose, traceWarn } from './logging'; +import { getConfiguration } from './workspace.apis'; /** * Parsed and validated PEP 723 `script` metadata block. @@ -404,6 +405,16 @@ function compareReleases(a: readonly number[], b: readonly number[]): number { const SETTING_SECTION = 'python-envs'; const SETTING_KEY = 'useInlineScriptMetadata'; +/** + * Fully-qualified setting key (`section.key`) for the experimental + * inline-script-metadata gate. Exported so other modules (notably + * `InlineScriptLazyDetector`) can filter `onDidChangeConfiguration` + * events against the same identifier the setting is registered under + * — keeping the listener and the accessor below in lockstep if the + * setting is ever renamed. + */ +export const INLINE_SCRIPT_METADATA_SETTING = `${SETTING_SECTION}.${SETTING_KEY}`; + /** * Returns `true` when the inline-script-metadata feature is enabled * for the given scope. The setting is `resource`-scoped, so callers @@ -418,5 +429,5 @@ const SETTING_KEY = 'useInlineScriptMetadata'; * feature invisible for that scope. */ export function isInlineScriptMetadataEnabled(scope?: ConfigurationScope): boolean { - return workspace.getConfiguration(SETTING_SECTION, scope).get(SETTING_KEY, false); + return getConfiguration(SETTING_SECTION, scope).get(SETTING_KEY, false); } diff --git a/src/features/inlineScriptLazyDetector.ts b/src/features/inlineScriptLazyDetector.ts index 5034c49c..1582db19 100644 --- a/src/features/inlineScriptLazyDetector.ts +++ b/src/features/inlineScriptLazyDetector.ts @@ -4,6 +4,7 @@ import * as path from 'path'; import { Disposable, TextDocument, Uri } from 'vscode'; import { + INLINE_SCRIPT_METADATA_SETTING, InlineScriptMetadata, isInlineScriptMetadataEnabled, readInlineScriptMetadataFromFile, @@ -12,11 +13,21 @@ import { traceInfo, traceVerbose, traceWarn } from '../common/logging'; import { getOpenTextDocuments, getWorkspaceFolder, + onDidChangeConfiguration, onDidOpenTextDocument, onDidSaveTextDocument, } from '../common/workspace.apis'; import { PythonProjectManager, PythonProjectsImpl } from '../internal.api'; +/** + * Fully-qualified configuration key for the experimental setting that + * gates the inline-script-metadata feature. Re-exported from + * `../common/inlineScriptMetadata` so the `onDidChangeConfiguration` + * filter and the accessor `isInlineScriptMetadataEnabled` are + * guaranteed to refer to the same setting. + */ +const SETTING_FQN = INLINE_SCRIPT_METADATA_SETTING; + /** * Lazy on-open / on-save detector for `.py` files that declare inline * script metadata (PEP 723). This is the PRIMARY detection path: when @@ -38,26 +49,6 @@ export class InlineScriptLazyDetector implements Disposable { constructor(private readonly projectManager: PythonProjectManager) {} - /** - * Subscribe to workspace text-document events. Safe to call once - * during extension activation. The detector starts working - * immediately; the experimental gate is re-checked on every event - * so toggling the setting takes effect without a reload. - * - * Listeners return the promise from `handleDocument` rather than - * void-ing it. VS Code's event bus does not await listener - * promises (so production behaviour is unchanged — still - * fire-and-forget), but returning the promise lets tests await - * the work triggered by a synthetic open/save event. - * - * After subscribing we also replay every document already open at - * activation time. Our `onLanguage:python` activation event fires - * AFTER VS Code has already opened any restored editors, so the - * `onDidOpenTextDocument` for the file that triggered activation - * (the most common case) is gone by the time we subscribe. The - * replay closes that gap; the per-URI dedup in `handleDocument` - * keeps it idempotent if a live event happens to arrive too. - */ /** * Subscribe to workspace text-document events. Safe to call once * during extension activation. The detector starts working @@ -84,24 +75,55 @@ export class InlineScriptLazyDetector implements Disposable { this.subscriptions.push( onDidOpenTextDocument((doc) => this.handleDocument(doc, 'open')), onDidSaveTextDocument((doc) => this.handleDocument(doc, 'save')), + // When the user toggles `python-envs.useInlineScriptMetadata` + // we replay the catch-up pass so already-open `.py` files + // get inspected without requiring a window reload or a + // manual save. The per-event gate inside `handleDocument` + // makes the off→still-off and on→still-on cases free; the + // useful case is off→on, where we discover scripts the + // user has had open all along. + onDidChangeConfiguration((e) => { + if (e.affectsConfiguration(SETTING_FQN)) { + this.replayOpenDocuments('config-change'); + } + }), ); // Defer the catch-up pass so we observe `workspace.textDocuments` // AFTER VS Code finishes registering the document that triggered // our activation. Running the loop synchronously here can race // against VS Code's own initialization on `onLanguage:*` activation. - const handle = setImmediate(() => { - const openDocs = getOpenTextDocuments(); - traceInfo( - `inlineScriptLazyDetector: activate() saw ${openDocs.length} open document(s): ` + - openDocs.map((d) => `[${d.languageId}:${d.uri.scheme}]${d.uri.toString()}`).join(', '), - ); - for (const doc of openDocs) { - void this.handleDocument(doc, 'open'); - } - }); + const handle = setImmediate(() => this.replayOpenDocuments('activate')); this.subscriptions.push(new Disposable(() => clearImmediate(handle))); } + /** + * Walk every currently-open text document and run it through + * `handleDocument` as if a synthetic `open` event had fired. Used + * both for the deferred activation catch-up and for the + * setting-toggle replay. The per-URI dedup in `handleDocument` + * keeps this safe to call repeatedly. + */ + private replayOpenDocuments(source: 'activate' | 'config-change'): void { + // Restrict the replay to documents that the per-event handler + // would actually look at. This keeps the activation log + // proportional to the work the detector will do — on an + // editor with many tabs open we would otherwise dump every + // URI just to throw most of them away inside + // `handleDocument`. + const openDocs = getOpenTextDocuments().filter((d) => shouldHandleUri(d.uri)); + if (openDocs.length === 0) { + traceVerbose(`inlineScriptLazyDetector: ${source} replay found no candidate .py documents`); + return; + } + traceVerbose( + `inlineScriptLazyDetector: ${source} replay over ${openDocs.length} candidate .py document(s): ` + + openDocs.map((d) => d.uri.fsPath).join(', '), + ); + for (const doc of openDocs) { + void this.handleDocument(doc, 'open'); + } + } + public dispose(): void { this.subscriptions.forEach((s) => s.dispose()); this.subscriptions.length = 0; @@ -110,12 +132,15 @@ export class InlineScriptLazyDetector implements Disposable { private async handleDocument(doc: TextDocument, trigger: 'open' | 'save'): Promise { const uri = doc.uri; - // Diagnostic: trace every event entering the detector so we - // can see, at the default `Info` channel log level, whether - // open/save events are reaching us at all. - traceInfo(`inlineScriptLazyDetector: event received (${trigger}) ${uri.toString()}`); + // Diagnostic: trace every event entering the detector. This + // is high-frequency (fires on every keystroke-triggered save + // and on every editor open) so it stays at `traceVerbose` — + // the `Trace` log level — to avoid flooding the default + // `Info` channel. First-time project registration is logged + // at `traceInfo` further down. + traceVerbose(`inlineScriptLazyDetector: event received (${trigger}) ${uri.toString()}`); if (!shouldHandleUri(uri)) { - traceInfo( + traceVerbose( `inlineScriptLazyDetector: skipped (${trigger}) ${uri.toString()} ` + `(scheme='${uri.scheme}', extname='${path.extname(uri.fsPath).toLowerCase()}', ` + `inWorkspace=${getWorkspaceFolder(uri) !== undefined})`, @@ -123,13 +148,13 @@ export class InlineScriptLazyDetector implements Disposable { return; } if (!isInlineScriptMetadataEnabled(uri)) { - traceInfo( + traceVerbose( `inlineScriptLazyDetector: skipped (${trigger}) ${uri.fsPath} ` + `(setting 'python-envs.useInlineScriptMetadata' is false)`, ); return; } - traceInfo(`inlineScriptLazyDetector: processing (${trigger}) ${uri.fsPath}`); + traceVerbose(`inlineScriptLazyDetector: processing (${trigger}) ${uri.fsPath}`); const key = uri.toString(); const existing = this.inFlight.get(key); if (existing) { @@ -174,10 +199,12 @@ export class InlineScriptLazyDetector implements Disposable { // passing edit would be surprising. We only clear the // cached metadata so downstream consumers don't act on // stale data. - traceInfo(`inlineScriptLazyDetector: no metadata block in ${uri.fsPath} (${trigger})`); + traceVerbose(`inlineScriptLazyDetector: no metadata block in ${uri.fsPath} (${trigger})`); if (existing instanceof PythonProjectsImpl && existing.inlineScriptMetadata !== undefined) { existing.inlineScriptMetadata = undefined; - traceInfo(`inlineScriptLazyDetector: cleared cached metadata for ${uri.fsPath} (${trigger}: no block)`); + traceVerbose( + `inlineScriptLazyDetector: cleared cached metadata for ${uri.fsPath} (${trigger}: no block)`, + ); } return; } @@ -187,7 +214,9 @@ export class InlineScriptLazyDetector implements Disposable { // (it may have changed on save; downstream code, e.g. // `getProjectInstallable`, is also free to re-read). existing.inlineScriptMetadata = metadata; - traceInfo(`inlineScriptLazyDetector: refreshed metadata for ${uri.fsPath} (${trigger}: already a project)`); + traceVerbose( + `inlineScriptLazyDetector: refreshed metadata for ${uri.fsPath} (${trigger}: already a project)`, + ); return; } diff --git a/src/managers/builtin/pipUtils.ts b/src/managers/builtin/pipUtils.ts index a67d21cd..450d8431 100644 --- a/src/managers/builtin/pipUtils.ts +++ b/src/managers/builtin/pipUtils.ts @@ -354,8 +354,8 @@ export async function getProjectInstallable( // `uri` is a `.py` file may carry inline dependency // declarations at the top of the file. We surface those // declared deps as `Installable` entries grouped under - // 'Inline metadata' so they appear in the same pre-install - // picker as `pyproject.toml` and `requirements*.txt` + // 'PEP 723' so they appear in the same pre-install picker + // as `pyproject.toml` and `requirements*.txt` // dependencies. This branch is gated by the experimental // setting and only runs for projects whose root URI is // itself a `.py` file — folder projects are never walked @@ -380,7 +380,7 @@ export async function getProjectInstallable( installable.push({ name: dep, displayName: dep, - group: 'Inline metadata', + group: 'PEP 723', args: [dep], description: InlineScriptStrings.installableDescription, uri: project.uri, diff --git a/src/test/common/inlineScriptMetadata.unit.test.ts b/src/test/common/inlineScriptMetadata.unit.test.ts index 9b98c502..cc8971f8 100644 --- a/src/test/common/inlineScriptMetadata.unit.test.ts +++ b/src/test/common/inlineScriptMetadata.unit.test.ts @@ -243,6 +243,26 @@ suite('inlineScriptMetadata', () => { assert.deepStrictEqual([...(md.dependencies ?? [])], ['a']); }); + test('valid script block followed by non-`script` block', () => { + // Reverse order: a valid `# /// script` block immediately + // before another block whose TYPE is not `script`. The + // parser must still recognize the script block — PEP 723 + // does not require it to be the first or last block in + // the file. + const text = script([ + '# /// script', + '# dependencies = ["b"]', + '# ///', + '', + '# /// pyproject', + '# foo = "bar"', + '# ///', + ]); + const md = readInlineScriptMetadata(text); + assert.ok(md, 'a script block adjacent to a pyproject block must be recognized'); + assert.deepStrictEqual([...(md.dependencies ?? [])], ['b']); + }); + test('range refers to the normalized text', () => { // Canonical regex requires at least one content line; use a // bare `#` so the block is the minimal accepted form. diff --git a/src/test/features/inlineScriptLazyDetector.unit.test.ts b/src/test/features/inlineScriptLazyDetector.unit.test.ts index 69579872..415f922a 100644 --- a/src/test/features/inlineScriptLazyDetector.unit.test.ts +++ b/src/test/features/inlineScriptLazyDetector.unit.test.ts @@ -5,7 +5,7 @@ import assert from 'assert'; import * as path from 'path'; import * as sinon from 'sinon'; import * as typmoq from 'typemoq'; -import { Disposable, TextDocument, Uri } from 'vscode'; +import { ConfigurationChangeEvent, Disposable, TextDocument, Uri } from 'vscode'; import { PythonProject } from '../../api'; import * as ism from '../../common/inlineScriptMetadata'; import * as wapi from '../../common/workspace.apis'; @@ -28,16 +28,20 @@ const VALID_METADATA: ism.InlineScriptMetadata = { suite('InlineScriptLazyDetector', () => { let onDidOpenStub: sinon.SinonStub; let onDidSaveStub: sinon.SinonStub; + let onDidChangeConfigStub: sinon.SinonStub; + let getOpenTextDocumentsStub: sinon.SinonStub; let getWorkspaceFolderStub: sinon.SinonStub; let readMetadataStub: sinon.SinonStub; let isEnabledStub: sinon.SinonStub; let openListener: ((doc: TextDocument) => unknown) | undefined; let saveListener: ((doc: TextDocument) => unknown) | undefined; + let configListener: ((e: ConfigurationChangeEvent) => unknown) | undefined; let projectManager: typmoq.IMock; setup(() => { openListener = undefined; saveListener = undefined; + configListener = undefined; onDidOpenStub = sinon.stub(wapi, 'onDidOpenTextDocument'); onDidOpenStub.callsFake((listener: (doc: TextDocument) => unknown) => { @@ -55,6 +59,19 @@ suite('InlineScriptLazyDetector', () => { }); }); + onDidChangeConfigStub = sinon.stub(wapi, 'onDidChangeConfiguration'); + onDidChangeConfigStub.callsFake((listener: (e: ConfigurationChangeEvent) => unknown) => { + configListener = listener; + return new Disposable(() => { + configListener = undefined; + }); + }); + + // Default to an empty list of open documents. Tests that + // exercise the catch-up replay override this. + getOpenTextDocumentsStub = sinon.stub(wapi, 'getOpenTextDocuments'); + getOpenTextDocumentsStub.returns([]); + getWorkspaceFolderStub = sinon.stub(wapi, 'getWorkspaceFolder'); // By default, every URI is treated as being inside a workspace // folder. Tests that want to exercise the "not in workspace" @@ -239,6 +256,153 @@ suite('InlineScriptLazyDetector', () => { projectManager.verify((pm) => pm.add(typmoq.It.isAny()), typmoq.Times.once()); detector.dispose(); }); + + // ---------- B1 / B2: catch-up replay over `getOpenTextDocuments` ---------- + + // Drain the microtask queue and the next `setImmediate` slot so + // the deferred catch-up replay can run before assertions. + function flushImmediate(): Promise { + return new Promise((resolve) => setImmediate(resolve)); + } + + test('activate() replays already-open .py documents via setImmediate', async () => { + const uriWithMeta = Uri.file(path.resolve('/ws/withMeta.py')); + const uriPlain = Uri.file(path.resolve('/ws/plain.py')); + const uriNonPy = Uri.file(path.resolve('/ws/skip.txt')); + readMetadataStub.callsFake(async (u: Uri) => + u.toString() === uriWithMeta.toString() ? VALID_METADATA : undefined, + ); + projectManager.setup((pm) => pm.get(typmoq.It.isAny())).returns(() => undefined); + getOpenTextDocumentsStub.returns([makeDoc(uriWithMeta), makeDoc(uriPlain), makeDoc(uriNonPy)]); + + const detector = new InlineScriptLazyDetector(projectManager.object); + detector.activate(); + // Wait for the deferred catch-up. + await flushImmediate(); + // Then await any in-flight reads kicked off by the replay. + await flushImmediate(); + + // The non-`.py` URI must be filtered out by `shouldHandleUri` + // BEFORE the read is attempted. + assert.strictEqual(readMetadataStub.callCount, 2, 'should read each candidate .py document exactly once'); + const readUris = readMetadataStub.getCalls().map((c) => (c.args[0] as Uri).toString()); + assert.ok(readUris.includes(uriWithMeta.toString())); + assert.ok(readUris.includes(uriPlain.toString())); + assert.ok(!readUris.includes(uriNonPy.toString()), 'should not read non-.py URI during replay'); + projectManager.verify((pm) => pm.add(typmoq.It.isAny()), typmoq.Times.once()); + detector.dispose(); + }); + + test('dispose() cancels the pending catch-up replay', async () => { + getOpenTextDocumentsStub.returns([makeDoc(Uri.file(path.resolve('/ws/never.py')))]); + const detector = new InlineScriptLazyDetector(projectManager.object); + detector.activate(); + // Tear down BEFORE the `setImmediate` slot fires. + detector.dispose(); + await flushImmediate(); + assert.ok(readMetadataStub.notCalled, 'dispose() must clear the pending setImmediate handle'); + }); + + // ---------- B3: URI registered with a non-impl project ---------- + + test('skips refresh when URI is registered with a non-PythonProjectsImpl project', async () => { + const uri = Uri.file(path.resolve('/ws/foreign.py')); + // A `PythonProject` that is NOT a `PythonProjectsImpl`. This + // mirrors the (rare) case where a third-party manager has + // registered the same URI under its own concrete class. + const foreign: PythonProject = { name: 'foreign.py', uri }; + readMetadataStub.resolves(VALID_METADATA); + projectManager.setup((pm) => pm.get(typmoq.It.isAny())).returns(() => foreign); + + const detector = new InlineScriptLazyDetector(projectManager.object); + detector.activate(); + await fireOpen(uri); + + projectManager.verify((pm) => pm.add(typmoq.It.isAny()), typmoq.Times.never()); + detector.dispose(); + }); + + // ---------- B6: multi-root per-folder gating ---------- + + test('respects per-folder setting scope when the feature is enabled in some folders only', async () => { + const onUri = Uri.file(path.resolve('/wsOn/script.py')); + const offUri = Uri.file(path.resolve('/wsOff/script.py')); + // Stub the gate so it returns true only for URIs that look + // like they live under `/wsOn`. + isEnabledStub.callsFake((scope?: { fsPath?: string }) => { + const p = scope?.fsPath; + if (typeof p !== 'string') { + return false; + } + return p.includes(`${path.sep}wsOn${path.sep}`) || p.endsWith(`${path.sep}wsOn`); + }); + readMetadataStub.resolves(VALID_METADATA); + projectManager.setup((pm) => pm.get(typmoq.It.isAny())).returns(() => undefined); + + const detector = new InlineScriptLazyDetector(projectManager.object); + detector.activate(); + await fireOpen(onUri); + await fireOpen(offUri); + + assert.strictEqual(readMetadataStub.callCount, 1, 'should read only the file in the enabled folder'); + const readUri = readMetadataStub.firstCall.args[0] as Uri; + assert.strictEqual(readUri.toString(), onUri.toString()); + detector.dispose(); + }); + + // ---------- C2: setting-toggle triggers replay ---------- + + test('onDidChangeConfiguration replays open documents when the experimental setting toggles', async () => { + const uri = Uri.file(path.resolve('/ws/late.py')); + // The file is open in the editor BEFORE the user toggles the + // setting. The activation replay finds it but bails because + // the feature is disabled. + getOpenTextDocumentsStub.returns([makeDoc(uri)]); + isEnabledStub.returns(false); + readMetadataStub.resolves(VALID_METADATA); + projectManager.setup((pm) => pm.get(typmoq.It.isAny())).returns(() => undefined); + + const detector = new InlineScriptLazyDetector(projectManager.object); + detector.activate(); + await flushImmediate(); + assert.ok(readMetadataStub.notCalled, 'replay during activation should bail because setting is off'); + + // User flips the setting on; the lazy detector should pick + // the file up without requiring a manual save or reload. + isEnabledStub.returns(true); + assert.ok(configListener, 'config-change listener should be registered after activate()'); + const event = { + affectsConfiguration: (key: string) => key === 'python-envs.useInlineScriptMetadata', + } as ConfigurationChangeEvent; + configListener!(event); + // The replay schedules `handleDocument` synchronously; let + // the awaited work resolve. + await flushImmediate(); + await flushImmediate(); + + assert.strictEqual(readMetadataStub.callCount, 1, 'config-change replay must inspect the open .py document'); + projectManager.verify((pm) => pm.add(typmoq.It.isAny()), typmoq.Times.once()); + detector.dispose(); + }); + + test('onDidChangeConfiguration ignores changes to unrelated settings', async () => { + getOpenTextDocumentsStub.returns([makeDoc(Uri.file(path.resolve('/ws/foo.py')))]); + const detector = new InlineScriptLazyDetector(projectManager.object); + detector.activate(); + await flushImmediate(); + // The activation replay invoked `handleDocument` and bailed + // inside the gate (read stub returned undefined). Reset. + readMetadataStub.resetHistory(); + + const event = { + affectsConfiguration: (_key: string) => false, + } as ConfigurationChangeEvent; + assert.ok(configListener); + configListener!(event); + await flushImmediate(); + assert.ok(readMetadataStub.notCalled, 'unrelated config change must not trigger a replay'); + detector.dispose(); + }); }); suite('shouldHandleUri', () => { diff --git a/src/test/managers/builtin/pipUtils.inlineScript.unit.test.ts b/src/test/managers/builtin/pipUtils.inlineScript.unit.test.ts index 954a0edd..7d47e9f4 100644 --- a/src/test/managers/builtin/pipUtils.inlineScript.unit.test.ts +++ b/src/test/managers/builtin/pipUtils.inlineScript.unit.test.ts @@ -77,7 +77,7 @@ suite('Pip Utils - getProjectInstallable (inline script metadata)', () => { const names = result.map((r) => r.name).sort(); assert.deepStrictEqual(names, ['requests<3', 'rich']); result.forEach((item) => { - assert.strictEqual(item.group, 'Inline metadata'); + assert.strictEqual(item.group, 'PEP 723'); assert.deepStrictEqual(item.args, [item.name]); assert.ok(item.uri && item.uri.toString() === scriptUri.toString()); }); From 962911a36fd7e8ebcd6ca6d869b8c574449adb83 Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Wed, 13 May 2026 10:37:23 -0700 Subject: [PATCH 6/9] Address PR #1 review feedback - inlineScriptMetadata: switch BLOCK_RE consumption to text.matchAll() so the global regex's lastIndex is never shared/mutated across calls. - inlineScriptLazyDetector: fix save-event coalescing bug by re-enqueuing a fresh read when a save races with an in-flight open (the previous cache may be stale). - inlineScriptLazyDetector: add a disposed flag and guard processOnce against post-disposal project registration. - inlineScriptDetector (creator): use uri.toString() for URI equality to match InlineScriptLazyDetector and avoid Windows drive-letter / trailing-separator divergence. - inlineScriptDetector (creator): replace showErrorMessage with showInformationMessage for the four "no scripts found" toasts (informational, not an error). - tests: replace the open+save coalescing test with two tests covering open-dedup and save-re-read separately; add a dispose-during-in-flight-read test; switch detector tests to stub showInformationMessage. --- src/common/inlineScriptMetadata.ts | 30 +++--- src/features/creators/inlineScriptDetector.ts | 34 ++++--- src/features/inlineScriptLazyDetector.ts | 58 ++++++++++- .../inlineScriptDetector.unit.test.ts | 24 +++-- .../inlineScriptLazyDetector.unit.test.ts | 97 +++++++++++++++++-- 5 files changed, 196 insertions(+), 47 deletions(-) diff --git a/src/common/inlineScriptMetadata.ts b/src/common/inlineScriptMetadata.ts index a66071d7..b2c7629b 100644 --- a/src/common/inlineScriptMetadata.ts +++ b/src/common/inlineScriptMetadata.ts @@ -42,13 +42,18 @@ export const MAX_HEADER_BYTES = 8 * 1024; * Canonical block regex from the PEP 723 spec, translated to JavaScript * (Python's `(?P...)` becomes `(?...)` in JS). The flag * combination `gm` is required so `^` / `$` anchor on line boundaries - * and so the engine can iterate multiple candidate blocks. + * and so `String.prototype.matchAll` can iterate every candidate block. * * Important: this regex assumes line endings have already been * normalized to `\n`. In Python's `re` module `.` matches `\r`, but in * JavaScript it does not, so a literal CRLF file would behave * inconsistently against this pattern. `readInlineScriptMetadata` * normalizes line endings before applying the regex. + * + * The pattern is consumed exclusively via `text.matchAll(BLOCK_RE)`, + * which constructs a fresh iterator each call and does NOT mutate the + * regex's `lastIndex`. Do not call `BLOCK_RE.exec` directly — that + * would reintroduce the stateful-lastIndex footgun. */ const BLOCK_RE = /^# \/\/\/ (?[a-zA-Z0-9-]+)$\s(?(^#(| .*)$\s)+)^# \/\/\/$/gm; @@ -84,19 +89,18 @@ export function readInlineScriptMetadata(scriptText: string): InlineScriptMetada // Collect ALL matches first so we can detect the "multiple script // blocks" error case the spec requires us to surface. - BLOCK_RE.lastIndex = 0; - const scriptMatches: RegExpExecArray[] = []; - let m: RegExpExecArray | null; - while ((m = BLOCK_RE.exec(text)) !== null) { + // + // `matchAll` constructs a fresh iterator and does not mutate the + // shared `BLOCK_RE.lastIndex`, so this loop is re-entrant and safe + // even if a caller (or an exception) ever interrupts a previous + // pass. + const scriptMatches: RegExpMatchArray[] = []; + for (const m of text.matchAll(BLOCK_RE)) { // Per spec, tools MUST NOT read non-standardized block types. // The only standardized type today is `script`. if (m.groups?.type === 'script') { scriptMatches.push(m); } - // Defensive: zero-width match would cause an infinite loop. - if (m.index === BLOCK_RE.lastIndex) { - BLOCK_RE.lastIndex += 1; - } } if (scriptMatches.length === 0) { @@ -112,6 +116,10 @@ export function readInlineScriptMetadata(scriptText: string): InlineScriptMetada const match = scriptMatches[0]; const rawContent = match.groups!.content; + // `index` is always populated for matches produced by + // `matchAll(regex)` when `regex` has the `g` flag, but the + // TypeScript lib type still marks it optional. Pin it locally. + const matchStart = match.index!; // Validate each content line and reconstruct the TOML payload, // applying the spec's content-extraction rule: @@ -197,7 +205,7 @@ export function readInlineScriptMetadata(scriptText: string): InlineScriptMetada // Range end: position immediately AFTER the closing `# ///` line's // newline. The regex's `$` anchor stops before the newline, so we // step over it explicitly when present. - let end = match.index + match[0].length; + let end = matchStart + match[0].length; if (text.charAt(end) === '\n') { end += 1; } @@ -206,7 +214,7 @@ export function readInlineScriptMetadata(scriptText: string): InlineScriptMetada requiresPython, dependencies, tool, - range: { start: match.index, end }, + range: { start: matchStart, end }, }; } diff --git a/src/features/creators/inlineScriptDetector.ts b/src/features/creators/inlineScriptDetector.ts index fdc5acce..28ace530 100644 --- a/src/features/creators/inlineScriptDetector.ts +++ b/src/features/creators/inlineScriptDetector.ts @@ -11,7 +11,7 @@ import { } from '../../common/inlineScriptMetadata'; import { InlineScriptStrings } from '../../common/localize'; import { traceInfo, traceVerbose } from '../../common/logging'; -import { showErrorMessage, showQuickPickWithButtons } from '../../common/window.apis'; +import { showInformationMessage, showQuickPickWithButtons } from '../../common/window.apis'; import { findFiles, getWorkspaceFolders } from '../../common/workspace.apis'; import { PythonProjectManager, PythonProjectsImpl } from '../../internal.api'; @@ -65,7 +65,7 @@ export class InlineScriptDetector implements PythonProjectCreator { // discovered candidates happens further down. if (!isAnyFolderEnabled()) { setImmediate(() => { - showErrorMessage(InlineScriptStrings.noScriptsFound); + showInformationMessage(InlineScriptStrings.noScriptsFound); }); return undefined; } @@ -73,20 +73,26 @@ export class InlineScriptDetector implements PythonProjectCreator { const candidates = await findFiles(SCRIPT_INCLUDE, SCRIPT_EXCLUDE, MAX_SCRIPTS_TO_SCAN); if (!candidates || candidates.length === 0) { setImmediate(() => { - showErrorMessage(InlineScriptStrings.noScriptsFound); + showInformationMessage(InlineScriptStrings.noScriptsFound); }); return undefined; } // Filter out: - // (a) candidates inside a workspace folder where the user has - // disabled the feature (resource-scoped setting), and - // (b) scripts that are already registered as a project with - // the exact same URI. Folder-scoped projects that happen - // to contain this script are intentionally NOT filtered - // out: a `.py` file with inline metadata is its own - // project and sits alongside its enclosing folder - // project. + // (a) candidates inside a workspace folder where the user has + // disabled the feature (resource-scoped setting), and + // (b) scripts that are already registered as a project with + // the exact same URI. Folder-scoped projects that happen + // to contain this script are intentionally NOT filtered + // out: a `.py` file with inline metadata is its own + // project and sits alongside its enclosing folder + // project. + // + // URI identity uses `uri.toString()` to match the equality + // check used by `InlineScriptLazyDetector` — keeping the two + // detectors agreed on what "the same project" means avoids + // Windows drive-letter / trailing-separator divergence that a + // raw `path.normalize` comparison can produce. const fresh = candidates.filter((uri) => { if (!isInlineScriptMetadataEnabled(uri)) { return false; @@ -95,7 +101,7 @@ export class InlineScriptDetector implements PythonProjectCreator { if (!existing) { return true; } - return path.normalize(existing.uri.fsPath) !== path.normalize(uri.fsPath); + return existing.uri.toString() !== uri.toString(); }); if (fresh.length === 0) { traceInfo( @@ -103,7 +109,7 @@ export class InlineScriptDetector implements PythonProjectCreator { `(disabled folders or already-registered).`, ); setImmediate(() => { - showErrorMessage(InlineScriptStrings.noScriptsFound); + showInformationMessage(InlineScriptStrings.noScriptsFound); }); return undefined; } @@ -115,7 +121,7 @@ export class InlineScriptDetector implements PythonProjectCreator { if (withMetadata.length === 0) { traceInfo(`InlineScriptDetector: scanned ${fresh.length} .py files, none declared inline metadata.`); setImmediate(() => { - showErrorMessage(InlineScriptStrings.noScriptsFound); + showInformationMessage(InlineScriptStrings.noScriptsFound); }); return undefined; } diff --git a/src/features/inlineScriptLazyDetector.ts b/src/features/inlineScriptLazyDetector.ts index 1582db19..024921a5 100644 --- a/src/features/inlineScriptLazyDetector.ts +++ b/src/features/inlineScriptLazyDetector.ts @@ -46,6 +46,11 @@ export class InlineScriptLazyDetector implements Disposable { // In-flight reads keyed by `uri.toString()` so rapid open+save // doesn't double-process the same file. private readonly inFlight = new Map>(); + // Flips to `true` in `dispose()`. Guards async continuations + // inside `processOnce` so an in-flight read that completes after + // disposal does not register a project on a detector the host + // has already torn down. + private disposed = false; constructor(private readonly projectManager: PythonProjectManager) {} @@ -125,6 +130,7 @@ export class InlineScriptLazyDetector implements Disposable { } public dispose(): void { + this.disposed = true; this.subscriptions.forEach((s) => s.dispose()); this.subscriptions.length = 0; this.inFlight.clear(); @@ -159,10 +165,46 @@ export class InlineScriptLazyDetector implements Disposable { const existing = this.inFlight.get(key); if (existing) { // A previous open/save is still in flight for the same - // URI. Wait for it and skip; that read's result is - // authoritative. - await existing; - return; + // URI. For an `open` trigger the previous read's result is + // authoritative — the file hasn't changed since it was + // opened — so we coalesce and bail. + // + // A `save` trigger means the file content may have + // changed since the in-flight read started reading, so the + // cached metadata could already be stale. Wait for the + // in-flight read to settle, then re-enqueue a fresh read + // to pick up the new content. This is safe even if the + // in-flight work itself was a save: the re-enqueued read + // will simply observe the latest on-disk content. + if (trigger !== 'save') { + await existing; + return; + } + try { + await existing; + } catch { + // The in-flight work owns its own error handling. We + // only awaited it to serialize against it; failures + // are not our concern here. + } + if (this.disposed) { + return; + } + // Re-check the gate in case the URI is no longer eligible + // (the workspace folder was removed, the setting was + // toggled off, etc.) between the original event and now. + if (!shouldHandleUri(uri) || !isInlineScriptMetadataEnabled(uri)) { + return; + } + // Fall through to enqueue a fresh read below. The previous + // in-flight entry has already been removed by its own + // `finally`. + if (this.inFlight.has(key)) { + // A third event raced in and is already handling the + // refresh. Defer to it. + await this.inFlight.get(key); + return; + } } const work = this.processOnce(uri, trigger).finally(() => { this.inFlight.delete(key); @@ -183,6 +225,14 @@ export class InlineScriptLazyDetector implements Disposable { return; } + // The detector may have been disposed while the read was + // outstanding. Bail before touching `projectManager` so we + // never register (or mutate) a project on a torn-down + // detector. + if (this.disposed) { + return; + } + // `projectManager.get()` does CONTAINMENT matching — for a // script inside a workspace folder it returns the folder // project, not undefined. That is the wrong answer here: a diff --git a/src/test/features/creators/inlineScriptDetector.unit.test.ts b/src/test/features/creators/inlineScriptDetector.unit.test.ts index 0101e7f7..49adeb37 100644 --- a/src/test/features/creators/inlineScriptDetector.unit.test.ts +++ b/src/test/features/creators/inlineScriptDetector.unit.test.ts @@ -29,7 +29,7 @@ const META_B: ism.InlineScriptMetadata = { suite('InlineScriptDetector (creator)', () => { let findFilesStub: sinon.SinonStub; let getWorkspaceFoldersStub: sinon.SinonStub; - let showErrorStub: sinon.SinonStub; + let showInfoStub: sinon.SinonStub; let showQuickPickStub: sinon.SinonStub; let readMetadataStub: sinon.SinonStub; let isEnabledStub: sinon.SinonStub; @@ -44,8 +44,12 @@ suite('InlineScriptDetector (creator)', () => { // override this. getWorkspaceFoldersStub.returns(undefined); - showErrorStub = sinon.stub(winapi, 'showErrorMessage'); - showErrorStub.resolves(undefined); + // The "no scripts found" message is informational — the scan + // worked, it just produced an empty result — so the creator + // uses `showInformationMessage`. Stubbing it lets tests assert + // the toast is surfaced without involving real VS Code UI. + showInfoStub = sinon.stub(winapi, 'showInformationMessage'); + showInfoStub.resolves(undefined); showQuickPickStub = sinon.stub(winapi, 'showQuickPickWithButtons'); showQuickPickStub.resolves(undefined); @@ -70,14 +74,14 @@ suite('InlineScriptDetector (creator)', () => { return new Promise((resolve) => setImmediate(resolve)); } - test('returns undefined and shows error when feature is disabled', async () => { + test('returns undefined and shows info toast when feature is disabled', async () => { isEnabledStub.returns(false); const detector = new InlineScriptDetector(pm.object); const result = await detector.create(); await waitForImmediate(); assert.strictEqual(result, undefined); assert.ok(findFilesStub.notCalled, 'should not scan when feature is disabled'); - assert.ok(showErrorStub.calledOnce, 'should show "no scripts found" error'); + assert.ok(showInfoStub.calledOnce, 'should show "no scripts found" info toast'); }); test('returns undefined when findFiles returns no candidates', async () => { @@ -86,7 +90,7 @@ suite('InlineScriptDetector (creator)', () => { const result = await detector.create(); await waitForImmediate(); assert.strictEqual(result, undefined); - assert.ok(showErrorStub.calledOnce); + assert.ok(showInfoStub.calledOnce); }); test('returns undefined when every candidate is already registered with the same URI', async () => { @@ -101,7 +105,7 @@ suite('InlineScriptDetector (creator)', () => { await waitForImmediate(); assert.strictEqual(result, undefined); assert.ok(readMetadataStub.notCalled, 'should not read metadata when nothing is fresh'); - assert.ok(showErrorStub.calledOnce); + assert.ok(showInfoStub.calledOnce); }); test('keeps candidate when only a folder project (different URI) contains it', async () => { @@ -122,7 +126,7 @@ suite('InlineScriptDetector (creator)', () => { assert.ok(showQuickPickStub.calledOnce, 'should present picker'); }); - test('returns undefined and shows error when no candidate has metadata', async () => { + test('returns undefined and shows info toast when no candidate has metadata', async () => { const uri = Uri.file(path.resolve('/ws/plain.py')); findFilesStub.resolves([uri]); readMetadataStub.resolves(undefined); @@ -132,7 +136,7 @@ suite('InlineScriptDetector (creator)', () => { await waitForImmediate(); assert.strictEqual(result, undefined); assert.ok(showQuickPickStub.notCalled, 'should not show picker when no metadata found'); - assert.ok(showErrorStub.calledOnce); + assert.ok(showInfoStub.calledOnce); }); test('returns undefined when the user cancels the picker', async () => { @@ -211,7 +215,7 @@ suite('InlineScriptDetector (creator)', () => { await waitForImmediate(); assert.strictEqual(result, undefined); assert.ok(findFilesStub.notCalled, 'should not scan when no folder has the feature enabled'); - assert.ok(showErrorStub.calledOnce); + assert.ok(showInfoStub.calledOnce); }); test('multi-root: scans when feature is enabled in at least one folder, filters candidates by per-folder setting', async () => { diff --git a/src/test/features/inlineScriptLazyDetector.unit.test.ts b/src/test/features/inlineScriptLazyDetector.unit.test.ts index 415f922a..5ed7e71b 100644 --- a/src/test/features/inlineScriptLazyDetector.unit.test.ts +++ b/src/test/features/inlineScriptLazyDetector.unit.test.ts @@ -238,25 +238,106 @@ suite('InlineScriptLazyDetector', () => { detector.dispose(); }); - test('coalesces concurrent open + save for the same URI', async () => { + test('coalesces concurrent open + save: open is deduped, save forces a re-read', async () => { const uri = Uri.file(path.resolve('/ws/race.py')); - // First call resolves with metadata, second would too if it - // were made — but we expect coalescing to avoid the second - // call entirely. + // First call resolves with the \"open-time\" metadata, second call + // (triggered by the save) resolves with the \"save-time\" + // metadata. The save MUST trigger a fresh read after the open + // completes \u2014 dropping it would cache stale data when the + // user edited the file between the open and save events. + const openTime: ism.InlineScriptMetadata = { + requiresPython: '>=3.10', + dependencies: ['old'], + tool: undefined, + range: { start: 0, end: 20 }, + }; + const saveTime: ism.InlineScriptMetadata = { + requiresPython: '>=3.12', + dependencies: ['new'], + tool: undefined, + range: { start: 0, end: 24 }, + }; + readMetadataStub.onFirstCall().resolves(openTime); + readMetadataStub.onSecondCall().resolves(saveTime); + + // After the first add(), the second pass should see the + // project already exists and refresh metadata on it instead + // of calling add() again. Capture the project that gets added + // so we can assert its metadata reflects the SAVE read. + let added: PythonProjectsImpl | undefined; + projectManager.reset(); + projectManager + .setup((pm) => pm.get(typmoq.It.isAny())) + .returns(() => added) + .verifiable(typmoq.Times.atLeastOnce()); + projectManager + .setup((pm) => pm.add(typmoq.It.isAny())) + .callback((arg: PythonProject | PythonProject[]) => { + added = (Array.isArray(arg) ? arg[0] : arg) as PythonProjectsImpl; + }) + .returns(() => Promise.resolve()); + + const detector = new InlineScriptLazyDetector(projectManager.object); + detector.activate(); + await Promise.all([fireOpen(uri), fireSave(uri)]); + + assert.strictEqual( + readMetadataStub.callCount, + 2, + `expected two reads (one per event, save re-reads), got ${readMetadataStub.callCount}`, + ); + projectManager.verify((pm) => pm.add(typmoq.It.isAny()), typmoq.Times.once()); + assert.ok(added, 'open-side processOnce should have registered the project'); + assert.deepStrictEqual( + added!.inlineScriptMetadata, + saveTime, + 'cached metadata must reflect the SAVE-time read, not the OPEN-time read', + ); + detector.dispose(); + }); + + test('concurrent open + open coalesces to a single read', async () => { + const uri = Uri.file(path.resolve('/ws/dedup.py')); readMetadataStub.resolves(VALID_METADATA); projectManager.setup((pm) => pm.get(typmoq.It.isAny())).returns(() => undefined); const detector = new InlineScriptLazyDetector(projectManager.object); detector.activate(); - await Promise.all([fireOpen(uri), fireSave(uri)]); + await Promise.all([fireOpen(uri), fireOpen(uri)]); - // The read may still be invoked once for the leading event; - // the second is coalesced. - assert.ok(readMetadataStub.callCount === 1, `expected exactly one read, got ${readMetadataStub.callCount}`); + // Two opens for the same URI \u2014 the second waits on the first + // and does not re-read (the file content cannot have changed + // between two opens). + assert.strictEqual(readMetadataStub.callCount, 1, 'open+open should coalesce to a single read'); projectManager.verify((pm) => pm.add(typmoq.It.isAny()), typmoq.Times.once()); detector.dispose(); }); + test('dispose() during an in-flight read prevents post-disposal project registration', async () => { + const uri = Uri.file(path.resolve('/ws/disposed.py')); + let resolveRead: ((meta: ism.InlineScriptMetadata) => void) | undefined; + readMetadataStub.returns( + new Promise((resolve) => { + resolveRead = resolve; + }), + ); + projectManager.setup((pm) => pm.get(typmoq.It.isAny())).returns(() => undefined); + + const detector = new InlineScriptLazyDetector(projectManager.object); + detector.activate(); + // Kick off the open without awaiting it; the read is parked + // on our manual resolver above. + const inFlight = openListener!(makeDoc(uri)) as Promise | undefined; + // Tear the detector down BEFORE the read settles. + detector.dispose(); + // Now let the in-flight read complete with metadata. Without + // the disposed guard this would call `projectManager.add()` + // on a torn-down detector. + resolveRead!(VALID_METADATA); + await inFlight; + projectManager.verify((pm) => pm.add(typmoq.It.isAny()), typmoq.Times.never()); + }); + // ---------- B1 / B2: catch-up replay over `getOpenTextDocuments` ---------- // Drain the microtask queue and the next `setImmediate` slot so From 6efa16f10eb4efdbeef4dd889bfac3bca61b78b3 Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Thu, 28 May 2026 14:42:41 -0700 Subject: [PATCH 7/9] Slim PEP 723 surface to silent observer for telemetry Strip user-facing PEP 723 inline-script-metadata wiring while keeping the lazy detector wired up as the planned telemetry ingest point. - Remove the python-envs.useInlineScriptMetadata setting from package.json + package.nls.json. - Delete the bulk-scan InlineScriptDetector project creator and its unit test. - Drop the PEP 723 dependency-extraction branch from pipUtils.getProjectInstallable and its unit test. - Remove isInlineScriptMetadataEnabled + setting constants from common/inlineScriptMetadata; keep the parser. - Remove the inlineScriptMetadata field from PythonProjectsImpl. - Remove the InlineScriptStrings localization namespace. - Slim InlineScriptLazyDetector to a no-arg observer with a TODO(pep723-telemetry) marker; rewrite its unit tests for the new shape. - Re-enable the lazy detector in extension.ts with an updated comment describing the telemetry-observer role. --- package.json | 9 - package.nls.json | 3 +- src/common/inlineScriptMetadata.ts | 37 +- src/common/localize.ts | 10 - src/extension.ts | 13 +- src/features/creators/inlineScriptDetector.ts | 223 ------------ src/features/inlineScriptLazyDetector.ts | 210 +++--------- src/internal.api.ts | 13 - src/managers/builtin/pipUtils.ts | 42 +-- .../inlineScriptDetector.unit.test.ts | 309 ----------------- .../inlineScriptLazyDetector.unit.test.ts | 324 ++---------------- .../pipUtils.inlineScript.unit.test.ts | 146 -------- 12 files changed, 85 insertions(+), 1254 deletions(-) delete mode 100644 src/features/creators/inlineScriptDetector.ts delete mode 100644 src/test/features/creators/inlineScriptDetector.unit.test.ts delete mode 100644 src/test/managers/builtin/pipUtils.inlineScript.unit.test.ts diff --git a/package.json b/package.json index 4d0893e9..0156334b 100644 --- a/package.json +++ b/package.json @@ -138,15 +138,6 @@ "description": "%python-envs.alwaysUseUv.description%", "default": true, "scope": "machine" - }, - "python-envs.useInlineScriptMetadata": { - "type": "boolean", - "markdownDescription": "%python-envs.useInlineScriptMetadata.description%", - "default": false, - "scope": "resource", - "tags": [ - "experimental" - ] } } }, diff --git a/package.nls.json b/package.nls.json index 8f576971..3a4ddcec 100644 --- a/package.nls.json +++ b/package.nls.json @@ -45,6 +45,5 @@ "python-envs.revealProjectInExplorer.title": "Reveal Project in Explorer", "python-envs.revealEnvInManagerView.title": "Reveal in Environment Managers View", "python-envs.runPetInTerminal.title": "Run Python Environment Tool (PET) in Terminal...", - "python-envs.alwaysUseUv.description": "When set to true, uv will be used to manage all virtual environments if available. When set to false, uv will only manage virtual environments explicitly created by uv.", - "python-envs.useInlineScriptMetadata.description": "Use inline script metadata (PEP 723) to manage environments for single-file Python scripts. When enabled, scripts with a `# /// script` block at the top are detected as projects, environments are created honoring `requires-python` and `dependencies`, and the extension watches for metadata changes. Disabled by default while the feature is in preview." + "python-envs.alwaysUseUv.description": "When set to true, uv will be used to manage all virtual environments if available. When set to false, uv will only manage virtual environments explicitly created by uv." } diff --git a/src/common/inlineScriptMetadata.ts b/src/common/inlineScriptMetadata.ts index b2c7629b..5082eddf 100644 --- a/src/common/inlineScriptMetadata.ts +++ b/src/common/inlineScriptMetadata.ts @@ -3,9 +3,8 @@ import * as tomljs from '@iarna/toml'; import * as fs from 'fs/promises'; -import { ConfigurationScope, Uri } from 'vscode'; +import { Uri } from 'vscode'; import { traceVerbose, traceWarn } from './logging'; -import { getConfiguration } from './workspace.apis'; /** * Parsed and validated PEP 723 `script` metadata block. @@ -405,37 +404,3 @@ function compareReleases(a: readonly number[], b: readonly number[]): number { } return 0; } - -/** - * Configuration section and key for the single user-facing setting that - * gates the inline-script-metadata feature. - */ -const SETTING_SECTION = 'python-envs'; -const SETTING_KEY = 'useInlineScriptMetadata'; - -/** - * Fully-qualified setting key (`section.key`) for the experimental - * inline-script-metadata gate. Exported so other modules (notably - * `InlineScriptLazyDetector`) can filter `onDidChangeConfiguration` - * events against the same identifier the setting is registered under - * — keeping the listener and the accessor below in lockstep if the - * setting is ever renamed. - */ -export const INLINE_SCRIPT_METADATA_SETTING = `${SETTING_SECTION}.${SETTING_KEY}`; - -/** - * Returns `true` when the inline-script-metadata feature is enabled - * for the given scope. The setting is `resource`-scoped, so callers - * SHOULD pass a `Uri` (typically the script's URI or a workspace - * folder URI) to get a workspace-folder-aware view of the - * configuration. Passing `undefined` falls back to the workspace / - * user value with no folder context. - * - * Every consumer of inline-script-metadata behavior — detection, env - * creation, watcher — MUST gate its work through this helper so that - * disabling the setting (globally or in a specific folder) makes the - * feature invisible for that scope. - */ -export function isInlineScriptMetadataEnabled(scope?: ConfigurationScope): boolean { - return getConfiguration(SETTING_SECTION, scope).get(SETTING_KEY, false); -} diff --git a/src/common/localize.ts b/src/common/localize.ts index 64f3b8a4..d3ce5c10 100644 --- a/src/common/localize.ts +++ b/src/common/localize.ts @@ -212,16 +212,6 @@ export namespace ProjectCreatorString { export const noProjectsFound = l10n.t('No projects found'); } -export namespace InlineScriptStrings { - export const detectorDisplayName = l10n.t('Find Inline-Metadata Scripts'); - export const detectorDescription = l10n.t( - 'Find single-file Python scripts that declare their dependencies inline (PEP 723).', - ); - export const selectScripts = l10n.t('Select inline-metadata scripts to add as projects'); - export const noScriptsFound = l10n.t('No Python scripts with inline metadata were found in this workspace.'); - export const installableDescription = l10n.t('Inline script dependency'); -} - export namespace EnvViewStrings { export const selectedGlobalTooltip = l10n.t('This environment is selected for non-workspace files'); export const selectedWorkspaceTooltip = l10n.t('This environment is selected for project files'); diff --git a/src/extension.ts b/src/extension.ts index d11f5b33..41c71e95 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -39,7 +39,6 @@ import { getConfiguration, getWorkspaceFolders } from './common/workspace.apis'; import { createManagerReady } from './features/common/managerReady'; import { AutoFindProjects } from './features/creators/autoFindProjects'; import { ExistingProjects } from './features/creators/existingProjects'; -import { InlineScriptDetector } from './features/creators/inlineScriptDetector'; import { NewPackageProject } from './features/creators/newPackageProject'; import { NewScriptProject } from './features/creators/newScriptProject'; import { ProjectCreatorsImpl } from './features/creators/projectCreators'; @@ -205,15 +204,13 @@ export async function activate(context: ExtensionContext): Promise { - // The feature is gated behind the experimental setting, which - // is `resource`-scoped — so in a multi-root workspace it may - // be on in some folders and off in others. Bail early only if - // it is off everywhere (no folder has it enabled, and the - // window-level fallback is also off). Per-folder filtering of - // discovered candidates happens further down. - if (!isAnyFolderEnabled()) { - setImmediate(() => { - showInformationMessage(InlineScriptStrings.noScriptsFound); - }); - return undefined; - } - - const candidates = await findFiles(SCRIPT_INCLUDE, SCRIPT_EXCLUDE, MAX_SCRIPTS_TO_SCAN); - if (!candidates || candidates.length === 0) { - setImmediate(() => { - showInformationMessage(InlineScriptStrings.noScriptsFound); - }); - return undefined; - } - - // Filter out: - // (a) candidates inside a workspace folder where the user has - // disabled the feature (resource-scoped setting), and - // (b) scripts that are already registered as a project with - // the exact same URI. Folder-scoped projects that happen - // to contain this script are intentionally NOT filtered - // out: a `.py` file with inline metadata is its own - // project and sits alongside its enclosing folder - // project. - // - // URI identity uses `uri.toString()` to match the equality - // check used by `InlineScriptLazyDetector` — keeping the two - // detectors agreed on what "the same project" means avoids - // Windows drive-letter / trailing-separator divergence that a - // raw `path.normalize` comparison can produce. - const fresh = candidates.filter((uri) => { - if (!isInlineScriptMetadataEnabled(uri)) { - return false; - } - const existing = this.pm.get(uri); - if (!existing) { - return true; - } - return existing.uri.toString() !== uri.toString(); - }); - if (fresh.length === 0) { - traceInfo( - `InlineScriptDetector: no fresh candidates after filtering ${candidates.length} .py file(s) ` + - `(disabled folders or already-registered).`, - ); - setImmediate(() => { - showInformationMessage(InlineScriptStrings.noScriptsFound); - }); - return undefined; - } - - // Read headers in bounded parallel batches. Each read is at - // most `MAX_HEADER_BYTES` (8 KiB) so the total I/O at the cap - // (500 files × 8 KiB ≈ 4 MiB) is small. - const withMetadata = await scanForInlineScripts(fresh); - if (withMetadata.length === 0) { - traceInfo(`InlineScriptDetector: scanned ${fresh.length} .py files, none declared inline metadata.`); - setImmediate(() => { - showInformationMessage(InlineScriptStrings.noScriptsFound); - }); - return undefined; - } - - const items = withMetadata - .map(({ uri }) => ({ - label: path.basename(uri.fsPath), - description: uri.fsPath, - uri, - })) - .sort((a, b) => a.label.localeCompare(b.label)); - - const selected = await showQuickPickWithButtons(items, { - canPickMany: true, - ignoreFocusOut: true, - placeHolder: InlineScriptStrings.selectScripts, - showBackButton: true, - }); - - let chosen: typeof items; - if (Array.isArray(selected)) { - chosen = selected as typeof items; - } else if (selected) { - chosen = [selected as (typeof items)[number]]; - } else { - // User cancelled the picker. - return undefined; - } - if (chosen.length === 0) { - return undefined; - } - - // Re-associate each chosen URI back to its metadata so we can - // cache it on the project. We do this rather than re-reading - // the file so a save between scan and pick can't change what - // the user just confirmed. - const metadataByUri = new Map(withMetadata.map((r) => [r.uri.toString(), r.metadata])); - const projects: PythonProject[] = chosen.map((c) => { - const proj = new PythonProjectsImpl(path.basename(c.uri.fsPath), c.uri); - proj.inlineScriptMetadata = metadataByUri.get(c.uri.toString()); - return proj; - }); - await this.pm.add(projects); - return projects; - } -} - -/** - * Returns `true` when the inline-script-metadata feature is enabled - * in at least one open workspace folder (or, if no folders are open, - * at the window / user level). The setting is `resource`-scoped, so a - * window-level read alone would miss folder-only opt-ins in a - * multi-root workspace. - */ -function isAnyFolderEnabled(): boolean { - const folders = getWorkspaceFolders() ?? []; - if (folders.length === 0) { - return isInlineScriptMetadataEnabled(); - } - return folders.some((f) => isInlineScriptMetadataEnabled(f.uri)); -} - -/** - * Read inline script metadata from each URI in `uris` with bounded - * parallelism, returning only the URIs whose `.py` file declared a - * well-formed `# /// script` block. Exported for unit-test use. - */ -export async function scanForInlineScripts( - uris: readonly Uri[], -): Promise> { - const results: Array<{ uri: Uri; metadata: InlineScriptMetadata }> = []; - let cursor = 0; - - const worker = async (): Promise => { - for (;;) { - const index = cursor; - cursor += 1; - if (index >= uris.length) { - return; - } - const uri = uris[index]; - try { - const metadata = await readInlineScriptMetadataFromFile(uri); - if (metadata !== undefined) { - results.push({ uri, metadata }); - } - } catch (err) { - traceVerbose(`InlineScriptDetector: failed to read ${uri.fsPath}:`, err); - } - } - }; - - const workers: Promise[] = []; - for (let i = 0; i < Math.min(SCAN_CONCURRENCY, uris.length); i++) { - workers.push(worker()); - } - await Promise.all(workers); - return results; -} diff --git a/src/features/inlineScriptLazyDetector.ts b/src/features/inlineScriptLazyDetector.ts index 024921a5..7023a7d7 100644 --- a/src/features/inlineScriptLazyDetector.ts +++ b/src/features/inlineScriptLazyDetector.ts @@ -3,43 +3,30 @@ import * as path from 'path'; import { Disposable, TextDocument, Uri } from 'vscode'; -import { - INLINE_SCRIPT_METADATA_SETTING, - InlineScriptMetadata, - isInlineScriptMetadataEnabled, - readInlineScriptMetadataFromFile, -} from '../common/inlineScriptMetadata'; -import { traceInfo, traceVerbose, traceWarn } from '../common/logging'; +import { readInlineScriptMetadataFromFile } from '../common/inlineScriptMetadata'; +import { traceVerbose, traceWarn } from '../common/logging'; import { getOpenTextDocuments, getWorkspaceFolder, - onDidChangeConfiguration, onDidOpenTextDocument, onDidSaveTextDocument, } from '../common/workspace.apis'; -import { PythonProjectManager, PythonProjectsImpl } from '../internal.api'; - -/** - * Fully-qualified configuration key for the experimental setting that - * gates the inline-script-metadata feature. Re-exported from - * `../common/inlineScriptMetadata` so the `onDidChangeConfiguration` - * filter and the accessor `isInlineScriptMetadataEnabled` are - * guaranteed to refer to the same setting. - */ -const SETTING_FQN = INLINE_SCRIPT_METADATA_SETTING; /** - * Lazy on-open / on-save detector for `.py` files that declare inline - * script metadata (PEP 723). This is the PRIMARY detection path: when - * the user opens or saves a `.py` script, we read the head of the - * file, parse any `# /// script` block, and register the script as a - * Python project so its environment and dependencies surface in the - * rest of the extension. + * Silent on-open / on-save detector for `.py` files that declare + * inline script metadata (PEP 723). The detector is intentionally + * observer-only: it parses the head of every eligible `.py` file the + * user opens or saves, but does not surface any UI, register + * projects, or otherwise change extension behavior. + * + * It is kept wired up so we have a single ingest point for PEP 723 + * telemetry. The TODO marker inside `processOnce` is the planned + * emission site; until the telemetry events are wired up the detector + * is effectively dead code that runs a cheap parse and discards the + * result. * * Detection is cheap (≤ 8 KiB read + regex + TOML parse) and runs - * only on files the user has already shown intent in. Workspace-wide - * scanning is the responsibility of the opt-in creator in - * `src/features/creators/inlineScriptDetector.ts`. + * only on files the user has already shown intent in. */ export class InlineScriptLazyDetector implements Disposable { private readonly subscriptions: Disposable[] = []; @@ -48,17 +35,13 @@ export class InlineScriptLazyDetector implements Disposable { private readonly inFlight = new Map>(); // Flips to `true` in `dispose()`. Guards async continuations // inside `processOnce` so an in-flight read that completes after - // disposal does not register a project on a detector the host - // has already torn down. + // disposal does not emit telemetry on a detector the host has + // already torn down. private disposed = false; - constructor(private readonly projectManager: PythonProjectManager) {} - /** * Subscribe to workspace text-document events. Safe to call once - * during extension activation. The detector starts working - * immediately; the experimental gate is re-checked on every event - * so toggling the setting takes effect without a reload. + * during extension activation. * * Listeners return the promise from `handleDocument` rather than * void-ing it. VS Code's event bus does not await listener @@ -80,18 +63,6 @@ export class InlineScriptLazyDetector implements Disposable { this.subscriptions.push( onDidOpenTextDocument((doc) => this.handleDocument(doc, 'open')), onDidSaveTextDocument((doc) => this.handleDocument(doc, 'save')), - // When the user toggles `python-envs.useInlineScriptMetadata` - // we replay the catch-up pass so already-open `.py` files - // get inspected without requiring a window reload or a - // manual save. The per-event gate inside `handleDocument` - // makes the off→still-off and on→still-on cases free; the - // useful case is off→on, where we discover scripts the - // user has had open all along. - onDidChangeConfiguration((e) => { - if (e.affectsConfiguration(SETTING_FQN)) { - this.replayOpenDocuments('config-change'); - } - }), ); // Defer the catch-up pass so we observe `workspace.textDocuments` // AFTER VS Code finishes registering the document that triggered @@ -104,11 +75,10 @@ export class InlineScriptLazyDetector implements Disposable { /** * Walk every currently-open text document and run it through * `handleDocument` as if a synthetic `open` event had fired. Used - * both for the deferred activation catch-up and for the - * setting-toggle replay. The per-URI dedup in `handleDocument` - * keeps this safe to call repeatedly. + * for the deferred activation catch-up. The per-URI dedup in + * `handleDocument` keeps this safe to call repeatedly. */ - private replayOpenDocuments(source: 'activate' | 'config-change'): void { + private replayOpenDocuments(source: 'activate'): void { // Restrict the replay to documents that the per-event handler // would actually look at. This keeps the activation log // proportional to the work the detector will do — on an @@ -142,8 +112,7 @@ export class InlineScriptLazyDetector implements Disposable { // is high-frequency (fires on every keystroke-triggered save // and on every editor open) so it stays at `traceVerbose` — // the `Trace` log level — to avoid flooding the default - // `Info` channel. First-time project registration is logged - // at `traceInfo` further down. + // `Info` channel. traceVerbose(`inlineScriptLazyDetector: event received (${trigger}) ${uri.toString()}`); if (!shouldHandleUri(uri)) { traceVerbose( @@ -153,58 +122,15 @@ export class InlineScriptLazyDetector implements Disposable { ); return; } - if (!isInlineScriptMetadataEnabled(uri)) { - traceVerbose( - `inlineScriptLazyDetector: skipped (${trigger}) ${uri.fsPath} ` + - `(setting 'python-envs.useInlineScriptMetadata' is false)`, - ); - return; - } - traceVerbose(`inlineScriptLazyDetector: processing (${trigger}) ${uri.fsPath}`); const key = uri.toString(); const existing = this.inFlight.get(key); if (existing) { - // A previous open/save is still in flight for the same - // URI. For an `open` trigger the previous read's result is - // authoritative — the file hasn't changed since it was - // opened — so we coalesce and bail. - // - // A `save` trigger means the file content may have - // changed since the in-flight read started reading, so the - // cached metadata could already be stale. Wait for the - // in-flight read to settle, then re-enqueue a fresh read - // to pick up the new content. This is safe even if the - // in-flight work itself was a save: the re-enqueued read - // will simply observe the latest on-disk content. - if (trigger !== 'save') { - await existing; - return; - } - try { - await existing; - } catch { - // The in-flight work owns its own error handling. We - // only awaited it to serialize against it; failures - // are not our concern here. - } - if (this.disposed) { - return; - } - // Re-check the gate in case the URI is no longer eligible - // (the workspace folder was removed, the setting was - // toggled off, etc.) between the original event and now. - if (!shouldHandleUri(uri) || !isInlineScriptMetadataEnabled(uri)) { - return; - } - // Fall through to enqueue a fresh read below. The previous - // in-flight entry has already been removed by its own - // `finally`. - if (this.inFlight.has(key)) { - // A third event raced in and is already handling the - // refresh. Defer to it. - await this.inFlight.get(key); - return; - } + // Coalesce repeated open/save events for the same URI. + // We only parse for observation (telemetry), so the most + // recent in-flight read is good enough; there is no + // cached state downstream that could go stale. + await existing; + return; } const work = this.processOnce(uri, trigger).finally(() => { this.inFlight.delete(key); @@ -214,76 +140,27 @@ export class InlineScriptLazyDetector implements Disposable { } private async processOnce(uri: Uri, trigger: 'open' | 'save'): Promise { - let metadata: InlineScriptMetadata | undefined; try { - metadata = await readInlineScriptMetadataFromFile(uri); + const metadata = await readInlineScriptMetadataFromFile(uri); + if (this.disposed) { + return; + } + if (metadata !== undefined) { + traceVerbose( + `inlineScriptLazyDetector: detected inline script metadata in ${uri.fsPath} (${trigger})`, + ); + // TODO(pep723-telemetry): emit a PEP 723 detection + // event here (e.g. `pep723.detected`) with + // anonymized fields such as `trigger`, presence of + // `requires-python`, and dependency count. This is + // the planned emission site the detector is being + // kept alive for. + } } catch (err) { // `readInlineScriptMetadataFromFile` already swallows I/O // errors internally. This catch is a defensive net for // unexpected synchronous throws (e.g. malformed URI). traceWarn(`inlineScriptLazyDetector: unexpected error while reading ${uri.fsPath}:`, err); - return; - } - - // The detector may have been disposed while the read was - // outstanding. Bail before touching `projectManager` so we - // never register (or mutate) a project on a torn-down - // detector. - if (this.disposed) { - return; - } - - // `projectManager.get()` does CONTAINMENT matching — for a - // script inside a workspace folder it returns the folder - // project, not undefined. That is the wrong answer here: a - // script project is distinct from the folder that contains - // it. Filter the result down to an exact-URI match before - // deciding whether this file is already registered. - const candidate = this.projectManager.get(uri); - const existing = candidate !== undefined && candidate.uri.toString() === uri.toString() ? candidate : undefined; - - if (metadata === undefined) { - // No (valid) block in the file. If it was previously - // registered as a script project we keep it — the user - // explicitly added it once, and yanking the project on a - // passing edit would be surprising. We only clear the - // cached metadata so downstream consumers don't act on - // stale data. - traceVerbose(`inlineScriptLazyDetector: no metadata block in ${uri.fsPath} (${trigger})`); - if (existing instanceof PythonProjectsImpl && existing.inlineScriptMetadata !== undefined) { - existing.inlineScriptMetadata = undefined; - traceVerbose( - `inlineScriptLazyDetector: cleared cached metadata for ${uri.fsPath} (${trigger}: no block)`, - ); - } - return; - } - - if (existing instanceof PythonProjectsImpl) { - // Already a project — just refresh the cached metadata - // (it may have changed on save; downstream code, e.g. - // `getProjectInstallable`, is also free to re-read). - existing.inlineScriptMetadata = metadata; - traceVerbose( - `inlineScriptLazyDetector: refreshed metadata for ${uri.fsPath} (${trigger}: already a project)`, - ); - return; - } - - if (existing !== undefined) { - // The URI is somehow already registered with a different - // `PythonProject` implementation. Don't replace it. - traceVerbose(`inlineScriptLazyDetector: ${uri.fsPath} is already a project (non-impl); skipping.`); - return; - } - - const project = new PythonProjectsImpl(path.basename(uri.fsPath), uri); - project.inlineScriptMetadata = metadata; - try { - await this.projectManager.add(project); - traceInfo(`inlineScriptLazyDetector: registered ${uri.fsPath} as a project (${trigger})`); - } catch (err) { - traceWarn(`inlineScriptLazyDetector: failed to register ${uri.fsPath} as a project:`, err); } } } @@ -292,8 +169,7 @@ export class InlineScriptLazyDetector implements Disposable { * Cheap, side-effect-free gate for which URIs the lazy detector * should look at. Filters out non-file schemes, non-`.py` * extensions, and files that are not inside an open workspace - * folder. Exported for test access and for re-use by the opt-in - * workspace-scan creator. + * folder. Exported for test access. */ export function shouldHandleUri(uri: Uri): boolean { if (uri.scheme !== 'file') { diff --git a/src/internal.api.ts b/src/internal.api.ts index a72a305d..3d896c0c 100644 --- a/src/internal.api.ts +++ b/src/internal.api.ts @@ -29,7 +29,6 @@ import { } from './api'; import { ISSUES_URL } from './common/constants'; import { CreateEnvironmentNotSupported, RemoveEnvironmentNotSupported } from './common/errors/NotSupportedError'; -import { InlineScriptMetadata } from './common/inlineScriptMetadata'; import { traceWarn } from './common/logging'; import { StopWatch } from './common/stopWatch'; import { EventNames } from './common/telemetry/constants'; @@ -467,18 +466,6 @@ export class PythonProjectsImpl implements PythonProject { tooltip?: string | MarkdownString; iconPath?: IconPath; - /** - * Parsed inline script metadata (PEP 723) for `.py` script - * projects, cached on the project after the lazy detector has - * read it. `undefined` for folder projects and for `.py` files - * that do not declare a `# /// script` block. - * - * Internal-only: not exposed on the public `PythonProject` - * interface. Consumers access it via an explicit `instanceof - * PythonProjectsImpl` guard so the cast stays in one place. - */ - inlineScriptMetadata?: InlineScriptMetadata; - constructor( name: string, uri: Uri, diff --git a/src/managers/builtin/pipUtils.ts b/src/managers/builtin/pipUtils.ts index 450d8431..2e7a7f85 100644 --- a/src/managers/builtin/pipUtils.ts +++ b/src/managers/builtin/pipUtils.ts @@ -4,8 +4,7 @@ import * as path from 'path'; import { l10n, LogOutputChannel, ProgressLocation, QuickInputButtons, QuickPickItem, Uri, window } from 'vscode'; import { PackageManagementOptions, PythonEnvironment, PythonEnvironmentApi, PythonProject } from '../../api'; import { EXTENSION_ROOT_DIR } from '../../common/constants'; -import { isInlineScriptMetadataEnabled, readInlineScriptMetadataFromFile } from '../../common/inlineScriptMetadata'; -import { InlineScriptStrings, PackageManagement, Pickers, VenvManagerStrings } from '../../common/localize'; +import { PackageManagement, Pickers, VenvManagerStrings } from '../../common/localize'; import { traceInfo } from '../../common/logging'; import { showQuickPickWithButtons, withProgress } from '../../common/window.apis'; import { findFiles } from '../../common/workspace.apis'; @@ -349,45 +348,6 @@ export async function getProjectInstallable( } }), ); - - // Inline-script-metadata (PEP 723) branch: any project whose - // `uri` is a `.py` file may carry inline dependency - // declarations at the top of the file. We surface those - // declared deps as `Installable` entries grouped under - // 'PEP 723' so they appear in the same pre-install picker - // as `pyproject.toml` and `requirements*.txt` - // dependencies. This branch is gated by the experimental - // setting and only runs for projects whose root URI is - // itself a `.py` file — folder projects are never walked - // for `.py` files (that would be an unbounded scan). - await Promise.all( - projects.map(async (project) => { - if (project.uri.scheme !== 'file') { - return; - } - if (path.extname(project.uri.fsPath).toLowerCase() !== '.py') { - return; - } - if (!isInlineScriptMetadataEnabled(project.uri)) { - return; - } - const metadata = await readInlineScriptMetadataFromFile(project.uri); - const deps = metadata?.dependencies; - if (!deps || deps.length === 0) { - return; - } - for (const dep of deps) { - installable.push({ - name: dep, - displayName: dep, - group: 'PEP 723', - args: [dep], - description: InlineScriptStrings.installableDescription, - uri: project.uri, - }); - } - }), - ); }, ); diff --git a/src/test/features/creators/inlineScriptDetector.unit.test.ts b/src/test/features/creators/inlineScriptDetector.unit.test.ts deleted file mode 100644 index 49adeb37..00000000 --- a/src/test/features/creators/inlineScriptDetector.unit.test.ts +++ /dev/null @@ -1,309 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -import assert from 'assert'; -import * as path from 'path'; -import * as sinon from 'sinon'; -import * as typmoq from 'typemoq'; -import { Uri } from 'vscode'; -import { PythonProject } from '../../../api'; -import * as ism from '../../../common/inlineScriptMetadata'; -import * as winapi from '../../../common/window.apis'; -import * as wapi from '../../../common/workspace.apis'; -import { InlineScriptDetector, scanForInlineScripts } from '../../../features/creators/inlineScriptDetector'; -import { PythonProjectManager, PythonProjectsImpl } from '../../../internal.api'; - -const META_A: ism.InlineScriptMetadata = { - requiresPython: '>=3.11', - dependencies: ['requests'], - tool: undefined, - range: { start: 0, end: 40 }, -}; -const META_B: ism.InlineScriptMetadata = { - requiresPython: '>=3.10', - dependencies: ['rich'], - tool: undefined, - range: { start: 0, end: 40 }, -}; - -suite('InlineScriptDetector (creator)', () => { - let findFilesStub: sinon.SinonStub; - let getWorkspaceFoldersStub: sinon.SinonStub; - let showInfoStub: sinon.SinonStub; - let showQuickPickStub: sinon.SinonStub; - let readMetadataStub: sinon.SinonStub; - let isEnabledStub: sinon.SinonStub; - let pm: typmoq.IMock; - - setup(() => { - findFilesStub = sinon.stub(wapi, 'findFiles'); - findFilesStub.resolves([]); - - getWorkspaceFoldersStub = sinon.stub(wapi, 'getWorkspaceFolders'); - // Default: no workspace folders open. Multi-root tests - // override this. - getWorkspaceFoldersStub.returns(undefined); - - // The "no scripts found" message is informational — the scan - // worked, it just produced an empty result — so the creator - // uses `showInformationMessage`. Stubbing it lets tests assert - // the toast is surfaced without involving real VS Code UI. - showInfoStub = sinon.stub(winapi, 'showInformationMessage'); - showInfoStub.resolves(undefined); - - showQuickPickStub = sinon.stub(winapi, 'showQuickPickWithButtons'); - showQuickPickStub.resolves(undefined); - - readMetadataStub = sinon.stub(ism, 'readInlineScriptMetadataFromFile'); - readMetadataStub.resolves(undefined); - - isEnabledStub = sinon.stub(ism, 'isInlineScriptMetadataEnabled'); - isEnabledStub.returns(true); - - pm = typmoq.Mock.ofType(); - pm.setup((p) => p.get(typmoq.It.isAny())).returns(() => undefined); - pm.setup((p) => p.add(typmoq.It.isAny())).returns(() => Promise.resolve()); - }); - - teardown(() => { - sinon.restore(); - }); - - // Helper to wait for the `setImmediate`-scheduled error toast. - function waitForImmediate(): Promise { - return new Promise((resolve) => setImmediate(resolve)); - } - - test('returns undefined and shows info toast when feature is disabled', async () => { - isEnabledStub.returns(false); - const detector = new InlineScriptDetector(pm.object); - const result = await detector.create(); - await waitForImmediate(); - assert.strictEqual(result, undefined); - assert.ok(findFilesStub.notCalled, 'should not scan when feature is disabled'); - assert.ok(showInfoStub.calledOnce, 'should show "no scripts found" info toast'); - }); - - test('returns undefined when findFiles returns no candidates', async () => { - findFilesStub.resolves([]); - const detector = new InlineScriptDetector(pm.object); - const result = await detector.create(); - await waitForImmediate(); - assert.strictEqual(result, undefined); - assert.ok(showInfoStub.calledOnce); - }); - - test('returns undefined when every candidate is already registered with the same URI', async () => { - const uri = Uri.file(path.resolve('/ws/a.py')); - findFilesStub.resolves([uri]); - pm.reset(); - pm.setup((p) => p.get(typmoq.It.isAny())).returns(() => ({ name: 'a.py', uri })); - pm.setup((p) => p.add(typmoq.It.isAny())).returns(() => Promise.resolve()); - - const detector = new InlineScriptDetector(pm.object); - const result = await detector.create(); - await waitForImmediate(); - assert.strictEqual(result, undefined); - assert.ok(readMetadataStub.notCalled, 'should not read metadata when nothing is fresh'); - assert.ok(showInfoStub.calledOnce); - }); - - test('keeps candidate when only a folder project (different URI) contains it', async () => { - const scriptUri = Uri.file(path.resolve('/ws/a.py')); - const folderUri = Uri.file(path.resolve('/ws')); - findFilesStub.resolves([scriptUri]); - pm.reset(); - pm.setup((p) => p.get(typmoq.It.isAny())).returns(() => ({ name: 'ws', uri: folderUri })); - pm.setup((p) => p.add(typmoq.It.isAny())).returns(() => Promise.resolve()); - readMetadataStub.resolves(META_A); - // User cancels picker → undefined, but the scan should have occurred. - showQuickPickStub.resolves(undefined); - - const detector = new InlineScriptDetector(pm.object); - const result = await detector.create(); - assert.strictEqual(result, undefined); - assert.ok(readMetadataStub.calledOnce, 'should still scan the candidate'); - assert.ok(showQuickPickStub.calledOnce, 'should present picker'); - }); - - test('returns undefined and shows info toast when no candidate has metadata', async () => { - const uri = Uri.file(path.resolve('/ws/plain.py')); - findFilesStub.resolves([uri]); - readMetadataStub.resolves(undefined); - - const detector = new InlineScriptDetector(pm.object); - const result = await detector.create(); - await waitForImmediate(); - assert.strictEqual(result, undefined); - assert.ok(showQuickPickStub.notCalled, 'should not show picker when no metadata found'); - assert.ok(showInfoStub.calledOnce); - }); - - test('returns undefined when the user cancels the picker', async () => { - const uri = Uri.file(path.resolve('/ws/a.py')); - findFilesStub.resolves([uri]); - readMetadataStub.resolves(META_A); - showQuickPickStub.resolves(undefined); - - const detector = new InlineScriptDetector(pm.object); - const result = await detector.create(); - assert.strictEqual(result, undefined); - pm.verify((p) => p.add(typmoq.It.isAny()), typmoq.Times.never()); - }); - - test('registers chosen projects with cached metadata', async () => { - const uriA = Uri.file(path.resolve('/ws/a.py')); - const uriB = Uri.file(path.resolve('/ws/b.py')); - findFilesStub.resolves([uriA, uriB]); - readMetadataStub.withArgs(sinon.match((u: Uri) => u.toString() === uriA.toString())).resolves(META_A); - readMetadataStub.withArgs(sinon.match((u: Uri) => u.toString() === uriB.toString())).resolves(META_B); - - // Simulate user picking BOTH. - showQuickPickStub.callsFake(async (items: Array<{ label: string; uri: Uri }>) => items); - - let captured: PythonProject | PythonProject[] | undefined; - pm.reset(); - pm.setup((p) => p.get(typmoq.It.isAny())).returns(() => undefined); - pm.setup((p) => p.add(typmoq.It.isAny())) - .callback((arg: PythonProject | PythonProject[]) => { - captured = arg; - }) - .returns(() => Promise.resolve()); - - const detector = new InlineScriptDetector(pm.object); - const result = await detector.create(); - - assert.ok(result && result.length === 2, 'should return both projects'); - const sorted = [...result!].sort((a, b) => a.uri.fsPath.localeCompare(b.uri.fsPath)); - const projA = sorted[0] as PythonProjectsImpl; - const projB = sorted[1] as PythonProjectsImpl; - assert.ok(projA instanceof PythonProjectsImpl); - assert.ok(projB instanceof PythonProjectsImpl); - assert.deepStrictEqual(projA.inlineScriptMetadata, META_A); - assert.deepStrictEqual(projB.inlineScriptMetadata, META_B); - pm.verify((p) => p.add(typmoq.It.isAny()), typmoq.Times.once()); - assert.ok(Array.isArray(captured), 'pm.add should receive an array of projects'); - assert.strictEqual((captured as PythonProject[]).length, 2); - }); - - test('handles single-item picker return (non-array) correctly', async () => { - const uri = Uri.file(path.resolve('/ws/only.py')); - findFilesStub.resolves([uri]); - readMetadataStub.resolves(META_A); - // Some VS Code wrappers return a single item rather than an - // array even with `canPickMany: true`. Be tolerant. - showQuickPickStub.callsFake(async (items: Array<{ label: string; uri: Uri }>) => items[0]); - - const detector = new InlineScriptDetector(pm.object); - const result = await detector.create(); - assert.ok(result && result.length === 1); - pm.verify((p) => p.add(typmoq.It.isAny()), typmoq.Times.once()); - }); - - test('multi-root: bails early when feature is disabled in every open folder', async () => { - const folderA = Uri.file(path.resolve('/wsA')); - const folderB = Uri.file(path.resolve('/wsB')); - getWorkspaceFoldersStub.returns([ - { uri: folderA, name: 'wsA', index: 0 }, - { uri: folderB, name: 'wsB', index: 1 }, - ]); - // Disabled in every folder AND at the no-scope level. - isEnabledStub.returns(false); - - const detector = new InlineScriptDetector(pm.object); - const result = await detector.create(); - await waitForImmediate(); - assert.strictEqual(result, undefined); - assert.ok(findFilesStub.notCalled, 'should not scan when no folder has the feature enabled'); - assert.ok(showInfoStub.calledOnce); - }); - - test('multi-root: scans when feature is enabled in at least one folder, filters candidates by per-folder setting', async () => { - const folderA = Uri.file(path.resolve('/wsA')); - const folderB = Uri.file(path.resolve('/wsB')); - const scriptA = Uri.file(path.resolve('/wsA/a.py')); - const scriptB = Uri.file(path.resolve('/wsB/b.py')); - getWorkspaceFoldersStub.returns([ - { uri: folderA, name: 'wsA', index: 0 }, - { uri: folderB, name: 'wsB', index: 1 }, - ]); - - // Per-folder setting: enabled for wsA, disabled for wsB and - // the no-scope read (window-level). - isEnabledStub.callsFake((scope?: Uri) => { - if (!scope) { - return false; - } - return scope.fsPath.startsWith(folderA.fsPath); - }); - - // findFiles returns one candidate from each folder. - findFilesStub.resolves([scriptA, scriptB]); - readMetadataStub.withArgs(sinon.match((u: Uri) => u.toString() === scriptA.toString())).resolves(META_A); - readMetadataStub.withArgs(sinon.match((u: Uri) => u.toString() === scriptB.toString())).resolves(META_B); - - // Capture what the picker is shown. - let pickerItems: Array<{ uri: Uri }> = []; - showQuickPickStub.callsFake(async (items: Array<{ label: string; uri: Uri }>) => { - pickerItems = items; - return items; - }); - - const detector = new InlineScriptDetector(pm.object); - const result = await detector.create(); - - assert.ok(result && result.length === 1, 'only the enabled-folder script should be offered'); - assert.strictEqual(result![0].uri.toString(), scriptA.toString()); - assert.strictEqual(pickerItems.length, 1, 'picker should only contain the enabled-folder script'); - assert.strictEqual(pickerItems[0].uri.toString(), scriptA.toString()); - // The disabled-folder candidate must not have been read. - assert.ok( - readMetadataStub.neverCalledWith(sinon.match((u: Uri) => u.toString() === scriptB.toString())), - 'should not read metadata for files in disabled folders', - ); - }); -}); - -suite('scanForInlineScripts', () => { - let readMetadataStub: sinon.SinonStub; - - setup(() => { - readMetadataStub = sinon.stub(ism, 'readInlineScriptMetadataFromFile'); - }); - - teardown(() => { - sinon.restore(); - }); - - test('returns only URIs whose metadata is defined', async () => { - const uriA = Uri.file(path.resolve('/ws/a.py')); - const uriB = Uri.file(path.resolve('/ws/b.py')); - const uriC = Uri.file(path.resolve('/ws/c.py')); - readMetadataStub.withArgs(sinon.match((u: Uri) => u.toString() === uriA.toString())).resolves(META_A); - readMetadataStub.withArgs(sinon.match((u: Uri) => u.toString() === uriB.toString())).resolves(undefined); - readMetadataStub.withArgs(sinon.match((u: Uri) => u.toString() === uriC.toString())).resolves(META_B); - - const results = await scanForInlineScripts([uriA, uriB, uriC]); - const got = new Map(results.map((r) => [r.uri.toString(), r.metadata])); - assert.strictEqual(got.size, 2); - assert.strictEqual(got.get(uriA.toString()), META_A); - assert.strictEqual(got.get(uriC.toString()), META_B); - }); - - test('swallows per-file read errors and continues', async () => { - const uriA = Uri.file(path.resolve('/ws/a.py')); - const uriB = Uri.file(path.resolve('/ws/b.py')); - readMetadataStub.withArgs(sinon.match((u: Uri) => u.toString() === uriA.toString())).rejects(new Error('boom')); - readMetadataStub.withArgs(sinon.match((u: Uri) => u.toString() === uriB.toString())).resolves(META_A); - - const results = await scanForInlineScripts([uriA, uriB]); - assert.strictEqual(results.length, 1); - assert.strictEqual(results[0].uri.toString(), uriB.toString()); - }); - - test('handles an empty input list', async () => { - const results = await scanForInlineScripts([]); - assert.deepStrictEqual(results, []); - assert.ok(readMetadataStub.notCalled); - }); -}); diff --git a/src/test/features/inlineScriptLazyDetector.unit.test.ts b/src/test/features/inlineScriptLazyDetector.unit.test.ts index 5ed7e71b..ea6938c0 100644 --- a/src/test/features/inlineScriptLazyDetector.unit.test.ts +++ b/src/test/features/inlineScriptLazyDetector.unit.test.ts @@ -4,13 +4,10 @@ import assert from 'assert'; import * as path from 'path'; import * as sinon from 'sinon'; -import * as typmoq from 'typemoq'; -import { ConfigurationChangeEvent, Disposable, TextDocument, Uri } from 'vscode'; -import { PythonProject } from '../../api'; +import { Disposable, TextDocument, Uri } from 'vscode'; import * as ism from '../../common/inlineScriptMetadata'; import * as wapi from '../../common/workspace.apis'; import { InlineScriptLazyDetector, shouldHandleUri } from '../../features/inlineScriptLazyDetector'; -import { PythonProjectManager, PythonProjectsImpl } from '../../internal.api'; // Build a minimal TextDocument stub. Only the `uri` field is read by // the detector; the rest exists to satisfy the type. @@ -28,20 +25,15 @@ const VALID_METADATA: ism.InlineScriptMetadata = { suite('InlineScriptLazyDetector', () => { let onDidOpenStub: sinon.SinonStub; let onDidSaveStub: sinon.SinonStub; - let onDidChangeConfigStub: sinon.SinonStub; let getOpenTextDocumentsStub: sinon.SinonStub; let getWorkspaceFolderStub: sinon.SinonStub; let readMetadataStub: sinon.SinonStub; - let isEnabledStub: sinon.SinonStub; let openListener: ((doc: TextDocument) => unknown) | undefined; let saveListener: ((doc: TextDocument) => unknown) | undefined; - let configListener: ((e: ConfigurationChangeEvent) => unknown) | undefined; - let projectManager: typmoq.IMock; setup(() => { openListener = undefined; saveListener = undefined; - configListener = undefined; onDidOpenStub = sinon.stub(wapi, 'onDidOpenTextDocument'); onDidOpenStub.callsFake((listener: (doc: TextDocument) => unknown) => { @@ -59,14 +51,6 @@ suite('InlineScriptLazyDetector', () => { }); }); - onDidChangeConfigStub = sinon.stub(wapi, 'onDidChangeConfiguration'); - onDidChangeConfigStub.callsFake((listener: (e: ConfigurationChangeEvent) => unknown) => { - configListener = listener; - return new Disposable(() => { - configListener = undefined; - }); - }); - // Default to an empty list of open documents. Tests that // exercise the catch-up replay override this. getOpenTextDocumentsStub = sinon.stub(wapi, 'getOpenTextDocuments'); @@ -84,12 +68,6 @@ suite('InlineScriptLazyDetector', () => { readMetadataStub = sinon.stub(ism, 'readInlineScriptMetadataFromFile'); readMetadataStub.resolves(undefined); - - isEnabledStub = sinon.stub(ism, 'isInlineScriptMetadataEnabled'); - isEnabledStub.returns(true); - - projectManager = typmoq.Mock.ofType(); - projectManager.setup((pm) => pm.add(typmoq.It.isAny())).returns(() => Promise.resolve()); }); teardown(() => { @@ -107,7 +85,7 @@ suite('InlineScriptLazyDetector', () => { } test('activate() subscribes to onDidOpen and onDidSave', () => { - const detector = new InlineScriptLazyDetector(projectManager.object); + const detector = new InlineScriptLazyDetector(); detector.activate(); assert.ok(onDidOpenStub.calledOnce, 'should subscribe to onDidOpenTextDocument'); assert.ok(onDidSaveStub.calledOnce, 'should subscribe to onDidSaveTextDocument'); @@ -115,7 +93,7 @@ suite('InlineScriptLazyDetector', () => { }); test('skips non-file URI schemes', async () => { - const detector = new InlineScriptLazyDetector(projectManager.object); + const detector = new InlineScriptLazyDetector(); detector.activate(); await fireOpen(Uri.parse('untitled:foo.py')); assert.ok(readMetadataStub.notCalled, 'should not read metadata for non-file URI'); @@ -123,7 +101,7 @@ suite('InlineScriptLazyDetector', () => { }); test('skips non-.py files', async () => { - const detector = new InlineScriptLazyDetector(projectManager.object); + const detector = new InlineScriptLazyDetector(); detector.activate(); await fireOpen(Uri.file(path.resolve('/ws/foo.txt'))); assert.ok(readMetadataStub.notCalled, 'should not read metadata for non-.py files'); @@ -132,188 +110,58 @@ suite('InlineScriptLazyDetector', () => { test('skips files outside any workspace folder', async () => { getWorkspaceFolderStub.returns(undefined); - const detector = new InlineScriptLazyDetector(projectManager.object); + const detector = new InlineScriptLazyDetector(); detector.activate(); await fireOpen(Uri.file(path.resolve('/elsewhere/foo.py'))); assert.ok(readMetadataStub.notCalled, 'should not read metadata for out-of-workspace files'); detector.dispose(); }); - test('no-op when feature is disabled', async () => { - isEnabledStub.returns(false); - const detector = new InlineScriptLazyDetector(projectManager.object); - detector.activate(); - await fireOpen(Uri.file(path.resolve('/ws/foo.py'))); - assert.ok(readMetadataStub.notCalled, 'should not read metadata when setting is disabled'); - detector.dispose(); - }); - - test('registers a new project when a .py file with metadata is opened', async () => { + test('reads metadata for an in-workspace .py file on open', async () => { const uri = Uri.file(path.resolve('/ws/foo.py')); readMetadataStub.resolves(VALID_METADATA); - projectManager.reset(); - projectManager.setup((pm) => pm.get(typmoq.It.isAny())).returns(() => undefined); - let captured: PythonProject | PythonProject[] | undefined; - projectManager - .setup((pm) => pm.add(typmoq.It.isAny())) - .callback((arg: PythonProject | PythonProject[]) => { - captured = arg; - }) - .returns(() => Promise.resolve()); - - const detector = new InlineScriptLazyDetector(projectManager.object); + const detector = new InlineScriptLazyDetector(); detector.activate(); await fireOpen(uri); - - projectManager.verify((pm) => pm.add(typmoq.It.isAny()), typmoq.Times.once()); - assert.ok(captured, 'pm.add should have been called with a project'); - assert.ok(!Array.isArray(captured), 'expected a single project, not an array'); - const project = captured as PythonProjectsImpl; - assert.ok(project instanceof PythonProjectsImpl, 'project should be a PythonProjectsImpl'); - assert.strictEqual(project.uri.toString(), uri.toString()); - assert.deepStrictEqual(project.inlineScriptMetadata, VALID_METADATA); + assert.strictEqual(readMetadataStub.callCount, 1, 'open should trigger exactly one read'); + assert.strictEqual((readMetadataStub.firstCall.args[0] as Uri).toString(), uri.toString()); detector.dispose(); }); - test('save event also registers a new project', async () => { + test('reads metadata for an in-workspace .py file on save', async () => { const uri = Uri.file(path.resolve('/ws/bar.py')); readMetadataStub.resolves(VALID_METADATA); - projectManager.setup((pm) => pm.get(typmoq.It.isAny())).returns(() => undefined); - - const detector = new InlineScriptLazyDetector(projectManager.object); + const detector = new InlineScriptLazyDetector(); detector.activate(); await fireSave(uri); - - projectManager.verify((pm) => pm.add(typmoq.It.isAny()), typmoq.Times.once()); - detector.dispose(); - }); - - test('does not register a project when there is no metadata', async () => { - readMetadataStub.resolves(undefined); - projectManager.setup((pm) => pm.get(typmoq.It.isAny())).returns(() => undefined); - - const detector = new InlineScriptLazyDetector(projectManager.object); - detector.activate(); - await fireOpen(Uri.file(path.resolve('/ws/plain.py'))); - - projectManager.verify((pm) => pm.add(typmoq.It.isAny()), typmoq.Times.never()); + assert.strictEqual(readMetadataStub.callCount, 1, 'save should trigger exactly one read'); detector.dispose(); }); - test('refreshes metadata on save when the project is already registered', async () => { - const uri = Uri.file(path.resolve('/ws/already.py')); - const existing = new PythonProjectsImpl('already.py', uri); - existing.inlineScriptMetadata = { - requiresPython: '>=3.10', - dependencies: ['old'], - tool: undefined, - range: { start: 0, end: 20 }, - }; + test('concurrent open + open coalesces to a single read', async () => { + const uri = Uri.file(path.resolve('/ws/dedup.py')); readMetadataStub.resolves(VALID_METADATA); - projectManager.setup((pm) => pm.get(typmoq.It.isAny())).returns(() => existing); - - const detector = new InlineScriptLazyDetector(projectManager.object); + const detector = new InlineScriptLazyDetector(); detector.activate(); - await fireSave(uri); - - // No add() call: the project is already registered. - projectManager.verify((pm) => pm.add(typmoq.It.isAny()), typmoq.Times.never()); - assert.deepStrictEqual(existing.inlineScriptMetadata, VALID_METADATA, 'metadata should be refreshed'); - detector.dispose(); - }); - - test('clears cached metadata when a save removes the block from a known project', async () => { - const uri = Uri.file(path.resolve('/ws/wasScript.py')); - const existing = new PythonProjectsImpl('wasScript.py', uri); - existing.inlineScriptMetadata = VALID_METADATA; - readMetadataStub.resolves(undefined); - projectManager.setup((pm) => pm.get(typmoq.It.isAny())).returns(() => existing); - - const detector = new InlineScriptLazyDetector(projectManager.object); - detector.activate(); - await fireSave(uri); - - projectManager.verify((pm) => pm.add(typmoq.It.isAny()), typmoq.Times.never()); - assert.strictEqual(existing.inlineScriptMetadata, undefined, 'metadata cache should be cleared'); + await Promise.all([fireOpen(uri), fireOpen(uri)]); + assert.strictEqual(readMetadataStub.callCount, 1, 'open+open should coalesce to a single read'); detector.dispose(); }); - test('coalesces concurrent open + save: open is deduped, save forces a re-read', async () => { + test('concurrent open + save coalesces to a single read', async () => { const uri = Uri.file(path.resolve('/ws/race.py')); - // First call resolves with the \"open-time\" metadata, second call - // (triggered by the save) resolves with the \"save-time\" - // metadata. The save MUST trigger a fresh read after the open - // completes \u2014 dropping it would cache stale data when the - // user edited the file between the open and save events. - const openTime: ism.InlineScriptMetadata = { - requiresPython: '>=3.10', - dependencies: ['old'], - tool: undefined, - range: { start: 0, end: 20 }, - }; - const saveTime: ism.InlineScriptMetadata = { - requiresPython: '>=3.12', - dependencies: ['new'], - tool: undefined, - range: { start: 0, end: 24 }, - }; - readMetadataStub.onFirstCall().resolves(openTime); - readMetadataStub.onSecondCall().resolves(saveTime); - - // After the first add(), the second pass should see the - // project already exists and refresh metadata on it instead - // of calling add() again. Capture the project that gets added - // so we can assert its metadata reflects the SAVE read. - let added: PythonProjectsImpl | undefined; - projectManager.reset(); - projectManager - .setup((pm) => pm.get(typmoq.It.isAny())) - .returns(() => added) - .verifiable(typmoq.Times.atLeastOnce()); - projectManager - .setup((pm) => pm.add(typmoq.It.isAny())) - .callback((arg: PythonProject | PythonProject[]) => { - added = (Array.isArray(arg) ? arg[0] : arg) as PythonProjectsImpl; - }) - .returns(() => Promise.resolve()); - - const detector = new InlineScriptLazyDetector(projectManager.object); - detector.activate(); - await Promise.all([fireOpen(uri), fireSave(uri)]); - - assert.strictEqual( - readMetadataStub.callCount, - 2, - `expected two reads (one per event, save re-reads), got ${readMetadataStub.callCount}`, - ); - projectManager.verify((pm) => pm.add(typmoq.It.isAny()), typmoq.Times.once()); - assert.ok(added, 'open-side processOnce should have registered the project'); - assert.deepStrictEqual( - added!.inlineScriptMetadata, - saveTime, - 'cached metadata must reflect the SAVE-time read, not the OPEN-time read', - ); - detector.dispose(); - }); - - test('concurrent open + open coalesces to a single read', async () => { - const uri = Uri.file(path.resolve('/ws/dedup.py')); readMetadataStub.resolves(VALID_METADATA); - projectManager.setup((pm) => pm.get(typmoq.It.isAny())).returns(() => undefined); - - const detector = new InlineScriptLazyDetector(projectManager.object); + const detector = new InlineScriptLazyDetector(); detector.activate(); - await Promise.all([fireOpen(uri), fireOpen(uri)]); - - // Two opens for the same URI \u2014 the second waits on the first - // and does not re-read (the file content cannot have changed - // between two opens). - assert.strictEqual(readMetadataStub.callCount, 1, 'open+open should coalesce to a single read'); - projectManager.verify((pm) => pm.add(typmoq.It.isAny()), typmoq.Times.once()); + await Promise.all([fireOpen(uri), fireSave(uri)]); + // The slim observer has no cached state to keep fresh, so + // simple URI-level dedup is sufficient: a save concurrent + // with an in-flight open coalesces with it. + assert.strictEqual(readMetadataStub.callCount, 1, 'concurrent open+save should coalesce to a single read'); detector.dispose(); }); - test('dispose() during an in-flight read prevents post-disposal project registration', async () => { + test('dispose() during an in-flight read bails out before logging detection', async () => { const uri = Uri.file(path.resolve('/ws/disposed.py')); let resolveRead: ((meta: ism.InlineScriptMetadata) => void) | undefined; readMetadataStub.returns( @@ -321,24 +169,23 @@ suite('InlineScriptLazyDetector', () => { resolveRead = resolve; }), ); - projectManager.setup((pm) => pm.get(typmoq.It.isAny())).returns(() => undefined); - const detector = new InlineScriptLazyDetector(projectManager.object); + const detector = new InlineScriptLazyDetector(); detector.activate(); // Kick off the open without awaiting it; the read is parked // on our manual resolver above. const inFlight = openListener!(makeDoc(uri)) as Promise | undefined; // Tear the detector down BEFORE the read settles. detector.dispose(); - // Now let the in-flight read complete with metadata. Without - // the disposed guard this would call `projectManager.add()` - // on a torn-down detector. + // Now let the in-flight read complete with metadata. The + // `disposed` guard inside processOnce should prevent any + // further work (in the future, this guard also protects the + // telemetry emission site from firing after disposal). resolveRead!(VALID_METADATA); - await inFlight; - projectManager.verify((pm) => pm.add(typmoq.It.isAny()), typmoq.Times.never()); + await assert.doesNotReject(inFlight ?? Promise.resolve()); }); - // ---------- B1 / B2: catch-up replay over `getOpenTextDocuments` ---------- + // ---------- catch-up replay over `getOpenTextDocuments` ---------- // Drain the microtask queue and the next `setImmediate` slot so // the deferred catch-up replay can run before assertions. @@ -353,10 +200,9 @@ suite('InlineScriptLazyDetector', () => { readMetadataStub.callsFake(async (u: Uri) => u.toString() === uriWithMeta.toString() ? VALID_METADATA : undefined, ); - projectManager.setup((pm) => pm.get(typmoq.It.isAny())).returns(() => undefined); getOpenTextDocumentsStub.returns([makeDoc(uriWithMeta), makeDoc(uriPlain), makeDoc(uriNonPy)]); - const detector = new InlineScriptLazyDetector(projectManager.object); + const detector = new InlineScriptLazyDetector(); detector.activate(); // Wait for the deferred catch-up. await flushImmediate(); @@ -370,120 +216,18 @@ suite('InlineScriptLazyDetector', () => { assert.ok(readUris.includes(uriWithMeta.toString())); assert.ok(readUris.includes(uriPlain.toString())); assert.ok(!readUris.includes(uriNonPy.toString()), 'should not read non-.py URI during replay'); - projectManager.verify((pm) => pm.add(typmoq.It.isAny()), typmoq.Times.once()); detector.dispose(); }); test('dispose() cancels the pending catch-up replay', async () => { getOpenTextDocumentsStub.returns([makeDoc(Uri.file(path.resolve('/ws/never.py')))]); - const detector = new InlineScriptLazyDetector(projectManager.object); + const detector = new InlineScriptLazyDetector(); detector.activate(); // Tear down BEFORE the `setImmediate` slot fires. detector.dispose(); await flushImmediate(); assert.ok(readMetadataStub.notCalled, 'dispose() must clear the pending setImmediate handle'); }); - - // ---------- B3: URI registered with a non-impl project ---------- - - test('skips refresh when URI is registered with a non-PythonProjectsImpl project', async () => { - const uri = Uri.file(path.resolve('/ws/foreign.py')); - // A `PythonProject` that is NOT a `PythonProjectsImpl`. This - // mirrors the (rare) case where a third-party manager has - // registered the same URI under its own concrete class. - const foreign: PythonProject = { name: 'foreign.py', uri }; - readMetadataStub.resolves(VALID_METADATA); - projectManager.setup((pm) => pm.get(typmoq.It.isAny())).returns(() => foreign); - - const detector = new InlineScriptLazyDetector(projectManager.object); - detector.activate(); - await fireOpen(uri); - - projectManager.verify((pm) => pm.add(typmoq.It.isAny()), typmoq.Times.never()); - detector.dispose(); - }); - - // ---------- B6: multi-root per-folder gating ---------- - - test('respects per-folder setting scope when the feature is enabled in some folders only', async () => { - const onUri = Uri.file(path.resolve('/wsOn/script.py')); - const offUri = Uri.file(path.resolve('/wsOff/script.py')); - // Stub the gate so it returns true only for URIs that look - // like they live under `/wsOn`. - isEnabledStub.callsFake((scope?: { fsPath?: string }) => { - const p = scope?.fsPath; - if (typeof p !== 'string') { - return false; - } - return p.includes(`${path.sep}wsOn${path.sep}`) || p.endsWith(`${path.sep}wsOn`); - }); - readMetadataStub.resolves(VALID_METADATA); - projectManager.setup((pm) => pm.get(typmoq.It.isAny())).returns(() => undefined); - - const detector = new InlineScriptLazyDetector(projectManager.object); - detector.activate(); - await fireOpen(onUri); - await fireOpen(offUri); - - assert.strictEqual(readMetadataStub.callCount, 1, 'should read only the file in the enabled folder'); - const readUri = readMetadataStub.firstCall.args[0] as Uri; - assert.strictEqual(readUri.toString(), onUri.toString()); - detector.dispose(); - }); - - // ---------- C2: setting-toggle triggers replay ---------- - - test('onDidChangeConfiguration replays open documents when the experimental setting toggles', async () => { - const uri = Uri.file(path.resolve('/ws/late.py')); - // The file is open in the editor BEFORE the user toggles the - // setting. The activation replay finds it but bails because - // the feature is disabled. - getOpenTextDocumentsStub.returns([makeDoc(uri)]); - isEnabledStub.returns(false); - readMetadataStub.resolves(VALID_METADATA); - projectManager.setup((pm) => pm.get(typmoq.It.isAny())).returns(() => undefined); - - const detector = new InlineScriptLazyDetector(projectManager.object); - detector.activate(); - await flushImmediate(); - assert.ok(readMetadataStub.notCalled, 'replay during activation should bail because setting is off'); - - // User flips the setting on; the lazy detector should pick - // the file up without requiring a manual save or reload. - isEnabledStub.returns(true); - assert.ok(configListener, 'config-change listener should be registered after activate()'); - const event = { - affectsConfiguration: (key: string) => key === 'python-envs.useInlineScriptMetadata', - } as ConfigurationChangeEvent; - configListener!(event); - // The replay schedules `handleDocument` synchronously; let - // the awaited work resolve. - await flushImmediate(); - await flushImmediate(); - - assert.strictEqual(readMetadataStub.callCount, 1, 'config-change replay must inspect the open .py document'); - projectManager.verify((pm) => pm.add(typmoq.It.isAny()), typmoq.Times.once()); - detector.dispose(); - }); - - test('onDidChangeConfiguration ignores changes to unrelated settings', async () => { - getOpenTextDocumentsStub.returns([makeDoc(Uri.file(path.resolve('/ws/foo.py')))]); - const detector = new InlineScriptLazyDetector(projectManager.object); - detector.activate(); - await flushImmediate(); - // The activation replay invoked `handleDocument` and bailed - // inside the gate (read stub returned undefined). Reset. - readMetadataStub.resetHistory(); - - const event = { - affectsConfiguration: (_key: string) => false, - } as ConfigurationChangeEvent; - assert.ok(configListener); - configListener!(event); - await flushImmediate(); - assert.ok(readMetadataStub.notCalled, 'unrelated config change must not trigger a replay'); - detector.dispose(); - }); }); suite('shouldHandleUri', () => { diff --git a/src/test/managers/builtin/pipUtils.inlineScript.unit.test.ts b/src/test/managers/builtin/pipUtils.inlineScript.unit.test.ts deleted file mode 100644 index 7d47e9f4..00000000 --- a/src/test/managers/builtin/pipUtils.inlineScript.unit.test.ts +++ /dev/null @@ -1,146 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -import assert from 'assert'; -import * as path from 'path'; -import * as sinon from 'sinon'; -import { CancellationToken, Progress, ProgressOptions, Uri } from 'vscode'; -import { PythonEnvironmentApi, PythonProject } from '../../../api'; -import * as ism from '../../../common/inlineScriptMetadata'; -import * as winapi from '../../../common/window.apis'; -import * as wapi from '../../../common/workspace.apis'; -import { getProjectInstallable } from '../../../managers/builtin/pipUtils'; - -suite('Pip Utils - getProjectInstallable (inline script metadata)', () => { - let findFilesStub: sinon.SinonStub; - let withProgressStub: sinon.SinonStub; - let readMetadataStub: sinon.SinonStub; - let isEnabledStub: sinon.SinonStub; - let mockApi: { getPythonProject: (uri: Uri) => PythonProject | undefined }; - - setup(() => { - findFilesStub = sinon.stub(wapi, 'findFiles'); - // Default: no requirements/pyproject files anywhere. - findFilesStub.resolves([]); - - withProgressStub = sinon.stub(winapi, 'withProgress'); - withProgressStub.callsFake( - async ( - _options: ProgressOptions, - callback: ( - progress: Progress<{ message?: string; increment?: number }>, - token: CancellationToken, - ) => Thenable, - ) => { - return await callback( - {} as Progress<{ message?: string; increment?: number }>, - { isCancellationRequested: false } as CancellationToken, - ); - }, - ); - - readMetadataStub = sinon.stub(ism, 'readInlineScriptMetadataFromFile'); - readMetadataStub.resolves(undefined); - - isEnabledStub = sinon.stub(ism, 'isInlineScriptMetadataEnabled'); - isEnabledStub.returns(true); - - const workspacePath = Uri.file(path.resolve('/test/path/root')).fsPath; - mockApi = { - getPythonProject: (uri: Uri) => { - if (uri.fsPath.startsWith(workspacePath)) { - return { name: 'workspace', uri: Uri.file(workspacePath) }; - } - return undefined; - }, - }; - }); - - teardown(() => { - sinon.restore(); - }); - - test('includes inline-metadata deps for a .py script project', async () => { - const scriptUri = Uri.file(path.resolve('/test/path/root/script.py')); - readMetadataStub.withArgs(sinon.match((u: Uri) => u.toString() === scriptUri.toString())).resolves({ - requiresPython: '>=3.11', - dependencies: ['requests<3', 'rich'], - tool: undefined, - range: { start: 0, end: 80 }, - }); - - const result = ( - await getProjectInstallable(mockApi as PythonEnvironmentApi, [{ name: 'script.py', uri: scriptUri }]) - ).installables; - - assert.strictEqual(result.length, 2, 'should produce one installable per dependency'); - const names = result.map((r) => r.name).sort(); - assert.deepStrictEqual(names, ['requests<3', 'rich']); - result.forEach((item) => { - assert.strictEqual(item.group, 'PEP 723'); - assert.deepStrictEqual(item.args, [item.name]); - assert.ok(item.uri && item.uri.toString() === scriptUri.toString()); - }); - }); - - test('ignores non-.py projects (folder projects are not walked)', async () => { - const folderProject: PythonProject = { - name: 'workspace', - uri: Uri.file(path.resolve('/test/path/root')), - }; - const result = (await getProjectInstallable(mockApi as PythonEnvironmentApi, [folderProject])).installables; - - assert.ok(readMetadataStub.notCalled, 'should not read metadata for folder projects'); - assert.strictEqual(result.length, 0); - }); - - test('no installables when the .py script has no inline metadata', async () => { - const scriptUri = Uri.file(path.resolve('/test/path/root/plain.py')); - readMetadataStub.resolves(undefined); - - const result = ( - await getProjectInstallable(mockApi as PythonEnvironmentApi, [{ name: 'plain.py', uri: scriptUri }]) - ).installables; - - assert.strictEqual(result.length, 0); - }); - - test('no installables when the .py script has zero declared dependencies', async () => { - const scriptUri = Uri.file(path.resolve('/test/path/root/empty.py')); - readMetadataStub.resolves({ - requiresPython: '>=3.11', - dependencies: [], - tool: undefined, - range: { start: 0, end: 40 }, - }); - - const result = ( - await getProjectInstallable(mockApi as PythonEnvironmentApi, [{ name: 'empty.py', uri: scriptUri }]) - ).installables; - - assert.strictEqual(result.length, 0); - }); - - test('skips metadata read when the experimental setting is off', async () => { - isEnabledStub.returns(false); - const scriptUri = Uri.file(path.resolve('/test/path/root/skip.py')); - - const result = ( - await getProjectInstallable(mockApi as PythonEnvironmentApi, [{ name: 'skip.py', uri: scriptUri }]) - ).installables; - - assert.ok(readMetadataStub.notCalled, 'should not read metadata when feature is disabled'); - assert.strictEqual(result.length, 0); - }); - - test('ignores .py projects with non-file URI schemes', async () => { - const result = ( - await getProjectInstallable(mockApi as PythonEnvironmentApi, [ - { name: 'untitled.py', uri: Uri.parse('untitled:untitled.py') }, - ]) - ).installables; - - assert.ok(readMetadataStub.notCalled, 'should not read metadata for non-file URI'); - assert.strictEqual(result.length, 0); - }); -}); From a334587f1e7aa3863d1de1612df23c8cd22f0975 Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Thu, 28 May 2026 15:08:59 -0700 Subject: [PATCH 8/9] Add PEP 723 detection telemetry Wires the existing silent inline-script-metadata detector to emit two anonymized telemetry events: - PEP723.DETECTED: fires once per (URI, session) the first time a valid `# /// script` block is observed. Properties: `trigger` (open|save), `hasRequiresPython` (bool). Measure: `dependencyCount` (int). This is the denominator for the `how many users actually see PEP 723 files` question. - PEP723.EDITED: fires once per (URI, session) the first time a previously-detected URI receives a non-empty content change. Measure: `duration` (ms since detection). Together with DETECTED this distinguishes viewers from editors. No URIs, paths, dependency names, or version strings are sent. Adds an `onDidChangeTextDocument` wrapper to `workspace.apis.ts` so the detector can subscribe through the same abstraction layer used for open/save. Extends the detector unit tests from 16 to 26 cases covering both events, per-URI dedup, empty-contentChanges no-ops, and disposal suppression. --- src/common/telemetry/constants.ts | 42 ++++ src/common/workspace.apis.ts | 9 + src/features/inlineScriptLazyDetector.ts | 101 +++++++-- .../inlineScriptLazyDetector.unit.test.ts | 199 +++++++++++++++++- 4 files changed, 324 insertions(+), 27 deletions(-) diff --git a/src/common/telemetry/constants.ts b/src/common/telemetry/constants.ts index 858529e7..55a012aa 100644 --- a/src/common/telemetry/constants.ts +++ b/src/common/telemetry/constants.ts @@ -209,6 +209,27 @@ export enum EventNames { * - errorType: string (only when outcome === 'failed') */ MIGRATION_SYSTEM_ENV_MANAGER = 'MIGRATION.SYSTEM_ENV_MANAGER', + /** + * Telemetry event fired once per session, per URI, the first time a `.py` + * file with a valid PEP 723 `# /// script` block is observed by the lazy + * detector. Used to size the population of users who actually see PEP 723 + * files — the denominator for the "view vs edit" question. + * Properties: + * - trigger: 'open' | 'save' (which workspace event surfaced the file) + * - hasRequiresPython: boolean (whether the block declares `requires-python`) + * Measures: + * - dependencyCount: number (number of entries in the `dependencies` list) + */ + PEP723_DETECTED = 'PEP723.DETECTED', + /** + * Telemetry event fired once per session, per URI, the first time a `.py` + * file that previously raised a `PEP723.DETECTED` event receives a real + * text edit. Together with `PEP723.DETECTED` this measures the fraction + * of users who do more than view PEP 723 scripts. + * Measures: + * - duration: number (ms between the detection and the first edit) + */ + PEP723_EDITED = 'PEP723.EDITED', } // Map all events to their properties @@ -657,4 +678,25 @@ export interface IEventNamePropertyMapping { outcome: 'removed' | 'partial' | 'not_set' | 'failed'; errorType?: string; }; + + /* __GDPR__ + "pep723.detected": { + "trigger": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "owner": "StellaHuang95" }, + "hasRequiresPython": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "owner": "StellaHuang95" }, + "dependencyCount": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "isMeasurement": true, "owner": "StellaHuang95" } + } + */ + [EventNames.PEP723_DETECTED]: { + trigger: 'open' | 'save'; + hasRequiresPython: boolean; + // Goes through the measures payload (numeric); listed here for GDPR only. + dependencyCount?: number; + }; + + /* __GDPR__ + "pep723.edited": { + "": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "isMeasurement": true, "owner": "StellaHuang95" } + } + */ + [EventNames.PEP723_EDITED]: never | undefined; } diff --git a/src/common/workspace.apis.ts b/src/common/workspace.apis.ts index 77f0c3cc..d571bcf2 100644 --- a/src/common/workspace.apis.ts +++ b/src/common/workspace.apis.ts @@ -9,6 +9,7 @@ import { FileSystemWatcher, GlobPattern, TextDocument, + TextDocumentChangeEvent, Uri, workspace, WorkspaceConfiguration, @@ -90,6 +91,14 @@ export function onDidSaveTextDocument( return workspace.onDidSaveTextDocument(listener, thisArgs, disposables); } +export function onDidChangeTextDocument( + listener: (e: TextDocumentChangeEvent) => any, + thisArgs?: any, + disposables?: Disposable[], +): Disposable { + return workspace.onDidChangeTextDocument(listener, thisArgs, disposables); +} + /** * Snapshot of the text documents VS Code has already opened. Useful * for extensions activated by `onLanguage:*` events, which miss the diff --git a/src/features/inlineScriptLazyDetector.ts b/src/features/inlineScriptLazyDetector.ts index 7023a7d7..f84dc499 100644 --- a/src/features/inlineScriptLazyDetector.ts +++ b/src/features/inlineScriptLazyDetector.ts @@ -2,28 +2,35 @@ // Licensed under the MIT License. import * as path from 'path'; -import { Disposable, TextDocument, Uri } from 'vscode'; +import { Disposable, TextDocument, TextDocumentChangeEvent, Uri } from 'vscode'; import { readInlineScriptMetadataFromFile } from '../common/inlineScriptMetadata'; import { traceVerbose, traceWarn } from '../common/logging'; +import { EventNames } from '../common/telemetry/constants'; +import { sendTelemetryEvent } from '../common/telemetry/sender'; import { getOpenTextDocuments, getWorkspaceFolder, + onDidChangeTextDocument, onDidOpenTextDocument, onDidSaveTextDocument, } from '../common/workspace.apis'; /** * Silent on-open / on-save detector for `.py` files that declare - * inline script metadata (PEP 723). The detector is intentionally - * observer-only: it parses the head of every eligible `.py` file the - * user opens or saves, but does not surface any UI, register - * projects, or otherwise change extension behavior. + * inline script metadata (PEP 723). The detector parses the head of + * every eligible `.py` file the user opens or saves and emits two + * anonymized telemetry events: * - * It is kept wired up so we have a single ingest point for PEP 723 - * telemetry. The TODO marker inside `processOnce` is the planned - * emission site; until the telemetry events are wired up the detector - * is effectively dead code that runs a cheap parse and discards the - * result. + * - `PEP723.DETECTED` once per (URI, session) the first time a + * valid `# /// script` block is observed. This is the denominator + * for the "how many users actually see PEP 723 files" question. + * - `PEP723.EDITED` once per (URI, session) the first time a + * previously-detected file receives a real text edit. Together + * with `DETECTED` this distinguishes viewers from editors. + * + * No URIs, file paths, or file content are sent. The detector does + * not register projects, surface UI, or otherwise change extension + * behavior; it is a pure observer. * * Detection is cheap (≤ 8 KiB read + regex + TOML parse) and runs * only on files the user has already shown intent in. @@ -33,6 +40,18 @@ export class InlineScriptLazyDetector implements Disposable { // In-flight reads keyed by `uri.toString()` so rapid open+save // doesn't double-process the same file. private readonly inFlight = new Map>(); + // URIs (as `uri.toString()`) for which we have already emitted + // `PEP723.DETECTED` in this session. Used to dedup the detection + // event across repeat opens/saves and to gate `PEP723.EDITED` so + // the latter only fires for files we already counted as detected. + private readonly detectedUris = new Set(); + // URIs for which we have already emitted `PEP723.EDITED` in this + // session. Each detected file emits at most one edited event. + private readonly editedUris = new Set(); + // Wall-clock ms (from `Date.now`) at which each URI's detection + // event fired. Used to compute the `duration` measure on the + // first-edit event. + private readonly detectionAtMs = new Map(); // Flips to `true` in `dispose()`. Guards async continuations // inside `processOnce` so an in-flight read that completes after // disposal does not emit telemetry on a detector the host has @@ -63,6 +82,7 @@ export class InlineScriptLazyDetector implements Disposable { this.subscriptions.push( onDidOpenTextDocument((doc) => this.handleDocument(doc, 'open')), onDidSaveTextDocument((doc) => this.handleDocument(doc, 'save')), + onDidChangeTextDocument((e) => this.handleChange(e)), ); // Defer the catch-up pass so we observe `workspace.textDocuments` // AFTER VS Code finishes registering the document that triggered @@ -145,17 +165,26 @@ export class InlineScriptLazyDetector implements Disposable { if (this.disposed) { return; } - if (metadata !== undefined) { - traceVerbose( - `inlineScriptLazyDetector: detected inline script metadata in ${uri.fsPath} (${trigger})`, - ); - // TODO(pep723-telemetry): emit a PEP 723 detection - // event here (e.g. `pep723.detected`) with - // anonymized fields such as `trigger`, presence of - // `requires-python`, and dependency count. This is - // the planned emission site the detector is being - // kept alive for. + if (metadata === undefined) { + return; } + const key = uri.toString(); + if (this.detectedUris.has(key)) { + // Already counted this file in the current session. + // Subsequent opens/saves of the same URI are silent. + return; + } + this.detectedUris.add(key); + this.detectionAtMs.set(key, Date.now()); + traceVerbose(`inlineScriptLazyDetector: detected inline script metadata in ${uri.fsPath} (${trigger})`); + sendTelemetryEvent( + EventNames.PEP723_DETECTED, + { dependencyCount: metadata.dependencies?.length ?? 0 }, + { + trigger, + hasRequiresPython: metadata.requiresPython !== undefined, + }, + ); } catch (err) { // `readInlineScriptMetadataFromFile` already swallows I/O // errors internally. This catch is a defensive net for @@ -163,6 +192,38 @@ export class InlineScriptLazyDetector implements Disposable { traceWarn(`inlineScriptLazyDetector: unexpected error while reading ${uri.fsPath}:`, err); } } + + /** + * Emit `PEP723.EDITED` the first time a previously-detected URI + * receives a real content change. The handler is hot (fires on + * every keystroke in every text document workspace-wide) so it + * bails out as cheaply as possible for the common case where the + * file is not a tracked PEP 723 script. + */ + private handleChange(e: TextDocumentChangeEvent): void { + if (this.disposed) { + return; + } + // `onDidChangeTextDocument` can fire with empty `contentChanges` + // (e.g. dirty-state toggles); skip those — they aren't user edits. + if (e.contentChanges.length === 0) { + return; + } + const key = e.document.uri.toString(); + if (!this.detectedUris.has(key)) { + return; + } + if (this.editedUris.has(key)) { + return; + } + this.editedUris.add(key); + const detectedAt = this.detectionAtMs.get(key); + const duration = detectedAt !== undefined ? Date.now() - detectedAt : 0; + traceVerbose( + `inlineScriptLazyDetector: first edit observed on ${e.document.uri.fsPath} (${duration}ms after detection)`, + ); + sendTelemetryEvent(EventNames.PEP723_EDITED, duration); + } } /** diff --git a/src/test/features/inlineScriptLazyDetector.unit.test.ts b/src/test/features/inlineScriptLazyDetector.unit.test.ts index ea6938c0..44e83e21 100644 --- a/src/test/features/inlineScriptLazyDetector.unit.test.ts +++ b/src/test/features/inlineScriptLazyDetector.unit.test.ts @@ -4,8 +4,10 @@ import assert from 'assert'; import * as path from 'path'; import * as sinon from 'sinon'; -import { Disposable, TextDocument, Uri } from 'vscode'; +import { Disposable, TextDocument, TextDocumentChangeEvent, TextDocumentContentChangeEvent, Uri } from 'vscode'; import * as ism from '../../common/inlineScriptMetadata'; +import { EventNames } from '../../common/telemetry/constants'; +import * as telemetrySender from '../../common/telemetry/sender'; import * as wapi from '../../common/workspace.apis'; import { InlineScriptLazyDetector, shouldHandleUri } from '../../features/inlineScriptLazyDetector'; @@ -15,9 +17,24 @@ function makeDoc(uri: Uri): TextDocument { return { uri } as TextDocument; } +// A non-empty change event payload. The actual content of the +// changes is not inspected by the detector; only `contentChanges.length` +// matters. +const NON_EMPTY_CHANGES: readonly TextDocumentContentChangeEvent[] = [ + { range: undefined as never, rangeOffset: 0, rangeLength: 0, text: 'x' }, +]; + +function makeChange(uri: Uri, changes: readonly TextDocumentContentChangeEvent[] = NON_EMPTY_CHANGES): TextDocumentChangeEvent { + return { + document: makeDoc(uri), + contentChanges: changes, + reason: undefined, + } as TextDocumentChangeEvent; +} + const VALID_METADATA: ism.InlineScriptMetadata = { requiresPython: '>=3.11', - dependencies: ['requests'], + dependencies: ['requests', 'rich'], tool: undefined, range: { start: 0, end: 40 }, }; @@ -25,15 +42,19 @@ const VALID_METADATA: ism.InlineScriptMetadata = { suite('InlineScriptLazyDetector', () => { let onDidOpenStub: sinon.SinonStub; let onDidSaveStub: sinon.SinonStub; + let onDidChangeStub: sinon.SinonStub; let getOpenTextDocumentsStub: sinon.SinonStub; let getWorkspaceFolderStub: sinon.SinonStub; let readMetadataStub: sinon.SinonStub; + let sendTelemetryStub: sinon.SinonStub; let openListener: ((doc: TextDocument) => unknown) | undefined; let saveListener: ((doc: TextDocument) => unknown) | undefined; + let changeListener: ((e: TextDocumentChangeEvent) => unknown) | undefined; setup(() => { openListener = undefined; saveListener = undefined; + changeListener = undefined; onDidOpenStub = sinon.stub(wapi, 'onDidOpenTextDocument'); onDidOpenStub.callsFake((listener: (doc: TextDocument) => unknown) => { @@ -51,6 +72,14 @@ suite('InlineScriptLazyDetector', () => { }); }); + onDidChangeStub = sinon.stub(wapi, 'onDidChangeTextDocument'); + onDidChangeStub.callsFake((listener: (e: TextDocumentChangeEvent) => unknown) => { + changeListener = listener; + return new Disposable(() => { + changeListener = undefined; + }); + }); + // Default to an empty list of open documents. Tests that // exercise the catch-up replay override this. getOpenTextDocumentsStub = sinon.stub(wapi, 'getOpenTextDocuments'); @@ -68,6 +97,8 @@ suite('InlineScriptLazyDetector', () => { readMetadataStub = sinon.stub(ism, 'readInlineScriptMetadataFromFile'); readMetadataStub.resolves(undefined); + + sendTelemetryStub = sinon.stub(telemetrySender, 'sendTelemetryEvent'); }); teardown(() => { @@ -84,11 +115,22 @@ suite('InlineScriptLazyDetector', () => { await saveListener!(makeDoc(uri)); } - test('activate() subscribes to onDidOpen and onDidSave', () => { + function fireChange(uri: Uri, changes: readonly TextDocumentContentChangeEvent[] = NON_EMPTY_CHANGES): void { + assert.ok(changeListener, 'change listener should be registered after activate()'); + changeListener!(makeChange(uri, changes)); + } + + // Filter `sendTelemetryStub.getCalls()` to a single PEP 723 event name. + function callsFor(name: EventNames): sinon.SinonSpyCall[] { + return sendTelemetryStub.getCalls().filter((c) => c.args[0] === name); + } + + test('activate() subscribes to onDidOpen, onDidSave, and onDidChange', () => { const detector = new InlineScriptLazyDetector(); detector.activate(); assert.ok(onDidOpenStub.calledOnce, 'should subscribe to onDidOpenTextDocument'); assert.ok(onDidSaveStub.calledOnce, 'should subscribe to onDidSaveTextDocument'); + assert.ok(onDidChangeStub.calledOnce, 'should subscribe to onDidChangeTextDocument'); detector.dispose(); }); @@ -161,7 +203,7 @@ suite('InlineScriptLazyDetector', () => { detector.dispose(); }); - test('dispose() during an in-flight read bails out before logging detection', async () => { + test('dispose() during an in-flight read bails out before emitting telemetry', async () => { const uri = Uri.file(path.resolve('/ws/disposed.py')); let resolveRead: ((meta: ism.InlineScriptMetadata) => void) | undefined; readMetadataStub.returns( @@ -178,11 +220,11 @@ suite('InlineScriptLazyDetector', () => { // Tear the detector down BEFORE the read settles. detector.dispose(); // Now let the in-flight read complete with metadata. The - // `disposed` guard inside processOnce should prevent any - // further work (in the future, this guard also protects the - // telemetry emission site from firing after disposal). + // `disposed` guard inside processOnce must prevent any + // further work — including the detection telemetry event. resolveRead!(VALID_METADATA); await assert.doesNotReject(inFlight ?? Promise.resolve()); + assert.strictEqual(callsFor(EventNames.PEP723_DETECTED).length, 0, 'no detection event after dispose'); }); // ---------- catch-up replay over `getOpenTextDocuments` ---------- @@ -228,6 +270,149 @@ suite('InlineScriptLazyDetector', () => { await flushImmediate(); assert.ok(readMetadataStub.notCalled, 'dispose() must clear the pending setImmediate handle'); }); + + // ---------- PEP723.DETECTED telemetry ---------- + + test('PEP723.DETECTED fires once with trigger=open + dependencyCount + hasRequiresPython', async () => { + const uri = Uri.file(path.resolve('/ws/detect.py')); + readMetadataStub.resolves(VALID_METADATA); + const detector = new InlineScriptLazyDetector(); + detector.activate(); + await fireOpen(uri); + + const detectedCalls = callsFor(EventNames.PEP723_DETECTED); + assert.strictEqual(detectedCalls.length, 1, 'detection event should fire exactly once'); + const [, measures, properties] = detectedCalls[0].args; + assert.deepStrictEqual(measures, { dependencyCount: 2 }); + assert.deepStrictEqual(properties, { trigger: 'open', hasRequiresPython: true }); + detector.dispose(); + }); + + test('PEP723.DETECTED fires with trigger=save when surfaced by a save event', async () => { + const uri = Uri.file(path.resolve('/ws/detectOnSave.py')); + readMetadataStub.resolves(VALID_METADATA); + const detector = new InlineScriptLazyDetector(); + detector.activate(); + await fireSave(uri); + + const detectedCalls = callsFor(EventNames.PEP723_DETECTED); + assert.strictEqual(detectedCalls.length, 1); + assert.strictEqual(detectedCalls[0].args[2].trigger, 'save'); + detector.dispose(); + }); + + test('PEP723.DETECTED does not fire when the file has no metadata block', async () => { + const uri = Uri.file(path.resolve('/ws/plain.py')); + readMetadataStub.resolves(undefined); + const detector = new InlineScriptLazyDetector(); + detector.activate(); + await fireOpen(uri); + assert.strictEqual(callsFor(EventNames.PEP723_DETECTED).length, 0); + detector.dispose(); + }); + + test('PEP723.DETECTED is deduplicated across repeated opens and saves of the same URI', async () => { + const uri = Uri.file(path.resolve('/ws/repeat.py')); + readMetadataStub.resolves(VALID_METADATA); + const detector = new InlineScriptLazyDetector(); + detector.activate(); + await fireOpen(uri); + await fireSave(uri); + await fireSave(uri); + await fireOpen(uri); + assert.strictEqual(callsFor(EventNames.PEP723_DETECTED).length, 1, 'detection event must dedup per session'); + detector.dispose(); + }); + + test('PEP723.DETECTED reports hasRequiresPython=false when not declared', async () => { + const uri = Uri.file(path.resolve('/ws/noPython.py')); + readMetadataStub.resolves({ + requiresPython: undefined, + dependencies: [], + tool: undefined, + range: { start: 0, end: 20 }, + } satisfies ism.InlineScriptMetadata); + const detector = new InlineScriptLazyDetector(); + detector.activate(); + await fireOpen(uri); + + const [, measures, properties] = callsFor(EventNames.PEP723_DETECTED)[0].args; + assert.deepStrictEqual(measures, { dependencyCount: 0 }); + assert.deepStrictEqual(properties, { trigger: 'open', hasRequiresPython: false }); + detector.dispose(); + }); + + // ---------- PEP723.EDITED telemetry ---------- + + test('PEP723.EDITED fires once on first content change after detection', async () => { + const uri = Uri.file(path.resolve('/ws/edit.py')); + readMetadataStub.resolves(VALID_METADATA); + const detector = new InlineScriptLazyDetector(); + detector.activate(); + await fireOpen(uri); + fireChange(uri); + + const editedCalls = callsFor(EventNames.PEP723_EDITED); + assert.strictEqual(editedCalls.length, 1, 'edited event should fire exactly once'); + // Second arg is the measure (number → { duration }); accept either form. + const measureArg = editedCalls[0].args[1]; + assert.strictEqual(typeof measureArg, 'number', 'measure should be a number (latency ms)'); + assert.ok((measureArg as number) >= 0, 'duration should be non-negative'); + detector.dispose(); + }); + + test('PEP723.EDITED is deduplicated across repeated edits of the same URI', async () => { + const uri = Uri.file(path.resolve('/ws/multiEdit.py')); + readMetadataStub.resolves(VALID_METADATA); + const detector = new InlineScriptLazyDetector(); + detector.activate(); + await fireOpen(uri); + fireChange(uri); + fireChange(uri); + fireChange(uri); + assert.strictEqual(callsFor(EventNames.PEP723_EDITED).length, 1); + detector.dispose(); + }); + + test('PEP723.EDITED does not fire for changes on a URI that was never detected', async () => { + const uri = Uri.file(path.resolve('/ws/notDetected.py')); + readMetadataStub.resolves(undefined); + const detector = new InlineScriptLazyDetector(); + detector.activate(); + await fireOpen(uri); + fireChange(uri); + assert.strictEqual(callsFor(EventNames.PEP723_EDITED).length, 0); + detector.dispose(); + }); + + test('PEP723.EDITED ignores change events with no content changes', async () => { + const uri = Uri.file(path.resolve('/ws/noOpChange.py')); + readMetadataStub.resolves(VALID_METADATA); + const detector = new InlineScriptLazyDetector(); + detector.activate(); + await fireOpen(uri); + // VS Code can fire a change event with an empty contentChanges + // array for things like dirty-state toggles; that's not a user + // edit and must not count. + fireChange(uri, []); + assert.strictEqual(callsFor(EventNames.PEP723_EDITED).length, 0); + // A real edit still counts after the no-op was ignored. + fireChange(uri); + assert.strictEqual(callsFor(EventNames.PEP723_EDITED).length, 1); + detector.dispose(); + }); + + test('PEP723.EDITED is suppressed after dispose()', async () => { + const uri = Uri.file(path.resolve('/ws/disposedEdit.py')); + readMetadataStub.resolves(VALID_METADATA); + const detector = new InlineScriptLazyDetector(); + detector.activate(); + await fireOpen(uri); + const grabbedChangeListener = changeListener!; + detector.dispose(); + grabbedChangeListener(makeChange(uri)); + assert.strictEqual(callsFor(EventNames.PEP723_EDITED).length, 0); + }); }); suite('shouldHandleUri', () => { From f16b1bf999ca042b3d2bd3db93f5451ab0f9ab80 Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Thu, 28 May 2026 15:09:06 -0700 Subject: [PATCH 9/9] Tighten PEP 723 detector activation comment and revert spurious package.json reformat - src/extension.ts: update the inline comment beside the `InlineScriptLazyDetector` activation site to describe its actual behavior (emits anonymized telemetry) instead of the stale `feature is not shipped` wording from the slim-down commit. - package.json: revert an unintended multi-line re-pretty-print of `python-envs.workspaceSearchPaths.default`. The branch now has zero drift on package.json against upstream/main. --- package.json | 5 +---- src/extension.ts | 5 ++--- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/package.json b/package.json index 0156334b..df79269f 100644 --- a/package.json +++ b/package.json @@ -124,10 +124,7 @@ "python-envs.workspaceSearchPaths": { "type": "array", "description": "%python-envs.workspaceSearchPaths.description%", - "default": [ - ".venv", - "*/.venv" - ], + "default": [".venv", "*/.venv"], "scope": "resource", "items": { "type": "string" diff --git a/src/extension.ts b/src/extension.ts index 41c71e95..6f609efe 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -207,9 +207,8 @@ export async function activate(context: ExtensionContext): Promise