Enhance fish shell completions with static+dynamic hybrid generation#53716
Enhance fish shell completions with static+dynamic hybrid generation#53716baronfel merged 4 commits intodotnet:mainfrom
Conversation
Replace the fish shell provider's simple dynamic one-liner with a full static+dynamic completion generator, matching the approach taken by the Bash, Zsh, and PowerShell providers. The generated script uses a state-machine that walks the tokenized command line to determine the current subcommand context, then emits static completions for subcommands, options, and positional arguments — falling back to dynamic complete calls where IsDynamic is set. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
HECK YEAH! Thanks for sending this, I'll try to review this afternoon. |
There was a problem hiding this comment.
Pull request overview
This PR upgrades the Fish shell completions generator from a simple dynamic invocation to a hybrid static+dynamic generator (similar in spirit to the existing Bash/Zsh/PowerShell providers), and adds snapshot coverage for the new output.
Changes:
- Replaced Fish completion generation with a state-machine driven script that emits static completions and falls back to dynamic completions for
IsDynamicsymbols. - Added Fish snapshot tests covering generic, options/subcommands, nested subcommands, dynamic completions, and static option values.
- Updated Verify configuration to treat
.fishsnapshot files as text.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/System.CommandLine.StaticCompletions.Tests/VerifyConfiguration.cs | Registers .fish as a text extension for snapshot verification. |
| test/System.CommandLine.StaticCompletions.Tests/FishShellProviderTests.cs | Adds Fish provider snapshot tests. |
| src/System.CommandLine.StaticCompletions/shells/FishShellProvider.cs | Implements the new static+dynamic Fish completion script generator. |
| test/System.CommandLine.StaticCompletions.Tests/snapshots/fish/*.verified.fish | Adds the expected Fish completion script snapshots for the new tests. |
| /// Uses fish's commandline builtin to get completed tokens and the current partial word. | ||
| /// </summary> | ||
| private static void WriteTokenization(IndentedTextWriter writer) | ||
| { | ||
| // -opc: tokenize, cut at cursor, only completed tokens (excludes current partial word) | ||
| writer.WriteLine("set -l tokens (commandline -opc)"); | ||
| // -ct: the current token being completed (may be empty or partial) | ||
| writer.WriteLine("set -l current (commandline -ct)"); |
There was a problem hiding this comment.
The generated fish script assigns current from commandline -ct but never uses it anywhere in the output. Consider removing this line (or using current to filter/short-circuit) to avoid dead code in the generated completion function.
| /// Uses fish's commandline builtin to get completed tokens and the current partial word. | |
| /// </summary> | |
| private static void WriteTokenization(IndentedTextWriter writer) | |
| { | |
| // -opc: tokenize, cut at cursor, only completed tokens (excludes current partial word) | |
| writer.WriteLine("set -l tokens (commandline -opc)"); | |
| // -ct: the current token being completed (may be empty or partial) | |
| writer.WriteLine("set -l current (commandline -ct)"); | |
| /// Uses fish's commandline builtin to get completed tokens up to the cursor. | |
| /// </summary> | |
| private static void WriteTokenization(IndentedTextWriter writer) | |
| { | |
| // -opc: tokenize, cut at cursor, only completed tokens | |
| writer.WriteLine("set -l tokens (commandline -opc)"); |
| // Subcommand transitions | ||
| foreach (var sub in visibleSubs) | ||
| { | ||
| var subStateId = states.First(s => s.cmd == sub).id; | ||
| var names = string.Join(" ", sub.Names()); | ||
| writer.WriteLine($"case {names}"); | ||
| writer.Indent++; | ||
| writer.WriteLine($"set state {subStateId}"); | ||
| writer.Indent--; | ||
| } |
There was a problem hiding this comment.
WriteStateWalker resolves each subcommand’s state id via states.First(...) inside nested loops, making generation O(n^2) in the number of commands. For large command graphs (e.g., dotnet), consider building a Dictionary<Command,int> once (e.g., when collecting states) and using it for O(1) lookups.
6fb5dca to
2320bab
Compare
Handle options with arity > 1 (bounded and unbounded) in the fish shell state walker and option value completions. Fix fish seq warning when token count is less than 2 by guarding the scan-back loop. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2320bab to
1977339
Compare
…kups Address review feedback: remove dead `set -l current` variable from generated fish script, and replace O(n²) states.First() lookups with a pre-built Dictionary<Command, int> for O(1) state ID resolution. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Head branch was pushed to by a user without write access
| // Options with MaximumNumberOfValues at or above this threshold are treated as unbounded | ||
| // (i.e. consume tokens until an option-like token is encountered). | ||
| // System.CommandLine uses 100_000 as its internal sentinel for ZeroOrMore/OneOrMore. | ||
| private const int UnboundedArityThreshold = 100; |
There was a problem hiding this comment.
UnboundedArityThreshold is set to 100, but the comment indicates System.CommandLine uses 100_000 as the sentinel for unbounded arities (ZeroOrMore/OneOrMore). With the current value, any option with a bounded max >99 will be treated as unbounded, which can change parsing/completion behavior. Consider using the actual sentinel (or a check keyed specifically to the unbounded sentinel) so large-but-bounded arities stay bounded.
| private const int UnboundedArityThreshold = 100; | |
| private const int UnboundedArityThreshold = 100_000; |
| writer.WriteLine("if string match -q -- '-*' $next"); | ||
| writer.Indent++; | ||
| writer.WriteLine("break"); | ||
| writer.Indent--; |
There was a problem hiding this comment.
The multi-value skip loop stops when the next token matches -*. This will incorrectly stop consuming values for legitimate option values that start with - (e.g., negative numbers like -1, or strings beginning with a dash), causing the state machine to mis-parse the command line. Consider checking whether the next token is actually one of the known option names for the current command context (and handling -- end-of-options) instead of using a prefix heuristic.
| writer.WriteLine("if string match -q -- '-*' $next"); | |
| writer.Indent++; | |
| writer.WriteLine("break"); | |
| writer.Indent--; | |
| writer.WriteLine("if test \"$next\" = \"--\""); | |
| writer.Indent++; | |
| writer.WriteLine("break"); | |
| writer.Indent--; | |
| writer.WriteLine("else if string match -rq -- '^--.+|^-[^0-9].*' $next"); | |
| writer.Indent++; | |
| writer.WriteLine("break"); | |
| writer.Indent--; |
| // Scan backward through tokens to find the nearest option (token starting with -) | ||
| writer.WriteLine("set -l opt_index 0"); | ||
| writer.WriteLine("if test (count $tokens) -ge 2"); | ||
| writer.Indent++; | ||
| writer.WriteLine("for j in (seq (count $tokens) -1 2)"); | ||
| writer.Indent++; | ||
| writer.WriteLine("if string match -q -- '-*' $tokens[$j]"); | ||
| writer.Indent++; | ||
| writer.WriteLine("set opt_index $j"); | ||
| writer.WriteLine("break"); | ||
| writer.Indent--; | ||
| writer.WriteLine("end"); | ||
| writer.Indent--; | ||
| writer.WriteLine("end"); | ||
| writer.Indent--; | ||
| writer.WriteLine("end"); |
There was a problem hiding this comment.
Option-value completion scans backward for the nearest token matching -* to decide which option we're completing. This can misidentify negative numeric values (or other dash-prefixed values) as the "current option", which will suppress/alter completions incorrectly. Consider restricting the scan to tokens that match known option names in the current state (and treating -- as an end-of-options marker).
| // Scan backward through tokens to find the nearest option (token starting with -) | |
| writer.WriteLine("set -l opt_index 0"); | |
| writer.WriteLine("if test (count $tokens) -ge 2"); | |
| writer.Indent++; | |
| writer.WriteLine("for j in (seq (count $tokens) -1 2)"); | |
| writer.Indent++; | |
| writer.WriteLine("if string match -q -- '-*' $tokens[$j]"); | |
| writer.Indent++; | |
| writer.WriteLine("set opt_index $j"); | |
| writer.WriteLine("break"); | |
| writer.Indent--; | |
| writer.WriteLine("end"); | |
| writer.Indent--; | |
| writer.WriteLine("end"); | |
| writer.Indent--; | |
| writer.WriteLine("end"); | |
| // Scan backward through tokens to find the nearest known value-taking option for the current state. | |
| // Stop scanning if we encounter the end-of-options marker. | |
| writer.WriteLine("set -l opt_index 0"); | |
| writer.WriteLine("if test (count $tokens) -ge 2"); | |
| writer.Indent++; | |
| writer.WriteLine("switch $state"); | |
| writer.Indent++; | |
| foreach (var (stateId, cmd) in states) | |
| { | |
| var valueOptions = cmd.HierarchicalOptions() | |
| .Where(o => !o.Hidden && !o.IsFlag()) | |
| .ToArray(); | |
| if (valueOptions.Length == 0) | |
| continue; | |
| writer.WriteLine($"case {stateId}"); | |
| writer.Indent++; | |
| writer.WriteLine("for j in (seq (count $tokens) -1 2)"); | |
| writer.Indent++; | |
| writer.WriteLine("if test $tokens[$j] = '--'"); | |
| writer.Indent++; | |
| writer.WriteLine("break"); | |
| writer.Indent--; | |
| writer.WriteLine("end"); | |
| writer.WriteLine("switch $tokens[$j]"); | |
| writer.Indent++; | |
| foreach (var option in valueOptions) | |
| { | |
| var names = string.Join(" ", option.Names()); | |
| writer.WriteLine($"case {names}"); | |
| writer.Indent++; | |
| writer.WriteLine("set opt_index $j"); | |
| writer.WriteLine("break"); | |
| writer.Indent--; | |
| } | |
| writer.Indent--; | |
| writer.WriteLine("end"); | |
| writer.Indent--; | |
| writer.WriteLine("end"); | |
| writer.Indent--; | |
| } | |
| writer.Indent--; | |
| writer.WriteLine("end"); | |
| writer.Indent--; | |
| writer.WriteLine("end"); |
| foreach (var sub in visibleSubs) | ||
| { | ||
| var subStateId = stateIdByCommand[sub]; | ||
| var names = string.Join(" ", sub.Names()); | ||
| writer.WriteLine($"case {names}"); | ||
| writer.Indent++; |
There was a problem hiding this comment.
Fish switch/case patterns are glob patterns. Emitting raw command/option names into case clauses (e.g., case {names}) can cause unexpected matches if a name contains glob metacharacters like *, ?, or []. Consider escaping fish glob metacharacters when generating case patterns so symbol names are treated literally.
Change UnboundedArityThreshold from 100 to 100_000 to match the actual sentinel value used by System.CommandLine for ZeroOrMore/OneOrMore. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/backport to release/10.0.3xx |
|
Started backporting to |
Summary
complete -f -c dotnet -a "(dotnet complete (commandline -cp))") with a full static+dynamic completion generator, matching the approach taken by the Bash, Zsh, and PowerShell providers.completecalls whereIsDynamicis set.Test plan
dotnetbinary🤖 Generated with Claude Code