Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PowerShell dynamic completion script is not valid PowerShell #5847

Closed
2 tasks done
jennings opened this issue Dec 17, 2024 · 3 comments · Fixed by #5848
Closed
2 tasks done

PowerShell dynamic completion script is not valid PowerShell #5847

jennings opened this issue Dec 17, 2024 · 3 comments · Fixed by #5848
Labels
A-completion Area: completion generator C-bug Category: bug E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Experience needed to fix: Medium / intermediate

Comments

@jennings
Copy link
Contributor

jennings commented Dec 17, 2024

Please complete the following tasks

Rust Version

rustc 1.83.0 (90b35a623 2024-11-26)

Clap Version

master

Minimal reproducible code

# Sorry for the lack of example, but I think the issue is clear without this.

Steps to reproduce the bug with the above code

Follow the given instructions to install PowerShell dynamic completion:

echo "COMPLETE=powershell your_program | Invoke-Expression" >> $PROFILE

Actual Behaviour

Using the PowerShell dynamic completion installation instructions results in the PowerShell error when the profile loads:

COMPLETE=powershell: The term 'COMPLETE=powershell' is not recognized as a name of a cmdlet, function, script file, or executable program.
Check the spelling of the name, or if a path was included, verify that the path is correct and try again.

Expected Behaviour

The argument completer should be added to the PowerShell environment successfully.

Additional Context

The PowerShell dynamic completion profile script is not valid PowerShell:

$results = Invoke-Expression "{var}=powershell &{completer} -- $($commandAst.ToString())";

These are the main problems:

  • COMPLETE=powershell mycommand does not work in PowerShell, there is no way to set environment variables inline like this. Unfortunately, the best solution is probably to assign the env var, run the command, then restore the previous value.
  • &{completer} may have double-quotes from the shlex::try_quote(completer) above, which results in a syntax error like this: Invoke-Expression "COMPLETE=powershell "C:\path\to\binary.exe" -- ..."
  • When a command is quoted, it needs to be prefixed with the call operator &.
  • The AST may include characters that PowerShell interprets, so we need the stop processing operator --%

This should instead be written something like this:

    $completer = {completer}
    $prev = $env:{var}
    $results = Invoke-Expression "& $completer -- --% $commandAst";
    if ($null -eq $prev) {{
        Remove-Item Env:\{var}
    }} else {{
        $env:{var} = $prev
    }}    

In addition, the instructions to install this in the profile are not correct:

echo "COMPLETE=powershell your_program | Invoke-Expression" >> $PROFILE

This has the same inline env var problem, but also PowerShell tries to execute this line by line. It needs to be piped to Out-String:

//! $env:COMPLETE = "powershell"
//! echo "your_program | Out-String | Invoke-Expression" >> $PROFILE
//! Remove-Item Env:\COMPLETE

Debug Output

No response

@jennings jennings added the C-bug Category: bug label Dec 17, 2024
@epage epage added A-completion Area: completion generator E-medium Call for participation: Experience needed to fix: Medium / intermediate E-help-wanted Call for participation: Help is requested to fix this issue. labels Dec 17, 2024
@epage
Copy link
Member

epage commented Dec 17, 2024

I knew PowerShell support was under-developed when I merged it but I figured it would get overlooked otherwise. For moving this forward, we need someone with PowerShell experience to finish it up.

@jennings
Copy link
Contributor Author

I don’t think it’s far off, if I put the corrected script in my profile, dynamic completions in jj start working.

I started trying to write the patch, but haven’t had time to figure out how to test it.

@epage
Copy link
Member

epage commented Dec 17, 2024

Let's not hold up working for the sake of testing. All of our old script generators had no tests anyways.

jennings added a commit to jennings/clap that referenced this issue Dec 17, 2024
PowerShell does not support inline syntax for assigning environment
variables, so we must instead set the value before running the completer
and restore it after it exits.

The completer will often, if not always, be surrounded by double quotes.
To avoid syntax errors, define the argument to Invoke-Expression as a
here-string so the quotes are part of the expression.

Updates the instructions for adding the argument completer to the
profile. Piping a native command to Invoke-Expression invokes each line
separately. Adding `Out-String` to the pipeline ensures that
Invoke-Expression receives the whole script as a single value from the
pipeline.

Fixes: clap-rs#5847
@epage epage closed this as completed in 99b6391 Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-completion Area: completion generator C-bug Category: bug E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Experience needed to fix: Medium / intermediate
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants