Improve fish completion script generation

[rgoldberg]

Improve fish completion script generation.

There are many issues with script generation. Some of them are documented in #679 & its comments.

This issue is for a first batch of fish completion script fixes.

Each commit should note the fix(es) that it contains.

Will rebase if main is updated (especially if #735 is merged).

Resolve #737

Checklist

• I've added at least one test that validates that my change is working, if appropriate
• I've followed the code style of the rest of the project
• I've read the Contribution Guidelines
• I've updated the documentation if necessary

[rgoldberg]

@natecook1000 Rebased on the new main.

[natecook1000]

The Swift code looks great, but I'm seeing a few issues in the generated script. It looks like there are two syntax errors in the completion script for math, which I've annotated, and then at execution time I see unexpected output in a couple of situations:

1. When trying to get completions for either of the arguments, I see this output when I press Tab after quantiles (I had to rename the executable to math2 to work around Fish's built-in math support):

~ $ math2 stats quantiles Error: The value '+' is invalid for '<values>'
Help:  <values>  A group of integers to operate on.
Usage: math add [--hex-output] [<values> ...]
  See 'math add --help' for more information.
test: Missing argument at index 3
-eq 2
      ^
in command substitution
Error: The value '+' is invalid for '<values>'
Help:  <values>  A group of integers to operate on.
Usage: math add [--hex-output] [<values> ...]
  See 'math add --help' for more information.
test: Missing argument at index 3
-eq 1
      ^
in command substitution

2. The --custom completion worked, but printed an extra error message first:

~ $ math2 stats quantiles --file Desktop/add-preamble.txt --directory Desktop/Desktop/ --shell Abaris --custom Error: The value '+' is invalid for '<values>'
Help:  <values>  A group of integers to operate on.
Usage: math add [--hex-output] [<values> ...]
~ $ math2 stats quantiles --file Desktop/add-preamble.txt --directory Desktop/Desktop/ --shell Abaris --custom hel
hello  helicopter  heliotrope

[natecook1000]

Fish error: missing an end for this switch statement.

[rgoldberg]

Pushed a fix for this.

[natecook1000]

Fish error: can't use the $(...) syntax, have to just use (...) instead:

Suggested change

	set tokens $tokens[..$index] \'\' $tokens[$(math $index + 1)..]
	set tokens $tokens[..$index] \'\' $tokens[(math $index + 1)..]

[rgoldberg]

A bit ago, while testing fish with 4.0b1, I got an error using (…) instead of using $(…). I switched to $(…), which fixed the error.

After fixing the missing end problem (but not having worked on non-mutually-exclusive positionals & subcommands nor having made any other changes), I just verified that the both syntaxes offer helicopter, heliotrope & hello as completions for the following command line when completing with the cursor at ^ (which is not a literal caret, but just a marker to indicate the cursor location) on my macOS 12.7.6 Mac mini (2014) using fish 3.7.1 (which is the current stable version of fish) installed via Homebrew core:

command math stats quantiles --custom ^

What command & where in it did you try to complete? What version of fish? Etc.

I set this up by copying testMathFishCompletionScript().fish from the repo to ~/.config/fish/completions/math.fish, started fish 3.7.1 without any arguments, then ran:

fish_add_path /Users/username/Code/swift-argument-parser/.build/debug/

then completed at the end of the aforementioned command math stats quantiles --custom command line.

The math command had previously been built via swift test from zsh 5.9 installed via Homebrew core on the same Mac mini.

[rgoldberg]

@natecook1000 Sorry, I forgot to make a change to the fish completion script (it currently incorrectly assumes that positionals & subcommands are mutually exclusive).

I'll work on the change, then push a new version of the PR.

I'll test it more thoroughly.

Might be later tonight or tomorrow.

Am sick (as of yesterday morning) & was too hastily trying to finish getting my changes merged that my look over was too lax.

[rgoldberg]

@natecook1000 Some improvements that I've tried from fish 4.0b1 have some problems. I had messaged the fish people a bit about them, but it all fell by the wayside.

It seems like they might release 4.0 in a few weeks without any more prereleases after 4.0b1, so whenever I have their attention, I will concentrate on trying to get them to implement some some fixes.

I don't know Rust, nor do I know the fish codebase, so I probably cannot contribute code effectively to fish itself.

In the meantime, I will fix the issues in the fish PR as much as I can, but the eventual code might need to be different than the current paradigm to overcome fish shortcomings (e.g., I might need to tokenize the whole command line myself instead of using their standard tokenization), if fish 4.0 doesn't include certain improvements.

[rgoldberg]

@natecook1000 Rebased on the new main.

Fixed the switch end issues.

Now support positionals & subcommands for the same (sub)command.

Other minor improvements.

Have been investigating improving other fish issues / asking fish to support more features in 4.0, but any changes for SAP will be in future PRs.

[natecook1000]

@rgoldberg This is awesome, looks good!

Re compatibility – I was very confused at why this wasn't working for me locally – fish --version told me that I'm running version 3.6.0, which should support set -f (introduced in v 3.4.0) and the $(...) expansion syntax (likewise?), but it wasn't. It turns out that my $PATH pointed to a different version of Fish than my chsh setting, which was still on version 3.3.0! Before I figured that out, I made a small patch that got things working locally again, which you can view here: 854c36c. The changes seem relatively small, so if they still look future compatible, perhaps we can take them for now. That said, you'll know better than I do the implications of supporting back this far, so I can defer on that point; version 3.3.0 is nearly five years old.

[rgoldberg]

@natecook1000 Thanks for investigating.

The safest way to check the version of fish that you're running is from fish itself, using:

echo $FISH_VERSION

I might not have time to get to testing your compatibility changes before I go away for the weekend.

-f -> -l is fine.

$(…) -> () will likely fail in fish 4.0b1. It might be a bug, though, so maybe it's already fixed on their main or could be fixed soon. If it's an intentional break, then I luse a fish version switch wherever that construct is used, or we could just only support fish from the first version on which the 4.0-compatible syntax works.

FYI: I have migrated fish to ToolInfoV0 locally, except that the default help subcommand is missing the --version flag (as was expected & as also occurs for bash) & that multiple flag ArgumentDefinitions are coalesced into one ArgumentInfoV0 with multiple long names. I haven't had a chance to fix that, or to even determine if ToolInfoV0 should be changed or if the fish completion code should accommodate the new setup. That probably must wait until next week.

Please note that there are numerous extant issues with all the shells. After the ToolInfoV0 refactor & other queued changes have been merged, I'll try to compile a list of verified or potential problems, then post them to #679 (or to separate issues), though if fixing an issue will take around the same time as documenting it, I'll just fix & submit.

[natecook1000]

@rgoldberg I'm not sure of the status for this PR – do you have more to do, or is this ready for merge? It looks like Fish shell v4.0 was released since we last checked in – all good there?

[natecook1000]

@swift-ci Please test

[rgoldberg]

@natecook1000 Sorry, I was unable to work on SAP for a bit. I'll look at the PR later today or tomorrow.

I'll test with fish 4.0.1, and I'll look into incorporating your changes for backwards compatibility with older fish versions (-f to -l or no switch is fine, but I need to check if (…) works instead of $(…)).

I have some more as-of-yet unsubmitted fixes to submit after this PR has been merged, including documentation updates, adding the 2 index parameters for custom completion closures, using ToolInfoV1 (IIRC, that's ready for zsh & bash, but only 80% or so done for fish), etc.

There are still extant completion issues for the various shells, but everything will be better than before once my queued PRs have been merged.

Afterward all merged, I can list the extant completion issues, then work on them when I have time (or others can work on them).

[natecook1000]

@rgoldberg No need to apologize! Just wanted to make sure you weren't waiting for me.

[rgoldberg]

@natecook1000 Sorry. I was unable to work on open source software for a while, then I had to scramble to fix externally caused issues in the project I mainly work on.

I'm now able to get back to SAP completions.

I tested the fish 3.3.x- backwards compatibility changes that you requested on the recent fish 4.0.2; they seem to work, so I've incorporated them. I've also rebased on current main (which didn't conflict with this PR).

There are still extant issues in completions for all 3 shells, but, if this is merged, fish will be closer to the changes I made to other 2 shells.

After this PR is merged, I will:

1. submit my DocC WIP PR, Improve CompletionKind DocC #740
2. submit a local branch as a PR to remove unnecessary spaces in custom completion calls
3. submit a local branch as a PR to introduce the 2 aforementioned index arguments to an overload for custom completions
4. submit a local branch as a PR for each shell to switch its completion to ToolInfoV0 (not complete yet for all shells; ToolInfoV0 might need to be fixed/modified for all shells to work with it)
5. Get back to improving completions for each individual shell

[natecook1000]

@swift-ci Please test

[natecook1000]

@rgoldberg Merging this! Let me know if that is premature – we can revert and iterate on it some more – but I wanted to unblock your work on #740.

[rgoldberg]

@natecook1000 Good to merge. Additional improvements can be made, but there shouldn't be any regressions, and this will allow other improvements to be merged, too.

Thanks!