feat: report diagnostics for malformed PEP 723 inline script metadata - #1785
Conversation
Malformed inline script metadata was previously invisible. A block with a typo -- a missing closing `# ///`, a content line without the required space after `#`, a TOML syntax error -- collapsed to `undefined` in the parser, so no CodeLens appeared and nothing explained why. The file simply looked like ordinary Python. The parser now returns a discriminated result (`valid` / `invalid` / `none`) carrying structured problems with source ranges, and a new `InlineScriptDiagnosticsPublisher` surfaces them as squiggles on open, save, debounced change (300ms) and clears on close, delete, rename and dispose. Diagnostics are validated against the live editor buffer so squiggles match what is on screen, while provisioning continues to read the saved file. Both share one parser, so the two views cannot disagree about what "valid" means. Gated behind the existing `python-envs.inlineScripts.enabled` setting. No Pylance or Python extension changes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🔒 Automated review in progress — Rich Chiodo (@rchiodo) is auto-reviewing this PR. |
|
Result: 🔴 Verification detailsVerification: The relevant tests could not be fully run in the isolated environment; this review is not fully verified. Summary: Targeted parser regression and diagnostics publisher tests were identified, including all 52 tests added by the PR. Execution could not begin because the container has neither `node` nor `npm`; the targeted compile-and-test command exited 127. Consequently, no meaningful test passed or failed, and confidence is low. Test runs: 2 not run
|
- Report a bad content line instead of a bogus "missing closing marker" when the closing `# ///` is present. A blank line between fields is the common trigger and now gets a message that names the real problem. Blocks below the leading comment region stay silent as before, so documentation examples are unaffected. - Cancel debounced validations for descendants of a deleted or renamed folder. Previously only published entries were swept, so a file edited within the debounce window could gain a squiggle after it was gone. - Rename the parse discriminant `valid` to `parsed`: it reports that metadata is usable, not that the input was problem-free, and it can carry error-severity problems from a second malformed block. - Assert the exact TOML detail and the exact localized diagnostic messages so a code-to-message mis-mapping cannot pass unnoticed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Result: 🔴 Verification detailsVerification: Isolated verification observed failures that were not classified as caused by this PR: Environment and test discovery. The relevant tests could not be fully run in the isolated environment; this review is not fully verified. Summary: Verification could not proceed because the disposable container has no Node.js; the discovery command exited 127 with `node: not found`. Consequently, TypeScript compilation and the targeted parser and diagnostics suites were not run. Source inspection found 61 newly added tests across two files, covering parser ranges, malformed metadata, diagnostics publishing, debouncing, and cleanup. Confidence is limited without executable results. Test runs: 1 failed, 3 not run
❌
|
Rich Chiodo (rchiodo)
left a comment
There was a problem hiding this comment.
Approved via Review Center.
17cd8ba
into
microsoft:main
Part of #1602 (PEP 723 inline script env support). Does not close it — this covers only diagnostics for malformed metadata.
Problem
Malformed PEP 723 metadata is currently invisible. The parser collapses every failure to
undefined, so a block with a typo — a missing closing# ///, a content line without the required space after#, a TOML syntax error — produces no CodeLens, no warning, nothing. The file just looks like ordinary Python and the user has no idea why the "Setup environment" action never appeared.Worse, a malformed block and a file with no block were indistinguishable, so we couldn't have told the user even if we wanted to.
What this does
The parser now returns a discriminated result —
valid/invalid/none— carrying structured problems with source ranges, and a newInlineScriptDiagnosticsPublishersurfaces them as squiggles.Gated behind the existing
python-envs.inlineScripts.enabledsetting (defaultfalse, latched at activation). No Pylance or Python extension changes; no cross-team dependencies.Spec conformance
Normative source: PyPA — Inline script metadata. Permalinks below are pinned to commit
9dd85f2, the latest commit touching that file, so line numbers can't drift.I extracted every RFC 2119 keyword sentence from the spec source programmatically: 20 total — 11 MUST, 2 MUST NOT, 1 SHOULD, 7 MAY. The mapping below is against that complete set, not a sampling.
Diagnostics backed by an explicit spec MUST
invalid-block-marker# /// TYPE… Block MUST end with the line# ///… TheTYPEMUST only consist of ASCII letters, numbers and hyphens"invalid-content-line#. If there are characters after the#then the first character MUST be a space"multiple-blocksTYPEdefined, tools MUST produce an error"unterminated-blockunterminated-blockis a warning, not an error, precisely because the spec says such blocks MUST be ignored — the file is valid Python and valid per spec. We only hint that the author probably meant to close it. See the scoping rule below.Also enforced without needing a diagnostic:
type === 'script'filter in both the block regex and the opener scanDeliberately NOT reported
These are conformance decisions, not oversights. Each is measured by a test.
# /// scriptimmediately followed by# ///)+), but the prose doesn't. Spec says prose wins, so this is valid, empty metadata.BLOCK_RE's content group is*with a comment marking the deliberate deviation.# /// scriptappearing below real codeheaderRegionEnd()suppresses the warning at/after the first non-blank, non-#line. The spec explicitly says behaviour inside multi-line strings "is tool-dependent and should not be relied on".# ///used as block content# /////////),## ///, non-scripttypesChecks that are ours, not the spec's
Worth calling out explicitly so reviewers can push back:
invalid-field-type(Error) fires whendependenciesisn't an array of strings,requires-pythonisn't a string, ortoolisn't a table. The spec describes these types in prose — "dependencies: A list of strings…", "requires-python: A string…" (L99-L104) — but attaches no MUST to the container type. The only MUST in those bullets governs entry validity.I kept it as an error because a non-array
dependencieshas no valid interpretation and the alternative is a confusing downstream resolver failure. But it is inference, not mandate. Happy to downgrade to a warning if reviewers prefer strict spec-only conformance.invalid-toml(Error) — the spec never literally states the content is TOML; it's implied by the[tool]table semantics and the reference implementation'stomllib.loads(content).Known gaps (not addressed here)
Two genuine spec MUSTs are not enforced. Verified by measurement — both parse as valid with no diagnostic today:
dependencies = ["!!! not a specifier !!!"]accepted silentlyrequires-python = "totally-not-a-version"accepted silentlyBoth need real grammar implementations and would meaningfully expand scope. Deferred intentionally; happy to file a follow-up issue.
Behaviour change reviewers must notice
This PR is not purely additive. One existing behaviour changes:
Before: parsed as no metadata -> no CodeLens.
After: parses as valid, empty metadata -> the setup CodeLens now appears.
That follows from the spec's "text takes precedence" tiebreak, and I believe it's correct, but it does affect provisioning and not just diagnostics. Everything else in this PR is diagnostics-only and leaves the provisioning path byte-identical — I verified that with a harness running the old and new parsers side by side over 20 scenarios.
Implementation notes
src/common/inlineScript/metadata.ts— the parser rework.{ kind: 'valid' | 'invalid' | 'none', problems, metadata? }.findScriptOpeners) — this is what makes "malformed block" distinguishable from "no block". The canonical regex alone can't tell them apart.toSourceRangeis the single offset-to-line/character translation point. Reviewers should focus here for range correctness.tomlErrorSourceRangerecoversline/colfrom@iarna/tomlerrors that were previously discarded, so the squiggle underlines the actual offending text instead of the whole block.src/features/inlineScript/diagnostics.ts— the publisher.createSimpleDebouncehelper). Debouncing matters here: without it we'd flash "missing closing marker" at someone who is still mid-edit.createSimpleDebounce(ms, cb).trigger()takes no arguments and owns one timer, so a shared instance would let documents clobber each other — hence the per-URIpendingmap.file:scheme plus.pyextension only.Validation reads the live editor buffer, so squiggles match what's on screen; provisioning keeps reading the saved file. Both call the same parser, so the two views can't disagree about what "valid" means.
Edge cases handled
\r\n?normalized to\nbefore matching. Note JS regex.doesn't match\rwhile Python's does, so the canonical regex can't be used verbatim.sliceHeaderBytesexactly as the provisioning path does, so the two can't diverge on large files.# ///is itself a legal content line, so back-to-back blocks are consumed as one.multiple-blockscorrectly requires a non-content line between them.Known limitation
Indented markers (
# /// script) are silent. The spec anchors markers at column 0, so an indented one isn't a block at all. Documented in the parser.Testing
npm run compile-testsbuild.npm run lintclean,tscclean, Prettier clean.Manual verification
Set
python-envs.inlineScripts.enabled: trueand reload the window (the flag is latched at activation). Then try:-> warning: missing closing
# ///-> error: content line must be exactly
#or start with#-> error: TOML error, underlining the offending token
Review focus
toSourceRangecorrectness — every range flows through it; BOM/CRLF offsets are the likeliest bug.invalid-field-typetoo aggressive? It's the one error not backed by a spec MUST.