Proposal-pattern-matching: Forbidding unreachable clauses?

[zkat]

Ref: #57
/cc @xtuc

There was some discussion before about the idea of forbidding unreachable clauses. The current proposal does not explicitly disallow this:

match (4) {
  1 => "one",
  _ =>"else",
  3: "unreachable"
}

Since clause 3 is unreachable, there's an argument to be made that this proposal should disallow that altogether. There was some previous discussion about this in #57 but it would be good to re-evaluate this in light of the new spec's semantics (since the else-leg thing has since been resolved, though this can still be relevant).

[zkat]

My own 2c:

I'm not sure there's any precedent for this? switch definitely doesn't care if you put something after default: { ...; break; }. if likewise has no special case for if (false) or other such literals.

I would argue that we should _not_ disallow this, specially because during development, it might be reasonable for people to insert catchalls into different branches in order to try different behaviors, or to sketch out behavior they want to write in the future without having to comment out the rest of the code.

match (x) {
  1 => 'it was a one!',
  _ => throw new Error('we should not be here yet'),
  2 => 'future code',
  x => 'code that broke before'
}

It seems extreme to explicitly disallow this, as I think this is something better left to linters, as there's many ways that a clause might become unreachable, not just fallthrough variables. So, I'm opposed. :)

[xtuc]

While I opened the issue in the first place, I have mixed feelings now:

Disallow

When we parse the clauses we already can know if it's impossible/unreachable, so why wouldn't we tell the user about it. Other languages are doing so (OCaml, etc).

I feel like we are helping the user by doing so.

Allow

Linters/type checkers will catch them (since they are basically made for that), but when you don't use one it will fail silently for ever.

There are many construction that will fail silently for ever that we don't prevent:

if (false) { ILikeTrains() }

const a = true
a = false // well that's not silent

Conclusion

🤷 (I hope that helps)

I will try to attend remotely the next meeting. Also note that it's easier to remove restrictions that adding them after.

[zkat]

Are you ok with completely disallowing users from injecting early escapes into their match statements altogether? Not everyone has a static-ish workflow. I know mine tends to be pretty dynamic, and temporary incorrect code is useful in playground situation.

Additionally, what's the full range of clauses that we should consider "unreachable"? Does this include guards? Does it include all falsy literals or only fallthrough variables? Is there any additional expected static analysis? Do you think this is worth breaking from significant prior precedent as far as other fallthroughs in the language goes?

The only precedent I could think of is that this isn't valid syntax, really:

if (x) {
  ...
} else {
  ...
} else {
  ...
}

I'd have to look at the grammar in more detail, but the "smell test" of trying it in the console tells me this doesn't even parse :) It has nothing to do with actual values, though.

[c-dante]

I feel this is the job of a linter, not the language. Javascript doesn't care about this:

return 10;
if (condition) {
  return 5;
}

So I don't think adding in that complexity is helping anyone. At best, I see this as a warning. The spec would have to include the special case of exactly "a catch-all happens above this pattern".

[ljharb]

I consider it a mistake that switch allows default in any position, and that it can be silently omitted. I think it’s very valuable to bake into the language what would otherwise require a linter - something far too many people don’t even use.

With the latest proposal, an unreachable match clause seems like the primary thing a linter rule would be needed for. I’d very much like to see any statically unreachable clause disallowed, even if we can’t prevent all unreachable clauses.

[c-dante]

Just to clarify, this syntax rule is explicitly the catch-all case can only be at the end, if it exists?

My apprehension is if we have one "unreachable code" error, how many should we have? For example, under the current way of null-punning, this is unreachable:

foo match {
  null => console.log('I match null and undefined'),
  undefined => console.error('unreachable code')
}

[leonardiwagner]

...so why wouldn't we tell the user about it. Other languages are doing so (OCaml, etc).
I feel like we are helping the user by doing so.

@xtuc How would we tell and help user about that? Throwing a runtime error? Because besides an error is not just "telling".. throwing an error for a perfectly runnable code isn't a help at all! 😄

You also gave us the OCaml example to make the argument stronger. It's a compiled language (opposite of Javascript), then I'll assume the mentioned warning is given at compile time, am I wrong? If so, this example isn't relevant to justify a runtime error on Javascript. Besides that, I tried this code on https://try.ocamlpro.com/, and I didn't get any error, runs just as expected:

let f x =
  match x with
     | "foo" -> "it is foo"
     | _     -> "our default: " ^ x
     | "bar" -> "will never get here, and I didnt get any sort of error";;

f "foo";;  (* prints "it is foo" *)
f "bar";;  (* prints "our default: bar" *)

Now, the @ljharb sentence:

I think it’s very valuable to bake into the language what would otherwise require a linter

Where comes the will to "bake into language" and FORCE the user to have a compile-time sort of warning (it's not even an error) in an interpreted language? It's against the reason why we choose to Javascript, and not a compiled language to develop software.

If you want something like a compiler in an interpreted environment, you should use a linter. If many people don't use, it's because is optional, and being optional is the awesome part!

[tabatkins]

The usefulness of being able to temporarily make a branch always match, just so you can test that code, I think overwhelms the usefulness of getting a parse-time error for (statically analyzable) unreachability. Linters have exactly the same amount of power here as the language would, and they don't harm exploratory coding.

So I'm definitely on the "allow unreachable branches" side.

[mAAdhaTTah]

I’d very much like to see any statically unreachable clause disallowed, even if we can’t prevent all unreachable clauses.

This behavior feels somewhat footgun-y. Allowing some unreachable clauses, but not all, to be valid still requires the developer to know the difference. Seems like an extra thing to learn without a lot of benefit.

[ljharb]

Requiring the developer to know the difference less often is still hugely valuable.

[c-dante]

@ljharb Would you consider it to be a syntax error?

If not, there isn't really a place for that error to be thrown, other than at evaluation time, in which case I would strongly argue against having it exception then.

I also agree with @tabatkins that being able to do this:

complexCase match {
  complexFoo => ...,
  complexBar => ...,
  debug => console.debug('We didn't pass?', debug),
  complexFooBar => ...,
}

while developing certainly outweighs accidentally leaving that case in.

[mAAdhaTTah]

It's not really "less often" because instead of it always being valid, they have to learn which unreachable clauses throw SyntaxErrors and which don't. It's the same amount of information. And if you expect to see an error when you have uneachable clauses, and you have a case that is unreachable but doesn't throw, it could produce more confusion.

[zkat]

I reiterate that I dislike the idea of having this break from precedent, and especially when it involves debugging-related situations -- debugging misbehaving clauses can already be quite a pain. I think disallowing fallthroughs altogether would make the situation even worse.

[zkat]

This proposal does not include any mention of forbidding unreachable clauses, and, as mentioned before, there is literally no precedent for this in the language, for any of its literals. I think this is much better off as the job of a linter and, should a host decide to detect this for its users, it's best left as a warning. So I'm just gonna close this for now.

[ljharb]

There’s also no precedent for this proposal, or a MatchError; i think this should remain open until the committee has discussed it.

[zkat]

It's true, there's no precedent for that one either in js.

If you'd like, make a bikeshed PR about errors and we can explore in more detail later. I won't be reopening this PR