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

Argument forwarding doesn't work with spaces in strings #647

Closed
Boscop opened this issue Jun 13, 2020 · 25 comments
Closed

Argument forwarding doesn't work with spaces in strings #647

Boscop opened this issue Jun 13, 2020 · 25 comments

Comments

@Boscop
Copy link

Boscop commented Jun 13, 2020

Related to #631

w +args="":
    cargo watch -x "r {{args}}"
> just w -- -a "D:\foo bar baz.mp3"
cargo watch -x "r -- -a D:\foo bar baz.mp3"
[Running 'cargo r -- -a D:\foo bar baz.mp3']
    Finished dev [optimized + debuginfo] target(s) in 0.65s
     Running `D:\projects\myproject.exe -a 'D:\foo' bar 'baz.mp3'`
Unrecognized argument: bar

error: process didn't exit successfully: `D:\projects\myproject.exe -a 'D:\foo' bar 'baz.mp3'` (exit code: 1)
[Finished running. Exit status: 0]
@rjsberry
Copy link
Contributor

rjsberry commented Jun 13, 2020

As a workaround escape the double quotes with \ when you run Just (existing \ also needs an escape):

> just w -- -a \"D:\\foo bar baz.mp3\"

@casey
Copy link
Owner

casey commented Jun 13, 2020

Unfortunately this is an issue that I don't have a good solution for.

Each recipe line is passed to the shell as a single string, and just has no knowledge of the shell's splitting and quoting rules.

It might be possible to add logic to quote argument values when interpolating, but this would probably be flakey and not always do what you wanted.

One possibility would be to allow arguments to be exported as environment variables, since I believe that most shells ignore spaces when interpolating into strings:

# tell just that `args` should be exported as an environment variable
w $+args="":
    cargo watch -x "r $args"

This is definitely ugly and weird though.

@casey
Copy link
Owner

casey commented Jun 13, 2020

Actually, I don't think exporting the argument as an environment variable does what you would want, so scratch that.

@rjsberry
Copy link
Contributor

rjsberry commented Jun 13, 2020

I think this could be resolved by single quoting the values in the argument interpolation.

For the Just invoc just foo "bar baz" qux, clap will parse ARGUMENTS as:

MatchedArg { occurs: 2, indices: [3, 4], vals: ["bar baz", "qux"] }

Say our justfile is:

foo +args:
  ls {{args}}

We'd get the desired behaviour by evaluating the args interpolation as 'bar baz' 'qux' (as parsed by clap).

@casey
Copy link
Owner

casey commented Jun 13, 2020

There are a couple issue that I can think of with automatically adding quotes, one is that it might not always be desired. For example:

foo file +flags:
  cc {{file}} {{flags}}

If the flags arg is intended to hold multiple compiler flags, surrounding them with single quotes will cause them to be passed to cc as a single argument.

Another is that it's shell specific. Just supports both switching the shell with the shell setting, and shebang recipes that use #!... directives to use an arbitrary interpreter. Shells and interpreters might have different quoting rules, so single quotes might not be desired in all cases.

@rjsberry
Copy link
Contributor

rjsberry commented Jun 15, 2020

Just to clarify I meant quoting each arg as parsed by the shell rather than the entire expansion.

foo file +flags:
  cc {{file}} {{flags}}

Using this justfile I would expect an implementation of quoting on expansion to give:

  • just foo foo.c -o foo.o: cc 'foo.c' '-o' 'foo.o'
  • just foo foo.c -o "foo bar.o": cc 'foo.c' '-o' 'foo bar.o'

Shells and interpreters might have different quoting rules, so single quotes might not be desired in all cases.

Hmm yeah, this one is tricky.

Intuitively it feels like Just should respect the argument quoting. Without applying some sort of quoting to the argument expansion I'm not sure how else this could be achieved (without introducing weird new syntax for expansion behaviours). Perhaps with #604 there could be an attribute to opt-out of quoting on expansion of variadic arguments? This would mean that expansion would "just work" by default, but the quoting could still be disabled to satisfy cases where it's not desired.

@casey
Copy link
Owner

casey commented Jun 16, 2020

This would have to be applied as the last step in interpolation. For example:

foo a b:
  echo {{a + b}}

If you do just foo x y you probably want echo xy or echo 'xy' and not echo 'x''y'.

Concatenating a variadic and non variadic argument is also tricky:

bar a +b:
  echo {{a + b}}

What should just bar a b c print? It isn't clear to me that there are any quoting rules that would make sense.

Also, if anything is inserted into a string literal, and not passed to the shell, then quoting would be undesirable:

checkpoint message:
  git commit -am "checkpoint: {{message}}"

@runeimp
Copy link

runeimp commented Jul 7, 2020

Escaping \ the spaces of a string would have the desired effect without introducing over specific quotes which may break the string in question which may potentially contain both single and double quotes. I believe this works on Windows CMD and PowerShell as well using ^

@Boscop
Copy link
Author

Boscop commented Jul 8, 2020

@runeimp You mean that just could insert the backslashes? Or that it can't, and that the only way to make this work is to manually insert the backslashes in the args passed to just?

@runeimp
Copy link

runeimp commented Jul 24, 2020

@Boscop Yes, just could look at an argument, determine it has spaces, then present that argument with spaces escaped when expanded. 🙂

@tomprince
Copy link

There could be a function to shell escape its argument.

@casey
Copy link
Owner

casey commented Nov 8, 2021

I added a quote function in #1022, which should work to quote strings for most shells.

However, having to call the quote function all the time would be tedious.

This would have some downsides:

  • All arguments would be quoted, even arguments that don't need to be. This might make output more verbose.
  • If you want not to quote arguments, for example, if you're passing multiple flags to a program, or you want the user to be able to pass globs, you would have no way to turn off quoting for a particular value.

I'm not sure such a setting would be worth it, and it might be better addressed in other ways.

@Aloso
Copy link

Aloso commented Mar 23, 2022

@casey The quote function does not work for this use case. +args can contain multiple arguments, but {{quote(args)}} merges them into one.

For example, with this justfile:

run +args:
    cargo run -- {{quote(args)}}

When you type just run foo "bar baz", it will be expanded to cargo run -- 'foo bar baz', but it should be cargo run -- 'foo' 'bar baz'.

I agree with @runeimp that escaping spaces would be a solution: cargo run -- foo bar\ baz works in sh, bash, fish and zsh. It doesn't work in Powershell or cmd, however.

@casey
Copy link
Owner

casey commented Mar 24, 2022

@casey The quote function does not work for this use case. +args can contain multiple arguments, but {{quote(args)}} merges them into one.

This is a limitation of how variadic arguments work. Just merges them into one space-separated string before they get to the quote function.

The best way to solve this is probably to pass arguments positionally:

use positional-arguments

run +args:
    cargo run -- "$@"

@Aloso
Copy link

Aloso commented Mar 24, 2022

@casey Thanks, this worked!

One nit: s/use positional-arguments/set positional-arguments/

@casey
Copy link
Owner

casey commented Mar 24, 2022

One nit: s/use positional-arguments/set positional-arguments/

Whoops, too much Rust 😅

@camsteffen
Copy link
Contributor

Does this work?

set positional-arguments

w +args="":
    #!/usr/bin/env zsh
    cargo watch -x "r ${(q-)@}"

@casey
Copy link
Owner

casey commented Jan 25, 2023

I think the recommended way to avoid splitting is to use set positional-arguments, so this can be closed. It's annoying that there isn't a better option without positional arguments, but I think that's just a limitation of how just variables work. (E.g. they use string interpolation and don't know anything about the shell.)

@casey casey closed this as completed Jan 25, 2023
@MayCXC
Copy link

MayCXC commented Sep 4, 2024

@casey would you accept this contribution?

w *VARGS:
  egcmd {{quote_each(VARGS)}}

to quote each variadic arg like this: egcmd {{quote(VARGS[0])}} {{quote(VARGS[1])}}...

@laniakea64
Copy link
Contributor

@MayCXC That would not work the way you think.

In just, everything is a string. The VARGS parameter is never a list. If you run just w 'foo bar' baz, the VARGS parameter will be set to foo bar baz, and a quote_each function would have no way to know not to return 'foo' 'bar' 'baz'.

Although, maybe that could be helped with a just setting to separate variadic parameters' arguments by an unlikely character (e.g. NULL character \x00) instead of space?
(Or maybe the setting could even allow specifying a custom character/string? But that would be more useful if just strings allowed more escape sequences.)

@MayCXC
Copy link

MayCXC commented Sep 4, 2024

@MayCXC That would not work the way you think.

In just, everything is a string. The VARGS parameter is never a list. If you run just w 'foo bar' baz, the VARGS parameter will be set to foo bar baz, and a quote_each function would have no way to know not to return 'foo' 'bar' 'baz'.

Although, maybe that could be helped with a just setting to separate variadic parameters' arguments by an unlikely character (e.g. NULL character \x00) instead of space? (Or maybe the setting could even allow specifying a custom character/string? But that would be more useful if just strings allowed more escape sequences.)

maybe we can just make a separate parameter egcmd {{quote_each(*VARGS)}} for variadic arguments? the "actual" args come in unconcatenated here:

if let Err(code) = just::run(std::env::args_os()) {
and set positional-args does stuff with them here https://github.com/casey/just/blob/d3bbde0c6049eb124d56fac396b670397e2c3977/src/justfile.rs#L335C18-L335C37 , so i think just already has everything it needs to do this, except a syntax.

@MayCXC
Copy link

MayCXC commented Sep 4, 2024

how about with a Function::NullaryPlus(fn(Context, &[String])) for quote_each? and a VariadicParameter<'src> structure or similar that unpacks in the last position of any _Plus function, so you can do shell(CMD, ARG, *VARGS) etc. this would also make it possible to just modify quote and not have a separate quote_each function.

@casey
Copy link
Owner

casey commented Sep 4, 2024

Yah, @MayCXC, @laniakea64 is correct, by the time any variable gets to a function, it has already been turned into a string, so there's no way to know if it was a variadic parameter.

Changing this would be a huge change to the code.

I actually have a very wacky idea for how to fix this, but is such a huge undertaking that it is almost certainly not worth it.

The idea is this: Just currently has a simple and safe static type system, because everything is a string. However, lists of strings are not supported, which prevents things like quoting variadic arguments, and also prevents implementing a lot of things that people want. So, what you could do is make it so that every value is a list of strings, with all existing constructs producing a length-one list, with the exception of variadic arguments, which produce a variable-length list of strings. For backwards compatibility, all existing functions must behave the same as they currently do, which means space-joining the list to produce a single value, and then operating on that value, but then you can add new functions which can do something different, for example quoting, or doing something for each entry in the list.

But, like I said, this would be a massive code change, so probably isn't worth it.

@MayCXC
Copy link

MayCXC commented Sep 4, 2024

The idea is this: Just currently has a simple and safe static type system, because everything is a string. However, lists of strings are not supported, which prevents things like quoting variadic arguments, and also prevents implementing a lot of things that people want. So, what you could do is make it so that every value is a list of strings, with all existing constructs producing a length-one list, with the exception of variadic arguments, which produce a variable-length list of strings. For backwards compatibility, all existing functions must behave the same as they currently do, which means space-joining the list

should i give it a shot and see if it's doable in like ~500LoC? you already have functions like shell that take a variable number of arguments, and parameters with ParameterKind::star have the needed list of strings for positional arguments. the grammar would just need ('*' | '+')? NAME in value, then a new field VariadicArgument in https://github.com/casey/just/blob/master/src/expression.rs#L10 . then

pub(crate) fn resolve(
can just handle two additional cases, a *NAME in a &[String] position, or a +NAME in a &str, &[String] position. VARGS is still just a string, so everywhere but function argument sequences should be able to ignore the change, and raise a parse error if a VariadicArgument value appears.

@casey
Copy link
Owner

casey commented Sep 4, 2024

If you can do it simply, then why not, although I suspect you'll run into issues and it'll start to balloon. The core issue is that, by the time anything happens, the var arg is just a single string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants