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

Do not error on native binary that handles @ to expand response file contents as args #12491

Closed
rkeithhill opened this issue Apr 25, 2020 · 16 comments
Labels
Issue-Enhancement the issue is more of a feature request than a bug Resolution-No Activity Issue has had no activity for 6 months or more WG-Language parser, language semantics
Projects

Comments

@rkeithhill
Copy link
Collaborator

Summary of the new feature/enhancement

This might be a stretch but this is another one of those unfortunate gotchas when a dev is trying to use PowerShell to invoke commands that have been documented. Here's the problem:

04-25 14:54:57 84> g++ timer.cpp @conanbuildinfo.args -o timer --std=c++11
ParserError:
Line |
   1 |  g++ timer.cpp @conanbuildinfo.args -o timer --std=c++11
     |                ~~~~~~~~~~~~~~~
     | The splatting operator '@' cannot be used to reference variables in an expression.
     | '@conanbuildinfo' can be used only as an argument to a command. To reference variables
     | in an expression use '$conanbuildinfo'.

[master+13 ~9 -0 !] /mnt/c/Users/hillr/GitHub/Conan-IO/training/consumer_gcc
04-25 14:55:28 85> g++ timer.cpp '@conanbuildinfo.args' -o timer --std=c++11

In this case, conanbuildinfo.args is a file, specifically a response file that contains a bunch of arguments to g++. It is not that uncommon for exes to use a response file specified with @response_filename.

Proposed technical implementation details (optional)

I'm not entirely sure what the solution is but maybe in this case before erroring when the variable is not found (conanbuildinfo in the example above), consider passing the arg as-is to the native binary assuming it will handle the @. This might be worth emitting a warning. Something like: Could not find splatted variable 'conanbuildinfo', passing argument as-is to the application. To avoid this warning, put single quotes around the argument.. Also, if strict mode is set, maybe this should go ahead and error?

@rkeithhill rkeithhill added the Issue-Enhancement the issue is more of a feature request than a bug label Apr 25, 2020
@bpayette
Copy link
Contributor

@rkeithhill I assume quoting the argument is all that's needed to make it work - correct? We could update the error message to say that "blah, blah ... quote arguments starting with '@' blah blah ...". As far as "fixing" this, I just worry that, given how complex parameter binding already is, adding more special cases could ultimately make things worse, not better. How much of an issue do think this is?

@rkeithhill
Copy link
Collaborator Author

rkeithhill commented Apr 26, 2020

I assume quoting the argument is all that's needed to make it work

That is correct. It is a fair point to ask if the increased parameter binding complexity is worth it. As for the question of how much of an issue this is ... that is hard to say but I expect it is more of an issue now that new (Linux/macOS) users are kicking the tires on PowerShell. The problem is sort of a "first impression" issue. For most folks, their first impression is how well PowerShell does at executing commands from the console. The command line I listed above was from a training class on Conan. The first attempt in PowerShell was a failure - not a good first impression.

That said, there are so many of these issues - ; as a statement sep, $ for variables () for expressions, quoting issues, etc. Maybe there should be a more holistic approach to all these native app arg issues? Yes, I know about the stop parsing operator but anybody kicking the tires on PowerShell will not. And there is nothing in the error message that gives the user any help or clue as to how to rectify the situation. That is not a good user experience. At the very least, if we detected certain types of parameter errors for a native app, we should offer some suggestions on how to rectify the situation - use --% or quote args, etc. Ideally, in an interactive situation, PowerShell would offer to re-execute the command in a dumb parsing mode. There are likely a number of other ways to address this to varying degrees. Most of them are probably better than what we have now.

@iSazonov
Copy link
Collaborator

Most of them are probably better than what we have now.

And all will be a huge breaking change. Do we really want this if --% was introduced many years ago?

@iSazonov iSazonov added the WG-Language parser, language semantics label Apr 28, 2020
@rkeithhill
Copy link
Collaborator Author

Adding some "mitigation tips" to error messages wouldn't be breaking. Right now we leave new folks with nothing really to go on. Look at the original post's error message. That error message IN NO WAY helps the user in this case.

I think we could be smarter about parameter errors when they are for native applications and provide some tips in the error message like using --% or show how to invoke a help topic on parameter issues when invoking native applications.

@iSazonov
Copy link
Collaborator

Another thought is that if we say about interactive user session we could introduce "native command mode" - in the mode first execute input string "as-is" without parsing arguments and use "&" to force parsing.

@rkeithhill
Copy link
Collaborator Author

rkeithhill commented Apr 29, 2020

introduce "native command mode"

If we could do that in a non-breaking way that would be cool. That might also be able to address another issue where you'd like to invoke a native app and bypass any aliases or functions that happen to have the same name. I remember this issue was discussed quite a while back but nothing ever came of it. Well, except that certain aliases like ls and sort were removed on Linux/macOS.

@iSazonov
Copy link
Collaborator

If we could do that in a non-breaking way that would be cool.

It is a breaking change I think. I like it.

PS >& <Enter>           # We could use '&' to switch mode
PS &>                   # Now we are in native command mode
PS &>& <enter>          # Switch back in PowerShell command mode
PS >                    # Now we are in PowerShell command mode

Perhaps @JamesWTruher @daxian-dbw and @TylerLeonhardt could share thoughts.

@SteveL-MSFT
Copy link
Member

For the splatting sigil, we could always treat that as literal for native commands since you can't splat to native commands. However, the other characters that match to pwsh syntax is problematic. One possible solution is to have a suggestion based on a non-zero exit code that could check if any of those characters were used and suggest quoting them or using `--%' (which makes that switch more discoverable as I'm sure most users aren't familiar with it)

@vexx32
Copy link
Collaborator

vexx32 commented Jun 10, 2020

Native commands can have arrays splatted to them, so we can't assume @ is always a literal:

$params = @(
    '/c'
    'ping 1.1.1.1'
)

cmd @params

Also, to provide such a suggestion... we should probably fix up the suggestions feature first, so that we don't end up increasing the necessary work for that in future (assuming that's still something we want to do sooner or later).

@SteveL-MSFT
Copy link
Member

@vexx32 yeah, forgot about that was only thinking about hashtables. Agree we need to fix up the suggestion feature first for other reasons :)

@rkeithhill
Copy link
Collaborator Author

One possible solution is to have a suggestion based on a non-zero exit code that could check if any of those characters were used and suggest quoting them or using `--%' (which makes that switch more discoverable as I'm sure most users aren't familiar with it)

Me like. :-)

@lzybkr
Copy link
Member

lzybkr commented Jun 11, 2020

Food for thought - maybe it's possible to work like people expect most of the time.

  • In VariableOps.GetVariableValue - after the check for a failed lookup, defer the strict mode check when varAst.Splatted is true.
  • In this case, return an instance of a new type SplattedUndefinedVariable whose ToString() implementation renders the variable name with the @ before so the native command parameter binder "just works" on the value.
  • In PipelineOps.AddCommand - where splatting is expanded, if the value's type is SplattedUndefinedVariable and the command is not a native command, perform the strict mode check, and if that doesn't throw, replace the value with $null.

Copy link
Contributor

This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

2 similar comments
Copy link
Contributor

This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

Copy link
Contributor

This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

@microsoft-github-policy-service microsoft-github-policy-service bot added Resolution-No Activity Issue has had no activity for 6 months or more labels Nov 16, 2023
Copy link
Contributor

This issue has been marked as "No Activity" as there has been no activity for 6 months. It has been closed for housekeeping purposes.

@microsoft-github-policy-service microsoft-github-policy-service bot removed this from the Future milestone Nov 23, 2023
Shell automation moved this from To do to Done Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Enhancement the issue is more of a feature request than a bug Resolution-No Activity Issue has had no activity for 6 months or more WG-Language parser, language semantics
Projects
Shell
  
Done
Development

No branches or pull requests

7 participants