Fix for map pattern "doesn't have type any()" warning

[erszcz]

Fix for #387.

[erszcz]

Ok, I think it's good enough for now - at least it fixes #387. Constraints could be improved according to the FIXME comment, but with no constraint solver it's not testable anyway, so will likely require some attention in the future.

[erszcz]

@zuiderkwast what do you think?

[zuiderkwast]

👍 I didn't take my time to fully understand it, but it looks like how we handle lists and other stuff.

[zuiderkwast]

I think we need a catch-all cause for non-map types here. Before, those types fell through in add_type_pat to throw({type_error, pattern, element(2, Pat), Pat, Ty}) but now add_type_pat catches all map patterns and calls expect_map_type instead.

Maybe we should have a test case for something like this? (IIRC how to reach add_type_pat)

f() ->
    G = g(),
    h(G).

-spec g() -> {tup, le}.
g() -> {tup, le}.

h(#{k := v} = _Map) ->
    ok.

[erszcz]

Good point, thanks 👍 I don't return {type_error, ...} and all the other expect_*_type functions do.

[erszcz]

I've added https://github.com/erszcz/Gradualizer/blob/f5b2468090636f38b8a1029287b6b862094b0c3c/test/should_fail/map_pattern_fail.erl#L21-L30 but it doesn't seem to trigger anything even withexpect_map_type not returning {type_error, ...}...

The g and h specs are required for the warning to show up:

     22 not_a_map_passed_as_map() ->
     23     G = g(),
  W  24     h(G).     ■ The variable G is expected to have type map() but it has type {tup, le}
     25
     26 -spec g() -> {tup, le}.
     27 g() -> {tup, le}.
     28
     29 -spec h(map()) -> ok.
     30 h(#{k := v} = _Map) ->
     31     ok.

So... it seems to just work 🤔

[erszcz]

In other words, the presence of this:

diff --git a/src/typechecker.erl b/src/typechecker.erl
index ebecda1..4d2fb80 100644
--- a/src/typechecker.erl
+++ b/src/typechecker.erl
@@ -4317,7 +4317,9 @@ expect_map_type({var, _, Var}, _Env) ->
     Cs = constraints:add_var(Var, constraints:upper(Var, type(map, any))),
     {assoc_tys, any, Cs};
 expect_map_type(?type(map, AssocTys), _Env) ->
-    {assoc_tys, AssocTys, constraints:empty()}.
+    {assoc_tys, AssocTys, constraints:empty()};
+expect_map_type(Ty, _Env) ->
+    {type_error, Ty}.
 
 %% Rewrite map_field_assoc to map_field_exact to return in pattern types.
 %%

Doesn't seem to make any difference whatsoever (I checked all combinations of specs present/missing).

[zuiderkwast]

If we have coverage on the other expect_map_type clauses, then we must be able to get coverage on the last clause too.

We really should know how to trigger the different parts of the code. We shouldn't rely on guess work and trial-and-error.

[erszcz]

We really should know how to trigger the different parts of the code. We shouldn't rely on guess work and trial-and-error.

Exactly, that's why I'm thoroughly testing it on examples (what could be called "trial-and-error", but I'd argue). The thing is I couldn't find any that would trigger this new clause. If there's no example triggering the code, the code's not needed - that's what I'm trying to say. Anyway, I'll trace execution and see if I can come up with something that would hit the new clause.

[zuiderkwast]

Keep up the good work. 👍 If you can hit the clause just above, then I don't understand how it can be impossible to cover the next clause.....

[zuiderkwast]

Of course guess work and testing all kinds of examples is useful to figure out how it works. We just shouldn't commit code that we don't know how it works. That's what I meant.

[erszcz]

Mystery solved:

 expect_map_type(?type(map, AssocTys), _Env) ->
-    {assoc_tys, AssocTys, constraints:empty()}.
+    {assoc_tys, AssocTys, constraints:empty()};
+expect_map_type(Ty, _Env) ->
+    {type_error, Ty}.

protects against a buggy spec as shown by 7aed2b2.

[erszcz]

@zuiderkwast Ok, looks good to me, but let me know what you think.

[zuiderkwast]

Yes, LGTM!