PowerShell / PowerShell

PowerShell for every system!
https://microsoft.com/PowerShell
MIT License
43.98k stars 7.12k forks source link

Argument with colon in it is incorrectly handled if not quoted #23819

Open CrendKing opened 1 month ago

CrendKing commented 1 month ago

Prerequisites

Steps to reproduce

I understand the -<arg_name>:<arg_value> syntax in PowerShell. But it seems there lacks a way to tell PowerShell to simply passing the argument as-is without parsing, without requiring caller to explicitly quoting the whole thing. I found the issue https://github.com/PowerShell/PowerShell/issues/6292#issuecomment-371344550 discussed this 6 years ago, but I can't find any fix for that issue. If I'm missing a solution to this, please let me know.

Why does this matter? Because some applications like ffmpeg extensively use the -<arg_name>:<arg_value> syntax. There is no way to write a function to wrap such application cleanly.

function test {
    Write-Host $args
}

function Call-ffmpeg {
    ffmpeg $args
}

test -a:b 1

Call-ffmpeg -i test.mp4 -filter:v null -f null -

Expected behavior

-a:b 1

<ffmpeg successfully processing the file>

Actual behavior

-a: b 1

<ffmpeg reports "Unable to choose an output format for 'null'; use a standard extension for the filename or specify the format manually.">

Error details

No response

Environment data

PSVersion                      7.4.2
PSEdition                      Core
GitCommitId                    7.4.2
OS                             Microsoft Windows 10.0.22631
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0…}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0

Visuals

No response

mklement0 commented 1 month ago

Unfortunately, there are a number of long-standing bugs relating to argument-passing to external (native) programs.

GitHub
Issues · PowerShell/PowerShell
PowerShell for every system! Contribute to PowerShell/PowerShell development by creating an account on GitHub.
jborean93 commented 1 month ago

In this case the problem isn't with the native parser but just how parameter binding in PowerShell works in general. Calling a native executable command directly will go through one binder that passes through -foo:bar as is but when you call a function it'll go through the normal binder where -foo:bar is treated as -foo bar.

So by the time Call-ffmpeg is calling the exe, the $args would have already converted -filter:v into distinct arguments -filter, v as that's how the parser and normal function binder works. You can see the difference between calling it in the function vs calling it normally.

function Call-ffmpeg {
    C:\temp\print_argv.exe $args
}

Call-ffmpeg -i test.mp4 -filter:v null -f null -

"C:\temp\print_argv.exe" -i test.mp4 -filter: v null -f null -
[0] -i
[1] test.mp4
[2] -filter:
[3] v
[4] null
[5] -f
[6] null
[7] -

C:\temp\print_argv.exe -i test.mp4 -filter:v null -f null -

"C:\temp\print_argv.exe" -i test.mp4 -filter:v null -f null -
[0] -i
[1] test.mp4
[2] -filter:v
[3] null
[4] -f
[5] null
[6] -

While I can see an argument about this being a bug I personally think it's just how things work and changing the behaviour today would probably result in more problems.

The simplest solution here is to just quote your argument or provide it as an array to your wrapper with the value already provided as a string:

function Call-ffmpeg {
    ffmpeg @args
}

Call-ffmpeg -i test.mp4 '-foo:bar'
Call-ffmpeg @('-i', 'test.mp4', '-foo:bar')
CrendKing commented 1 month ago

Ideally there could be something like a

function Call-ffmpeg {
    [CmdletBinding(PassArgumentsAsIs = $true)]  # <--
    param(
        [Parameter(Mandatory, ValueFromRemainingArguments)]
        $args
    )
    ...
}

that allows user to change how the "binder" works.

mklement0 commented 1 month ago

@jborean93, you're right, the problem is the parameter binder for PowerShell commands in this case.

That it is a bug, even by the rules of PowerShell commands, is exemplified by the following:

# !! -> '-foo: bar' - space inserted (due to mistaken interpretation as a parameter name-value pair)
Write-Host -- -foo:bar
# -> !! turns into the equivalent of -foo:$TRUE
& { & { param([switch] $foo) $PSBoundParameters } @args } -foo:$false

As for fixing the bug in the context of relaying arguments to native programs - probably the most likely scenario - @BrucePay suggested the following in the aforementioned #6360 (see there for more context):

One way to fix this is to propagate the "NoSpace" token property into metadata on the corresponding string value. The NativeCommandParameterBinder could then check for this metadata when converting its arg array into a string. It's probably worth thinking about this a bit more to see if there are other cases (especially *nix specific cases) that should be addressed.

jhoneill commented 1 month ago

Why does this matter? Because some applications like ffmpeg extensively use the -<arg_name>:<arg_value> syntax. There is no way to write a function to wrap such application cleanly.

It is perfectly possible to call command like the ffmpeg one you gave from PowerShell . The problem is that you seem to want to run a PowerShell command as if it were ffmpeg with normal PowerShell-isms about commands suspended.

Really the job of an external-command wrapper is to take parameters in PowerShell style (naming each one, allowing them to be in any order if named, suggesting names but removing names when they are not compatible with parameters already provided, providing tab completion for values where possible yada yada yada) and transforming those into whatever weird and/or wonderful way of doing things seemed a good idea to the external command's authors when they first though about it.

$args is a throwback to V1 of PowerShell and does not work as soon as you add [cmdletBinding()] or declare parameter attributes``` function foo { param($bar) $bar ; $args}

foo 1 2 3 1 2 3 function foo { param([Parameter(ValueFromPipeline)]$bar) $bar ; $args} foo 1 2 3 foo: A positional parameter cannot be found that accepts argument '2'.


if not officially deprecated, its use is widely considered to be ill advised.   Using `externalCommand $args` is a pretty bad idea all round, but doing it right does mean more code in the wrapper than you might like.      
CrendKing commented 1 month ago

@jhoneill I'm so sorry I don't understand what you are talking about. What I asked for is a way to remove this inconsistency as follows:

  1. Run ffmpeg -i test.mp4 -filter:v null -f null - in PowerShell. Every works fine.
  2. User wants to always hide the ffmpeg banner. Since ffmpeg doesn't allow any environment variable or config file to turn it off, user has to always pass the -hide_banner argument. Since the only way in PowerShell to make a command alias with arguments included is function, naturally user creates
    function ffmpeg {
    D:\ffmpeg\bin\ffmpeg.exe -hide_banner $args
    }
  3. Suddenly the same ffmpeg -i test.mp4 -filter:v null -f null - no longer works. User has to do ffmpeg -i test.mp4 '-filter:v' null -f null - from now on.

In Bash, one can just alias ffmpeg="D:\ffmpeg\bin\ffmpeg.exe -hide_banner" and it works. It is a problem that was solved 35 years ago since 1989, yet PowerShell still struggles today. Are we expecting too much?

MartinGC94 commented 1 month ago

You have a valid point but bringing up the age of the tool as if that's the reason why it's not working like Bash is silly. PowerShell aliases are simply different from Bash. I don't like the way Bash handles whitespace but I don't expect them to change it just because other shells do it differently.

You can do whatever you want inside a function so just add some additional processing inside your function to join the colon parameters with your value, here's an example to get you started:

    function ffmpeg
    {
        $Param = ""
        $NewArgs = foreach ($Item in $args)
        {
            if ($Item.EndsWith(':'))
            {
                $Param = $Item
                continue
            }

            if ($Param -ne "")
            {
                $Param + $Item
                $Param = ""
            }
            else
            {
                $Item
            }
        }

        D:\ffmpeg\bin\ffmpeg.exe -hide_banner $NewArgs
    }
CrendKing commented 1 month ago

But why do we have to do this if without the function the "binder" can already correctly process the colons? Why can't there be an option to allow user to use that "binder" for function? I brought up Bash because I think Bash got their priority right. The consistency of syntax with or without function should be the most important to be preserved. PowerShell already has advanced parameter attributes. It should have the colon splatting optional unless a special attribute presents, not the other way:

param(
    [switch]$a,
    [Parameter(ColonSplat)]
    $b,
    $c
)

test -a:$false -b:value_of_b -c:this_is_part_of_the_whole_argument_not_just_value_of_c
mklement0 commented 1 month ago

@CrendKing

jhoneill commented 1 month ago

@jhoneill I'm so sorry I don't understand what you are talking about.

OK. Basic premise is that PowerShell commands should all work in the same way. That means they don't have unnamed parameters, and the use of $Args has been discouraged for as long as I can remember.

If you write a function to do a bunch of things which use ffmpeg that function should work like everything else in PowerShell , and should take parameters in PowerShell style and translate them when calling ffmpeg.

What I asked for is a way to remove this inconsistency as follows:

  1. Run ffmpeg -i test.mp4 -filter:v null -f null - in PowerShell. Every works fine.

So far so good, and you can build a function to call that...

  1. User wants to always hide the ffmpeg banner. Since ffmpeg doesn't allow any environment variable or config file to turn it off, user has to always pass the -hide_banner argument. Since the only way in PowerShell to make a command alias with arguments included is function,

Ah, what I hadn't picked up from the first post is that your not doing a bunch of stuff which involves ffmpeg, you just want an alias-with-parameters which PowerShell doesn't naturally do. One way to do what bash does and replace ffmpeg with ffmpeg.exe -no_banner is function ffmpeg {Invoke-Expression ($MyInvocation.statement -replace "^.*ffmpeg", "ffmpeg.exe -hide_banner") }

invoke-expression has its detractors but it's the lesser of multiple evils here

This

function ffmpeg {
    D:\ffmpeg\bin\ffmpeg.exe -hide_banner $args
}

Is the way many new to PowerShell users who expect their scripts to refer to arguments 1 , 2 and 3 etc would do it writing a PowerShell wrapper for all the parameters to do a simple fix like this is horribly laborious - but when it it is fixing a particularly awkward utility PowerShell's assumptions about prepping stuff for a PowerShell command mean the roof falls in.

In Bash, one can just alias ffmpeg="D:\ffmpeg\bin\ffmpeg.exe -hide_banner" and it works.

Yup. PowerShell aliases were to replace names. Re-writing the command and all its parameters, use $MyInvocation.statement not args

It is a problem that was solved 35 years ago since 1989, yet PowerShell still struggles today. Are we expecting too much?

Like so many other things if you know where to look PowerShell had a solution back at the start.

CrendKing commented 1 month ago

Thank you! Invoke-Expression + $MyInvocation.Statement is indeed a trick to solve the "dumb" aliasing use case.

mklement0 commented 1 month ago

@CrendKing:

While it's always good to know a workaround, we shouldn't lose sight of the bigger picture:


The - obscure - Invoke-Expression workaround may work in simple cases, but it isn't robust, because blindly re-invoking a statement's source code, as reflected in $MyInvocation.Statement, can repeat operations that have side effects.

Here's a simple example (run on Windows):

function foo { iex ($MyInvocation.Statement -replace '^foo\b', 'cmd /c echo') }
$i = 0; foo (++$i)

This prints 2 rather than 1, because the ++ operation was performed twice.

(It also doesn't address automatic support for pipeline input, though you can't fault it for that, as even fixing the bug at hand wouldn't address that; only https://github.com/PowerShell/PowerShell/issues/12962#issuecomment-2096785319 would.)