Proposal: Warn on assert %{} = x

[Wojtek Mach]

Hi,

Developers can easily shoot themselves in the foot if they write:

    assert %{} = x

but really what they meant was to write:

    assert %{} == x

The mistake is writing `=` instead of `==`, an easy one to make. The difference is of course that the former will succeed on _any_ map and the latter will _only_ succeed on an _empty_ map.

I'd like to propose ExUnit warn on `assert %{} = x` and tell users to instead write `assert is_map(x)`.

Thoughts?

P.S. In my projects I'd either write `assert actual == expected` OR `assert expected = actual` and never `assert expected == actual` exactly because it is easy to make the mistake. Maybe there is a Credo check to enforce such style.

[Ben Wilson]

To me this feels like a good use of credo or similar linter, not something that the Elixir compiler itself should warn about. `assert %{} = x` isn't the most idiomatic way to match but it isn't incoherent or invalid, just probably not best practice.

[Wojtek Mach]

Just to be clear what I proposed was not a change in the compiler, just changing ExUnit's assert macro to emit the warning under that specific scenario.

-- 
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/b70a4bb4-14d4-4ea3-8b49-45b408abf750n%40googlegroups.com.

[Sabiwara Yukichi]

It seems really close to this discussion: https://groups.google.com/g/elixir-lang-core/c/1wixK5zNBEw/m/FVNpANbPBQAJ.

I don't think this would solve the issue completely though: should we then warn for nested maps like `[%{}] =` or `{:ok, %{}} =`? (which might be intended)

Also, people might be writing `%{name: "foo"} =` actually meaning `%{name: "foo"} ==`, but I don't think we should warn on these.

> my recommendation is to always do `assert actual == expected` and `assert expected = actual`, never `assert expected == actual`

I really like this suggestion you made in the previous thread, it helps a lot distinguishing between matches and equality, maybe it could be encouraged in the official docs?

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/D3AD3ABD-1F0F-48F8-B9C2-A3E6B67E8EB1%40wojtekmach.pl.

[Wojtek Mach]

Oh wow, I totally forgot about that thread from last year. There was some more discussion [1], a PR with reference implementation of this change [2] and even a PR in the wild that this change would have caught [3]. Thank you for linking to it!

[1] https://github.com/elixir-lang/elixir/issues/11331

[2] https://github.com/elixir-lang/elixir/pull/11333

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

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

[luk...@niemier.pl]

Yeah, I have proposed that and I still strongly believe that it is what should be done in the repo, as it is something that you can easily trip over.