search

WhatsApp / erlang-language-platform

Erlang Language Platform. LSP server and CLI.
https://whatsapp.github.io/erlang-language-platform/
Apache License 2.0
256 stars 20 forks source link

False positives when detecting unused imports (RabbitMQ codebase) #30

Open mkuratczyk opened 5 months ago

mkuratczyk commented 5 months ago

Describe the bug

I tried to use elp to remove unused imports across RabbitMQ codebase. However, there are two imports that I need to restore afterwards, for RabbitMQ to start.

To Reproduce**

git clone https://github.com/rabbitmq/rabbitmq-server.git
cd rabbitmq-server
elp lint --diagnostic-filter W0020 --apply-fix --in-place --include-tests
make start-cluster

The imports that are incorrectly removed are here:

  1. https://github.com/rabbitmq/rabbitmq-server/blob/main/deps/rabbit/src/rabbit_amqp_util.erl#L9 Here the problem I think is related to the fact that amqp10_framing.hrl is a generated file. rabbit_amqp_util.erl includes rabbit_amqp.hrl which in turn includes amqp10_common/include/amqp10_framing.hrl but this file doesn't exist if you start from a fresh repo. If you run make start-cluster first and then run the same elp command, rabbit_amqp_uti.erl is not modified by elp.

  2. https://github.com/rabbitmq/rabbitmq-server/blob/main/deps/rabbitmq_peer_discovery_consul/src/rabbitmq_peer_discovery_consul_health_check_helper.erl#L18 In this case, the need for the removed include file is "hidden" by a macro -?CONFIG_MAPPING expands to code that uses the peer_discovery_config_entry_meta record.

Context

Thank you!

robertoaloi commented 5 months ago

@mkuratczyk Thanks for reporting! While we work on a fix, you can prepend the problematic line with something like:

% elp:ignore L1500 W0020 - false positive (see https://github.com/WhatsApp/erlang-language-platform/issues/30)

Which will silent the linter (so you can run it from CLI periodically or in CI)

@alanz L1500 was the old error code for the diagnostic. It looks like we still report both.

alanz commented 5 months ago

L1500 is long gone, make sure you are using a recent version of ELP

michalmuskala commented 5 months ago

ELP generally assumes header files are self-contained - this is violated in the 2nd case you describe since rabbit_peer_discovery_consul.hrl can't be included independently.

The "proper" fix from the view point of ELP and self-contained headers would be to move -include_lib("rabbitmq_peer_discovery_common/include/rabbit_peer_discovery.hrl"). into the header that defines the macro using the records from there, notably rabbit_peer_discovery_consul.hrl. The include_lib can be later removed from modules including rabbit_peer_discovery_consul.hrl.

mkuratczyk commented 5 months ago

yeah, that makes sense. we can solve the second case on our side