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

[paulo-ferraz-oliveira]

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 erlang/otp#4576 (comment).

@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]

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.

[bjorng]

You no longer need to wrap the term in a fun.

[michaelklishin]

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]

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]

@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]

@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]

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 erlang/otp#4598.

[michaelklishin]

Thank you @bjorng

[paulo-ferraz-oliveira]

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]

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

[bjorng]

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

Yes, this pull request is still relevant.

[Vagabond]

This PR looks fine to me. Is it complete?

[paulo-ferraz-oliveira]

• If possible, I'd like to ask you to check out why appveyor's still here.
• On the other hand, as per Prevent "a term is constructed, but never used" warning #547 (comment) do you think anything else should be done? e.g. another place we can make the compiler understand this is generated code, for example?
• Should we wait until Support suppressing opportunistic compiler warnings erlang/otp#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]

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]

Cool, I mean... erlang/otp#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]

erlang/otp#4598 is now merged.

[michaelklishin]

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]

@michaelklishin: I haven't checked this yet (flag) since I was waiting for replies on #547 (comment). 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]

Compiler flags are controlled by the build system used.

[paulo-ferraz-oliveira]

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]

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

[michaelklishin]

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

[paulo-ferraz-oliveira]

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]

No matter what I tried I cannot get this PR or the flags from erlang/otp#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]

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

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

[jadeallenx]

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]

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

[paulo-ferraz-oliveira]

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]

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]

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

[michaelklishin]

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]

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. 👍