assert %{} = value should warn that author probably meant assert %{} == value

[luk...@niemier.pl]

Follow up of https://github.com/elixir-lang/elixir/issues/11331

The goal is to warn on `assert %{} = value` that user meant `assert %{} == value` and inform user that if they want to check whether the returned value is really map, then they should use `assert is_map(value)` instead.

PR with sample implementation - https://github.com/elixir-lang/elixir/pull/11333

[Wojtek Mach]

This:

    x = :foo; assert %{} = x

produces:

     match (=) failed
     code:  assert %{} = :foo
     left:  %{}
     right: :foo
            ^^^^
     stacktrace:
       foo_test.exs:11: (test)

(emphasis mine)

This:

    x = :foo; assert is_map(x)

gives:

     Expected truthy, got false
     code: assert is_map(x)
     arguments:

         # 1
         :foo
         ^^^^

I personally find the former error message a bit nicer, but maybe it is I'm just used to seeing left/right of diffs and that's what I expect the vast majority of time, anything else gives a small pause.

In any case, my recommendation is to always do `assert actual == expected` and `assert expected = actual`, never `assert expected == actual`. And yeah, I think you're making a good point about matching on map is definitely a possible foot gun, but I personally don't think it should be enforced by the tooling.

 --
 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/44b23c21-5f6c-4cc1-82e5-543b02a343f8n%40googlegroups.com.

[Łukasz Niemier]

> In any case, my recommendation is to always do `assert actual == expected` and `assert expected = actual`, never `assert expected == actual`. And yeah, I think you're making a good point about matching on map is definitely a possible foot gun, but I personally don't think it should be enforced by the tooling.

I do not fact that the order of the expected and actual value depends on the used operator. I always use `assert <expected> <op> <actual>` to have some uniformity.

However even if someone uses them in the way you pointed there, there are examples where it failed in the wild [1]. While I can support fact that language should not force particular style of writing code (which is funny thing to say as `Code.format_string/{1,2}` is built in), we should help users with informing them that they used potentially problematic form (just as we do with `foo` vs `foo()`).

[1]: https://github.com/phoenixframework/phoenix/pull/4559/files

--

Łukasz Niemier
luk...@niemier.pl

[Wiebe-Marten Wijnja]

I wonder if maybe a Credo check might be better suited for this rather
than a built-in compiler warning.

That said, another thing that we could improve in the language (or
rather in ExUnit), is the test feedback when assertions on builtin
guards are used,
e.g. for `assert is_map(foo)` we could show "Expected a map, got foo"
rather than "Expected truthy, got false".

~Marten/Qqwy

[Łukasz Niemier]

> I wonder if maybe a Credo check might be better suited for this rather than a built-in compiler warning.

The point is that not all projects use Credo, and as pointed above, this issue is happening also in some "high-grade" applications (Phoenix). Also Credo checks can be quite slow, so not everyone is running it regularly, while running single test can be much faster.

The main reason why I think that it should be in ExUnit is that it can easily result in **wrong test**. It is not like `assert length(list) == 1` that still will be correct, just can be slow. It is not `a = %{a: 1}; b = %{a: 1, b: 2}; assert ^a = b` that will just fail as the match isn't exact. `assert %{} = value` is **dangerous** as it will silently allow any map without telling user that they have problem in their code until that will blow in their face in production.

That is the problem, this code will work, it will happily report success (even when user expected it to fail) and will work, until there will be change that will cause it to silently accept wrong result.

--

Łukasz Niemier
luk...@niemier.pl