Invalid define is unused warning

[tothlac]

Bug Description

It's a very nice plugin, so probably we would like to use it on our repositories.
Unfortunately, it looks like sometimes there are false positive warnings.

To Reproduce

Run rebar3 hank on this simple code

-module(myapp).

-define(EXCEPTION(C, R, Stacktrace), C:R:Stacktrace).

-export([f/0]).

-define(RESULT, 1).

-spec f() -> term().
f() ->
    try
        ?RESULT
    catch ?EXCEPTION(_, _Reason, _St) ->
              ok
    end.

Expected Behavior

hank returns the following false positive:

(base) tothlac:myapp rebar3 hank
===> Looking for code to kill with fire...
===> The following pieces of code are dead and should be removed:
src/myapp.erl:7: ?RESULT is unused

, and it should not complain.

[elbrujohalcon]

Oh, man! Can I hate macros even more??

I didn't check but most likely hank can't parse your function and that's why it doesn't detect macro usage.

I'm sorry but fixing this will not be easy. You'll be better off adding a -hank attribute to your module to ignore the warning.

[paulo-ferraz-oliveira]

This reminded me of:

• Non-atom catch element reported as atom #2 inaka/elvis_core#186
• Non-atom catch element reported as atom inaka/elvis_core#170

[pbrudnick]

This Node is difficult to handle:

text - {tree,text,
             {attr,10,[],none},
             "f( ) -> try ?RESULT catch ?EXCEPTION( _ , _Reason , _St ) -> ok end .\n"}

[tothlac]

Hi Brujo. The plugin is really nice, it will be definitely quite useful for our repositories.
Today I was able to find a huge amount of dead code (not used parameters, not used defines) in our code.

The problem is that we have 100 repositories. In these 100 repositories, there are at least 2 thousand modules, and I'm afraid this fake warning occurs at least in 100 modules because we use macros like EXCEPTION in a lot of places. Actually, hank works fine without the EXCEPTION macro, so this module is ok:

-module(myapp).

-define(EXCEPTION(C, R, Stacktrace), C:R:Stacktrace).

-export([f/0]).

-define(RESULT, 1).

-spec f() -> term().
f() ->
    try
        ?RESULT
    catch C:R:Stacktrace ->
              ok
    end.

So most likely I will delete the EXCEPTION macro from everywhere (We used it when we were switching from OTP 21 to 22).

Adding it to the list of ignored modules would not be really good because that way we would hide lots of other warnings in those modules.

I'm going to check what warnings it will report without the EXCEPTION and STACKTRACE macros. Hopefully, there won't be any kind of false positives after this modification.

[paulo-ferraz-oliveira]

It's also marginally linked to erlang/otp#4479. Once Erlang/OTP can detect same-module unused macros, part of your problem will go away, unless you're using a shared .hrl, and then it's a different class of issues altogether, as Brujo points out in the linked issue.

[elbrujohalcon]

Adding it to the list of ignored modules would not be really good because that way we would hide lots of other warnings in those modules.

FWIW... You don't need to ignore the whole module. Hank allows you to ignore specific warnings. Check the Readme for the appropriate syntax.

[tothlac]

I've seen that, just like in elvis you can add a file to the ignore list of some specific rules. That's fine, but it wold be still probably 50-100 modules in all of our repositories, so I don't think ignoring those modules would be the best option.

[tothlac]

Anyways, as you have said fixing this issue would take quite some time, the ticket should be closed.

[elbrujohalcon]

I've seen that, just like in elvis you can add a file to the ignore list of some specific rules. That's fine, but it wold be still probably 50-100 modules in all of our repositories, so I don't think ignoring those modules would be the best option.

But also, somewhat similarly to Elvis as well, you can add a specific flag to your modules to ignore only the warning that you want to ignore but keep analyzing the rest of the file. Something like…

-hank([{unused_macros, ["RESULT"]}]).

…or more specifically…

-hank([{unused_macros, [{"RESULT", none}]}]).

[elbrujohalcon]

Anyways, as you have said fixing this issue would take quite some time, the ticket should be closed.

I'll keep it open, but I'll move it to the Eventually 🙄 milestone 😉

[paulo-ferraz-oliveira]

I fiddled around with erl_syntax, erl_scan and erl_parse. Do you think the answer will go through these?

[elbrujohalcon]

Most likely… But also ktn_dodger… or epp_dodger… or… maybe… eventually… the new parser from erlfmt is introduced into OTP… it becomes the official parser and then we switch to it.

[tothlac]

Hi. Maybe it is another error but looks like there are other special cases when hank is not able to parse everything correctly. For example when you use string concatenation without ++ operator, like in the example below:

-module(hank_problem).

-export([world/0]).

-define(WORLD, "world").

world() ->
    "hello " ?WORLD.

WORLD macro is used. hank reports this:

===> The following pieces of code are dead and should be removed:
src/hank_problem.erl:5: ?WORLD is unused

[elbrujohalcon]

Same thing. String concat with macros is unparseable.

[tothlac]

Thanks. That's why I created it as part of this ticket.

[tothlac]

Well, probably the easiest workaround here is to use the ++ opeator.

[tothlac]

I've tried and fortunately using that syntax hank does not complain.