Remove unreachable case branches in case ... of

[lue-bird]

A fresh attempt at implementing #99 (slightly less ambitious), having stopped #103.

• multi-variant
• list literal
• (un)cons
• tuple with 2 parts
• tuple with 3 parts
• single-variant, only if any inner variant value can be simplified

As always, suggested refactors and nitpicks welcome.

Deliberately not included

Allowing all pattern (_) for the last pattern
→ requires exhaustiveness check! (Because otherwise the case removal could lead to a compiler error)
→ out of scope?

This means that for example literal char, string, int/hex patterns are not checked for whether they are unnecessary because they require a _ or variable pattern case as the last case.

If you think we should tackle allowing _ patterns some time in the future, leave #99 open, otherwise this PR closes it.

Discussion

There are situations where no cases are "unnecessary" but parts of the pattern and cased expression are because they're always matched (in all examples, the second part in the tuple is unnecessary):

case ( a, () ) of
    ( Something, () ) -> b
    _ -> c

type Blank
    = Blank
case ( a, Blank ) of
    ( Something, Blank ) -> b
    _ -> c

type Thing
    = Thing Int Int Int
case ( a, Thing i0 i1 i2 ) of
    ( Something, Thing _ _ _ ) -> b
    _ -> c

case ( a, b ) of
    ( Something, _ ) -> c
    _ -> d

Should I open an issue to handle those separately or would you like for them to be included in this PR?
The last 2 could also fit into no-unused I guess?

[jfmengels]

Let's add a test as well to make sure we don't do anything when one of the branches has an alias for the pattern.

[lue-bird]

"alias for the pattern"? Does that mean import alias? something with type alias?
Not sure what you mean. If it's about detecting the real full qualification, the test "should remove multiple unnecessary cases of project-local variant with multiple attachments when all cases are variant patterns" should already cover that.

[jfmengels]

(I think that) I meant a pattern alias, such as ([ _, _ ] as something) -> 4

[jfmengels]

I don't think this change is better, it makes the code quite harder to read (at least for the situation in this test and the previous one).

I think we should limit ourselves to change lists to tuples of the same size:

case [ x ] of
  [ 1 ] -> a
-->
case x of 
  1 -> a

case [ x, y ] of
  [ 1, 2 ]  -> a
  _ -> b
-->
case ( x, y ) of
  ( 1, 2 )  -> a
  _ -> b

case [ x, y, z ] of
  [ 1, 2, 3 ]  -> a
  _ -> b
-->
case ( x, y, z ) of
  ( 1, 2, 3 )  -> a
  _ -> b

and then remove the impossible branches.

That said with my proposal, I believe it is possible to introduce compiler errors where one branch is entirely covered by another without exhaustive checking. I'm not sure whether your approach has the same problem or not. I don't think so?

---

Anyway, I don't think this change in the current form is a straight simplification. It's a simplification plus a transformation that can be controversial.

If it's just about removing unreachable case branches, then I think I would be in favor of removing them directly if there is a _ -> branch without the transformation to a tuple. That's also what the error message recommends.

If there is no wildcard case, then I'm unsure. I imagine replacing the pattern one for one of the impossible branches to be _ -> could be okay. We might need to move it to be the last though. Oh, and it should not introduce any variables to the scope, otherwise that's a compiler error.

If none of the existing branches fit, then I'm unsure what we should do. Maybe not provide a fix?

[lue-bird]

First: Currently, we don't simplify case-ofs with wildcard patterns at all, see the PR body section.
Second: Not converting (the beginning of) a list with known elements to a tuple defeats the whole purpose of the check IMO.
In my eyes, there is no difference between leaving case multi-variant of or case list with some elements of instead of matching on a structure where we know for a fact that the values exist and we don't need these ugly (potentially unnecessary) cases to handle the rest.

If this is only about readability, maybe the new nesting is better?

[jfmengels]

Sorry for the (very) delayed review of this. I made some remarks, but I don't think there are blockers on this so I'll merge this and we can always improve on them in follow-up work.

Thank you a lot for the work 🙏

(PS: I tried this at work, but unfortunately it didn't report anything 😞 )

There are situations where no cases are "unnecessary" but parts of the pattern and cased expression are because they're always matched (in all examples, the second part in the tuple is unnecessary)
Should I open an issue to handle those separately or would you like for them to be included in this PR?

Please open a new issue. I think this would be a great addition.

The last 2 could also fit into no-unused I guess?

Agreed, I also think they could go in both 🤷

[jfmengels]

Is exposedPotentialTypeAlias the right name? This is dealing with custom types, not type aliases, right?
Same for recordTypeAlias a few lines below.

[jfmengels]

For later: This can probably be sped up using a single Dict.fold

[jfmengels]

... and 3rd case matches on a different variant which means you can remove it.

Should be "cases matches" and "you can remove them", and maybe also "on different variants" (which I think we can use as the wording regardless of the number of cases).
(not a blocking issue)

[jfmengels]

(I think that) I meant a pattern alias, such as ([ _, _ ] as something) -> 4

[jfmengels]

We can remove the List.concat here, every branch returns a List with a single fix.

Note that the rule could almost have caught this. We do List.concat (List.map (\x -> [ f x]) list) -> List.concatMap (\x -> [ f x]) list -> List.map (\x -> f x) list if I'm not mistaken. But we don't do any checks for List.concat (List.map2 ... list).