Repository navigation
refactor(tokenizer): make Dialect a struct of independent grammar axes - #4008
Merged
Merged
Conversation
📊 Automated PR Analysis
SummaryRefactors arg_tokenizer's Dialect from a two-variant enum into a Copy struct of five independent grammar axes (single-dash behavior, attach separator, dash-dash semantics, name case, slash flags), preserving Posix and Msbuild as presets and adding unused Maven/Gradle/GoFlag presets for future callers. Validated with an 88,740-vector differential test against a frozen pre-refactor oracle, mutation testing, runtime output comparisons, and benchmarks showing no perf regression. Review Checklist
Analyzed automatically by wshm · This is an automated analysis, not a human review. |
KuSh
force-pushed
the
feat/tokenizer-dialect-axes
branch
from
September 17, 2026 22:40
7573cd4 to
a41cc10
Compare
This was referenced Sep 18, 2026
`Dialect` welded four independent axes into two variants, so a tool whose grammar mixed them had to compensate in its own filter rather than declare it. Measured against the real binaries, maven, gradle and go each want a combination neither `Posix` nor `Msbuild` offers. Split it into five `Copy` axes — `single_dash`, `attach`, `dash_dash`, `name_case`, `slash_flags` — with `Dialect::Posix` and `Dialect::Msbuild` kept as presets, so no call site changes. Adds `Maven`, `Gradle` and `GoFlag` presets for the grammars now evidenced. Behaviour preservation is checked by a differential test against a frozen copy of the pre-axes implementation: 88,740 arg vectors (every combination up to four tokens over an alphabet covering each construct the scanner branches on) x 3 value grammars, asserted token-for-token identical under both presets, plus the lookup helpers that observe the case axis. Mutating any one axis of either preset fails it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The `/flag`-vs-path guard split on a hard-coded `['=', ':']`, which only matched the one preset that has both `slash_flags` and `:`. With the axes independent, a `slash_flags` dialect attaching on `=` alone would hide the second `/` in `/opt:a/b` and promote the path to a flag. Also scopes `before_dashdash`'s warning to every role that keeps classifying, not just `Forwards`, and records that `EndsGlobalOptions` tokenizes identically to `Forwards` and differs only in whose the tail is. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A preset name has to answer "can my tool reuse this?". "Is my tool commons-cli?" is checkable — `maven:3-eclipse-temurin-21` ships `commons-cli-1.11.0.jar`, and the multi-character short options (`-pl`, `-am`, `-gs`, `-emp`) are the library's property, not Maven's. "Is my tool Maven?" is not, so `Dialect::Maven` becomes `Dialect::CommonsCli`. Drops `Dialect::Gradle`. Gradle's parser is its own `org.gradle.cli` (`gradle:jdk21` ships `gradle-cli-*.jar` and no commons-cli), shared with nothing, and it is already just `Posix` with one axis changed — so the gradlew caller composes it at its own call site. The `EndsGlobalOptions` axis value stays; only the named preset was unjustified. Records the rule in `src/core/README.md`: a preset names a grammar family several tools can share; a single tool's bespoke parser composes its axes at the call site. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`SingleDash::Atomic` tags its flags `TokenKind::Long`, so a caller keying its predicate on `Short` — the word its own tool uses for `-pl` — gets `None` back and the flag silently stops claiming its value. Nothing said so, and no test pinned it. `Dialect::CommonsCli` does not model commons-cli's Java-property options: `-DskipTests=true` is the flag `DskipTests`, not `D` with a value. Also documented and pinned, along with the fact that it stays a non-positional, which is all goal detection needs. Makes the `single_dash` prefix match exhaustive, so a future variant is a build failure rather than a silent `double_dash: false`, and scopes `injection_point`'s justification to cover `EndsGlobalOptions` too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
KuSh
force-pushed
the
feat/tokenizer-dialect-axes
branch
from
October 6, 2026 22:35
a41cc10 to
cd9d7a6
Compare
Open
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
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.
arg_tokenizer::Dialectwelded four independent grammar axes into two variants. Measured against the real binaries, five tools want five different combinations:--semantics/flag==or:===--flag≡-flag)Because the axes were fused, three callers compensated in their own filter instead of declaring their grammar.
The axes
Dialectis now aCopystruct of five fields. Each earned its place from a row above that no existing variant could express:single_dashCluster/Atomic/AtomicAliasingLongmvn -Bo validateerrors whilemvn -B -o validatesucceeds, and-plcould not exist if single-dash clustered;gradle -qi tasksdoes cluster; go's-runis also--runattachEquals/EqualsOrColon--logger:trxand--logger=trxare both valid dotnet syntax; nobody else takes:dash_dashEndsOptions/Forwards/EndsGlobalOptionsname_caseSensitive/Folded/nologo==/NoLogo); commons-cli distinguishes-bfrom-Bslash_flagsbool/bl:x.binlogis a switch for MSBuild and a path everywhere elsefind's-exec cmd ;is deliberately out of scope: a variadic value ended by a sentinel is anAttachmentvariant, not aDialectaxis.The presets
A preset names a grammar family several tools can share — a parser library, or a real convention — so the name answers "can my tool reuse this?". A single tool's bespoke parser gets no preset; its caller composes the axes at its own call site. That rule is what stops this list growing one variant per tool, and it is written down in
src/core/README.md.Dialect::PosixandDialect::Msbuildstay, as associated consts, so every existing call site compiles and behaves unchanged.Dialect::Posix— the GNU/POSIX convention:Cluster,=,EndsOptions, sensitive, no/flag. git, cargo, rg, golangci-lint.Dialect::Msbuild—Atomic,=/:,Forwards, folded,/flag. dotnet.Dialect::CommonsCli— Apache commons-cli: Posix withAtomic.maven:3-eclipse-temurin-21shipscommons-cli-1.11.0.jar, and the whole-word short options (-pl,-am,-gs,-emp) are the library's property, not Maven's — which is whymvn -Bo validateerrors while-B -osucceeds. Maven is the first consumer, not the definition.Dialect::GoFlag— Go's stdlibflagpackage: Posix withAtomicAliasingLong.Gradle gets no preset. Its parser is its own
org.gradle.cli(gradle:jdk21shipsgradle-cli-*.jarand zero commons-cli), used by nothing else. It isPosixwithdash_dash: EndsGlobalOptionsand nothing more — gradle clusters (-qi tasksworks) and its value-taking shorts are theValueSpec::solo_only()shape git's-nalready uses (-qp /wfails,-p /wworks) — sogradlew_cmd.rscomposes thatconstat its own call site. TheEndsGlobalOptionsaxis value stays: it is shared infrastructure, and only the one-tool preset was unjustified.The presets with no in-tree caller yet carry
#[allow(dead_code)]until mvn and go migrate, as doesEndsGlobalOptionsuntil gradlew does.Two contracts a migrating caller would otherwise trip over are documented and pinned by tests:
SingleDash::Atomictags its flagsTokenKind::Long(a predicate keyed onShort— the word mvn's own docs use for-pl— never fires), andDialect::CommonsClidoes not model commons-cli's Java-property options, so-DskipTests=trueis the flagDskipTests, notDwith a value. It stays a non-positional either way, which is all goal detection needs.Differential evidence
src/core/arg_tokenizer/frozen.rsis the pre-axes implementation, kept verbatim as an oracle.differential.rsgenerates 88,740 arg vectors — every combination of length 1..=4 over a 17-token alphabet covering clusters, attached values,=/:forms,--in every position,/flagagainst/path, digit runs (-20), a bare-, empty strings and non-ASCII in both a cluster and a long name — crosses each with 3 value grammars (nothing takes a value, a mixed table exercising everyAttachmentandclaims_dash_dash, everything takes a value), and asserts the newPosixandMsbuildpresets are token-for-token identical to the frozen implementation on all sevenTokenfields, plushas_flag/has_double_dash_flag/double_dash_flag_valueover 8 lookup names (the only way the case axis is observable). 266,220 comparisons per preset; all pass.Mutation-checked: flipping any one of the five axes on either preset fails the test.
Runtime check:
rtk git log -10,log --oneline -20,log -n 2 --stat,log --grep -p,log -20 -- src/core,branch -aanddiff HEAD~3 -- srcproduce byte-identical output from binaries built before and after.cargo test --allis green with zero changes togit.rs,search.rs,dotnet_cmd.rsorgolangci_cmd.rs— the diff touches onlyarg_tokenizerandsrc/core/README.md. The documented-20digit-run asymmetry (guarded under clustering, absent under atomic) is preserved exactly; it now falls out of thesingle_dashaxis rather than being a dialect special case.hyperfine --warmup 5 -N 'rtk git log -10': 6.4 ms ± 0.4 before, 6.2 ms ± 0.3 after.Also fixes
Dialect's doc comment, which linkedtokenize_dialect— a function folded intotokenize_grammarbefore merge.Axis independence, enforced
The
/flag-vs-path guard used to split on a hard-coded['=', ':']. That was correct only whileslash_flagsandattachwere welded together in one variant; with them independent, aslash_flagsdialect attaching on=alone would hide the second/in/opt:a/band emitLong("opt:a/b")instead of a positional. The guard now derives its separator fromdialect.attachviasplit_attached, and a test covers it (it fails against the hard-coded form).