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

feat(syntax): add match operator #2267

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

t3hmrman
Copy link
Contributor

This commit adds a match expression, similar to the construct in the
Rust programming language.

An example of the match statement follows:

match true {
  true => "yes",
  false => "no",
}

@laniakea64
Copy link
Contributor

I'm confused. The original request in #1990 appeared to be asking for something like

some_var := match os() {
  "linux" => "foo",
  "macos" => "bar",
  "windows" => "baz",
  other => "[" + other + "]"
}

Which I agree would be nicer than the equivalent chain of if statements.

Looking at the syntax example and the tests in this PR, looks like this implementation of match statements is only an alias for if statements? 😕

This implementation appears to be introducing a boolean type, which in just would be weird and out of place: Everything being a string is a feature of just. And matching on true and false anyway doesn't seem useful. The case where a match-like construct would shine in just is testing a single string value against a series of other string values.

@casey
Copy link
Owner

casey commented Jul 20, 2024

Yah, I thnk @laniakea64 is right, matching on strings is most useful, since for bools there's if / else.

To keep the initial feature simple, I would skip bothering to try to bind fallthrough matches to a variable, and require a match arm with _ as the variable, which runs if nothing matches, and which doesn't bind anything:

some_var := match os() {
  "linux" => "foo",
  "macos" => "bar",
  "windows" => "baz",
  _ => "other"
}

Also for simplicity, all match arms should be required to be unique.

A follow-up PR can allow actually matching and binding in the fall-through case.

@laniakea64
Copy link
Contributor

match arm with _ as the variable, which runs if nothing matches, and which doesn't bind anything:

Should the match statement initially land as unstable? If _ in match is initially not bound to a variable, things could get weird where justfile has a variable named _

@t3hmrman
Copy link
Contributor Author

Ah so maybe my original request wasn't clear! I originally was hoping this would be a generic matchstatement -- similar to the kind in Rust proper -- match in general isn't limited to strings, but obviously works on strings as well.

As a response to @laniakea64 :

I'm not sure I share the opinion that everything being a string is a feature of Just (is it? I've never even thought of this, but I guess it is true!) -- but I wonder why limit this to working on strings when it can work on expressions?

Looking at the syntax example and the tests in this PR, looks like this implementation of match statements is only an alias for if statements? 😕

What else would match be? I didn't meant to implement it as an alias for if statements (maybe I did something terribly wrong), but that's what match statements are right, checking a branch expression against the original expression and then executing the one that matches...?

I don't intend to add any fancy stuff like destructuring or anything of course

This implementation appears to be introducing a boolean type, which in just would be weird and out of place:

Uhh I don't think that's what this is doing -- Maybe this is a sign that the comment I put in to describe this PR is incredibly bad -- this introduces a new kind of expression, which is the match expression, that evaluates what it gets against match arms...

It's not like I actually added a thing that does match true -- booleans are just simple expressions that are easy to test with.

As a response to @casey :

Yah, I thnk @laniakea64 is right, matching on strings is most useful, since for bools there's if / else.

So the bool was just the simplest expression to write -- but I do think it's more powerful to bind on all expressions, and I don't think it takes that much more work than matching on strings.

To keep the initial feature simple, I would skip bothering to try to bind fallthrough matches to a variable, and require a match arm with _ as the variable, which runs if nothing matches, and which doesn't bind anything:

Ah so this is actually what the impl looks like -- there is a fallthrough for matches (and it's _):

https://github.com/casey/just/pull/2267/files#diff-592a0b80618b04a780e1d598fc607ad1027faa95d37e5f5c933edb8f1b589536R217

Am I misunderstanding your suggestion maybe?

Also for simplicity, all match arms should be required to be unique.

This is new, and a good suggestion -- will add this!

A follow-up PR can allow actually matching and binding in the fall-through case.

I'm a little bit confused on how these would actually be separated -- maybe the approach I'm taking here is just so off-course compared to the simplest approach that people were envisioning.

How would you start the simple approach? I just assumed it would be adding an Expression variant but maybe that's wrong

@t3hmrman
Copy link
Contributor Author

@laniakea64

Should the match statement initially land as unstable? If _ in match is initially not bound to a variable, things could get weird where justfile has a variable named _

IMO it definitely should regardless!

We can make sure that _ is never evaluated if it's encountered inside a match statement this code here should probably change

@t3hmrman
Copy link
Contributor Author

Also thanks for the feedback ya'll -- I actually have some bugs that I was stuck on so more eyes (on how to actually fix the lexer issues) would be great --- tests don't pass in the PR as is 😞

@laniakea64
Copy link
Contributor

I'm not sure I share the opinion that everything being a string is a feature of Just

Not an opinion, casey has said this many times before about current just, e.g. #1889 (comment)

What else would match be?

Sorry for the unclear wording. Ideally, match statement is a simpler, more concise and more readable way to express a chain of multiple if statements whose conditions all test the same value. i.e. more than an alternative way to write a single if/else statement.

This implementation appears to be introducing a boolean type, which in just would be weird and out of place:

Uhh I don't think that's what this is doing

The true and false in the match arms are boolean tokens. There isn't anything like that in existing justfile syntax.

@laniakea64
Copy link
Contributor

Also thanks for the feedback ya'll -- I actually have some bugs that I was stuck on so more eyes (on how to actually fix the lexer issues) would be great --- tests don't pass in the PR as is 😞

@t3hmrman I investigated this and decided to try the following change, which fixed the lexer test failure, but caused different test failure

diff --git a/src/lexer.rs b/src/lexer.rs
index 853e41f..6520d7b 100644
--- a/src/lexer.rs
+++ b/src/lexer.rs
@@ -486,7 +486,7 @@ impl<'src> Lexer<'src> {
       ',' => self.lex_single(Comma),
       '/' => self.lex_single(Slash),
       ':' => self.lex_colon(),
-      '=' => self.lex_choices('=', &[('=', EqualsEquals), ('~', EqualsTilde)], Equals),
+      '=' => self.lex_choices('=', &[('=', EqualsEquals), ('~', EqualsTilde), ('>', EqualsGreaterThan)], Equals),
       '?' => self.lex_single(QuestionMark),
       '@' => self.lex_single(At),
       '[' => self.lex_delimiter(BracketL),
diff --git a/src/parser.rs b/src/parser.rs
index 5b8df2d..b21accf 100644
--- a/src/parser.rs
+++ b/src/parser.rs
@@ -2154,8 +2154,8 @@ mod tests {
 
   test! {
     name: _match,
-    text: "a := match b == c { true => d, false => e }",
-    tree: (justfile (assignment a (match a == b true d false e))),
+    text: "a := match b { c => d, e => f }",
+    tree: (justfile (assignment a (match b c d e f))),
   }
 
   test! {

The reason for changing the test in src/parser.rs is because it looks to me like your current parsing of match statements is actually closer to a #2267 (comment) style match than the tests in this PR? (Not really sure, I haven't looked at this part of just code in this much detail before.)

@t3hmrman
Copy link
Contributor Author

Thanks @laniakea64 ! I'm going to add that to this PR -- I certainly missed that part of the lexer implementation for lexing things that look like =*

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

Successfully merging this pull request may close these issues.

3 participants