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

The "no LineTerminator here" restriction before the brace in match is inconsistent with existing control flow operators #68

Closed
dead-claudia opened this issue Mar 26, 2018 · 56 comments

Comments

@dead-claudia
Copy link

All existing control flow operators allow a line break before their block bodies, including with switch, and there's even an ESLint rule that can enforce it as a preference.

// Example taken from there
if (foo)
{
  bar();
}
else
{
  baz();
}

This proposal blocks it for clear ASI concerns*, but for some teams, this could prove to be a problem (not all JS users use 1TBS or Stroustrup style**), and I don't believe we should be enforcing this opinion now after not having enforced it previously.

I have previously proposed (in multiple places, although with no specific bug) that we use case instead of match, although @ljharb has voiced some opposition in one of them.


* In a potentially statement-terminating context, match could end up interpreted as a call rather than an expression when a newline is present.

** It's not uncommon among C# or Windows devs who happen to write JS on occasion, since Allman style is the default VS style for C/C++/C# there. (I've also seen it a few times with Java.)

@dead-claudia dead-claudia changed the title The "no LineTerminator here" within match is inconsistent with existing control flow operators The "no LineTerminator here" restriction before the brace in match is inconsistent with existing control flow operators Mar 26, 2018
@ljharb
Copy link
Member

ljharb commented Mar 26, 2018

Essentially it boils down to, either we use an existing reserved word, or we have a NLT restriction. Existing words that have other meanings - including case - are likely to garner objections because of various forms of confusion, and the unused reserved words are unlikely to be applicable.

I don’t think there’s any other option here besides NLT - while it’s unfortunate if JS forces a specific style, a) NLT has other precedent in the language, and b) the style that’s unintentionally forced is, at least, widely preferred in the community (which isn’t a justification but certainly mitigates the turbulence that might otherwise be caused).

@dead-claudia
Copy link
Author

and b) the style that’s unintentionally forced is, at least, widely preferred in the community (which isn’t a justification but certainly mitigates the turbulence that might otherwise be caused).

That is true, and 1TBS/Stroustrup style is so prevalent tools have been known to forget about Allman style on occasion when handling JS code. Also, we do kind of force it some with return, due to ASI rules (if that counts as precedent). So I'm not 100% against leaving it this way.

I filed this more as a thing that should be addressed explicitly, so we are at least aware of the potential confusion. (Syntax errors from erroneously using Allman style are going to be misleading unless engines/parsers specifically check for match(expr) preceding block statements and rethrow with the misplaced brace's line number, so they should know that this may require such a check.)

@zkat
Copy link
Collaborator

zkat commented Mar 26, 2018

@isiahmeadows I'm honestly not sure how appropriate using if like this is, but I'm comfortable considering this a good use-case for piggybacking on that keyword. I believe that solves this without any weird NoLineTerminator stuff. That means the following are all valid and unambiguous now:

if match (foo)
{
...
}

if match
(foo) {
...
}

if
match (foo)
{ ... }

And it has the benefit that it doesn't need a cover grammar at all. I considered using case, but I still feel like using match as a word is important here, and case, while more terse, might be too weird to use to mark the beginning of an expression, rather than an item inside a switch. And, again, my desire to distance this from switch.

@tabatkins
Copy link
Collaborator

but I still feel like using match as a word is important here,

Can you explain why that is? match isn't a universal name for this sort of expression, I don't think - case or cond appear in some languages.

@ljharb
Copy link
Member

ljharb commented Mar 26, 2018

case is something i will fight tooth and nail to prevent; i want no similarity to switch statements. Other words besides match and case - ideally non-abbreviated, for learning’s sake - I’m totally open to, but i have no suggestions in mind.

@tabatkins
Copy link
Collaborator

I mean case as the actual block name, like:

let x = case(foo) {
  {y} => 1;
};

That bothers you too? (I can understand not wanting a case prefix for each clause, to avoid switch similarities.)

@ljharb
Copy link
Member

ljharb commented Mar 26, 2018

That bothers me less, but bothers me still - i want no overlap with switch in any way, so i can continue to teach that switch statements are abomination (and hopefully teach that this proposal is the solution)

@dead-claudia
Copy link
Author

dead-claudia commented Mar 27, 2018

@ljharb BTW, I meant case as in what @tabatkins was talking about, and was basically suggesting that very syntax change (s/match/case/g). (Just dropping by to clarify, in case it wasn't clear from context.)

@zkat
Copy link
Collaborator

zkat commented Mar 27, 2018

I can't believe I didn't remember this, but...

Both Erlang and Elixir, two of the most prominent languages that use pattern matching, use case as the keyword for the analogous construct.

Combine this with the fact that using that keyword would solve pretty much all grammar-related issues... I think I'm on board with renaming this to case

@zkat
Copy link
Collaborator

zkat commented Mar 27, 2018

And yes, as the name for what we now call match, not as clauses.

zkat added a commit that referenced this issue Mar 27, 2018
Fixes: #68

This renaming removes all outstanding grammar-related concerns
and greatly simplifies things. It also has precedent: Elixir and
Erlang are the two most prominent dynamic languages with pattern
matching support, and are both languages whose semantics this
proposal draws from. It seems perfectly appropriate to use their
name for this construct rather than stick to `match`.
zkat added a commit that referenced this issue Mar 27, 2018
Fixes: #68

This renaming removes all outstanding grammar-related concerns
and greatly simplifies things. It also has precedent: Elixir and
Erlang are the two most prominent dynamic languages with pattern
matching support, and are both languages whose semantics this
proposal draws from. It seems perfectly appropriate to use their
name for this construct rather than stick to `match`.
@zenparsing
Copy link
Member

zenparsing commented Mar 27, 2018

I agree with the general sentiment that we should avoid cover grammars and excessive line terminator rules if possible.

@ljharb If this construct effectively replaces and eliminates the need for the switch statement, why not just reuse switch? The first token after { would disambiguate.

@zkat
Copy link
Collaborator

zkat commented Mar 27, 2018

@zenparsing it's one thing to disambiguate a grammar, and another to disambiguate it when teaching people significantly new concepts.

@ljharb
Copy link
Member

ljharb commented Mar 27, 2018

@zenparsing since switch will always exist, teaching the difference is most easy when the visual/syntactic difference is widest (while still intuitive for the new thing, ofc).

@dead-claudia
Copy link
Author

dead-claudia commented Mar 28, 2018

@zenparsing Reusing switch for both would need more than that to disambiguate. Consider this as a statement:

switch (expr) { }

Currently, this doesn't throw. But the current match (expr) { } does. If you were to require at least one case (which would make sense either way - what's the point of a zero-case match?), that would fix this ambiguity.

@zenparsing
Copy link
Member

@isiahmeadows Good point.

One valid approach to introducing a new pattern matching control flow construct in JS is to create something entirely new (e.g. a new expression form, with a new keyword, instead of a statement). Another valid approach would be to take the existing control flow construct and improve it (e.g. by introducing a new form of switch statement, disambiguated by the token after {).

If one were going to use the second approach, I think allowing

switch (expr) {}

to continue to be a no-op would be reasonable.

The broader point that a new form of switch statement would be confusing remains to be seen, I think.

@dead-claudia
Copy link
Author

@zenparsing I personally agree with @ljharb that we do need some level of distinction. I could see a variety of points of confusion among people learning the syntax (especially newer people to JS):

  1. Why can't we mix/match the different case syntaxes?
  2. Why does one version use case and the other doesn't?
  3. Why does one version allow assignment and the other doesn't?
  4. Why does one version require break and the other doesn't?
  5. Why does one require => before and a comma after, while the other just requires a : before?
  6. People are invariably going to try (and fail) to use case {foo: bar}: for pattern matching or get confused and try case 1 => for "normal" cases in expression contexts, forgetting that the expression syntax is pattern =>, the statement syntax is case expr:, and that you can't mix the two.

These are obvious to us designing the feature, but not necessarily to those who simply using it. At least using a different keyword (any keyword) to kick off the brain to visually parse it differently from a procedural C-style switch will keep people from making that mistake.

@zenparsing
Copy link
Member

At least using a different keyword (any keyword) to kick off the brain to visually parse it differently from a procedural C-style switch will keep people from making that mistake.

Sure, but it's not free: we have to pay with cover grammars and newline restrictions if we use a contextual keyword, or mental tax if we overload an existing keyword.

In any case, I'm not particularly arguing for switch at this point; just testing the idea!

@arcanis
Copy link

arcanis commented Mar 28, 2018

since switch will always exist, teaching the difference is most easy when the visual/syntactic difference is widest (while still intuitive for the new thing, ofc).

Piggybacking a bit on #77, if this proposal was to use switch, the case keyword could be replaced by match to make the difference more visible.

const val = switch (res) {
  match {status: 200, headers: {'Content-Length': s}} => `size is ${s}`,
  match {status: 404} => 'JSON not found',
  match {status} if (status >= 400) => throw new RequestError(res)
};

@rattrayalex
Copy link
Contributor

What about repurposing with? It's illegal in strict mode (thus presumably free for use in strict mode), but with is still a reserved keyword (eg; function with () {} throws a SyntaxError.

eg;

const val = with (res) {
  {status: 200, headers: {'Content-Length': s}} => `size is ${s}`,
  {status: 404} => 'JSON not found',
  {status} if (status >= 400) => throw new RequestError(res)
}

Of course, this would be super confusing to explain, eg "JavaScript has a new pattern matching feature called 'with'! Oh and and by the way there was an old feature called with that you've probably never heard of and should never use".

Personally I'd much prefer match (x) {} with an odd linebreak limitation or match x {}, but thought I should throw this out there anyway.

@ljharb
Copy link
Member

ljharb commented Apr 15, 2018

I'd be pretty against repurposing anything; I'm not interested in making it harder to learn JavaScript than it already is.

@borela
Copy link

borela commented Apr 15, 2018

@ljharb is it possible to add a new match keyword and require something like "use strict v2";?

@ljharb
Copy link
Member

ljharb commented Apr 15, 2018

@borela no, more modes aren’t an option (desirable or achievable).

@thysultan
Copy link

One alternative is to require a prefix keyword that would allow parsers to disambiguate it with a look-ahead. For example, the case keyword cannot appear in the following context.

function match () {}

match(...)
{
case ...
}

This would mean that a look ahead for the first keyword case would disambiguate the match from the above, this works especially well since match requires at-least one case.

const val = match (res) {
  case {status: 200} =>
  	`size is ${s}`
  case {status: 404} =>
  	'JSON not found'
}

That said the => syntax is somewhat of a confusing point/restriction, 1. because it assumes the above is invalid because of the newline after =>, 2. It looks confusingly like arrow function but diverges from the semantics of arrow functions and always assumes an implicit return instead of an explicit return that unlike the former(implicit) an explicit return also allows you to exit early.

For example consider the following, one might assume val would be undefined instead of the length of the array.

const val = match (input) {
  {x} => {
    array.push(x)
  }
}

Using an explicit return might look like:

const val = match (res) {
  case {status: 200}:
  	return `size is ${s}`
  case {status: 404}:
  	return 'JSON not found'
  case {status}:
  	if (status >= 400)
  		return new RequestError(res)
}

and avoids the afore-mentioned issues.

@dead-claudia
Copy link
Author

@thysultan

This is based on the impression that at least one case is required, though that may yet prove to be a bad idea. How would one archive a default clause in its current form?

It's pretty simple:

match (foo) {
    bar => ..., // `bar` unconditionally captures the argument
}

There's been talks elsewhere before (I forget where) about adding an else, but it's not completely necessary given the above.

What does this mean? Would it restrict the following:

match (window.foo = 1, {...something}) {}

No, I'm referring to this (note the top-level rest parameter):

match (window.foo = 1, ...otherArgs)
{
    // ...
}

@borela
Copy link

borela commented Apr 16, 2018

@isiahmeadows
Could we upgrade the switch statement into an expression as suggested here tc39/proposal-do-expressions#11? It won't need new keywords, just an extension of its functionality to allow the pattern matching.

@dead-claudia
Copy link
Author

@borela #68 (comment)

@dead-claudia
Copy link
Author

Also, my explanation was that we could upgrade match to a contextual keyword with some grammatical hacking to avoid the need for the concern I brought up in this issue when filing it.

@zkat
Copy link
Collaborator

zkat commented Apr 18, 2018

I'm totally onboard with super switch (...) {}. It could be the new smoosh()!

@borela
Copy link

borela commented Apr 19, 2018

Personally I don't understand the hate on switch statements, the reason that it is difficult to teach makes no sense, I learned it when I was 13 and english is not even my mothertongue.

@dead-claudia
Copy link
Author

@borela The term match comes from pattern matching. switch is like a railway switch, where you basically say "take me to this route", given a set of named routes. match is subtly different: you're going "here's what I have, match this against your routes and take me to the first one it matches". It's fuzzier than just "here's the route I want to take, take me to it", but it's similar to the difference between string === equality and regexp matching, where your operand is the string, and your pattern is the regexp.

This is why match is preferred over switch for naming this - it just makes more sense. It occupies a bit of a middle ground between the flexible-yet-verbose if/else and the highly rigid switch, with on one end, F#'s highly flexible, Turing-complete "active patterns" and Scala's magic apply/unapply methods, and on the other, Swift's almost-C-style switch statement (up to and including it supporting explicit fallthrough and early breaking) and C#'s recent switch updates that bring it barely within the realm of pattern matching (this appears to be only part of what's coming, though.

@borela
Copy link

borela commented Apr 19, 2018

@isiahmeadows That makes sense. Thank you.

@thysultan
Copy link

Brain dump of possible varients:

match (value) for {
	case "1" => expression
	case "2" case "3" => expression
}

// easiest on the tongue.
match (value) with {
	case "1" => expression
	case "2" case "3" => expression
}

// explicitly implies an expression is evaluated.
match (value) return {
	case "1" => expression
	case "2" case "3" => expression
}

@dead-claudia
Copy link
Author

@thysultan Not a fan of the extra keywords here... (BTW, I did come up with a way to require very minimal edits to the current proposal, so I'm no longer convinced we must do anything drastic.)

@thysultan
Copy link

thysultan commented Apr 20, 2018

I feel uncomfortable with both the current heuristics that prevents a line break before { and the heuristics that prevent you from using an empty block, i feel like this introduces a new dynamic to the design of the languages control flow semantics that deviates from the symmetry of present control flow operators.

That is to say – syntax that breaks away from a design that allows all of the following to be considered valid syntax.

<if|else|switch|while> (expression) {

}
<if|else|switch|while> (expression)
{

}

Even as an edge case, the fact that the following could break existing/future code also doesn't sit well.

match (res)
{
  foo => bar
}

But at least that could be fixed by the use of a prefix keyword, for example –

match (res)
{
  case foo => bar
}

I think the other mentioned issues would contribute to more "WTF" moments when learning, using and teaching JavaScript.

@dead-claudia
Copy link
Author

@thysultan

Even as an edge case, the fact that the following could break existing/future code also doesn't sit well.

match (res)
{
  foo => bar
}

Wouldn't be the first. ES6 did it with a less obscure case that could've been useful, in both strict and sloppy mode:

var let = {}
var foo = "foo"

function store(value) {
    // ES5: This assigns `value` to `let["foo"]` and returns `"foo"`
    // ES6: This assigns `value[0]` to `foo` and returns it
    let [foo] = value
    return foo
}

This one would qualify as less breaking, since the use cases are incredibly obscure. (x => { return y }(1) isn't even a valid expression, much less a statement, and that's about the only thing that could potentially be useful without at least parentheses to disambiguate.)

@zkat
Copy link
Collaborator

zkat commented May 3, 2018

This is the longest issue thread in this entire repo.

I would like to propose going back to @isiahmeadows's proposal of using case for this. I believe it fixes all the most important concerns around syntax right now, and is an acceptable name for this sort of construct:

Pros:

  • It removes the need for overly-clever syntax backflips.
    • no need for noLineTerminator thing
    • no need for special keywords or syntax for every internal clause
    • gives full flexibility for internal matcher syntax going forward
  • It is the closest s/x/y/ replacement for the current syntax (so we can preserve all the other current work).
  • It has significant precedent in existing languages. Particularly, the two most prominent dynamic languages that have this construct (Erlang and Elixir).
  • It uses a reserved token in a backwards-compatible format and so is guaranteed to not break the web.
  • It saves a whole character (case vs match). 😏

Cons:

  • Reminiscent of case clauses inside switch, but not easy to actually confuse in practice.
  • Potential documentation search issues from the overlap. (unclear about actual impact of this once case is released and matures).
  • match as the word "match" in it, and it's a pattern matching thing.

I would appreciate it if y'all would consider this again, as I still think it's the most elegant solution to a bikeshed that continues to get longer.

@dantman
Copy link

dantman commented May 3, 2018

A case..match statement would have the same benefits as case but without the cons of case on its own not being suggestive of pattern matching. And would leave room for future extensions to the language.

#91

@loganfsmyth
Copy link

Not necessarily a blocker, but I believe using case would require special-casing to either explicitly disallow it inside existing case clauses, or potentially a cover grammar if that would work. e.g.

switch (true) {
  case true:
    case (value) {
      pattern => {}
    }
}

Since we're inside a switch, the case keyword could either be the start of a pattern match, or the start of another clause in the switch statement. The : makes it specifically one or the other, but you can't know which is which up front.

@dantman
Copy link

dantman commented May 3, 2018

For those as confused as I was.switch(true) {case (true): } is valid syntax because case tests can be expressions that evaluate to the value for case to test. So you can't differentiate between case true: and case(true) {} based on the parenthesis. And while you know that the first case in a switch is definitely a SwitchCase the second case (false) may be either another SwitchCase or may be a case statement in the body of the first case.

I would submit that if we don't want the backtracking that parsing until the : or { requires it may be acceptable to disallow case statements used directly in the consequent of a SwitchCase (i.e. inside switch (true) { case true: ... }). Because on the off chance that someone actually needs a case statement inside of a switch it is possible to wrap it in a block statement to remove the ambiguity.

switch (true) {
  case true:
    {
      case (value) {
        pattern => {}
      }
    }
}

@zenparsing
Copy link
Member

We should try to maintain a very high bar for introducing both cover grammars and one-off grammar restrictions.

@zkat
Copy link
Collaborator

zkat commented May 3, 2018

@loganfsmyth it's pretty unambiguous by looking one token past the (). Since you just need to spot the : (parens are allowed in switch cases).

@arcanis
Copy link

arcanis commented May 3, 2018

Slightly extending my suggestion in 376844097, here is something possibly silly but still possible:

const val = switch * (res) {
  match {status: 200, headers: {'Content-Length': s}}
    => `size is ${s}`,
  match {status: 404}
    => 'JSON not found',
  match {status} if (status >= 400)
    => throw new RequestError(res)
};

The * symbol has already been used to differentiate generators from regular functions, so it's not entirely unknown. This structure works:

  • Even if the switch* statement is used as a statement
  • Even if the switch* statement is used as an expression
  • Even if the switch* statement is empty (which I think is important for generated code)

Afaik this doesn't generate any ambiguity, since until now * could never follow switch.

Using the match keyword (instead of case) would make super-explicit that this is not a classic switch, which is a concern that I saw mentioned a few times.

@ljharb
Copy link
Member

ljharb commented May 4, 2018

I continue to feel very very strongly that the word "switch" and "case" must be avoided, and I don't see the NLT requirement as problematic. Additionally, the NLT requirement is something that a number of existing proposals will need in order to be viable, so citing that as a reason to change this proposal will set an unfortunate precedent that I think is best avoided.

@dead-claudia
Copy link
Author

@ljharb What do you think of adding these two restrictions to avoid the NLT requirement?

  • Require match statements to include at least one case.
  • Ban the sequence [ BindingIdentifier => ] within BlockStatements.

I don't believe this is too much to do, since:

  1. It addresses the issue of the NLT restriction without mandating an opinion.
  2. Who exactly is going to use an empty match expression? In that case, you could just as easily convert match (foo()) {} to void foo(), and that's not hard for code generators to adapt to (about the only use case for zero-case match expressions).
  3. Who's going to use top-level arrow functions within statements? Except in rare eval cases the user isn't even likely to ever run into, it's something that's trivially and consistently discarded.

Additionally, the NLT requirement is something that a number of existing proposals will need in order to be viable

Could you give examples of these? I'm not quite familiar with any others that have had NLT-specific issues. (I'm familiar with quite a few with ASI issues, but that's about it.)

@bathos
Copy link

bathos commented May 10, 2018

@isiahmeadows Regarding item 3, that’s true today, but it would stop being true if do expressions happen.

@ljharb
Copy link
Member

ljharb commented May 10, 2018

@dead-claudia
Copy link
Author

Okay. (And I should've known at least the block params one - I've actually discussed it some with the proposal's author 🙂)

@dead-claudia
Copy link
Author

@ljharb So in light of those, I'm not super against closing this (but I'd like feedback on if it still needs to remain open).

@thysultan
Copy link

@ljharb I can't see how the protocols and extended-numeric-literals proposals will need NLT?

That said seeing as this is intended as a new control flow operator in the same space as if, switch, while, etc, don't you think these all serve as precedence *against* NTL?

@ljharb
Copy link
Member

ljharb commented May 10, 2018

@thysultan protocol { vs protocol \n {, same as match { / 1px vs 1 \n px. and the existing “precedent” would effectively preclude any new forms from being added (that weren’t piggybacking on the existing ones), so i don’t think it’s a useful or important precedent to follow.

@zkat
Copy link
Collaborator

zkat commented May 25, 2018

I'm gonna consider this specific problem resolved. Most of the reaction in-committee seemed to be that case was an acceptable solution to the NLTH problem, and so I've moved the proposal to be that by default, so we can work from there.

Because this issue has become so long and hard to parse, I'm going to close this. If anyone has specific proposals for alternative names that DO NOT USE NLTH, then please open an issue for that specific suggestion. NLTH was deemed pretty much a non-starter by a few committee members and I think it's safe to say we won't be able to ship this proposal if that stays, period.

@zkat zkat closed this as completed May 25, 2018
@tc39 tc39 locked as resolved and limited conversation to collaborators May 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests