Commands for generating shell completion scripts + statically-generated bash script #42416

baronfel · 2024-07-28T01:31:57Z

Implements:

provider model for shell completions
new 'completions' command with 'script' and 'register' subcommands

Shell Support

shell	recording	static completions	dynamic completions (e.g. nuget packages)	slower, fallback completions
bash	🎥	✔️	✔️	❌
zsh	🎥	✔️	✔️	❌
pwsh	🎥	✔️	❌	❌
nushell	❌	❌	❌	✔️
fish	❌	❌	❌	✔️

for nushell/fish, the generate command just generates a correct-but-slow fallback wrapper
for bash and zsh, comprehensive and very fast static completions have been implemented, even for dynamic completions like nuget packages
for powershell, completions for staticlly-known portions of the command graph have been implemented. dynamic support is missing.

baronfel · 2024-07-28T14:19:56Z

I've implemented completion script generate for bash based partially on the excellent patterns that the Swift ArgumentParser library has used. Here's an example of the generated script (warning, 2100 lines of bash await you):

dotnet-bash-completions.txt

It works remarkably well - one of the harder parts was figuring out when truly dynamic completions were required and injecting a call to the dotnet CLI's completion command to generate suggestions for those cases. But where completions can be known statically we inject them. There is no good handling for file-based completions yet - one hurdle I faced was the very-pluggable completions model currently doesn't give any sense of attribution of the different completion sources - if I knew a given Func<CompletionContext, CompletionItem[]> was a certain kind of completion source I could maybe skip them for purposes of static script generation. As it is I had to create my own signal for that kind of data.

src/Cli/dotnet/commands/dotnet-completions/shells/Bash.cs

… possible

…names. Addresses the final addressable warning about top-level matching on -?. -? is a pattern in bash, so it needs to be escaped.

…yle options

…or options without any completions.

…re empty

…ro to prevent them from being generated. This is breaking from the parsing perspective, but all of these have single-digit usages on GitHub.com

Technically breaking from a parser perspective, but GitHub searches of usages for these all showed either zero with arguments or ~5.

…ure we include recursive options from parents

Forgind

I didn't look at shell-specific code or test code, but this looks great! I appreciated the videos in the description, but you might need to work on your typing speed (or crop the first half of the video) 😉

src/Cli/dotnet/CommonOptions.cs

src/Cli/dotnet/commands/dotnet-add/AddCommandParser.cs

Forgind · 2024-12-31T04:24:13Z

src/Cli/dotnet/commands/dotnet-add/AddCommandParser.cs

@@ -13,7 +13,8 @@ internal static class AddCommandParser

        public static readonly CliArgument<string> ProjectArgument = new CliArgument<string>(CommonLocalizableStrings.ProjectArgumentName)
        {
-            Description = CommonLocalizableStrings.ProjectArgumentDescription
+            Description = CommonLocalizableStrings.ProjectArgumentDescription,
+            Hidden = true


Why no Arity for this? It seems like you added it to a lot of things.

You get some arities by default in System.CommandLine based on the type of the CliArgument/CliOption - this is one of those cases. You can see the logic used here. In this case, I just needed to hide the argument to paper over some completions pain points. Since we think very few users use this arg (this is one of the discussion points in your verb/noun PR, remember!) this should be fine while also not breaking existing users.

Regarding the ArgumentArity.Zero stuff you see around this PR - We have a lot of flags in the CLI. We don't currently have a unified way of modeling those (i.e. a CliOption subclass). It might be nice to do that (and/or S.CL to have that concept already)

src/Cli/dotnet/commands/dotnet-complete/CompleteCommandParser.cs

Forgind · 2024-12-31T04:37:19Z

src/System.CommandLine.StaticCompletions/CompletionsCommand.cs

+    }
+}
+
+public class RegisterScriptCommand : CliCommand


Can you explain the value of this command? Seems like it does strictly less than GenerateScript

This is some ground work that I should probably delete because it's vestigial at this time. The thinking is we should have an explicit command that users run to generate the completion script for their shell, and that makes total sense to do. Users that want completions can open their Powershell $PROFILE and add dotnet completions generate pwsh | Out-String | Invoke-Expression to it and have up-to-date completions each time they load their shell. This can of course be optimized, etc. but this is the main idea of many shell-completion frameworks that exist today.

However, you still need to get users to do that step. This is why some other frameworks have an 'install' and 'uninstall' semantic. This looks something like dotnet completions register pwsh, and the logic would be, roughly:

determine the shell the user is requesting for

ask the shell provider to see if the dotnet completions are registered

this will be heuristic-based, likely, and require probing contents of files like $PROFILE for pwsh, .zshrc for Zsh, etc.

if not, inject a section to call the dotnet completions generate command into the shell configuration files

(optionally) support uninstalling as well - do the same but find and remove the special section

The 'fuzziness' of this command is why I haven't implemented it yet. But once it exists, we could then have a discussion about if the SDK first-run experience should set up this completion registration automatically - we know today that so very very few of our users use completions, and having them set up would be a huge usability increase - as well as make it so that any fixes/enhancement to the completions we might make would be automatically applied!

I like the idea of a dotnet completions register pwsh command, but even more than that, I like the idea of making this part of the first-run welcome message but in a slightly different way. I'm envisioning two options:

A message like "If you want to see what options are available, run 'dotnet completions register pwsh'."

Optionally pause before executing their command with a question like "do you want completions? y/n" and let them choose.

I think something like that would dramatically increase usage, and I do agree with "huge usability increase" if people start using it...but I also don't like the idea of opting people into something without telling them. I actually do see an argument in favor of not having completions, namely that it can take up a lot of screen real estate, and it may not ultimately be helpful if we do a poor job naming our subcommand/options, and people feel the need to go online to read more about them anyway. As we're busy telling people how to opt in (or giving them the y/n option), we can make sure to mention something like "if you ever don't want this anymore, run 'dotnet completions unregister pwsh'." Though I'll admit that unregister might go the way of unregister in MSBuildLocator in becoming vestigial, I think it's positive to offer it as an option initially. It makes it less scary to opt into something if I know I can change my mind later.

As a small aside, I like install/uninstall more than register/unregister, but maybe that's just me.

src/System.CommandLine.StaticCompletions/DynamicArgument.cs

src/System.CommandLine.StaticCompletions/HelpGenerationExtensions.cs

src/System.CommandLine.StaticCompletions/System.CommandLine.StaticCompletions.csproj

baronfel · 2024-12-31T05:18:31Z

src/Cli/dotnet/Parser.cs

+        public static readonly CliOption<bool> VersionOption = new("--version")
+        {
+            Arity = ArgumentArity.Zero,
+        };

-        public static readonly CliOption<bool> InfoOption = new("--info");
+        public static readonly CliOption<bool> InfoOption = new("--info")
+        {
+            Arity = ArgumentArity.Zero,
+        };

-        public static readonly CliOption<bool> ListSdksOption = new("--list-sdks");
+        public static readonly CliOption<bool> ListSdksOption = new("--list-sdks")
+        {
+            Arity = ArgumentArity.Zero,
+        };

-        public static readonly CliOption<bool> ListRuntimesOption = new("--list-runtimes");
+        public static readonly CliOption<bool> ListRuntimesOption = new("--list-runtimes")
+        {
+            Arity = ArgumentArity.Zero,
+        };


Really these look like options but are more like commands - I'd like to model them that way in the future but previous PRs have broken and I haven't gone back to look at it. In any case, these aren't every actually invoked by us (they are handled by the runtime), they just exist here to correctly model our CLI for help/argument parsing purposes.

baronfel · 2024-12-31T05:23:24Z

src/System.CommandLine.StaticCompletions/CompletionsCommand.cs

+    { }
+
+    private CompletionsCommand(Dictionary<string, IShellProvider> shellMap)
+        : base("completions", "Commands for generating and registering completions for supported shells")


TODO: need localizations for the strings introduced in this command..

Evangelink · 2024-12-31T07:28:30Z

@baronfel I'm guessing this can only work when the options are statically registered and so it won't work for our new dotnet test experience, right?

baronfel · 2024-12-31T13:49:46Z

@Evangelink Depending on the shell we would support 'dynamic completions' - those that require calling into the System.CommandLine completion mechanisms to get values like NuGet packages or TFMs in a project file. Depending on how your system is set up this might be a viable pathway for you. We could chat about it and try to figure out what could work.

baronfel · 2024-12-31T15:43:31Z

...System.CommandLine.StaticCompletions.Tests/System.CommandLine.StaticCompletions.Tests.csproj

+    <!-- None of these deps should be hard-coded, but something's wonky unless I opt into the whole massive Microsoft.NET.TestFramework mess.
+         I don't want that. This should be able to stand alone. -->
+    <PackageReference Include="System.CommandLine" Version="2.0.0-beta4.24528.1"/>
+    <PackageReference Include="FluentAssertions" Version="6.12.0" />
+    <PackageReference Include="XUnit" Version="2.9.0" />
+    <PackageReference Include="xunit.runner.visualstudio" Version="2.8.2" />
+    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.13.0-preview-24610-01" />
+    <PackageReference Include="Microsoft.TestPlatform" Version="17.13.0-preview-24610-01" />
+    <PackageReference Include="Verify.Xunit" Version="25.0.2" />
+    <PackageReference Include="Verify.DiffPlex" Version="3.0.0" />


Note for the SDK team: I'm trying on purpose to keep this test project as slim as possible, but that has two side-effects that I'm having trouble reasoning about:

the package versions aren't coming through in the way I'd expect, requiring this duplication

I suspect Helix is mad due to some missing convention

I'm really hoping to not have to take a dependency on the rather large SDK test framework project because that massively expands cycle times.

Does central package versioning not get us the versions? Or rather, why not?

I can try again and dig into the binlog, but the CPM setup was not working for this project and I didn't want to get blocked on the actual feature work (making tests, etc). Will take another pass at this.

baronfel · 2024-12-31T15:45:38Z

The new tests seems to be failing due to some permissions issues - I notice that our version of Verify is quite old. I'm going to trigger package ingestion for that family of packages and then see if bumping those fixes the problem.

…aterially impacted the end user experience

…de effects in CI

am11 · 2025-01-01T21:15:35Z

src/System.CommandLine.StaticCompletions/shells/BashShellProvider.cs

+        var initialFunctionName = command.FunctionName().MakeSafeFunctionName();
+        return
+            $"""
+            #! /bin/bash


A nit: the proper syntax is without the space and this one is recommended to make scripts portable across the distros:

Suggested change

#! /bin/bash

#!/usr/bin/env bash

Thanks - I'll apply that locally because it'll also cause changes in the snapshots that I'll need to cover at the same time. Keep the correctness feedback coming - over time the generator can get incrementally better!

There is a note about the spaced #! / at https://tldp.org/LDP/abs/html/sha-bang.html#AEN214, which links to http://www.in-ulm.de/~mascheck/various/shebang/#details. In short, the space is not required.

I have a little hesitation with respect to the security of #!/usr/bin/env bash. I've normally used #!/usr/bin/bash in the past, and that pinpoints the exact executable to use rather than grabbing the one specified in $PATH, which is far easier to tamper with.

Just delete the #! line altogether. See #42416 (comment)

/usr/bin/bash is not guaranteed on all platforms and it is recommended to use environment. This is how we are doing it in dotnet repos for several years. Compromised environment is not going to take us far.

No need to delete #! either, it helps testing the script in isolation. I always put it in interpreter as well as include scripts. Does not hurt. CRLF issue is also not limited to the interpreter line.

src/System.CommandLine.StaticCompletions/shells/BashShellProvider.cs

KalleOlaviNiemitalo · 2025-01-01T22:58:12Z

How does this handle CRLF vs. LF line endings in generated scripts? .gitattributes includes:

* text=auto

*.sh text eol=lf

so the C# files may have either CRLF or LF, depending on the system, and that choice is then baked into the binaries; but test/dotnet-completions.Tests/snapshots/bash/DotnetCliSnapshotTests.VerifyCompletions.verified.sh always has LF. I didn't find code that replaces CRLF with LF. Does Verifier ignore the CRLF vs. LF difference by default?

If the #!/usr/bin/env bash line ends with CRLF, then the CR will be taken as part of the command name and the command won't be found.

KalleOlaviNiemitalo · 2025-01-01T23:07:21Z

The #! line is not needed in the Bash completion script anyway. If the script were executed as a program, then the #! line would tell the kernel which shell to start for the script; but the script cannot be used in that way, because the shell process would exit immediately after defining the completions, and the user would have no opportunity to use the completions. The script must be instead dot-sourced by an existing shell process, and #! is then ignored.

I suppose a text editor could recognize #!/usr/bin/env bash and highlight the script according to Bash syntax; but these scripts are generated and not intended to be edited.

KalleOlaviNiemitalo · 2025-01-01T23:10:54Z

Is .NET SDK going to ship an Authenticode-signed PowerShell completion script?

baronfel · 2025-01-02T00:00:09Z

@KalleOlaviNiemitalo that's an interesting question. I'm leaning towards no due to ease for this PR. No other competing CLI framework I've seen attempts to support that, though I do see the use case.

baronfel · 2025-01-02T00:02:29Z

In addition, that wouldn't be something that source-build could easily support, for example. A user that requires signed scripts could always generate and sign the script themselves.

baronfel · 2025-01-02T02:14:10Z

@KalleOlaviNiemitalo said:

How does this handle CRLF vs. LF line endings in generated scripts?

I anticipated this an instead chose to always emit newlines only in the script generation. Each script generator uses this configuration, as it's well-understood on all platforms and keeps the testing more similar so that we don't have to script for OS-specific newlines.

KalleOlaviNiemitalo · 2025-01-02T07:18:09Z

@baronfel, I meant the multiline ''' string that includes the #! line; does that get CRLF or just LF? AFAICS the TextWriter newline setting does not apply to it.

slang25 · 2025-01-02T18:51:47Z

src/System.CommandLine.StaticCompletions/shells/FishShellProvider.cs

+        """;
+
+    public string GenerateCompletions(System.CommandLine.CliCommand command) => _dynamicCompletionScript;
+}


Will this interfere with the baked in completions provided here:
https://github.com/fish-shell/fish-shell/blob/master/share/completions/dotnet.fish

I'm keen to have descriptions on the completions, but I get that it's a bit of a stretch.

Thanks for the pointer to this, I didn't know it existed. They are 5 years old however ;) I'm not actually sure how those are used by a fish user (are they just...ambient? or is there a user gesture required to enable them?), but in any case the completions in this PR would require a user to run the dotnet completions generate fish command and use that output in their fish configuration, so nothing automatic would happen.

Separately, if someone implemented the IShellProvider for fish then users would have always-correct, up-to-date completions ready to go! So in short, this PR would make slower but more up to date completions available for fish users, but the groundwork is right there to make fast and up to date completions possible for fish users easily for all System.CommandLine users.

It looks like if a user opts into our completions they will supersede the built-in definitions:

By default, Fish searches the following for completions, using the first available file that it finds:

A directory for end-users to keep their own completions, usually ~/.config/fish/completions (controlled by the XDG_CONFIG_HOME environment variable);

A directory for systems administrators to install completions for all users on the system, usually /etc/fish/completions;

A user-specified directory for third-party vendor completions, usually ~/.local/share/fish/vendor_completions.d (controlled by the XDG_DATA_HOME environment variable);

A directory for third-party software vendors to ship their own completions for their software, usually /usr/share/fish/vendor_completions.d;

The completions shipped with fish, usually installed in /usr/share/fish/completions; and

Completions automatically generated from the operating system’s manual, usually stored in ~/.local/share/fish/generated_completions.

dotnet-issue-labeler bot added Area-Infrastructure untriaged Request triage from a team member labels Jul 28, 2024

baronfel changed the title ~~Completions registration~~ Commands for generating shell completion scripts + statically-generated bash script Jul 28, 2024

baronfel force-pushed the completions-registration branch from 4fa8920 to 6f266e8 Compare July 28, 2024 15:15

baronfel added Area-CLI cli-ux Issues and PRs that deal with the UX of the CLI (exit codes, log output, verbs/options, and so on) labels Jul 28, 2024

This was referenced Jul 28, 2024

Test failure - AllWebProjectsRestoreAndBuild fails with Unbalanced Condition for F# MVC templates #40073

Open

Roslyn analyzer throws error AD0001 NullReferenceException dotnet/dnceng#3305

Open

baronfel force-pushed the completions-registration branch from 64269f3 to af2fa73 Compare July 31, 2024 21:35

build-analysis bot mentioned this pull request Aug 1, 2024

Failure on Helix tests - Tests timing out #40074

Open

KalleOlaviNiemitalo reviewed Aug 1, 2024

View reviewed changes

src/Cli/dotnet/commands/dotnet-completions/shells/Bash.cs Outdated Show resolved Hide resolved

src/Cli/dotnet/commands/dotnet-completions/shells/Bash.cs Outdated Show resolved Hide resolved

baronfel added 13 commits August 23, 2024 13:55

Spike out initial completion script emit commands

3e887ff

Update contents to include locations and commented instructions where…

6ce7323

… possible

Complete rewrite to support dynamic completions

4893636

Refactor to make certain scary portions of the generation have nicer …

4d1098b

…names. Addresses the final addressable warning about top-level matching on -?. -? is a pattern in bash, so it needs to be escaped.

Enhancement: do not emit completions for option arguments for flag-st…

6b3b424

…yle options

Optimize static completions for bash: don't emit option completions f…

3a7fc72

…or options without any completions.

Completion optimization - don't emit option handler stanzas if they a…

31699b0

…re empty

Completion optimization: mark several flag-style commands as Arity ze…

9d2700a

…ro to prevent them from being generated. This is breaking from the parsing perspective, but all of these have single-digit usages on GitHub.com

Completions optimization: mark many flag-style options as Arity Zero

73f997a

Technically breaking from a parser perspective, but GitHub searches of usages for these all showed either zero with arguments or ~5.

Fix accidental zero-arity arguments in tool install and restore

1a1401a

update baselines for new completions command

10086ec

strip odd help option aliases, suggest only best option name, and ens…

9944fcf

…ure we include recursive options from parents

Add tests for shell completion helpers

b93f1b8

baronfel force-pushed the completions-registration branch from af2fa73 to b93f1b8 Compare August 23, 2024 18:55

baronfel and others added 3 commits December 20, 2024 09:27

Merge branch 'main' into completions-registration

5a77682

fix bad merge

21b7561

WIP test placeholders

ec027ec

build-analysis bot mentioned this pull request Dec 20, 2024

dotnet-watch tests failing #40006

Open

Forgind approved these changes Dec 31, 2024

View reviewed changes

baronfel added 2 commits December 30, 2024 23:10

Code review fixes and renames from @Forgind

22cc730

Remove unused option to minimize diff

ebbfbef

baronfel commented Dec 31, 2024

View reviewed changes

This was referenced Dec 31, 2024

slow macOS - "##[error]The job running on agent Azure Pipelines 9 ran longer than the maximum time of 60 minutes." dotnet/dnceng#1883

Open

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

baronfel commented Dec 31, 2024

View reviewed changes

baronfel added 7 commits December 31, 2024 10:23

Fix two logical test failures

4bc45bf

Remove unnecessary arity and hidden flags from arguments where that m…

b7bbaa6

…aterially impacted the end user experience

Update Verify packages to hopefully get around some initialization si…

7fa2231

…de effects in CI

Try to work around Verify directory usage in CI

f692ba1

Minor rename to align type name with file name

42149b6

Add dotnet CLI generate script approval tests to the CLI-level tests

0d65128

Try to be even more explicit

d517deb

am11 reviewed Jan 1, 2025

View reviewed changes

Apply bash env change from @am11 and update snapshots

427f24d

zivkan reviewed Jan 1, 2025

View reviewed changes

src/System.CommandLine.StaticCompletions/shells/BashShellProvider.cs Outdated Show resolved Hide resolved

try to fix errant path computation in tests

d8c2107

slang25 reviewed Jan 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Commands for generating shell completion scripts + statically-generated bash script #42416

Commands for generating shell completion scripts + statically-generated bash script #42416

baronfel commented Jul 28, 2024 •

edited

Loading

baronfel commented Jul 28, 2024 •

edited

Loading

Forgind left a comment

Forgind Dec 31, 2024

baronfel Dec 31, 2024

baronfel Dec 31, 2024

Forgind Dec 31, 2024

baronfel Dec 31, 2024

Forgind Dec 31, 2024

baronfel Dec 31, 2024

baronfel Dec 31, 2024

Evangelink commented Dec 31, 2024

baronfel commented Dec 31, 2024 •

edited

Loading

baronfel Dec 31, 2024

Forgind Dec 31, 2024

baronfel Jan 1, 2025

baronfel commented Dec 31, 2024

am11 Jan 1, 2025

baronfel Jan 1, 2025

KalleOlaviNiemitalo Jan 1, 2025

Forgind Jan 2, 2025 •

edited

Loading

KalleOlaviNiemitalo Jan 2, 2025

am11 Jan 2, 2025

KalleOlaviNiemitalo commented Jan 1, 2025 •

edited

Loading

KalleOlaviNiemitalo commented Jan 1, 2025

KalleOlaviNiemitalo commented Jan 1, 2025

baronfel commented Jan 2, 2025

baronfel commented Jan 2, 2025

baronfel commented Jan 2, 2025

KalleOlaviNiemitalo commented Jan 2, 2025

slang25 Jan 2, 2025

baronfel Jan 2, 2025 •

edited

Loading

baronfel Jan 2, 2025

Commands for generating shell completion scripts + statically-generated bash script #42416

Are you sure you want to change the base?

Commands for generating shell completion scripts + statically-generated bash script #42416

Conversation

baronfel commented Jul 28, 2024 • edited Loading

Shell Support

baronfel commented Jul 28, 2024 • edited Loading

Forgind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Evangelink commented Dec 31, 2024

baronfel commented Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baronfel commented Dec 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Forgind Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KalleOlaviNiemitalo commented Jan 1, 2025 • edited Loading

KalleOlaviNiemitalo commented Jan 1, 2025

KalleOlaviNiemitalo commented Jan 1, 2025

baronfel commented Jan 2, 2025

baronfel commented Jan 2, 2025

baronfel commented Jan 2, 2025

KalleOlaviNiemitalo commented Jan 2, 2025

Choose a reason for hiding this comment

baronfel Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baronfel commented Jul 28, 2024 •

edited

Loading

baronfel commented Jul 28, 2024 •

edited

Loading

baronfel commented Dec 31, 2024 •

edited

Loading

Forgind Jan 2, 2025 •

edited

Loading

KalleOlaviNiemitalo commented Jan 1, 2025 •

edited

Loading

baronfel Jan 2, 2025 •

edited

Loading