[Proposal] More compiler warnings for impossible pattern matches (map/struct)

[Tobias Pfeiffer]

Hello everyone,

as usual thanks for all your outstanding work on Elixir & everything else.

Courtesy of a reddit discussion [1] I'd like to propose implementing some more compiler warnings for matches that are impossible.

## Background

Elixir is very programmer friendly and issues compiler warnings in many cases if something is impossible. So if we match just against `variable` in a function head and further down match against a more specific value we get a warning.

Similarly if we match against `__struct__` and the actual struct we get a warning:

def struct_match(%{__struct__: Range}), do: "__struct__"

def struct_match(%Range{}), do: "struct"

This one warns as:

    warning: this clause cannot match because a previous clause at line 17 always matches
    │
 18 │   def struct_match(%Range{}), do: "struct"
    │   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    │
    └─ lib/compiler_warnings_impossible_matches.ex:18

This is great! The idea here is to take it further.

## Proposal

The code examples used here can be found here: https://github.com/PragTob/elixir_playground/blob/main/lib/compiler_warnings_impossible_matches.ex

I tried the examples on 1.18.0-dev (ed67d6b) with OTP 27.0 to make sure none of them emit a warning right now.

In short, it's more warnings where for some of which I thought they'd already warn you about impossible matches but found out they don't. I think these warnings would be helpful to avoid bugs & help newcomers.

### Simple map match vs. Struct

def map_match(%{}), do: "map"

def map_match(%Range{}), do: "range"

Ideally this should warn as the match is impossible. Possibly also when using the `is_map` guard.

### Map matching on struct specific keys

def key_match(%{first: _first}), do: "map"

def key_match(%Range{}), do: "range"

If we had a map where all keys overlap with a struct matched further down in the function headers, ideally this should also warn. Of course, if we add a second key to the match that the struct doesn't have it should not warn.

Of this proposal, I think this is the one that I see causing bugs most easily.

### atoms, nil and booleans

def atom_match(value) when is_atom(value), do: "atom"

def atom_match(nil), do: "nil"

def atom_match(value) when is_boolean(value), do: "bool"

I'm not sure how much guards should be taken into account with these kinds of warnings, but these are also "unmatchable".

## Implementation

I'm aware there is a chance you're aware of this and it wasn't implemented for compilation performance considerations. If so, sorry and happy to learn how I could look something like this up.

If you agree that this could be worthwhile to implement, I'd be happy to give the implementation a shot myself but I'd at least need basic guidance as I don't know the internals all that well :)

Thanks y'all and have a splendid day!

[1] https://www.reddit.com/r/elixir/comments/1cibtia/comment/l2ckqjr/

[José Valim]

Hi Tobias!

This feature would indeed by very useful however:

1. Implementing overlapping warnings can be quite complex. Those examples are trivial but a correct, complete, and precise implementation needs to take into account patterns, nesting, guards, equality, comparisons,. structs, and more

2. Today's warnings are emitted by the Erlang compiler for the most obvious cases

If we want to fully tackle the problem, one approach is a type system that understands the semantics of patterns and guards. That's what we have been working on. But I also want to be clear that this is months of work, if you want them to be correct and complete.

That said, I think the first warning would be valuable to propose to Erlang, since both languages share the issue that an empty map matches all maps. Erlang does not guarantee to find overlaps but this may be cheap enough to perform. The Erlang/OTP team would know best, so I suggest proposing those there, if nobody has done that yet.

--
You received this message because you are subscribed to the Google Groups "elixir-lang-core" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-co...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/CAG3Z5YTVO1Wrvx76fg%2BBEmqC3PgZsRG0rEpLwVt7fdOWFZ8n2w%40mail.gmail.com.

[Tobias Pfeiffer]

Heyo,

thanks for the quick reply (as always!) José!

Completely understood and known. My thinking was, that we could start with easy cases and build up to the more complex cases iteratively building this up (i.e. first not care about guards & more). I hadn't thought about this in the realms of the type system, but of course that makes sense. Probably best to leave it to all of your type system work then :)

I'll propose the map case to the erlang team then.

Thanks for the guidance!

Tobi

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/CAGnRm4Lq6%2BaHqZwY%2BhvcqtuhSinfUKBWL%3D9U0shByqu0rGTgVw%40mail.gmail.com.

[Jason Axelson]

I quite like the idea of more compilation-warnings for impossible pattern matches! However, it does seem like a lot of work will be needed to get them reliable and performant enough to get there and I don't have any insight to add there.

But I do want to point out that the "Map matching on struct specific keys" isn't an impossible match since not all structs are well-formed, e.g. the "range" function head can be hit like so:

```

iex(1)> defmodule MapVsStruct do
...(1)>   def key_match(%{first: _first}), do: "map"
...(1)>   def key_match(%Range{}), do: "range"
...(1)> end
{:module, MapVsStruct,
 <<70, 79, 82, 49, 0, 0, 5, 188, 66, 69, 65, 77, 65, 116, 85, 56, 0, 0, 0, 209,
   0, 0, 0, 20, 18, 69, 108, 105, 120, 105, 114, 46, 77, 97, 112, 86, 115, 83,
   116, 114, 117, 99, 116, 8, 95, 95, 105, ...>>, {:key_match, 1}}
iex(2)> MapVsStruct.key_match(%{__struct__: Range})
"range"

```

So given that I'm not sure we'd want a compilation warning for that pattern match, so perhaps that warning would make more sense in a linter like Credo.

-Jason

--

[Tobias Pfeiffer]

To make it full circle, I've filed a feature request with erlang: https://github.com/erlang/otp/issues/8558

@Jason: Good point - thanks, I had forgotten that these are a thing. If elixir was to consider this, I'd still vote for a warning though. Like, yes technically it can match. Practically, I don't think these ill-formed structs are something that is commonly used or the default that's expected. And so, possibly degrading DX for them isn't great imo. It's also just  a warning, and I think using them in this context is warning worthy :) And if this was the behavior one wanted, one could just match against the :__struct__ key. I also think that with the full complexity of this, including it in something like credo is too much to ask - this is very much a compiler/type checker concern as you need knowledge of the struct keys etc.

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/CAHMz6kwcOeu3aVrvbGQPN3t1DznfXwguMWwwEH8O7hgnU06O%3DA%40mail.gmail.com.

[Tobias Pfeiffer]

Update: The OTP team implemented it! Should be coming to an erlang version near you soon. And I hope/think that elixir should automatically pick it up (@jose ?) https://github.com/erlang/otp/pull/8600

[José Valim]

Correct. It should just work!

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/CAG3Z5YS%2BOuMTorNWDA8H%3DO5p7z2%2BGWnyTL%3DOgW0uHxXcGsdTGw%40mail.gmail.com.