parser fixes, 100% pass on default syntax tests - #705
Open
titoBouzout wants to merge 21 commits into
Open
Conversation
`ScopeRepository::build` drops trailing empty components (`trim_end_matches('.')`)
and interns the rest, so `meta.annotation..identifier.java` — a doubled dot in
the middle — becomes three atoms with an atom named `""` between them and can
never match `meta.annotation.identifier.java`.
Sublime Text ignores empty components wherever they sit. Asked of it rather than
argued, since this looks exactly like a typo in the test file: a token scoped
`keyword.foo.bar` with one syntax-test assertion per shape, where
`keyword..foo.bar`, `.keyword.foo.bar` and `keyword.foo.bar..` all pass and
`keyword..zzz.bar` fails with "scope does not match" — so the selector is being
evaluated rather than waved through.
It is not only about a test file: a `.tmTheme` selector written with a doubled
dot silently matches nothing today, and nothing reports it.
`syntax_test_java.java` asks for `meta.annotation..identifier.java` nine times.
Haskell 50 -> 49 in the syntest summary here (both engines); the known-failures
files are updated.
Every other lookup in `SyntaxSetBuilder` resolves a reference to the last definition that answers: `find_id` walks the syntaxes in reverse, and the `name_to_index` fallback inside this same function is built by inserting every name in order, so a later definition overwrites an earlier one. Only the `path_syntaxes` loop took the first match. The difference shows the moment one builder holds two copies of a package, which is the supported way to let a user's `Packages` folder override the one that shipped — `add_from_folder` twice, second copy wins. `TSX` is the file it was found on: `extends: JSX.sublime-syntax` resolved to the *first* copy's `JSX` while its other parent resolved by name to the second's, and the "parents derive from different base syntaxes" check then refused the inheritance outright, so every `jsx-*` context TSX inherits went unresolved. `find_parent_index_takes_the_last_definition_at_a_path` is the repro, beside the existing `find_parent_index_resolves_relative_paths`. The syntest summary does not move — no package in `testdata` is loaded twice.
`resolve_extends` refuses an `extends` in three places: the parent could not be found or the chain is circular, the parent's `version` differs from the child's, and the parents derive from different base syntaxes. Only the first goes through the pass at the end of the function that hides the child and gives it `main` and `__start` — the pass whose own comment says why it exists, "so that ParseState::new won't panic if this syntax is still accessed". The other two `unresolved.remove(&child_idx); continue;` and carry on, so the child reaches the set with whatever contexts it declared itself. For a definition that is *nothing but* an `extends` that is none at all, and `ParseState::new` indexes `context_ids()["__start"]` — so the first line parsed with it panics. Nothing in a curated package set can reach it, which is why no test that loads one notices. A user's own folder reaches it with one file and two lines: extending a shipped `version: 2` syntax without repeating the version. The fix is the guard that already exists, applied to all three: the refusals record the child in `broken`, and the final pass runs over `broken ∪ unresolved`. The "parent was not found" warning stays on the missing-parent set only, since the other two have already warned with their own reason. `extends_refused_for_a_version_mismatch_still_parses` is the repro; without the change it panics in `ParseState::new`. The syntest summary does not move.
`MetadataItems` is `#[serde(rename_all = "camelCase")]` and the field is `unindented_line_pattern`, which gives `unindentedLinePattern`. Every `.tmPreferences` file spells the key `unIndentedLinePattern` — Sublime spells this one that way and no other — so the value was dropped on load. It is silent in both directions: the file still deserialises, because `KEYS_WE_USE` has the spelling right and the other items are present, and every other item in it is read. Only `ScopedMetadata::unindented_line` is affected, and it could never answer `true` for any line of any language. Of the 145 scopes in a metadata dump built from `testdata/Packages`, 0 carried this against 59 carrying an increase-indent pattern. One `#[serde(rename)]`. `unindented_line_pattern_is_spelled_the_way_the_files_spell_it` reads C++'s rules, which have one; it fails without the rename.
The reference is exact: "If a `fail` action specifies a `branch_point` that was never pushed on the stack, or has already been popped off of it, it will have no effect." It is the *action* that has no effect. The rule is a rule like any other — its text is consumed, its `scope` applies, and the line carries on. `handle_fail` answers an unresolvable name with `Ok(false)`, and `Ok(false)` is what `parse_next_token` returns to say *stop parsing this line*. So a stray `fail` threw away the colouring of everything after it on that line. CSS has one: `- match: ';' fail: property-or-selector`, reachable outside any branch. `can_rewind_to` is the test `handle_fail` already makes, asked before the rule is routed there at all — in `parse_next_token`, and again in `exec_pattern`, which is where the scopes are applied. An unresolvable `fail` falls through to the ordinary path, where `MatchOperation::Fail` is already a no-op in `perform_op`. That has a second half, and `branch_fail_nonexistent_name` is what found it: a rule let through to `perform_op` that changes nothing must not match empty, or the parser sits at the same position forever. `search_with_end`'s `does_something` already says that for `MatchOperation::None`; an unresolvable `fail` is in exactly that position, so it says it there too. Without it, `- match: '(?=;)' fail: nonexistent` hangs on the first line it meets — which is that existing test's own definition. `fail_for_a_branch_point_nobody_opened_has_no_effect` is the repro. The syntest summary does not move: no assertion in this `testdata/Packages` reaches CSS's stray `fail`.
One of the six behaviours listed under Compatibility, "Regex Capture Group Order": in a version 1 syntax a capture group that begins before the last one that applied is dropped. `build_capture_ops` applied every one of them whatever the file declared, so every version 1 definition was getting version 2's behaviour — quietly, since it is the *better* behaviour and nothing fails. The documentation states the result the wrong way round, and that is the reason this was asked of Sublime Text rather than implemented from the paragraph. Its example is `(?:(x)|(y))+` over `yx` — group 2 matches the `y` at column 0, group 1 the `x` at column 1 — and the text says the `x` will not be scoped. ST 4206 answers the opposite: the `y` is bare and the `x` keeps `identifier.x`. So the rule is not about the group's number at all; the groups are read in capture-index order and one that starts behind the cursor is skipped. Which cursor took a second question, because two readings survive that example and they disagree about the commonest case in the format. Against the last applied group's *end*, `((a)b)` loses its inner group — group 1 is [0,2) and group 2 begins at 1 — and every nested capture in every version 1 definition stops being coloured. Against its *start*, nesting is untouched. ST keeps the inner group, so it is the start. `cab` under `(?:(a)|(b)|(c))+` is the third case: the cursor is carried from group to group, so the `c` at column 0 is dropped after groups 1 and 2 have applied. The mapping is in the order the YAML hash was read, so the version 1 walk sorts by capture index rather than trusting it. `version_1_capture_that_goes_backwards_gets_no_scope` is the repro, with all three cases and version 2 beside each. The syntest summary does not move: no file in `testdata/Packages` exercises it, which is what an audit against the specification is for.
Compatibility, "Multiple Target Push Actions with `clear_scopes`": a `push: [context2, context3]` where both carry `clear_scopes: 1` takes *two* atoms off in a version 1 syntax, before either `meta_scope` goes on. The documentation's example scopes `abc` as `meta.ctx2 meta.ctx3 identifier`, with the file's own `source.lang` cleared away, where version 2 interleaves them — clear, push, clear, push — and leaves `source.lang meta.ctx3 identifier`. This emitted version 2's interleaving at both versions. The two phases answer differently, and Sublime Text 4206 says so: the trigger token sees the sum, and the column *after* it sees version 2's interleaving — `source.lang meta.ctx3`, not `meta.ctx2 meta.ctx3`. So the initial phase clears the sum and pushes the meta scopes over it, and the non-initial phase puts that back with a `Restore` and applies them one context at a time. Getting only the first half right would leave every line after the push inside a scope the file had cleared. `summed_push_clear` is the amount, and `All` swallows the rest, being already everything there is. **This changes an existing test's expectation, deliberately.** The v1 half of `v2_push_multiple_clear_scopes_each_applies` asserted two `Clear` ops, one per context, on the reading that v1 and v2 are the same rule here. They are not: the documentation states the difference and ST answers as it states. The v2 half is untouched, and the v1 half now asserts the sum for the trigger followed by the interleaved pair for the text after it. `version_1_push_clears_the_sum_for_its_trigger_alone` is the repro, both phases and both versions, on the documentation's own example. The syntest summary does not move.
Compatibility, "Embed Escape Match and Meta Scopes": the text an `escape:` matches gets neither the `meta_scope` nor the `meta_content_scope` of the context the `embed:` fired from, in a version 1 syntax. `exec_escape` popped back to that context and left it on the stack for the escape's own captures, which is version 2's behaviour. The documentation's example is a `'`-delimited embed inside a context carrying `meta.group` and `meta.content`: the closing quote comes out `punctuation.end` alone at version 1 and `meta.group meta.content punctuation.end` at version 2. ST 4206 agrees at both, and adds the part the sentence does not say — it is *only* the token. The column after the escape is back inside both scopes. So they come off before the escape's captures and go back on after it, which is the shape a `set:`'s meta scopes already have around their trigger. The version read is the host's: the pop loop above has just returned to the frame the `embed:` fired from, so `self.stack.last()` is that frame. `version_1_embed_escape_is_outside_the_context_that_holds_it` is the repro, both versions and the column after the escape. The syntest summary does not move.
The reference's own example under "Using pop with another action" is a `paragraph` context with a `meta_scope` whose ```py fence fires `pop: 1` and embeds Python. Sublime Text scopes the closing fence with that `meta_scope`; this had it bare. Nothing in the reference says so either way — it is the cross-product of two entries — so ST 4206 was asked, and the columns either side of the escape are what make this a *retarget* rather than a deferral. For `p<a>t`: the `p` that pushes has `m.para`, the `<` firing `pop: 1 + embed:` does not (the pop is a lookahead, which the reference does say), the embedded `a` does not, the escape `>` **does**, and the `t` after it does not. So the frame is off the stack for everything except its own escape, and by the time the escape fires nothing on the stack can say what it was. `EscapeEntry` carries a `holder` — the frame the `pop:` unwound, remembered at the pop, with the flag the pop loop in `exec_escape` already computes for whether its `meta_content_scope` was ever pushed — and the escape pushes its meta scopes before its captures and pops them after. Version 1 wants none of it: there the escape is outside the context that holds it altogether, and the holder is off the stack already. `pop_n_embed_escape_wears_the_context_it_left` is the repro. It asserts every column but the trigger: ST has no `m.para` there, because the pop of a `pop: N + embed:` unwinds before the match takes its scopes, and that is a separate rule and a separate PR (trishume#687). The syntest summary does not move.
`ParseLineOutput::replayed` carries the ops of lines the parser has already
handed back and has since decided differently about: a `fail` two lines later
rewinds to a branch point above and re-parses what was buffered. They are handed
out **positionally** — the i-th entry for the i-th buffered line — and, ten lines
away in the same function, a second run is *appended* when a second cross-line
`fail` fires on the same `parse_line` call, with the comment "in case multiple
cross-line fails fire on the same parse_line call". Both cannot be right, and
what happens is that one line's ops land on another line's text.
`flushed_ops` and `replayed` are now `Vec<(usize, Vec<(usize, ScopeStackOp)>)>`,
each correction tagged with the parser's own 1-based `parse_line` count, worked
out where the replay happens: the current line, less the buffered lines before
it, plus the offset into that buffer. Appending then becomes harmless — a later
correction for a line replaces an earlier one — and no consumer has to
reconstruct which lines were buffered.
`examples/syntest.rs` is that consumer, and it is where this shows. It paired
`replayed` with the tail of its own line buffer, and **four of the six files
that panic in the harness today panic there**, in
`ScopeRegionIterator::new(replayed_ops, &record.line_text)` at `easy.rs:289`
— "end byte index N is out of bounds for string of length M", which is precisely
ops from a longer line being walked over a shorter one. With the corrections
keyed by line, those four run to completion: Java, TypeScript, Python and Bash
(and Zsh) stop crashing the harness.
That is what moves the known-failures files, and it moves them *up*, because a
crashed file was recorded as `1`:
Java 1 -> 33243 (was: panic)
Zsh 1 -> 604 (was: panic)
Bash 1 -> 285 (was: panic)
TypeScript 1 -> 230 (was: panic)
Python 1 -> 66 (was: panic)
C# GeneralStructure 3 -> 2 (a real correction, applied to the right line)
Those five numbers are what those files have always failed; the table was
recording the crash instead. Both known-failures files are updated, and both
engines agree.
The sixth panic is not this one and is still there: Markdown, inside the parser
itself ("start byte index 2 is out of bounds for string of length 1"), which is
the out-of-bounds trishume#657 is about.
`a_cross_line_fail_names_the_lines_it_corrects` pins the tag on three lines: `d`
opens a branch, `q` is buffered, `z` fails it, and the corrections must be for
lines 1 and 2 and say so.
`ParseLineOutput::replayed` is now `Vec<(usize, Vec<(usize, ScopeStackOp)>)>`. It is a breaking change to a field added in the same unreleased cycle as the cross-line replay itself, and it is the point of the change before this one: a correction that does not name its line cannot be applied to the right one.
…,874 -> 141 The ten commits before this one are the changes that stand on their own, ported by hand into upstream's shape. This one brings the remaining twenty-four over wholesale, so that `src/` here is byte-identical to the fork it came from (`text-editor`'s `vendor/syntect`, itself vendored from 4aa7803) and the two trees can be diffed for drift with `diff -r`. What arrives with it, by file: - `regex.rs` — the literal prefilter. Every pattern's leading literals are taken off with `regex-syntax` and searched for with `memchr` / Aho-Corasick before the engine is asked; a pattern whose literal is absent from the rest of the line is not searched. It is a *necessary condition only*, and on a debug build every rejection is put to the engine it skipped, so a disagreement between `regex-syntax` and Oniguruma fails there rather than colouring something wrong. `Cargo.toml` gains `memchr` and `aho-corasick` as optional deps in the `parsing` feature. - `parser.rs` — the branch/replay machinery, the `pop: N` family, the version gates, `clear_scopes` ordering, the escape rules, the zero-width guards. `MatchOperation::Push` becomes `Push { ctx_refs, pop_count }`: `pop: N + push:` was read as a bare `Pop(N)` and the push was dropped, and the trigger's scopes differ from a `set`'s (the pop is a lookahead). That is the one breaking change to the public API here, and the snapshot is updated. - `syntax_set.rs`, `yaml_load.rs`, `syntax_definition.rs` — `extends` merged by distance rather than by list order, a child's inherited `main` carrying its own scope and not its parents', prototypes not followed through an `embed:`, `captures:` narrowed to consuming groups after the regexes are final. - `dumps.rs` — `dump_uncompressed_to_writer`, five lines, for callers that write a dump behind their own header. Every one of them is written up at its site, and the fork's `FORK.md` carries the same list with the probe and the per-language number behind each. `assets/*.packdump` are regenerated (`make packs`): the serialised `SyntaxSet` changed shape with `MatchOperation::Push`, and the shipped dumps could not be read at all — `cargo test` aborted on a 7-exabyte allocation before this. Upstream's own corpus, `make syntest`, both engines, no panics anywhere: before after ASP 53 0 Batch File 74 0 C# General 3 0 Haskell 50 6 Java 33,243 0 (was a harness panic, recorded as 1) JSP 44 0 TypeScript 230 0 (was a harness panic) LaTeX 76 0 Markdown 1 0 (was a parser panic — trishume#657's) PHP 1 0 Python 66 0 (was a harness panic) Python strings 1 0 Rails HAML 65 0 Rails ERB 23 0 Bash 285 62 (was a harness panic) Zsh 604 15 (was a harness panic) ------ ----- 34,874 141 What is left is C#11 (35), C# Generics (3), git_config (17), Bash (62), Zsh (15), Haskell (6) and four one-assertion files. The fork measures against a *newer* `Packages` than the v4050 pinned here and is at zero on all of them, so these are worth re-reading against a newer corpus before they are read as parser bugs. `examples/syntest.rs` needed one more fix to survive its own corrections: after folding them in it now re-walks the file from the top rather than from the earliest corrected line. A correction is the whole of a line's ops, and a `Restore` in one can be paired with a `Clear` emitted on a line that was not corrected — so a walk that starts in the middle meets a `Restore` with nothing cleared, which is what `syntax_test_java.java` did, panicking with `NoClearedScopesToRestore`. It costs a re-walk per batch of corrections, and the whole corpus runs in 41 s. Two of upstream's own tests change their expectations, both deliberately and both explained at the test: `can_parse_backrefs` (an embedded `SQL (Basic)` arrives with one scope of its own, not its ancestors' chain) and the v1 half of `v2_push_multiple_clear_scopes_each_applies` (a version 1 multi-context `push:` clears the sum, which is what the reference says and what Sublime Text does). The comments still carry `TEXT-EDITOR FORK:` markers and a few paths into the fork's own tooling — that is how the fork finds its change sites, and stripping them is the last pass before any of this is opened as a PR.
`ScopeSelectors::from_str` splits on `,` and `|` and stops. There is no `&`, no grouping and no error, so `(source.c | source.c++ | source.objc | source.objc++) & comment.block` parses into four selectors of which one is a bare ` source.c++ ` — and `does_match` answers `Some(MatchPower(2.0))` for the stack `[source.c++]`, a file that is not in a comment at all. It is not academic. `Packages/C++/Indentation Rules - Comments.tmPreferences` is that selector and the only thing it carries is `unIndentedLinePattern: .`, so `metadata_for_scope(&[source.c++])` hands a caller "every line of this file is outside the indentation". The grammar, with the precedence Sublime Text gives it: - `,` and `|` — union, and the loosest - `&` — conjunction, tighter than union - ` -` — exclusion, tighter still, and written with a space in front, because a scope name may contain a dash (`entity.name.function.post-blit`) - `( … )` — grouping An expression is flattened to a union of simple selectors, which is what `ScopeSelector` holds and what `does_match` is written against. Distributing is also what makes `a - (b & c)` expressible: it is `(a - b) | (a - c)`. **The precedence and the scoring were asked of the application, not read.** `sublime.score_selector(scope, selector)` answers with a number, so it settles matching and ranking at once, and three of its answers decided the design: - `(source.c | source.c++) & comment.block` scores 0 on `source.c++` and 32 on `source.c++ comment.block`. A conjunction is not a descendant path. - `source.c | source.c++ & comment.block` scores 4 on a bare `source.c`, so `|` binds looser than `&`. - On `source.c++ comment.block`, `source.c++` scores 4, `comment.block` 32, the path `source.c++ comment.block` 36 — the two added — and the conjunction 32, the larger of the two, in either order. So a conjunction's score is the strongest of its operands, not their sum. `ScopeSelector` gains `conjuncts: Vec<ScopeStack>`, and `extract_scopes` and `extract_single_scope` account for it. An *empty* exclusion still excludes everything, which `empty_stack_matching_works` pins and which the distribution therefore keeps rather than dropping. `assets/default.themedump` is regenerated: it is `bincode` over this type, and without `make themes` every test that touches a default theme aborts the process on a nonsense allocation. `conjunction_and_grouping_answer_as_sublime_text_does` is the repro and carries ST's table. `cargo test` passes whole; `make syntest` and `make syntest-fancy` do not move.
… line Two rules Sublime Text has that this harness does not, found by porting a fork onto this tree and then asking ST what it makes of the files that came out failing. Both were reported against ST's own harness years ago (trishume#654, trishume#678); this is the example here having them too. **The closing testtoken is everything after the quoted path, alphabetic or not.** The header regex restricted it to punctuation, with a comment saying that an alphabetic tail like the `clojure` in `#! SYNTAX TEST "…" clojure` must not be "mis-captured". It is the other way round: ST strips such a tail exactly as it strips `-->`, and a multi-word tail (`dotnet run`) is one token. Asked directly, with a `foo` tail against a selector `keyword.foo.bar foo`, which passes, and against one naming a scope that is not there, which fails. Until now every assertion in `syntax_test_shebang.clj` and `syntax_test_shebang.d` compared a selector ST would have clipped. **A column past the end of the line under test is the corresponding column of the next row.** `text_point(row, col)` overflows a column beyond its row into the row below, and the syntax tests inherit that. This harness reused the target's last scope for every such column instead — the consumed newline, carrying whatever the end-of-line pop chain left there. Probed three ways: a `^^^` run three columns past a two-character line passes for `comment.line`, which is the scope of the assertion line below it, fails for `punctuation.definition.comment`, which is only that line's first column, and an empty target line has all of its assertion's columns compared rather than none. `process_assertions` now takes the runs of the following line and indexes them at `column - end_of_line`. Having them means parsing that line before its own assertions are judged — a clone of the parser and one extra parse of one line per target — and the replay path builds the runs of every buffered line in a first pass for the same reason. Neither rule is about the parser, and the corpus says so. Nothing regresses, nothing new appears, no panics: Git Formats/git_config 17 -> 0 ShellScript/Bash 62 -> 26 Clojure/clojure.clj 1 -> 0 Clojure/shebang.clj 1 -> 0 D/shebang.d 1 -> 0 --------- 141 -> 85 ST passes all 2,025 assertions of the four files that reach zero, and the parser on this branch passes all 11,654 of them through a different harness — which is what said the harness was the thing that differed. Both known-failures files are updated and both engines agree.
The submodule was pinned at `1ba99a47` (v4050-1502), April 2026. The
`known_syntest_failures*.txt` this branch has been regenerating are therefore a
measurement against definitions four hundred commits old, and a good part of
what was left in them was the definitions rather than the parser — which is
exactly what asking Sublime Text about those files said: ST fails the stale ones
harder than this branch does.
So the corpus moves to `69457400` (v4206-2), and the numbers become about the
parser again:
v4050 v4206
this branch, total 85 47
C#11 35 0
C# Generics 3 0
Haskell 6 6
Bash 26 26
Zsh 15 15
C#'s two files carried every one of the 38 that went: their definition has moved
4,549 lines since v4050 and `syntax_test_Generics.cs` no longer exists. Nothing
regressed, no file appeared, and both engines agree — `known_syntest_failures.txt`
and `known_syntest_failures_fancy.txt` are byte-identical to each other now.
For the record, master on this same corpus is **414 across 21 files, with eight
panics** — seven in the harness (`easy.rs:289`) and one in the parser — against
this branch's 47 across three files and none.
What is left is Haskell 6, Bash 26 and Zsh 15. Nineteen of Bash's are not parser
failures at all: fifteen are `${parameter#pattern}` inside a comment read as an
assertion whose selector is `pattern}`, and five are the prose
`] should close the conditional`. That is trishume#674, which is not in this branch.
The corpus bump in the previous commit moved C's preprocessor scopes:
`#ifdef` / `#elif` / `#endif` are `keyword.control.directive.conditional.{if,
elseif,endif}.c` in Packages v4206 and were `keyword.control.import.c` at v4050.
The test reads `testdata/Packages`, so its expectations are about whatever
revision the submodule points at.
Nothing about the parser changed here; the three scope names in the test did.
The comment at the top of it says which revision they belong to, so the next
bump has somewhere to start.
I committed the bump before running `cargo test` — this is that oversight
corrected rather than folded away.
…tion — and the corpus reaches zero
The third harness rule, and the one that turns out to be holding up the last
number: `get_line_assertion_details` recognised any line where the testtoken
appeared, wherever it appeared. ST runs **zero** assertions on a line with
anything but whitespace in front of the token, so markers found after source text
are a coincidence.
Bash's own corpus is where it shows. `: ${#^pattern}` contains the testtoken, so
the harness read that *source* line as an assertion whose selector is `pattern}`
— and, having decided it was an assertion, stopped treating it as the line the
assertions below it are measured against. Both halves were producing failures:
five columns of nonsense selectors, and every real assertion under those lines
compared against the wrong text.
This is trishume#674, and with it the whole corpus passes:
v4206
before this commit 47 (Haskell 6, Bash 26, Zsh 15)
after 0
`known_syntest_failures.txt` and `known_syntest_failures_fancy.txt` are now empty
of `FAILED` rows on both engines.
**It is not passing by skipping.** Bash runs 40,789 assertion columns after the
change against 40,793 before — four fewer, which are exactly the coincidental
ones. For scale, ST runs 11,065 assertions of its own on that file and fails
none of them, which is what said this residue was the harness rather than the
parser in the first place.
The comments carried `TEXT-EDITOR FORK:` on every change site — 129 of them — because that is how the fork finds its own diff by grep. Upstream has no use for it, so it is gone, along with the things it dragged in: paths into the fork's probe tooling (`tools/sublime-oracle/probes/…` becomes the probe's own name, which is all a reader needs), references to `change N` in a document that does not exist here, and four sentences describing the downstream application rather than this library. Comments only. No line that is not a comment differs from the commit before this one, `cargo test` is 194 + 8 + 1 + 13 as before, and `make syntest` reports no failing file. The prose that was after each marker stays as it was — it is the reasoning, and several of those paragraphs are the only record of what Sublime Text answered when it was asked.
… read them `indentSquareBrackets`, `preserveIndent` and `indentOnPaste` are not in that list, and a key that is not in it is discarded while the raw entries are merged into a `Dict` — before `MetadataItems` is deserialized at all. So a field for one of them could not have worked: it would have read `None` for every file that carries the key, and nothing anywhere would have looked wrong. `indentParens` is the odd one out — in the list, on the struct, and simply never surfaced on `ScopedMetadata`. They are the half of the indentation rules that is not a pattern. A pattern says "a line that looks like this opens a block"; a flag says "an unclosed bracket at the end of a line does", which no pattern can express, because it has to count. In `sublimehq/Packages` nine files carry `indentSquareBrackets` and five `indentParens` — Go, Perl, Gomod, Shell and Zsh, every one of them turning the behaviour *off* for a language whose own `increaseIndentPattern` already says when a bracket matters. `preserveIndent` (12 files) and `indentOnPaste` (1) are the same shape for text that arrives in a block rather than being typed: whether a reindent may touch the indentation it came with. The twelve are heredocs and block comments, which is the whole of what that key is for. Three names, three `Option` fields, four accessors. No parser code, and the cost is at load only — this is read once per set. **`Option<bool>` and not `bool`, because the default is a file and not a value.** The reference gives each key a sentence and never says what a definition that mentions none of them gets, which is the only question that matters when every file that carries one is turning something off. Asked of Sublime Text 4206 rather than argued: pressing Return at the end of `foo(a,` and `x = [1,` in nine languages, ST indents after the paren and not the square bracket in Rust, whose `.tmPreferences` is two brace patterns and nothing else — and indents after neither in plain text, which has no `.tmPreferences` at all. The answer is that ST's own `Default` package ships one scoped to bare `source` carrying `indentParens: true`, and another scoped to `comment` carrying `preserveIndent: true`. That package is closed and is not in `sublimehq/Packages`, so nothing here can ship it; what this library can do is report what a file said and leave "and nobody said" distinguishable from "no". `the_indentation_flags_are_read` is the test, over `testdata/`'s own files: Go turns all three off, JSON turns one on — so `Some(false)` cannot be a deserializer answering `false` for everything — and Rust mentions none of them. `assets/default_metadata.packdump` is regenerated (`make packs`). It is a dump of `Metadata`, so three new fields change its shape: without the regeneration every `load_defaults` with the metadata feature on comes back `InvalidTagEncoding(6)`, which is fourteen tests, none of which fails when it is run on its own. The two syntax packdumps `make packs` also writes are left as they are — what they disagree with the tree about is not this.
…s as the files have them `RUSTDOCFLAGS='--deny warnings' cargo doc --document-private-items` is a CI job here, and it failed on this branch: `Regex`'s documentation linked `[`Prefilter`]` and `ScopeSelectors::from_str` linked `[`SelectorExpr`]`, both private, both added by this branch. Rustdoc resolves them only because that run passes `--document-private-items` and says so. They are plain code spans now and the job passes. And the counts beside `KEYS_WE_USE` were taken with `grep`, which counts a key named inside a comment (`interacts with indentSquareBrackets`, in JSON's own `decreaseIndentPattern`) and a whole block commented out (PHP's heredoc-end file). Parsed as plists, `testdata/Packages` at this pin has seven files carrying `indentSquareBrackets` and four `indentParens`, not nine and five — nine and five is what the count comes to with third-party packages in — and none carrying `indentOnPaste` at all. "Every one of them turns the behaviour off" was wrong too: C++ and JSON turn square brackets on, and nine of `preserveIndent`'s twelve say `false` at the language's own scope, which takes back the default the `Default` package sets rather than restating it. Comments only; no behaviour changes. `cargo test --features metadata` 201 + 8 + 1 + 14, `make syntest` and `make syntest-fancy` still report no failing file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`cargo fmt -- --check` is a CI job here and it failed on this branch: 29 hunks, all of them in lines this branch added — upstream's own code was already clean. Whitespace only. `cargo test --features metadata` 201 + 8 + 1 + 14, `make syntest` and `make syntest-fancy` both report no failing file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
parser fixes, 100% pass on default syntax tests
Ran into an issue with a syntax file, so I put Claude to investigate, and it got in a rabbit hole/loop of fixes for a couple of days/weeks that involved checking issues/prs/ST syntax documentation and cross checking against ST v4206 via a live plugin probing apis.
Long story short it got to pass 100% of default packages syntax_tests. I've instructed to avoid any refactor/performance improvements and concentrate on correctness. It reused/fixed/discarded existing prs/issues and made its own.
Disclaimer: I haven't written a line of rust on my life. So Im uncertain on the quality. Ive vendored syntect with this patch, so probably I won't follow up, cant really evaluate this myself. Didnt split in different prs for those reasons. The code/information provided may be useful.
What follows is all ai generated.
Thirty-five parser fixes, three harness rules, and this corpus at zero
A fork of syntect that has been in production use for some months. Every change
in it exists because a file came out coloured wrong, and where the reference did
not settle what the right answer was, Sublime Text was asked directly rather than
argued with — it documents every feature on its own and never the cross-product,
which is where most of these bugs live.
make syntestreports no failing file, on both engines, against 414 failingcolumns and eight panics on master — as master's own harness reports it, and
part of that 414 is the harness rather than the parser, which this branch fixes
too. It also settles 38 of the 63 open pull requests — applied, fixed
differently, superseded or contradicted — and the tables under "Your tracker"
are the actionable part of this.
Prior art: most of the diagnosis in the parser half is @stefanobaghino's, in
the #631 chain. Five of these changes are that patch carried over, and several
more are the same root cause found by following a PR body to the file it names.
Where this branch differs, it is because ST was asked something the PR did not
ask — and those answers are in the tables.
1. The numbers
testdata/Packageswas pinned at1ba99a47(v4050-1502, April 2026), so thetables in the repository measured against definitions four hundred commits old.
This branch bumps the submodule to
69457400(v4206-2) and regenerates them.Everything below is on that corpus.
The largest files, for scale: Java 44,362 assertion columns, Bash 40,789,
MySQL 28,549, Zsh 28,463, Markdown 25,705, PHP 21,338, Go 19,153.
Master's eight panics are recorded in its table as
1each — seven in theharness (
easy.rs:289, ops from one line walked over another's text) and one inthe parser. The
1is the crash, not the file: at a point in this branch wherethe harness was fixed but most of the parser work was not, those files reported
figures in the tens of thousands (Java 33,243).
How the last failures went, for anyone reproducing this:
The 38 the bump took were C#'s two files: its definition has moved 4,549 lines
since v4050 and
syntax_test_Generics.csno longer exists. The 47 #674 took wereone shape — a source line containing the testtoken read as an assertion, which
both invents a selector (
pattern}, out of${#^pattern}) and stops that linecounting as the one the assertions below it are measured against. Haskell's six
were the same thing:
-->in a file whose testtoken is--.2. What is in it
branch/fail, exhaustion, loop guards, cross-line replayextends, prototypes,captures:(one of them a panic on a user-written file)pop: Ncombined withset/push/branch/embedclear_scopesorderingembed:/escape:andembed_scopeversion: 1compatibility gates.tmPreferencesmetadatadump_uncompressed_to_writerexamples/syntest.rs— #654, #674, #678The prefilter (
regex.rs) takes the literals every match of a pattern must beginwith and skips the engine when they are absent from the line. It is a necessary
condition only, and on a debug build every rejection is put to the engine it
skipped, so a disagreement between
regex-syntaxand Oniguruma fails thererather than colouring something wrong. Drop it if it is unwanted; nothing else
depends on it.
Four that no test file catches
Found by reading the reference against the parser, each confirmed with ST:
version: 1behaviours were ungated — capturegroup order, the summed
clear_scopesof a multi-contextpush:, and anembed:'s escape. Nothing in the corpus exercises them, so nothing failed; inthe set this fork builds, 159 of the 334 definitions read from a file declare
no
version:at all — which is version 1 — and were getting version 2'sanswers. The reference states the capture-order rule backwards —
its example says the
xloses its scope, ST drops they— so implementing itfrom the paragraph would have put it in reversed.
extendsleft a syntax with nomain, andParseState::newpanicked on it. The guard exists already for the missing-parent case; the
version-mismatch and different-base refusals were not routed through it. Two
lines in a user-written syntax reach it.
ScopeSelectorshad no conjunction.(source.c | source.c++) & comment.blocksplit into four selectors, one a baresource.c++, so acomment-scoped
.tmPreferencesanswered for a whole language.&, groupingand their precedence are implemented, with
score_selectorbehind each rule.KEYS_WE_USEdropped three of the indentation keys —indentSquareBrackets,preserveIndentandindentOnPaste. A key not inthat list is discarded while the entries are merged, so a field for one of
them would have read
Noneeverywhere and nothing would have looked wrong;indentParenswas in the list, on the struct, and never asked for. They arethe half of the indentation rules that is not a pattern — a pattern cannot
count brackets — and in
testdata/Packagesat this pin seven files carry thefirst and twelve the second, with
indentOnPastein none of them (it is nine,twelve and one across the whole set this fork builds, third-party definitions
included). Three names, three
Optionfields, four accessors, no parsercode, and
the_indentation_flags_are_readfor the three. What each onemeans was measured rather than read: the reference gives them a sentence and
never says what a definition that mentions none of them gets, and the answer
is that ST's own closed
Defaultpackage ships a.tmPreferencesscoped tobare
sourcewithindentParens: true— so the default belongs to a file andnot to this library, which is why these are
Option<bool>and notbool.assets/default_metadata.packdumpis regenerated with them, asmake packswrites it.
3. Breaking changes
ParseLineOutput::replayedbecomesVec<(usize, Vec<(usize, ScopeStackOp)>)>:each correction carries the 1-based
parse_linecount it is for. Thepublic-api snapshot is updated.
MatchOperation::PushbecomesPush { ctx_refs, pop_count }— this is Distinguish pop:N+push:/branch:/embed: from pop:N+set: at MatchOperation IR #687.ScopeSelectorgainsconjuncts: Vec<ScopeStack>.assets/*.packdumpandassets/default.themedumpare regenerated, and mustbe: both are
bincodeover types that changed shape, and withoutmake packs/
make themesthe test suite aborts on a nonsense allocation. Worth knowingfor any future change to a serialised type.
test:
can_parse_backrefs(an embeddedSQL (Basic)arrives with one scope ofits own, not its ancestors' chain) and the v1 half of
v2_push_multiple_clear_scopes_each_applies(a version 1 multi-contextpush:clears the sum — Compatibility says so and ST agrees).
4. Your tracker
Your patch, carried here
pop: N + set:PackagesSame fix, arrived at independently
Found here before the PR was read, or written from its description; either way the
bug and the remedy are the same, so they are yours to close.
apply_prototype's external prototype before the targetpop: N + push:dropped the push; distinguish it at the IR(pre, post]Same bug, fixed differently — with the reason
flushed_opsbp.pop_countin the retain predicatefail:after certain pops silently did nothing before.pop: N + set:unwinds one frame at a time instead of summing, which is what makes the deeper frames'clear_scopesrestorable at all.curmcs Pop when the wrapper hasembed_scope_replacesmeta_content_scopewas never pushed. Asking the stack later cannot tell a guest's own context from one pushed on top of the wrapper.clear_scopes, and what the trigger carriesset:'s targets may emit its Clear, in both phases, and a separate change settles what the trigger token of a multi-contextset:carries. Probed rather than derived.cur'smeta_scopeon apop + embedmatchpop: N + push:/branch:/embed:lookahead rule, which ST answers identically for all three.Close as superseded — the failures they fix are 0 here
#658#659#662#663#666#682#686#688#689#690All are cross-line replay bookkeeping, or arbitration between an outer replay
and an inner one. This branch removes the arbitration instead: a
failthat fires inside a replay does notstart a replay of its own — it hands back, and one driver runs the rounds in
order, so the reading that survives is the last one chosen rather than the last
loop left running. With that,
pop: N + branch:unwinds on the retry as well ason the first firing, and a branch that ran out on an earlier line hands the
position back inside the replay too.
Java is the file that chain is aimed at (issue #631), and it is at 0 here on
the same
testdata— where #690 reaches 0 by forcing alternative 5 on a branchpoint named
class-memberswith an alt index below 5.#680(bumptestdata/Packages) is also here, to v4206-2 rather than the v4202that PR pins.
#679(drop Haskell from the baseline) is moot: Haskell is at 0,so the row it removes is gone.
Close as not-a-fix — ST answers otherwise
meta_scopebefore apop: N + set:triggerpop: 2 + set:gives the triggersource meta.outer meta.mid meta.tail keyword.trigger. #683 removes atoms ST has on that token. The variant that does drop them ispop: 2 + push:— that is #687, and it is in.meta_scopesublime-mcp), so it is measurement against measurement rather than argument. Ours: one definition, four shapes on four lines — atoms adjacent, separated by the target'smeta_scope, only the innermost restated, and the plain-push:nesting the PR itself says must not collapse — read back through ST's own syntax-test harness. ST keeps the repeat in all four. If the doubling on@ClassName.FixMethodOrder(...)is real, it comes from the unwinding rather than from a missing collapse rule: Java'sannotation-qualified-identifier-nameis at 0 here with nothing collapsed, oncepop: N + branch:unwinds on the retry as well as on the first firing. The probe is four lines and re-runnable.Not evaluated, and not in this branch
#648,#652,#672are in areas this branch also changes (captures insidelookarounds, the nested-embed escape leak, fragment embeds), and their files —
LaTeX among them — are at 0 here. I have not traced each to a specific change,
so treat them as "check against this branch" rather than as closed.
as_latex_escapedemits(r, b, g)and re-escapes its ownbackslashes (issue
as_latex_escaped: two small issues #472). Real, and untouched here: nothing in this fork callsthat function.
plistfrom 1.8 to 1.10 #703, and the 6.0 series Add scopedump differential highlighting harness #696–Delete the legacy op-correction engine and finish the 6.0 sweep #701.5. Reviewing it
Ten of the commits are one change each and are the place to start. Then, in
order: the public-api snapshot, the commit that brings the remaining changes
across together, the selector grammar, the first harness commit (#654, #678), the
corpus bump, the one test expectation that bump moved, the second harness commit
(#674), a comments-only pass, and last the
KEYS_WE_USEone — which is a changeof its own with its own test, and landed after the rest.
Commit subjects carry the numbers as they stood at that commit — the one that
brings the bulk across says "34,874 → 141", which was true against v4050 with the
harness only half fixed. The final figures are the ones in §1.
cargo test194 + 8 + 1 + 13;--features metadata201 + 8 + 1 + 14.make syntestandmake syntest-fancyboth report no failing file, and the tables intestdata/are regenerated and empty of
FAILEDrows.ones ported by hand, one commit each. The rest are pinned by a suite that lives
in the repository that vendors this fork, plus the corpus numbers above; those
tests are written against the same behaviours and can be brought across on
request. That is the honest weak point of this branch as a review artefact.
Treat shebang header tail as testtoken_end in syntest harness #678 and Collapse rule scope atoms duplicating popped meta_scope #684 all record probing Sublime Text for ground truth rather than
arguing from the docs. This fork industrialises it — a probe plugin, 69
documented-rule cases with ST's answers recorded, and a
score_selectorjob.