Incorrectly identified unused_record_fields in KAZOO

[jamesaimonetti]

Bug Description

Record fields that appear "obviously in use" in the code are detected as unused.

To Reproduce

Well, this is the tricky bit as we don't use rebar3 in KAZOO. Hooked into hank:analze/5 directly.

Here's the basic escript:

#!/usr/bin/env escript
%%! +A0 -sname kazoo_hank
%% -*- coding: utf-8 -*-

-mode('compile').

-export([main/1]).

%% API

main([]) ->
    AppFiles = lists:foldl(fun add_app_path/2, [], kz_ast_util:project_apps()),
    main(AppFiles);
main(Files) ->
    #{results := Results
     ,stats := Stats
     } = hank:analyze(Files
                     ,hank_ignores()
                     ,hank_rules()
                     ,parsing_style()
                     ,hank_context()
                     ),
    print_stats(Stats),
    io:format("results(~p):~n", [length(Results)]),
    {_LastRule, Counts} = lists:foldl(fun print_result/2, {'undefined', #{}}, Results),
    io:format("rule violations: ~p~n", [Counts]).

add_app_path(App, Acc) ->
    filelib:wildcard(
      filename:join([code:lib_dir(App), "**/*.[e|h]rl"])
     ) ++ Acc.

print_stats(#{ignored := Ignored
             ,parsing := ParsingMs
             ,analyzing := AnalysisMs
             ,total := TotalMs
             }) ->
    io:format("analysis took ~pms parsing, ~pms analyzing, ~pms total, ignore ~p files~n"
             ,[ParsingMs, AnalysisMs, TotalMs, Ignored]
             ).

print_result(#{file := File
              ,line := Line
              ,pattern := Pattern
              ,rule := Rule
              ,text := Text
              }
            ,{Rule, RuleCounts}
            ) ->
    io:format("~s:~p: ~s~n~n"
             ,[File, Line, rule_text(Rule, Text, Pattern)]
             ),
    {Rule, maps:update_with(Rule, fun(V) -> V+1 end, 1, RuleCounts)};
print_result(#{rule := NewRule}=Result
            ,{_OldRule, RuleCounts}
            ) ->
    io:format("rule violations for ~s:~n", [NewRule]),
    print_result(Result, {NewRule, RuleCounts}).

rule_text(_Rule, Text, _Pattern) ->
    Text.

-spec hank_ignores() -> [hank_rule:ignore_spec()].
hank_ignores() -> [].

hank_rules() ->
    hank_rule:default_rules().

parsing_style() ->
    'sequential'. % or parallel

hank_context() ->
    Apps = [{App, code:lib_dir(App)}
            || App <- kz_ast_util:project_apps()
           ],
    hank_context:new(maps:from_list(Apps), []).

Here's an example of the output:

src/omnip_subscriptions.erl:52: Field ready in record state is unused

src/omnip_subscriptions.erl:54: Field other_nodes_count in record state is unused

The record in question: https://github.com/2600hz/kazoo/blob/master/applications/omnipresence/src/omnip_subscriptions.erl#L51
The record fields being used: https://github.com/2600hz/kazoo/blob/master/applications/omnipresence/src/omnip_subscriptions.erl#L164

This is all very much first pass, quick hack to correct low-hanging fruit; fully expect there are better ways to call into hank or provide it more context.

Expected Behavior

Not detect record fields as unused

Additional Context

• OS: Debian
• Erlang version 23.1
• rebar3 NA

I will say this: just on unused_record_fields, the first pass of hank detected 133 instances, of which it appears 7 are false positives. The above represents 2 of the 7. So I think that is phenomenal and really fun to dig through KAZOO's cobwebs and oxtail code (love that btw!).

@elbrujohalcon great preso on hank and thanks for poking at KAZOO. Hope we can get hank integrated into CI after this initial pass.

[elbrujohalcon]

Hi @jamesaimonetti and @filipevarjao…
First of all…

Nice escript!! Would you mind publishing it as a separate repo with this one as a dependency (in the same way we have https://github.com/inaka/elvis that depends on https://github.com/inaka/elvis_core ? 🙏🏻 🙏🏻 🙏🏻

Second…
It's oxbow code, not oxtail code, and yes… I love that name, too! @lauramcastro helped me find it and we immediately fell in love with the multi-layered analogy.

Third…
Thanks for the kind words! 😊

And now…
Let me dig into the record usage issues… I'll come back to you soon with more information (and probably a pull request).

[elbrujohalcon]

…aaaaaand… can you guess what's the culprit of this issue?
I wouldn't think it could be any other thing, actually…
The problem lies in…

…wait for it…

😡 F**KING MACROS!! 😡

My nemesis

The parser can't understand that clause, so it fails to parse the whole function… and therefore it doesn't detect the usage of the fields.

I'll see if I can fix that somehow, but sadly… I can't promise it. With macros, you never know.

[elbrujohalcon]

Nope. Sorry, @jamesaimonetti, @filipevarjao… This goes to the eventually 🙄 milestone.
You'll have to add some ignore rules for now, since fixing this would likely require changes in erl_parse, erl_syntax, and ktn_dodger.

If we ever get to a point where ktn_dodger:parse_file/1,2 can tackle a file like the one below, we will be able to fix this issue by just upgrading the version of katana_code in the dependencies.

-module(a).

-export([x/1]).

-define(M(T), {m, T}).

x(?M(T)) -> T.

[jamesaimonetti]

@elbrujohalcon lol on oxtail, i'm preparing ingredients for oxtail bone broth so, once again, my stomach overrides my brain.

The escript does rely on a kazoo-specific module (kz_ast_util) in our ast parsing application (core/kazoo_ast in the kazoo repo). I can make a repo for it and just highlight that area for folks to fill in with their choice of generator.

Thanks for looking into it; I'll keep in mind the macro stuff for more false positives I find. If they don't involve macros, I'll file another issue.