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

Support parentheses around match cases #12387

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from
Open

Conversation

lpw25
Copy link
Contributor

@lpw25 lpw25 commented Jul 18, 2023

This PR adds support for the following syntax:

match e1 with (
| p1 -> e2
| p2 -> e3
)

The aim is to support nested pattern matches:

let foo x =
  match x with
  | Some y ->
      match y with (
      | true -> ()
      | false -> ()
      )
  | None -> ()

Personally, I would be much happier writing this than writing:

let foo x =
  match x with
  | Some y ->
      (match y with
      | true -> ()
      | false -> ())
  | None -> ()

or:

let foo x =
  match x with
  | Some y -> begin
      match y with
      | true -> ()
      | false -> ()
    end
  | None -> ()

Rather than make any large claims about the superiority of the new syntax, I would like to argue that the cost of supporting it is tiny. The implementation is four lines of code and there isn't really new syntax to learn because it is just parentheses behaving like parentheses do everywhere else.

This syntax would also make it easier to support multi-case pattern guards as proposed in this RFC, which is what got me thinking of this change.

There was a similar proposal using square brackets a while back, also partly motivated by multi-case pattern guards. I'm hoping that parentheses are less controversial.

match_cases:
xs = preceded_or_separated_nonempty_llist(BAR, match_case) %prec below_BAR
{ xs }
| LPAREN xs = match_cases RPAREN
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd drop support for nested delimiters, which doesn't seem to add anything useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put them in for consistency with other places you can use parentheses, I agree that they are not useful and would be fine with removing them.

Copy link
Member

@yallop yallop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an obvious improvement, it's backwards compatible, and it'll help to support further extensions to pattern matching later on. I don't see any drawbacks to merging it.

@MisterDA
Copy link
Contributor

Should this be documented somewhere? Or shown as an example?
Would it make sense to support parentheses around cases that follow the function keyword?

@gasche
Copy link
Member

gasche commented Jul 18, 2023

Should this be documented somewhere? Or shown as an example?

Indeed, @lpw25 should at least update the BNF:

https://v2.ocaml.org/manual/expr.html#pattern-matching

pattern-matching:
[ '|' ] pattern ['when' expr] '->' expr
{ '|' pattern ['when' expr] '->' expr }
;

Would it make sense to support parentheses around cases that follow the function keyword?

Yes, this is supported by the PR as-is.

@xavierleroy
Copy link
Contributor

Too much syntactic sugar is bad for your teeth. I need convincing that this third syntax for something that can already be expressed in two different ways ((match ...) and begin match ... end) is actually beneficial, esp. to newcomers.

For instance, so far, parentheses are something you can wrap around expressions: core language expressions, type expressions, module expressions, etc. Does this new syntax imply that pat1 -> expr1 | ... | patN -> exprN is some kind of expression? Because it is not...

@gasche
Copy link
Member

gasche commented Jul 19, 2023

I agree with @lpw25 and @yallop : I think that this has very little cost and would make various things nicer.

Does this new syntax imply that pat1 -> expr1 | ... | patN -> exprN is some kind of expression?

I think of these program fragments as belonging to a grammar of handlers, and it makes sense to me to allow parentheses to nest handlers.

Strictly today for disambiguation my favorite form is begin match ... end, with begin match on the same line. But I think that having parentheses for handlers makes a lot of sense.

On the other hand, a question is looming over this proposal: wouldn't people expect support for the M.(...) syntax in handlers? Do we want to put the work to implement this?

@lpw25
Copy link
Contributor Author

lpw25 commented Jul 20, 2023

Indeed, @lpw25 should at least update the BNF:

This is now done.

wouldn't people expect support for the M.(...) syntax in handlers?

Personally, I don't think we should consider it a rule of the language that everywhere you can write parentheses you can use local open. I don't think that users should have this expectation, and I'd prefer not to support it here. If someone wants to propose it in a later PR then they can, but let's not get distracted by it in this PR.

I need convincing that this third syntax for something that can already be expressed in two different ways ((match ...) and begin match ... end) is actually beneficial, esp. to newcomers

The downside of the current syntax is that they put the delimiter at the start of the line/statement ahead of match. This goes against the general rule that the start of the line/statement is the most valuable real estate and you want the most important thing to go there. This is one reason why most curly-brace languages put the delimiters after the construct rather than before. The notable exception from this general approach are the lisps, but I think it would be fair to describe their choice here as controversial and not generally considered easy to read for newcomers.

Putting the delimiter at the front also gives nested matches a different alignment from unnested matches. The | stops lining up with the match, which I at least find visually distracting.

In practice, what I observe from users now, including sometimes myself, is that they dislike the current choices enough that they will restructure their code to avoid them. For instance, changing:

match x with
| Some y ->
   (match y with
   | Foo -> ...
   | Bar -> ...)
| None ->
   ...

to:

match x with
| None ->
   ...
| Some y ->
   match y with
   | Foo -> ...
   | Bar -> ...

or replacing:

foo ();
(match bar with
| None -> do_bar_none ()
| Some x -> do_bar_some x);
baz ();

with:

foo ();
let () = 
  match bar with
  | None -> do_bar_none ()
  | Some x -> do_bar_some x
in
baz ();

I suspect that users may not perform such rewrites if they have the proposed syntax available.

As I said, I don't think the benefits here are large, it's just a small improvement. But weighed against the genuinely tiny cost, it seems like a good trade.

@alainfrisch
Copy link
Contributor

Nothing against the proposal, but I just wanted to share a personal style-rule that matching delimiters should always be vertically or horizontally aligned (i.e. either on the same line or the same column). This makes it easier (I believe) to visually match the delimiters; the eyes only need two 1d scan, instead of a full 2d scan.

This rule would for instance reject:

let foo x =
  match x with
  | Some y ->
      (match y with
      | true -> ()
      | false -> ())
  | None -> ()

in favor of:

let foo x =
  match x with
  | Some y ->
      (match y with
      | true -> ()
      | false -> ()
      )
  | None -> ()

With this rule, the proposed solution would give:

match e1 with 
(
| p1 -> e2
| p2 -> e3
)

(which "wastes" one line)
or:

match e1 with 
( p1 -> e2
| p2 -> e3
)

(which makes the code less regular -- for instance swapping the two cases requires more operations than just moving lines)

So I'd probably personally not use the new syntax. FWIW, my preferred style has been for a long time:

begin match ... with
| ...
| ...
end

with "being match" being an almost-keyword for me (and I might even use these form even when parentheses are not required).

Btw, I apply the same vertical-or-horizontal alignment to let ... in ..., which allows:

let x = ... in ...
let x =
  ...
in
...

but rejects:

let x =
  .... in
...

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

Successfully merging this pull request may close these issues.

None yet

6 participants