Repository: casey/just
Pull request #1320 (2022-08-26 05:01:07)

makes slash operator default lhs value to an empty string
I was looking for a _good first issue_ and came across https://github.com/casey/just/issues/1298

~The change is that the parser will now default an empty string literal on the left hand side of the `/` operator if it fails unwrapping a value for the lhs. This makes it possible to create a path with an expression like: `/ "Users" / user`.~

~Lemme know if there is anything that should be changed!~

Edit:
The approach was changed to wrapping the left-hand side (lhs) value of a join expression (using the `/` operator) to an `Option`.

erikkrieg: Hey, @casey. Anything I am missing or should do to make this ready for review?
casey: Sorry for the delay! I was out of town for the weekend, and just now catching up to my GitHub notifications. Thanks for taking this on! I left some comments on the code. The `unwrap_or` will might leave the parser in a bad state, so best to avoid it, and I think it's cleaner to make `lhs` an `Option` instead of the empty string.

I'd also like to see a few more tests. For example, in what context could the `unwrap_or` cause problems? Is there a test that would have caught it?
erikkrieg: Thanks for the feedback! I'll take a look tonight.
erikkrieg: I went on quite the journey trying to make `lhs` an optional expression for `Join` before putting a pin in that approach and going back to something close to my first pass. I went through the following files while changing `lhs` to an option:
- src/assignment_resolver.rs
- src/evaluator.rs
- src/expression.rs
- src/node.rs
- src/parser.rs
- src/string_kind.rs
- src/summary.rs
- src/variables.rs

The number of places to make the change wasn't so much a problem (tho a bit messy, I think) but the real issue was that I hit a snag with lifetimes on a borrow in `src/variables.rs` when needing to push a ref to a boxed expression onto the stack. In the case where `lhs.is_none()` I needed to provide an expression containing a string literal and I couldn't figure out a way to create one in a way that satisfied the lifetime requirements.

So I came back around full circle, except without the potentially breaking `unwrap` on `parse_value`.
casey: How about this:

```
        // if `lhs` is `Some`:
        Expression::Concatenation { lhs, rhs } | Expression::Join { Some(lhs), rhs } => {
          self.stack.push(rhs);
          self.stack.push(lhs);
        }
       // If `lhs` is `None`:
        Expression::Join { None, rhs } => {
          self.stack.push(rhs);
        }
```

Variables is an iterator that must return all variables (and thus traverse all sub-expressions) in an expression. If `lhs` is `None`, it contains no variables, so nothing needs to be pushed onto the stack.
erikkrieg: Okay, your suggestion for variables makes sense. I have an another branch locally with those changes, so I can give that a shot and see if that was truly the last compiler error for the option approach.

The question, then, is do you prefer the `None` case being handled in all those modules downstream of the parser? Or creating an empty string literal expression at the parser and leaving the rest unchanged? 
casey: I like handling the `None` case better, it seems more explicit.
erikkrieg: > Added some more comments. Also, some tests using prefix `/` in different contexts would be good.

I added tests for when lhs is `None` that seem to mirror the pre-existing tests for the slash operator. I also added a test for  the error case when rhs values/expression is not provided. Are there any other tests besides what are in the `slash_operator` test module that I should be looking at?
casey: These look good! And the PR looks good too, I think just a couple of pub(crate)s that aren't necessary.

It's really interesting that now `// "foo"` is valid, and is basically `"" / "" / "foo"`.

I'm not sure if that should be valid syntax, the thought being that maybe in the future, we want to use `//` for something else, and if we allow `// "foo"` now, then using `//` for something else would be a breaking change.

If, god forbid, I ever have to implement support for arithmetic in Just, then it would be pretty natural to use `//` for division. Also, I think it's pretty uncommon to want double slashes, although I suppose you could do: "https:" // "ordinals.com" / "page" to build a URL. Also, windows has UNC paths, which start with `//`, so you could use `//` there.

What do you think?

Either way, let's not hold this PR up. Even if we decide to make `//` illegal, we can merge this PR, and I can add a PR that changes `//` into an error, as long as I do it before I cut a release.
erikkrieg: That is an good question... 

**TL;DR:** I think `//"foo"` should be a valid expression.

Without having a very intimate knowledge of Just's internals, here are a few ideas for arithmetic:
1. Treat operators, like `+`, `*` and `/`, differently for ints and floats so that arithmetic can be written with a syntax familiar to many. 
2. Use functions for arithmetic, like `div(4,2)`, `add(1,2)`, `mul(2,2)`. It is more verbose, but very transparent.
3. Wrap expressions to indicate arithmetic should be performed, i.e. `[ 4 / 2 ]` works but `4 / 2` does not.

I am a bit less in favour personally of using `//` for division because I feel like that approach is most likely to lead to confusion both writing a reading equations in Justfiles compared to the other approaches. Though this is just opinion and speculation. I also don't have the best idea of what the actual effort to implement the different options is. Maybe `//` is much easier to implement, but I wasn't weighing that because I don't really know if it is the case or not.

Even if one of those options I listed were to be used in the case that arithmetic is added to Just, it doesn't answer you main question, but to me it does mitigate the concern about having a `//` to represent division. So then the question maybe become whether it is useful to have `//"foo"` evaluate to `"//foo"`. I suppose the simple answer is "yes", and you gave a couple cases. It also seems a bit more intuitive to me, but then again, I have been looking at this feature for a few days, so take that with a heap of salt.

casey: That all sounds pretty reasonable. Also, if we have arithmetic, we'll also have static typing, so that could be used to disambiguate joins from division.

I just merged master into this branch, and assuming the tests pass, is this ready to merge?
erikkrieg: > is this ready to merge?

Yes, it is!

Appreciate all the feedback and patience. 
casey: Sorry for the delay! Just added some docs. Thanks for sticking with this, this is a nice improvement.

Patch:
diff --git a/README.md b/README.md
index 8eb95f40..61ff9211 100644
--- a/README.md
+++ b/README.md
@@ -924,6 +924,17 @@ $ just --evaluate bar
 a//b
 ```
 
+Absolute paths can also be constructed:
+
+```make
+foo := / "b"
+```
+
+```
+$ just --evaluate foo
+/b
+```
+
 The `/` operator uses the `/` character, even on Windows. Thus, using the `/` operator should be avoided with paths that use universal naming convention (UNC), i.e., those that start with `\?`, since forward slashes are not supported with UNC paths.
 
 #### Escaping `{{`
diff --git a/src/assignment_resolver.rs b/src/assignment_resolver.rs
index 87e458ad..b1fcd6e8 100644
--- a/src/assignment_resolver.rs
+++ b/src/assignment_resolver.rs
@@ -101,10 +101,16 @@ impl<'src: 'run, 'run> AssignmentResolver<'src, 'run> {
           self.resolve_expression(c)
         }
       },
-      Expression::Concatenation { lhs, rhs } | Expression::Join { lhs, rhs } => {
+      Expression::Concatenation { lhs, rhs } => {
         self.resolve_expression(lhs)?;
         self.resolve_expression(rhs)
       }
+      Expression::Join { lhs, rhs } => {
+        if let Some(lhs) = lhs {
+          self.resolve_expression(lhs)?;
+        }
+        self.resolve_expression(rhs)
+      }
       Expression::Conditional {
         lhs,
         rhs,
diff --git a/src/evaluator.rs b/src/evaluator.rs
index 7f45c91d..2d4cb757 100644
--- a/src/evaluator.rs
+++ b/src/evaluator.rs
@@ -176,9 +176,11 @@ impl<'src, 'run> Evaluator<'src, 'run> {
         }
       }
       Expression::Group { contents } => self.evaluate_expression(contents),
-      Expression::Join { lhs, rhs } => {
-        Ok(self.evaluate_expression(lhs)? + "/" + &self.evaluate_expression(rhs)?)
-      }
+      Expression::Join { lhs: None, rhs } => Ok("/".to_string() + &self.evaluate_expression(rhs)?),
+      Expression::Join {
+        lhs: Some(lhs),
+        rhs,
+      } => Ok(self.evaluate_expression(lhs)? + "/" + &self.evaluate_expression(rhs)?),
     }
   }
 
diff --git a/src/expression.rs b/src/expression.rs
index 0393159d..4c0be54f 100644
--- a/src/expression.rs
+++ b/src/expression.rs
@@ -32,7 +32,7 @@ pub(crate) enum Expression<'src> {
   Group { contents: Box<Expression<'src>> },
   /// `lhs / rhs`
   Join {
-    lhs: Box<Expression<'src>>,
+    lhs: Option<Box<Expression<'src>>>,
     rhs: Box<Expression<'src>>,
   },
   /// `"string_literal"` or `'string_literal'`
@@ -51,7 +51,11 @@ impl<'src> Display for Expression<'src> {
   fn fmt(&self, f: &mut Formatter) -> Result<(), fmt::Error> {
     match self {
       Expression::Backtick { token, .. } => write!(f, "{}", token.lexeme()),
-      Expression::Join { lhs, rhs } => write!(f, "{} / {}", lhs, rhs),
+      Expression::Join { lhs: None, rhs } => write!(f, "/ {}", rhs),
+      Expression::Join {
+        lhs: Some(lhs),
+        rhs,
+      } => write!(f, "{} / {}", lhs, rhs),
       Expression::Concatenation { lhs, rhs } => write!(f, "{} + {}", lhs, rhs),
       Expression::Conditional {
         lhs,
diff --git a/src/node.rs b/src/node.rs
index 6afc0a43..60eb0422 100644
--- a/src/node.rs
+++ b/src/node.rs
@@ -118,7 +118,11 @@ impl<'src> Node<'src> for Expression<'src> {
       } => Tree::string(cooked),
       Expression::Backtick { contents, .. } => Tree::atom("backtick").push(Tree::string(contents)),
       Expression::Group { contents } => Tree::List(vec![contents.tree()]),
-      Expression::Join { lhs, rhs } => Tree::atom("/").push(lhs.tree()).push(rhs.tree()),
+      Expression::Join { lhs: None, rhs } => Tree::atom("/").push(rhs.tree()),
+      Expression::Join {
+        lhs: Some(lhs),
+        rhs,
+      } => Tree::atom("/").push(lhs.tree()).push(rhs.tree()),
     }
   }
 }
diff --git a/src/parser.rs b/src/parser.rs
index e04c1da9..effb9ca3 100644
--- a/src/parser.rs
+++ b/src/parser.rs
@@ -406,11 +406,15 @@ impl<'tokens, 'src> Parser<'tokens, 'src> {
 
     let expression = if self.accepted_keyword(Keyword::If)? {
       self.parse_conditional()?
+    } else if self.accepted(Slash)? {
+      let lhs = None;
+      let rhs = Box::new(self.parse_expression()?);
+      Expression::Join { lhs, rhs }
     } else {
       let value = self.parse_value()?;
 
       if self.accepted(Slash)? {
-        let lhs = Box::new(value);
+        let lhs = Some(Box::new(value));
         let rhs = Box::new(self.parse_expression()?);
         Expression::Join { lhs, rhs }
       } else if self.accepted(Plus)? {
@@ -1991,6 +1995,7 @@ mod tests {
         Identifier,
         ParenL,
         ParenR,
+        Slash,
         StringToken,
       ],
       found: Eof,
@@ -2010,6 +2015,7 @@ mod tests {
         Identifier,
         ParenL,
         ParenR,
+        Slash,
         StringToken,
       ],
       found: InterpolationEnd,
diff --git a/src/summary.rs b/src/summary.rs
index 7d467ddb..89578b5a 100644
--- a/src/summary.rs
+++ b/src/summary.rs
@@ -197,7 +197,7 @@ pub enum Expression {
     operator: ConditionalOperator,
   },
   Join {
-    lhs: Box<Expression>,
+    lhs: Option<Box<Expression>>,
     rhs: Box<Expression>,
   },
   String {
@@ -258,7 +258,7 @@ impl Expression {
         rhs: Box::new(Expression::new(rhs)),
       },
       Join { lhs, rhs } => Expression::Join {
-        lhs: Box::new(Expression::new(lhs)),
+        lhs: lhs.as_ref().map(|lhs| Box::new(Expression::new(lhs))),
         rhs: Box::new(Expression::new(rhs)),
       },
       Conditional {
diff --git a/src/variables.rs b/src/variables.rs
index f1bd506f..9d620a23 100644
--- a/src/variables.rs
+++ b/src/variables.rs
@@ -53,10 +53,16 @@ impl<'expression, 'src> Iterator for Variables<'expression, 'src> {
           self.stack.push(lhs);
         }
         Expression::Variable { name, .. } => return Some(name.token()),
-        Expression::Concatenation { lhs, rhs } | Expression::Join { lhs, rhs } => {
+        Expression::Concatenation { lhs, rhs } => {
           self.stack.push(rhs);
           self.stack.push(lhs);
         }
+        Expression::Join { lhs, rhs } => {
+          self.stack.push(rhs);
+          if let Some(lhs) = lhs {
+            self.stack.push(lhs);
+          }
+        }
         Expression::Group { contents } => {
           self.stack.push(contents);
         }


Test patch:
diff --git a/tests/slash_operator.rs b/tests/slash_operator.rs
index 5995a32e..31859e3b 100644
--- a/tests/slash_operator.rs
+++ b/tests/slash_operator.rs
@@ -18,6 +18,45 @@ fn twice() {
     .run();
 }
 
+#[test]
+fn no_lhs_once() {
+  Test::new()
+    .justfile("x := / 'a'")
+    .args(&["--evaluate", "x"])
+    .stdout("/a")
+    .run();
+}
+
+#[test]
+fn no_lhs_twice() {
+  Test::new()
+    .justfile("x := / 'a' / 'b'")
+    .args(&["--evaluate", "x"])
+    .stdout("/a/b")
+    .run();
+  Test::new()
+    .justfile("x := // 'a'")
+    .args(&["--evaluate", "x"])
+    .stdout("//a")
+    .run();
+}
+
+#[test]
+fn no_rhs_once() {
+  Test::new()
+    .justfile("x := 'a' /")
+    .stderr(
+      "
+      error: Expected backtick, identifier, '(', '/', or string, but found end of file
+        |
+      1 | x := 'a' /
+        |           ^
+    ",
+    )
+    .status(EXIT_FAILURE)
+    .run();
+}
+
 #[test]
 fn default_un_parenthesized() {
   Test::new()
@@ -39,6 +78,27 @@ fn default_un_parenthesized() {
     .run();
 }
 
+#[test]
+fn no_lhs_un_parenthesized() {
+  Test::new()
+    .justfile(
+      "
+      foo x=/ 'a' / 'b':
+        echo {{x}}
+    ",
+    )
+    .stderr(
+      "
+      error: Expected backtick, identifier, '(', or string, but found '/'
+        |
+      1 | foo x=/ 'a' / 'b':
+        |       ^
+    ",
+    )
+    .status(EXIT_FAILURE)
+    .run();
+}
+
 #[test]
 fn default_parenthesized() {
   Test::new()
@@ -52,3 +112,17 @@ fn default_parenthesized() {
     .stdout("a/b\n")
     .run();
 }
+
+#[test]
+fn no_lhs_parenthesized() {
+  Test::new()
+    .justfile(
+      "
+      foo x=(/ 'a' / 'b'):
+        echo {{x}}
+    ",
+    )
+    .stderr("echo /a/b\n")
+    .stdout("/a/b\n")
+    .run();
+}