Prevent "a term is constructed, but never used" warning

paulo-ferraz-oliveira commented 3 years ago

We use a construct, as suggested by @bjorng, to modify the generated marker.

This potentially closes #546, as reported by @michaelklishin, with the implementation suggested by @bjorng in https://github.com/erlang/otp/issues/4576#issuecomment-791188167.

@Vagabond: this prevents any change to consumer code since we actually just address the issue. Lemme know if it's acceptable.

@michaelklishin, once tests are passing, if you have time, I invite you to test this version to see if the issue goes away completely, for you.

paulo-ferraz-oliveira commented 3 years ago

Hm... appveyor is still running with the tests. Does the app need to be uninstalled, @Vagabond?

Edit: this is otherwise misleading at https://github.com/pulls.

michaelklishin commented 3 years ago

If I replace deps/lager with this branch and gmake distclean individual repos (doing so from repo root will wipe all 3rd party dependencies), rabbit_common now compiles but rabbit does not:

 DEPEND rabbit.d
 ERLC   amqqueue.erl amqqueue_v1.erl background_gc.erl code_server_cache.erl gatherer.erl gm.erl internal_user.erl internal_user_v1.erl lager_exchange_backend.erl lqueue.erl mirrored_supervisor_sups.erl pg_local.erl pid_recomposition.erl rabbit.erl rabbit_access_control.erl rabbit_alarm.erl rabbit_amqqueue.erl rabbit_amqqueue_process.erl rabbit_amqqueue_sup.erl rabbit_amqqueue_sup_sup.erl rabbit_auth_backend_internal.erl rabbit_auth_mechanism_amqplain.erl rabbit_auth_mechanism_cr_demo.erl rabbit_auth_mechanism_plain.erl rabbit_autoheal.erl rabbit_backing_queue.erl rabbit_basic.erl rabbit_binding.erl rabbit_boot_steps.erl rabbit_channel.erl rabbit_channel_interceptor.erl rabbit_channel_sup.erl rabbit_channel_sup_sup.erl rabbit_channel_tracking.erl rabbit_channel_tracking_handler.erl rabbit_classic_queue.erl rabbit_client_sup.erl rabbit_config.erl rabbit_confirms.erl rabbit_connection_helper_sup.erl rabbit_connection_sup.erl rabbit_connection_tracking.erl rabbit_connection_tracking_handler.erl rabbit_control_pbe.erl rabbit_core_ff.erl rabbit_core_metrics_gc.erl rabbit_credential_validation.erl rabbit_credential_validator.erl rabbit_credential_validator_accept_everything.erl rabbit_credential_validator_min_password_length.erl rabbit_credential_validator_password_regexp.erl rabbit_dead_letter.erl rabbit_definitions.erl rabbit_diagnostics.erl rabbit_direct.erl rabbit_direct_reply_to.erl rabbit_disk_monitor.erl rabbit_epmd_monitor.erl rabbit_event_consumer.erl rabbit_exchange.erl rabbit_exchange_decorator.erl rabbit_exchange_parameters.erl rabbit_exchange_type_direct.erl rabbit_exchange_type_fanout.erl rabbit_exchange_type_headers.erl rabbit_exchange_type_invalid.erl rabbit_exchange_type_topic.erl rabbit_feature_flags.erl rabbit_ff_extra.erl rabbit_ff_registry.erl rabbit_fhc_helpers.erl rabbit_fifo.erl rabbit_fifo_client.erl rabbit_fifo_index.erl rabbit_fifo_v0.erl rabbit_file.erl rabbit_framing.erl rabbit_guid.erl rabbit_health_check.erl rabbit_lager.erl rabbit_limiter.erl rabbit_log_tail.erl rabbit_looking_glass.erl rabbit_maintenance.erl rabbit_memory_monitor.erl rabbit_metrics.erl rabbit_mirror_queue_coordinator.erl rabbit_mirror_queue_master.erl rabbit_mirror_queue_misc.erl rabbit_mirror_queue_mode.erl rabbit_mirror_queue_mode_all.erl rabbit_mirror_queue_mode_exactly.erl rabbit_mirror_queue_mode_nodes.erl rabbit_mirror_queue_slave.erl rabbit_mirror_queue_sync.erl rabbit_mnesia.erl rabbit_mnesia_rename.erl rabbit_msg_file.erl rabbit_msg_record.erl rabbit_msg_store.erl rabbit_msg_store_ets_index.erl rabbit_msg_store_gc.erl rabbit_networking.erl rabbit_node_monitor.erl rabbit_nodes.erl rabbit_osiris_metrics.erl rabbit_parameter_validation.erl rabbit_password.erl rabbit_password_hashing_md5.erl rabbit_password_hashing_sha256.erl rabbit_password_hashing_sha512.erl rabbit_peer_discovery.erl rabbit_peer_discovery_classic_config.erl rabbit_peer_discovery_dns.erl rabbit_plugins.erl rabbit_policies.erl rabbit_policy.erl rabbit_policy_merge_strategy.erl rabbit_prelaunch_cluster.erl rabbit_prelaunch_enabled_plugins_file.erl rabbit_prelaunch_feature_flags.erl rabbit_prelaunch_logging.erl rabbit_prequeue.erl rabbit_priority_queue.erl rabbit_queue_consumers.erl rabbit_queue_decorator.erl rabbit_queue_index.erl rabbit_queue_location_client_local.erl rabbit_queue_location_min_masters.erl rabbit_queue_location_random.erl rabbit_queue_location_validator.erl rabbit_queue_master_location_misc.erl rabbit_queue_master_locator.erl rabbit_queue_type.erl rabbit_queue_type_util.erl rabbit_quorum_memory_manager.erl rabbit_quorum_queue.erl rabbit_ra_registry.erl rabbit_reader.erl rabbit_recovery_terms.erl rabbit_restartable_sup.erl rabbit_router.erl rabbit_runtime_parameters.erl rabbit_ssl.erl rabbit_stream_coordinator.erl rabbit_stream_queue.erl rabbit_sup.erl rabbit_sysmon_handler.erl rabbit_sysmon_minder.erl rabbit_table.erl rabbit_trace.erl rabbit_tracking.erl rabbit_upgrade.erl rabbit_upgrade_functions.erl rabbit_upgrade_preparation.erl rabbit_variable_queue.erl rabbit_version.erl rabbit_vhost.erl rabbit_vhost_limit.erl rabbit_vhost_msg_store.erl rabbit_vhost_process.erl rabbit_vhost_sup.erl rabbit_vhost_sup_sup.erl rabbit_vhost_sup_wrapper.erl rabbit_vm.erl supervised_lifecycle.erl tcp_listener.erl tcp_listener_sup.erl term_to_binary_compat.erl vhost.erl vhost_v1.erl
compile: warnings being treated as errors
src/rabbit_amqqueue.erl:437:5: a term is constructed, but never used
%  437|     rabbit_log:info("Starting queue rebalance operation: '~s' for vhosts matching '~s' and queues matching '~s'",
%     |     ^

src/rabbit_amqqueue.erl:455:5: a term is constructed, but never used
%  455|     rabbit_log:info("Finished queue rebalance operation"),
%     |     ^

src/rabbit_amqqueue.erl:458:5: a term is constructed, but never used
%  458|     rabbit_log:warning("Queue rebalance operation is in progress, please wait."),
%     |     ^

src/rabbit_amqqueue.erl:484:13: a term is constructed, but never used
%  484|             rabbit_log:info("All queue masters are balanced"),
%     |             ^

src/rabbit_amqqueue.erl:514:21: a term is constructed, but never used
%  514|                     rabbit_log:warning("Migrating queue ~p from node ~p with ~p queues to node ~p with ~p queues",
%     |                     ^

src/rabbit_amqqueue.erl:518:29: a term is constructed, but never used
%  518|                             rabbit_log:warning("Queue ~p migrated to ~p", [Name, NewNode]),
%     |                             ^

src/rabbit_amqqueue.erl:521:29: a term is constructed, but never used
%  521|                             rabbit_log:warning("Error migrating queue ~p: ~p", [Name, Reason]),
%     |                             ^

src/rabbit_amqqueue.erl:526:13: a term is constructed, but never used
%  526|             rabbit_log:warning("Node ~p contains ~p queues, but all have already migrated. "
%     |             ^

src/rabbit_amqqueue.erl:530:13: a term is constructed, but never used
%  530|             rabbit_log:warning("Node ~p only contains ~p queues, do nothing",
%     |             ^

src/rabbit_amqqueue.erl:629:21: a term is constructed, but never used
%  629|                     rabbit_log:debug("Unexpected alive queue process ~p~n", [QPid]),
%     |                     ^

src/rabbit_amqqueue.erl:1237:5: a term is constructed, but never used
% 1237|     rabbit_log:error("Failed to fetch number of queues in vhost ~p:~n~p~n",
%     |     ^

paulo-ferraz-oliveira commented 3 years ago

Hm... that's odd. I guess we'll have to dig deeper into what might be causing that, now. When I did my local tests rabbit was OK, but I'm not used to erlang.mk so might have done something wrong while getting good results.

Edit: it seems like it might be complaining about a missing _ in the case return (?), but I don't see why it'd do this, lest the compiler assumed no side-effect functions could ever occur. (and this is an issue the dialyzer already cares about). Need to dig deeper.

michaelklishin commented 3 years ago

@paulo-ferraz-oliveira I think it's fair to ask for a way to disable this warning in erlc now that anonymous function "inlining" is much more aggressive and people run into this more often.

paulo-ferraz-oliveira commented 3 years ago

@michaelklishin, maybe (?) In our 100+ lib.s I didn't see this happening yet. And many are using lager (30+). So we can ask, but I'm not sure how useful/used it'll actually be. 😄

bjorng commented 3 years ago

I think it's fair to ask for a way to disable this warning in erlc now that anonymous function "inlining" is much more aggressive and people run into this more often.

I agree. I've created https://github.com/erlang/otp/pull/4598.

michaelklishin commented 3 years ago

Thank you @bjorng

paulo-ferraz-oliveira commented 3 years ago

Cool. We'd just have to add to rebar.config in lager, right? (at the same time, I'm still not sure where that warning is coming from now, but haven't dug deeper, either).

paulo-ferraz-oliveira commented 3 years ago

At the same, time, the pull request still seems relevant, since it removes a "hack".

bjorng commented 3 years ago

At the same, time, the pull request still seems relevant, since it removes a "hack".

Yes, this pull request is still relevant.

Vagabond commented 3 years ago

This PR looks fine to me. Is it complete?

paulo-ferraz-oliveira commented 3 years ago

[x] If possible, I'd like to ask you to check out why appveyor's still here.
[x] On the other hand, as per https://github.com/erlang-lager/lager/pull/547#issuecomment-792250328 do you think anything else should be done? e.g. another place we can make the compiler understand this is generated code, for example?
[x] Should we wait until https://github.com/erlang/otp/pull/4598 is made available in a release candidate and then chose the appropriate flag to shush lager? (I'd ask @michaelklishin to try it out, afterwards, if he still has time/patience for this 😄)

michaelklishin commented 3 years ago

I'd be happy try it and the new flag with a slightly out of date version of RabbitMQ as we have migrated to OTP logger in master. Any of these changes will be necessary for 3.8 at least for a few months.

paulo-ferraz-oliveira commented 3 years ago

Cool, I mean... https://github.com/erlang/otp/pull/4598 hasn't yet been merged, but if you're willing to compile/test from it, it'd be nice. I'm still wondering what's causing the new warnings, though. Any ideas?

paulo-ferraz-oliveira commented 3 years ago

erlang/otp#4598 is now merged.

michaelklishin commented 3 years ago

I'm afraid I cannot get RabbitMQ 3.8 to compile on OTP 24 with this patch. It can be merged if there's evidence that it works for other codebases. I guess we will have to go with the new compiler flag for 3.8 and in future 3.9 Lager is no longer used.

paulo-ferraz-oliveira commented 3 years ago

@michaelklishin: I haven't checked this yet (flag) since I was waiting for replies on https://github.com/erlang-lager/lager/pull/547#issuecomment-794151464. Is it possible to somehow add that flag to lager? (FWIW, as-is, even without this patch, lager's working OK for us - i.e. 3.9.1).

michaelklishin commented 3 years ago

Compiler flags are controlled by the build system used.

paulo-ferraz-oliveira commented 3 years ago

Compiler flags are controlled by the build system used.

Yeah, I forgot you're not using rebar3.config. 😞

Does erlang.mk not have an "options system" (similar to rebar.config that we could add here too)?

What would be required to import erlang.mk (I expect lager maintainers prefer to be inclusive)

michaelklishin commented 3 years ago

erlang.mk supports Rebar dependencies but applications ultimately can override the flags.

michaelklishin commented 3 years ago

@lhoguin confirmed that erlang.mk will respect rebar.config compiler options except for stripping -Werror. We are experimenting with the new flags from https://github.com/erlang/otp/pull/4598.

paulo-ferraz-oliveira commented 3 years ago

Good news. Once you know more (or, eventually, have this working for you), let me know what I can change, in lager, to move forward).

michaelklishin commented 3 years ago

No matter what I tried I cannot get this PR or the flags from https://github.com/erlang/otp/pull/4598 to avoid or silence the warning.

I suggest that this PR is merged regardless. We will add _ = … to every log event. While incredibly annoying, we had to revisit every single log message when switching to logger to avoid extra trailing newlines. So I already hate all the popular logging libraries in Erlang anyway.

jadeallenx commented 3 years ago

I already hate all the popular logging libraries in Erlang anyway.

It's only a matter of time for any logging library ;)

jadeallenx commented 3 years ago

Hm... appveyor is still running with the tests. Does the app need to be uninstalled, @Vagabond?

I think I have cleared out the lager projects at appveyor's CI website.

Vagabond commented 3 years ago

Yeah, I think we can kill appveyor, I don't know who even added it.

paulo-ferraz-oliveira commented 3 years ago

Hm... appveyor is still running with the tests. Does the app need to be uninstalled, @Vagabond?

I think I have cleared out the lager projects at appveyor's CI website.

@mrallen1, I think you might need to do something in lager's GitHub Settings.

jadeallenx commented 3 years ago

OK, cleared out appveyor and like a bunch of b*sho and travis webhooks - so i think all of that is gone now. @paulo-ferraz-oliveira

paulo-ferraz-oliveira commented 3 years ago

I'm OK to merge. As per @michaelklishin's recent comment it seems we got his go ahead.

michaelklishin commented 3 years ago

appveyor was added by a former member of our team after we hit a Windows-specific issue. If Windows testing is covered elsewhere, Lager can probably drop it.

paulo-ferraz-oliveira commented 3 years ago

appveyor was added by a former member of our team after we hit a Windows-specific issue. If Windows testing is covered elsewhere, Lager can probably drop it.

There is Windows testing (I added it) but it's now done via GitHub Actions. 👍

erlang-lager / lager

Prevent "a term is constructed, but never used" warning #547