Fix multi-arg option modelling: build systems and language tooling - #344
Conversation
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Warp Factory.
|
|
This PR was generated with Warp. Comment    |
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![warp-agent-staging[bot]](https://avatars.githubusercontent.com/in/2196476?s=60&v=4){kind=small}
There was a problem hiding this comment.
Per-hunk reasoning for the audit, inline below. Each comment carries the ground truth that settles that option's arity and where it came from. Every verdict is from running the tool, except clang and trex, which could not be installed in this sandbox and were settled against the projects' own source.
Three of the sixteen options in scope are genuinely multi-valued and deliberately untouched, so they leave no hunk to anchor to:
cargo fmt -- --print-config—rustfmt --help(1.92.0):--print-config [default|minimal|current] PATH. Genuinely two values.just --set—just --help(1.58.0):--set <VARIABLE> <VALUE>. Genuinely two values.trex i -p/--pkg— needs Deno, not installable here.crewdevio/Trex@master,handlers/handle_packages.ts:157-158binds both positions:const [name, url] = pkgRepo(args[2], args[3]), matching the README'strex install --pkg oakserver/oak@main/mod.ts oak. Genuinely two values.
One thing is unresolved rather than fixed — see the nx generate --force comment: its arity is corrected, its existence is not settled.
Responding as wilson: Open session · View in factory
| "template": "folders" | ||
| } | ||
| ] | ||
| "args": { |
There was a problem hiding this comment.
rustc --help -v gives -L [<KIND>=]<PATH> — one token with an optional prefix, not two values. Running it confirms the consequence: rustc -L native /tmp t.rs fails with error: multiple input filenames provided (first two filenames are '/tmp' and 't.rs'), because native was swallowed as the entire -L value and /tmp fell through to the input files.
The kind values now carry the = themselves because the insertValue fields they previously leaned on were dead weight: the Suggestion type in completion-metadata/src/fig_types.rs has no such field, so serde silently dropped them and the suggestion inserted a bare dependency — which rustc reads as a directory name.
| }, | ||
| { | ||
| "name": "--l", | ||
| "name": "-l", |
There was a problem hiding this comment.
Renamed rather than removed, because --l is not quite the dead flag it looks like. rustc --l dylib=c t.rs really does link libc, and --l dylib=nosuchlib fails at link time exactly as -l does, because Rust's getopts normalises a one-character long option back to its short form.
But it appears in no help output and no rustc documentation, and the spec carried no -l entry at all — so the documented flag had no completion while an accident of the argument parser did. rustc --help -v gives the real syntax as -l [<KIND>[:<MODIFIERS>]=]<NAME>[:<RENAME>], again a single token.
| } | ||
| ] | ||
| "description": "Pass <arg> to the target offloading toolchain (use -Xopenmp-target=<triple> to identify the toolchain)", | ||
| "args": { |
There was a problem hiding this comment.
clang could not be installed in this sandbox, so this is settled against LLVM's own driver table. clang/include/clang/Driver/Options.td at llvmorg-19.1.0 declares def Xopenmp_target : Separate<["-"], "Xopenmp-target"> with MetaVarName<"<arg>">, and Separate means exactly one following token.
The two-arg modelling almost certainly came from the second, distinct definition two lines below it: def Xopenmp_target_EQ : JoinedAndSeparate<["-"], "Xopenmp-target="> with MetaVarName<"<triple> <arg>">. There the triple is joined to the flag and never appears as a token of its own, which is why the description now points at that spelling instead.
| "generatorName": "list_targets" | ||
| } | ||
| ] | ||
| "args": { |
There was a problem hiding this comment.
make --help (GNU Make 4.3) gives -j [N], --jobs[=N] Allow N jobs at once; infinite jobs with no arg — the count is optional, and target names are make's own operands. Nothing is lost by dropping the second arg: this file's top-level target arg already serves them through the list_targets generator.
isOptional follows the sibling -l, --load-average entry a few options down, whose help syntax -l [N] is identical and which was already modelled exactly this way.
| "template": "folders" | ||
| } | ||
| ] | ||
| "args": { |
There was a problem hiding this comment.
@antfu/ni v30.5.0 handles this at dist/src-DSImyIFd.mjs:3063: if (args[0] === "-C") { cwd = resolve(cwd, args[1]); args.splice(0, 2); }. It binds args[1] and splices exactly two tokens, so everything after is a package operand rather than part of -C.
Confirmed with ni's own dry run: ni ? -C /tmp/nitest axios resolves to npm i axios, with axios surviving as the package.
| "name": "false" | ||
| } | ||
| ], | ||
| "args": { |
There was a problem hiding this comment.
--defaults is not declared in nx generate --help on nx 23.1.1, but nx does consume it: dist/src/command-line/generate/generate.js:184 reads defaults: generatorOptions.defaults into the generator run options. However it is declared, it is a boolean and never a two-valued option, so the arity correction holds either way.
| "name": "false" | ||
| } | ||
| ], | ||
| "args": { |
There was a problem hiding this comment.
From nx generate --help on nx 23.1.1, run inside a minimal workspace: -d, --dryRun Preview the changes without updating files. [boolean] [default: false], and --interactive When false disables interactive input prompts for options. [boolean] [default: true]. Both are yargs booleans, so the two allowed values belong on one argument as suggestions — which is what the {true|false} already in each description was trying to say.
isOptional is deliberate, since --dryRun with no value is the normal invocation. It does diverge from the 12 existing {"suggestions": ["true","false"]} entries elsewhere in this file, which omit it; those belong to other subcommands and were out of scope, but they look like the same omission.
| "name": "false" | ||
| } | ||
| ], | ||
| "args": { |
There was a problem hiding this comment.
Flagging this one honestly: the arity is fixed here, but the option's existence is unresolved and with Andy.
I could not find --force declared anywhere in nx 17.3.2, 19.8.4 or 23.1.1 — absent from --help, from withGenerateOptions in command-line/generate/command-object.js, and from the generate handler. It may be a leftover from the Angular-devkit-derived runner, or it may be relied on as a passthrough to an individual generator's own schema, since nx forwards unrecognised options. Deleting it is a different decision from correcting its shape, so I only made the second one.
| ] | ||
| } | ||
| ] | ||
| "args": { |
There was a problem hiding this comment.
dart format --help on Dart SDK 3.13.1 gives -o, --output Set where to write formatted output. followed by [write] (default), [show], [json], [none] — one value chosen from four, not four arguments.
| } | ||
| }, | ||
| { | ||
| "name": "--show", |
There was a problem hiding this comment.
These two options are not new here — they were sitting inside -o's args, because the whole Output options: block of the help text had been pasted in wholesale. dart format --help --verbose shows them as flags in their own right: --show takes all|changed (default)|none and --summary takes line (default)|profile|none.
So they are restored in place rather than deleted along with the mis-modelling, which would have silently dropped two real options. The changed description also read "Nly"; it now reads "Only".
![warp-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR updates command-signature JSON entries for build-system and language-tooling options so options that consume one value are modeled as one argument, restores Dart formatter suboptions as separate options, and adjusts value suggestions for composite Rust arguments.
Concerns
- No blocking concerns found in the annotated diff.
- Security pass: no changed code paths, secrets, dependency changes, authentication/authorization behavior, or untrusted-input handling were introduced.
- Spec pass:
spec_context.mdcontains no approved or repository spec context, so there were no spec commitments to compare against this implementation. - Comment/test audit: the diff does not add code comments or tests; changed JSON
descriptionfields are command metadata rather than source comments.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Warp Factory.
|
There was a problem hiding this comment.
One correction to my earlier reasoning, from GUI validation on a real build. My comment on the -j hunk claimed nothing was lost by dropping the second arg — that turned out to be true only after c50af18. Details on the new hunk below, and in the PR description under Validation.
Responding as wilson: Open session · View in factory
| "name": "target", | ||
| "generatorName": "list_targets" | ||
| "generatorName": "list_targets", | ||
| "isVariadic": true |
There was a problem hiding this comment.
This is load-bearing, not tidying. GUI validation on a real build found that making -j's value optional cost make -j 4 its target list, and make -j 4 bu stopped completing build — with the value optional the parser hands the next token to the positional slot, and target being a single argument meant 4 consumed the only slot with nothing left to complete. The old two-arg -j had been serving targets from the option's second argument, which is why the wrong model looked right.
It is also correct of make on its own terms, independent of this branch: Usage: make [options] [target] ... in GNU Make 4.3's help, and make build test runs both. It additionally fixes make build offering no further targets, which was broken before this branch too.
This supersedes my claim on the -j hunk that nothing was lost by dropping the second arg — that only became true here.
|
@cla-bot check |
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Warp Factory.
|
|
The cla-bot has been summoned, and re-checked this pull request! |
Audit of options declaring two or more args across build systems and language tooling, following the same class of error fixed for `ln` in #341. When an option declares two or more args the completer resolves the last one regardless of which value is being typed, so a mis-modelled option actively breaks completion. Operands written into the option (extra args belong to the command, not the flag): - make -j: `-j [N]` takes an optional job count; targets are make's own operands, already served by the top-level `target` arg. - trunk check --sample, trunk fmt --sample: `--sample` takes N; paths are operands of both subcommands, already served by top-level args. - ni -C: consumes exactly one directory (`args.splice(0, 2)`); packages are ni's own operands. One value modelled as two: - rustc -L: real syntax is `[<KIND>=]<PATH>`, a single token. - rustc --l: not the documented flag; corrected to `-l`, whose real syntax is `[<KIND>[:<MODIFIERS>]=]<NAME>[:<RENAME>]`, a single token. - yarn --mutex: `<type>[:specifier]`, a single token. Now matches the 16 other `--mutex` entries already in the same file. - clang -Xopenmp-target: a `Separate` option taking one value; the triple is joined to the flag in the `-Xopenmp-target=<triple>` form. A value set enumerated as args: - dart format -o/--output: one value from write|show|json|none. The `--show` and `--summary` entries swept into its args are separate dart format flags, restored as their own options. - nx generate --defaults, --dryRun, --force, --interactive: booleans, now one optional argument carrying true/false as suggestions. Left alone as genuinely multi-valued: `cargo fmt -- --print-config`, `just --set`, `trex i --pkg`.
GUI validation of the previous commit found that `make -j 4 ` stopped offering the target list, and `make -j 4 bu` no longer completed `build`. Cause: with `-j`'s value now correctly optional, the parser hands the following token to the positional slot instead of binding it to the option. `target` was a single non-variadic argument, so `4` consumed the only positional slot and nothing remained to complete after it. The old two-arg `-j` model had been serving targets from the option's second argument, which is why it appeared to work. `Usage: make [options] [target] ...` in GNU Make 4.3's own help output shows targets are variadic regardless, and `make build test` runs both. Marking the top-level arg variadic restores completion after `make -j 4 ` without putting the operand back inside the option. It also fixes `make build ` offering no further targets, a pre-existing gap unrelated to this branch.
c50af18 to
3615705
Compare
1 participant
Audit of options declaring two or more
args, for the build systems and language tooling group. Same class of error asln -s, fixed in #341.Because the completer resolves the last declared arg regardless of which value is being typed, a mis-modelled option does not merely describe the tool wrongly — it actively breaks completion. 16 options were in scope; 13 were wrong and are fixed here, 3 are genuinely multi-valued and are untouched.
Every verdict below was checked against the tool itself where it could be installed, and against the project's own source or official reference where it could not. No verdict is inferred from the argument names in our spec.
Options changed
Category 1 — operands written into the option
make -j-j [N], --jobs[=N] Allow N jobs at once; infinite jobs with no arg.make --helptargetarg — targets are make's own operands and are already served by the top-leveltargetarg (list_targets).Nis now a single optional arg, mirroring the sibling-l, --load-averageentry whose help syntax (-l [N]) is identical. Required a follow-up: see GUI validation below.trunk check --sampleusage: trunk check [flags] [paths...]/paths files or directories to check/--sample run each linter on N filestrunk check --helppathsarg —pathsis an operand oftrunk checkand is already the subcommand's top-level variadic arg.trunk fmt --sampleusage: trunk fmt [flags] [paths...]/paths files or directories to format/--sample run each linter on N filestrunk fmt --helpni -Cif (args[0] === "-C") { cwd = resolve(cwd, args[1]); args.splice(0, 2); }— consumes exactly one value, everything after is passed through@antfu/niv30.5.0,dist/src-DSImyIFd.mjs:3063. Confirmed withni ? -C /tmp/nitest axios→ resolves tonpm i axiostargetarg;-Ctakes one directory.Category 2 — one value modelled as two
rustc -L-L [<KIND>=]<PATH> Add a directory to the library search path. The optional KIND can be one of <dependency|crate|native|framework|all> (default: all).rustc --help -v. Also confirmed empirically:rustc -L native /tmp t.rs→error: multiple input filenames provided (first two filenames are '/tmp' and 't.rs')=prefix (dependency=, …), so the inserted text is a valid prefix. The oldinsertValuefields were dropped: this repo'sSuggestiontype has no such field, so they were silently ignored and the suggestions inserted a bare invaliddependency.rustc --l-l [<KIND>[:<MODIFIERS>]=]<NAME>[:<RENAME>] Link the generated crate(s) to the specified native library NAME.rustc --help -v-land collapsed to one arg. See the note below on why renamed rather than removed.yarn --mutex--mutex <type>[:specifier] use a mutex to ensure only one yarn instance is executingyarn --helptype[:specifier]arg. This makes the entry identical to the 16 other--mutexentries already present inyarn.json; this one occurrence was the outlier.clang -Xopenmp-targetdef Xopenmp_target : Separate<["-"], "Xopenmp-target">, HelpText<"Pass <arg> to the target offloading toolchain.">, MetaVarName<"<arg>">;and separatelydef Xopenmp_target_EQ : JoinedAndSeparate<["-"], "Xopenmp-target=">, MetaVarName<"<triple> <arg>">;clang/include/clang/Driver/Options.tdatllvmorg-19.1.0, lines 1041–1045; corroborated by the official Clang Command Line ReferenceSeparatemeans exactly one value token. The triple is never a separate token — it is joined to the flag in the-Xopenmp-target=<triple>form, andMetaVarName<"<triple> <arg>">on that second definition is precisely what misled the original spec. Collapsed to onearg; the=<triple>form is noted in the description.Category 3 — a value set enumerated as args
dart format -o/--output-o, --output Set where to write formatted output.with[write] (default),[show],[json],[none]dart format --helpdestinationarg carrying the four values as suggestions, default marked.dart format --show,--summaryOutput options:—--showtakesall|changed (default)|none;--summarytakesline (default)|profile|nonedart format --help --verbose-o's args along with its value set — the whole help block had been pasted in. Rather than delete real data, they are restored as their own options with their own value sets. Also fixed the"Nly"typo in thechangeddescription.nx generate --dryRun-d, --dryRun Preview the changes without updating files. [boolean] [default: false]nx generate --helpin a minimal workspacetrue/falsesuggestions.nx generate --interactive--interactive When false disables interactive input prompts for options. [boolean] [default: true]nx generate --helpnx generate --defaults--help, but consumed by nx itself:defaults: generatorOptions.defaultsindist/src/command-line/generate/generate.js:184nx generate --force--help, fromcommand-object.js, and fromgenerate.jsin nx 17.3.2, 19.8.4 and 23.1.1unpkgcopies of the older versionsThe four
nxbooleans getisOptional: trueon the argument, since--dryRunwith no value is the normal invocation. That is a small divergence from the 12 existing{"suggestions": ["true","false"]}entries elsewhere innx.json, which omit it; those are for other subcommands and were not in scope, but they look like the same omission.Verified correct, left alone
cargo fmt -- --print-config [verbosity, PATH]--print-config [default|minimal|current] PATH Dumps a default or minimal config to PATH.rustfmt --help. Genuinely two-valued.just --set [variable, value]--set <VARIABLE> <VALUE> Override <VARIABLE> with <VALUE>just --help. Genuinely two-valued.trex i -p/--pkg [repository, Package Name]trex install --pkg [user]/[repo or repo@tag/branch]/[path/to/file] [packageName], e.g.trex install --pkg oakserver/oak@main/mod.ts oakcrewdevio/Trex@master,handlers/handle_packages.ts:157-158—else if (flags.pkg.includes(args[1])) { const [name, url] = pkgRepo(args[2], args[3]); }, i.e.--pkgbinds bothargs[2]andargs[3]. Genuinely two-valued.Notes for the reviewer
rustc --lwas renamed, not removed. The brief expected it to be a non-existent flag. It is subtler than that:rustc --l dylib=c t.rsreally does linklibc, andrustc --l dylib=nosuchlib t.rsfails at link time exactly as-ldoes, because Rust'sgetoptsnormalizes a single-character long option back to its short form. So--lworks, but it is an accident of the argument parser — it appears in no help text and no rustc documentation. Meanwhile the spec had no entry for-lat all, so the real, documented flag had no completion. Renaming fixes both: users get the documented spelling, and nothing that previously worked stops working. Removing the entry outright would have left-luncovered.nx generate --forcemay not belong in the spec at all. I could not find it declared anywhere in nx 17.3.2, 19.8.4 or 23.1.1 — not in--help, not inwithGenerateOptions, not in the generate handler. It is plausibly a leftover from the Angular-devkit-derived runner, or it is being relied on as a passthrough to an individual generator's own schema (nx does forward unrecognized options to the generator). Its arity is unambiguous either way, which is what this PR fixes; whether the flag should exist is a separate question I did not want to answer by deleting something. Flagging it rather than acting on it.Nothing was left unresolved. Every one of the 16 options reached a verdict backed by the tool or its source.
Out of scope, reported not touched
yarn.jsonduplicates its entire top-level global-option block: 35 options including--mutexappear twice at the top level. Only one--mutexcopy carried the two-arg bug and that one is fixed, but the duplication itself is pre-existing and untouched.verbosityarg name oncargo fmt -- --print-configis a misnomer — the values aredefault|minimal|current, nothing to do with verbosity. The arity is correct, so per the brief the option was left alone.make -jalso accepts--jobs[=N], andnx generate --dryRunalso accepts-d. Both aliases are missing from the specs. Not added, to keep this diff strictly about the arity error.Validation
cargo test -p warp-command-signatures— 176 passed, 0 failed.npm run formatreports all edited files unchanged, so they already match the repo's prettier configuration.GUI validation caught one regression, fixed in
c50af18Exercising the specs in a real Warp build found that the
make -jfix cost something:make -j 4stopped offering the target list, andmake -j 4 bustopped completingbuild. Measured on two binaries from the same completer branch, one pinned to the old specs and one to the new.The cause is worth recording, because it is a general consequence of making an option's value optional rather than anything specific to make. With
-j's value optional, the parser hands the following token to the positional slot instead of binding it to the option;make.json's top-leveltargetwas a single non-variadic argument, so4consumed the only positional slot and nothing was left to complete after it. The old two-arg-jhad been serving targets from the option's second argument, which is exactly why the broken model appeared to work.The fix is
"isVariadic": trueon that top-leveltargetargument — so the variadic is load-bearing, not decoration. It is also true of make independently of this branch:Usage: make [options] [target] ...in GNU Make 4.3's own help, andmake build testruns both. It restores completion aftermake -j 4without putting the operand back inside the option, and it additionally fixesmake buildoffering no further targets, a pre-existing gap this branch did not cause.