Rewrite merge of clause variable tables (in case, try, etc)

[nox]

erl_lint:icrt_export/4 has been rewritten to make the code really follow the scoping rules of Erlang, and not just in most situations by accident.

• The function should not depend on calling unused_vars/3 because that function does not return variables which begins with an underscore, something that only matters when emitting warnings. This could cause a compiler crash if such a variable was reused afterwards.
• The variable tables from each clause are first merged together, lists:merge/1 is safe to use because they are orddicts and thus already sorted. This list is then traversed parallelly to the old variable table, again taking advantage of their sorted order.
• The function does not emit warnings itself, there is no need to pass around the lint state. In the same vein, vtunsafe/3 has been rewritten to do more things by itself, given that all of its calls were similar. Finally, compiled-out code has been removed.
• This reverts the code in 9ce148b, which fixed the compiler crash and made erl_lint remember unsafe variables, but forget about unused variables in the process.
• Other places of the code which relied on the old clunky behaviour were also updated: unused and unsafe old variables are forgotten when merging fun clauses and boolean shortcircuiting operators do not rely on icrt_export/3 anymore.

I had submitted this already in the past but had retracted it with the hope of finding a cleaner fix. I didn't find one.

[OTP-Maintainer]

Patch has passed first testings and has been assigned to be reviewed

[nox]

Has this been looked at by anyone yet?

[proxyles]

Hi,

The patch fixes several bugs, which is nice. Thank you!

However, due to the many cases and combinations, we feel that more testcases are needed.

It is the first clause of icrt_export/5 that is not covered by erl_lint_SUITE, as it seems. The comment of that clause is quite specific, but we're having a hard time coming up with corresponding examples. Could you have a look at it?

Best regards,

[nox]

Yeah, this patch is quite complicated. I tried many times to make it simpler, to no avail. I will take a look at the first clause.

[nox]

The comment is wrong. This clause is there because otherwise, the next clause will declare any reused variable as bound, even though it could still be an exported variable, in which case any use of that variable in a pattern will not emit a warning. The two first clauses can probably be merged together.

[nox]

See following code, btw:

-module(t).
-compile(export_all).

t(X, Y) ->
    if true -> Z = X end,
    case Y of
        1 -> Z;
        2 -> Z = X
    end,
    Z = X.

Replace Z = X in the second case clause with X and comment the first clause and no warning will be emitted.

[nox]

In fact, it does not trigger a warning even with the first clause.

[nox]

I fixed expr_var, which was actually the problem, and I added a test that covers the first clause.

[proxyles]

Hi,

The patch is definitely an improvement, and although I haven't checked each and every case (there are many!), I consider it reviewed now. Thank you!

I see a few (new) code lines that are 80 characters long. The old OTP Coding Rules state that "Source code lines must not be longer that 78 characters." Not everyone follows the rule, and the patch is OK in its present shape. I just mention it in case you want to adhere to the rule in future patches.

Best regards,

Hans Bolinder, Erlang/OTP team, Ericsson

[nox]

in case you want to adhere to the rule in future patches

I do. :) The shorter the lines, the better, I didn't know about the 78 limit so I limited myself at 80, will do 78 next time.

[josevalim]

I have also ran this patch on Elixir codebase and everything passes. Furthermore, we get (correct) warnings we did not get in the past.