From 665fd82860080c8d23baa51bd4add037c70d3f76 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Tue, 14 Jul 2026 17:45:09 +0100 Subject: [PATCH] =?UTF-8?q?fix(highlight):=20PR=20#118=20round=201=20?= =?UTF-8?q?=E2=80=94=20drop=20locals-predicate=20captures;=20stale=20comme?= =?UTF-8?q?nts?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit [P2] The shared JavaScript highlights query guards its builtin captures (console, require, …) with `#is-not? local`, a PROPERTY predicate (`Query::property_predicates`) that needs a scope map from the grammar's LOCALS_QUERY — which pmacs does not run. `compute_highlight_spans` took every capture, so a locally-shadowed `console`/`require` still surfaced as `@variable.builtin`/`@function.builtin`; a theme distinguishing `.builtin` would mis-style the shadowed local. Full locals processing is substrate work; conservatively fail-closed instead: drop captures whose pattern carries an `#is?`/`#is-not? local` property predicate (the identifier falls back to its non-builtin capture). The text predicates (`#eq?`/`#match?`/`#any-of?`, already applied by the capture iterator) and `#set!` settings are untouched. This is a general engine fix — it corrects the same latent mis-styling for any grammar using the locals predicate, not just JS/TS. - javascript_shadowed_builtin_is_not_mislabeled: a local `const console` produces no `*.builtin` capture (directly observed to fail — two `variable.builtin` captures — before the fix). [P3] Comments this PR invalidated: `lsp.lua` no longer claims Python has no grammar; `syntax.lua`'s `_has_language` gate comment uses a still-grammarless example (an init.lua `shebangs.ruby`) instead of python/javascript; and the rewritten Ruby shebang test's doc no longer describes it as a Python test. Gates: fmt; clippy -D warnings; test --lib; --features crdt; m4_acceptance --skip basedpyright; GPU; full workspace sweep; git diff --check. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01YJ9FQ832QwftJXCD9LeFan --- builtin/runtime/lsp.lua | 12 +++++---- builtin/runtime/syntax.lua | 8 +++--- src/syntax.rs | 55 ++++++++++++++++++++++++++++++++++++++ tests/m4_acceptance.rs | 8 +++--- 4 files changed, 72 insertions(+), 11 deletions(-) diff --git a/builtin/runtime/lsp.lua b/builtin/runtime/lsp.lua index befc3ad..eb4f1e4 100644 --- a/builtin/runtime/lsp.lua +++ b/builtin/runtime/lsp.lua @@ -201,11 +201,13 @@ pmacs.lsp.config.zig = pmacs.lsp.config.zig or { } -- LSP-side extension → language map, deliberately independent of the --- tree-sitter detection in `pmacs.parse` (which is grammar-gated: --- Python has an LSP server but no bundled grammar). Consulted only --- when `pmacs.parse.language_for_path` finds nothing, so grammar- --- backed languages keep their existing detection. Extensible from --- init.lua: `pmacs.lsp.filetypes.foo = "bar"`. +-- tree-sitter detection in `pmacs.parse`. Consulted only when +-- `pmacs.parse.language_for_path` finds nothing (an extension with a +-- server but no bundled grammar), so grammar-backed languages keep their +-- existing detection. Every language with an LSP config now also ships a +-- grammar, so this is mainly the LSP-only fallback that keeps a language +-- id stable if a grammar is ever dropped, plus the seam for user-added +-- mappings. Extensible from init.lua: `pmacs.lsp.filetypes.foo = "bar"`. pmacs.lsp.filetypes = pmacs.lsp.filetypes or {} pmacs.lsp.filetypes.py = pmacs.lsp.filetypes.py or "python" pmacs.lsp.filetypes.pyi = pmacs.lsp.filetypes.pyi or "python" diff --git a/builtin/runtime/syntax.lua b/builtin/runtime/syntax.lua index f988d24..b639377 100644 --- a/builtin/runtime/syntax.lua +++ b/builtin/runtime/syntax.lua @@ -220,9 +220,11 @@ local function attach_for_active_buffer() -- and silently swap the grammar — and diverge from the LSP side, which -- keeps its existing attachment across the switch. A first-seen buffer -- (no view yet) resolves normally. Gate dispatch on `_has_language`: the - -- resolution chain also yields languages with no grammar (python, - -- javascript), and dispatching one would raise "unknown language" - -- (caught, but noise) and never gives a wrong-grammar tree. + -- resolution chain can still yield a language with no grammar — a + -- shebang or filetype mapping to a server-only or unsupported language + -- (e.g. an init.lua `pmacs.parse.shebangs.ruby = "ruby"`) — and + -- dispatching one would raise "unknown language" (caught, but noise) and + -- never gives a wrong-grammar tree. local lang = pmacs.parse._has_view(buf) and parse_lang_by_buffer[key] or resolve_active_language(buf) if not lang or not pmacs.parse._has_language(lang) then return end diff --git a/src/syntax.rs b/src/syntax.rs index 4b0b8bf..6b77371 100644 --- a/src/syntax.rs +++ b/src/syntax.rs @@ -886,6 +886,24 @@ pub fn compute_highlight_spans_in_range( let root = bundle.tree.root_node(); let mut iter = cursor.captures(query, root, source); while let Some((qmatch, capture_idx)) = iter.next() { + // Fail-closed on the locals property predicate. The capture + // iterator already applies text predicates (`#eq?`/`#match?`/ + // `#any-of?`), but `#is? local` / `#is-not? local` are *property* + // predicates (`Query::property_predicates`) that need a scope map + // built from the grammar's LOCALS_QUERY, which pmacs does not run. + // Applying such a capture regardless mis-styles shadowed locals — + // e.g. a local `console`/`require` in JS/TS would still capture as + // `@variable.builtin`/`@function.builtin`. Until locals processing + // exists, drop captures whose pattern carries one; the identifier + // falls back to its non-builtin capture. `#set!` (property + // *settings*) is a different API and is not consulted here. + if query + .property_predicates(qmatch.pattern_index) + .iter() + .any(|(prop, _)| &*prop.key == "local") + { + continue; + } let cap = qmatch.captures[*capture_idx]; spans.push(HighlightSpan { start_byte: cap.node.start_byte() as u32, @@ -1331,6 +1349,43 @@ mod tests { } } + #[test] + fn javascript_shadowed_builtin_is_not_mislabeled() { + // `#is-not? local` (JS/TS use it for console/require/etc.) needs a + // scope map from the LOCALS_QUERY we don't run, so + // `compute_highlight_spans` drops captures guarded by it. Here + // `console` is a LOCAL declaration — it must not surface as a + // `*.builtin` capture (which is what a naive run of the shared JS + // query would produce). + let reg = SyntaxRegistry::new(); + let language = reg.language("javascript").expect("javascript loads"); + let query = reg + .highlights_query("javascript") + .expect("javascript highlights compile"); + let mut buf = fresh_buffer("shadow.js"); + buf.apply_edit(EditOp::Insert { + pos: 0, + bytes: b"const console = 5;\nconsole;\n", + }) + .unwrap(); + let view = ParseView::new(&buf, language, "javascript".to_owned()); + let handle = view.handle(); + let _vid = buf.attach_view(Box::new(view)); + let bundle = parse_synchronously(&handle); + let spans = compute_highlight_spans(&query, &bundle); + assert!(!spans.is_empty(), "the JS query produced highlight spans"); + let names = query.capture_names(); + let builtin: Vec<&str> = spans + .iter() + .map(|s| names[s.capture_index as usize]) + .filter(|n| n.contains("builtin")) + .collect(); + assert!( + builtin.is_empty(), + "a locally-shadowed `console` must not get a *.builtin capture; got {builtin:?}" + ); + } + #[test] fn gap_grammar_extensions_resolve() { let reg = SyntaxRegistry::new(); diff --git a/tests/m4_acceptance.rs b/tests/m4_acceptance.rs index 90d41c0..f922253 100644 --- a/tests/m4_acceptance.rs +++ b/tests/m4_acceptance.rs @@ -5994,11 +5994,13 @@ fn m4_shebang_does_not_override_extension() { ); } -/// Finding-1 gate: an extensionless `#!/usr/bin/env python3` script -/// resolves to python for LSP, but python has no grammar — syntax must +/// Finding-1 gate: an extensionless `#!/usr/bin/env ruby` script resolves +/// to `ruby` via the shebang map, but ruby has no grammar — syntax must /// skip it *silently*. Without the `_has_language` gate, `_dispatch` -/// raises "unknown language: python" (caught by the after-load pcall and +/// raises "unknown language: ruby" (caught by the after-load pcall and /// reported through `pmacs.error`), which we assert does NOT happen. +/// (python/js/lua/bash all ship grammars now, so the gate needs a +/// genuinely grammarless example.) #[test] fn m4_shebang_extensionless_grammarless_language_is_silent() { use pmacs::editor::EditorState;