AdRoll / rebar3_hank

The Erlang Dead Code Cleaner
MIT License
68 stars 9 forks source link

Crash upon unconventional `.config` file #157

Closed g-andrade closed 3 years ago

g-andrade commented 3 years ago

(Great lib, by the way!)

Bug Description

I've got this .config file containing a map (in Erlang syntax) and a . at the end.

file:consult/1 can read it fine but Hank crashes when validating unused_configuration_options

To Reproduce

  1. Create a file named crash.config, with content #{foobar => 3.14}., under a project that uses rebar_hank (sorry for the extra work, GitHub does not allow .config files to be attached)
  2. Invoke rebar3 hank on said project and confirm the crash
  3. Open an Erlang shell and see how file:consult("crash.config") succeeds by comparison

Expected Behavior

I expected it to not crash (but nothing in particular beyond that, as I was unaware of the unused_configuration_options check until this happened.)

rebar3 Logs

hank crash log.txt

Additional Context

g-andrade commented 3 years ago

The crash is happening right here, presumably because ConfigTuples is not a list.

Therefore the fix looks superficially easy, but that depends on what counts as a valid .config file. Is it anything that file:consult/1 can parse? Or is its definition stricter?

g-andrade commented 3 years ago

Is it anything that file:consult/1 can parse? Or is its definition stricter?

The documentation doesn't explicitly restrict it, but it does sort of suggest it's the latter.

So it's possible I should just not use .config as an extension in my use case, albeit at some inconvenience (I've got Erlang syntax highlighting setup for it.)

What do you think?

elbrujohalcon commented 3 years ago

I think Hank should just ignore files that can't be properly parsed.

But I also think that using a different extension in your case will help you reduce the number of potential issues with different tools that (maybe incorrectly) assume that all .config files are formatted as sys.config (like Hank and rebar3_format, for instance).

elbrujohalcon commented 3 years ago

Will be fixed in next release, @g-andrade. Thanks for spotting the issue!

g-andrade commented 3 years ago

👍 Thanks for the quick response!