casey / just

🤖 Just a command runner
https://just.systems
Creative Commons Zero v1.0 Universal
20.58k stars 452 forks source link

Shell expanded string syntax, `x"STRING"` is ambiguous in recipe dependencies, `foo: (bar x "STRING")` #2074

Closed laniakea64 closed 4 months ago

laniakea64 commented 4 months ago

One of my justfiles contains code along these lines for creating new Cargo projects -

_new_common name vcs extra_toml:
    cargo new --vcs {{vcs}} {{quote(name)}}
    echo {{quote(extra_toml)}} >> {{quote(name / 'Cargo.toml')}}

new_foo name vcs='none': (_new_common name vcs '''

[dependencies.clap]
version = "4"
''')

In just 1.26.0, just --evaluate works without error and the recipes run as expected.

With just master, just --evaluate gives this error -

error: Expected identifier, '(', ')', '+', '/', or string, but found identifier
 ——▶ justfile:5:44
  │
5 │ new_foo name vcs='none': (_new_common name vcs '''
  │                                            ^^^
casey commented 4 months ago

Thanks for the report, and thank you very much for testing master!

I figured out what's going on, and it's quite annoying.

I added a new kind of string literal in #2055. These strings are of the form x'FOO', are evaluated at compile time, and perform environment variable and homedir (~ and ~USER) substitution. These are intended primarily to be used it mod and import statements, as well as settings like dotenv-path.

This is all well and good, since in most contexts, IDENT STRING is not a valid sequence of tokens.

However, when passing arguments to recipe dependencies, IDENT STRING is valid, i.e.:

foo a b:
bar a: (foo a 'c')

I think it's probably unfortunate that I chose the somewhat odd, lisp style (foo a 'c') instead of the more normal foo(a, 'c'). In particular because we already use FUNCTION(ARG, ARG) for function calls.

Anyways, that ship has failed, so I probably need to rethink the x-string syntax, which is unfortunate because I wanted to use f'FOO' for format strings, copying python.

casey commented 4 months ago

Where is this mentioned in the documentation ?

I'm not sure what you mean. This was an unintentional regression and will be fixed before the next release.

laniakea64 commented 4 months ago

I'm not sure what you mean.

I think they were confused by my Cargo.toml pseudocode, which I used because the actual code in my justfile is an expression more complex than necessary to reproduce the issue.

Changed to something more likely to happen in actual use. Sorry for the confusion.

gl-yziquel commented 4 months ago

I think they were confused by my Cargo.toml pseudocode, which I used because the actual code in my justfile is an expression more complex than necessary to reproduce the issue.

Precisely.

For a moment, I thought there was a way to specify inline the version of just that a Justfile requires. (Which would have been useful for me as I relied on rather recent just functionalities, and for weird path management related reasons on machine, I ended up with a way too old version of just that barked on my justfiles.)

I was confused. Sorry.

casey commented 4 months ago

For a moment, I thought there was a way to specify inline the version of just that a Justfile requires. (Which would have been useful for me as I relied on rather recent just functionalities, and for weird path management related reasons on machine, I ended up with a way too old version of just that barked on my justfiles.)

Ahh, okay, no worries.

casey commented 4 months ago

Okay, as shitty as it is, my only real idea here is to distinguish between general expression contexts and recipe dependencies, and in recipe dependencies, require parenthesis if you want a shell-expanded string.

So this would pass x and "STRING" to bar:

foo x: (bar x "STRING")

And this would pass the shell-expanded string to bar:

foo x: (bar (x "STRING"))

This is very lame, but recipe dependency syntax is just weird. If I could go back in time, I would make it look like function call syntax, but here we are 😅

laniakea64 commented 4 months ago

Why not require that there be no space between x and the string for a shell-expanded string? so that x'STRING' would be a shell-expanded string but e.g. the following wouldn't

foo := x 'STRING'
bar := x \
'STRING'

Or would it help to use a non-IDENT character instead of x?

casey commented 4 months ago

Why not require that there be no space between x and the string for a shell-expanded string?

We could do that, but it would still technically be backwards incompatible, since there's nothing stopping someone from writing their recipe dependencies as:

foo x: (bar x"bar")

As odd as it might be. I could bust out Janus and do a run of all justfiles on GitHub, to see if anyone actually does that.

However, Rust, Python, an C++ all require that there be no space between the string prefix and the string, so perhaps we should also enforce this.

Or would it help to use a non-IDENT character instead of x?

I was thinking about this, but that would have the downside of not allowing f"STRING" as the future syntax for stings which allow interpolation, and I'd really like to keep that as f"STRING", since that's the syntax that python uses.

casey commented 4 months ago

Actually, I think probably if we require there to be no space, and we only try to parse a shell-expanded string if the ident is x, then that's probably fine.

Although this is a valid justfile:

foo x: (bar x"bar")

I think the chances are vanishingly small that a user has a parameter named x that they're forwarding to a dependency, and they have a string afterwards with no spaces.

casey commented 4 months ago

I just merged #2083, which forbids whitespace between the x and the string token, and makes the parser more precise by checking for an ident with lexeme x followed by a string token, instead of just checking for any ident followed by a string token.

God save the just users with foo x: (bar x"bar") in their justfile 🫡

casey commented 4 months ago

And thanks again @laniakea64 for reporting this! I definitely would have missed it and it probably would have gotten into a release otherwise 😅

laniakea64 commented 4 months ago

And thanks again @laniakea64 for reporting this!

You're welcome. :+1:

God save the just users with foo x: (bar x"bar") in their justfile 🫡

If it's that concerning, then...

I could bust out Janus and do a run of all justfiles on GitHub, to see if anyone actually does that.

... maybe worth such run to check for this and foo f: (bar f'string')?

casey commented 4 months ago

I'm not suuuuuuuper worried. x"FOO" and f"FOO" are probably rare, if they ever appear at all. I dusted off janus, but it's currently defunct so I couldn't run it. janus uses screen scraping, and the HTML changed enough to break it, and going via the actual search API only returns 1000 results, which isn't much. I asked support for an exception so I can get all the paths of all files named justfile and *.just on GitHub, but we'll see if they respond.