Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent dialyzer warnings on OTP 23 #2412

Merged

Conversation

paulo-ferraz-oliveira
Copy link
Contributor

@paulo-ferraz-oliveira paulo-ferraz-oliveira commented Oct 28, 2020

As initially discussed in #2411. [Fix #2411]

Adding @pablocostass as per his request.

@paulo-ferraz-oliveira paulo-ferraz-oliveira force-pushed the fix/dialyzer_warnings_over_otp_23 branch 2 times, most recently from 3f97c1d to 4ee12ec Compare October 28, 2020 22:14
@paulo-ferraz-oliveira
Copy link
Contributor Author

rebar_git_resource: rebar_uri:parse will never return {error, _} since the code states {error, _, _}. In any case, no error is ever returned when OTP_RELEASE is defined, which means that code is unused on OTP 23. I changed the code a bit to make sure the error is "caught".

@paulo-ferraz-oliveira paulo-ferraz-oliveira force-pushed the fix/dialyzer_warnings_over_otp_23 branch 4 times, most recently from 77289e4 to b4dfee4 Compare October 29, 2020 00:04
src/rebar_compiler.erl Outdated Show resolved Hide resolved
src/rebar_compiler_dag.erl Outdated Show resolved Hide resolved
@paulo-ferraz-oliveira
Copy link
Contributor Author

paulo-ferraz-oliveira commented Oct 29, 2020

rebar_packages' get_package_repo_config will return a map that eventually contains key name. That key isn't part of hex_core's config. map. Even if it causes no issue, shouldn't the expected map be passed? Is this name expected, or should it actually be repo_name?

@paulo-ferraz-oliveira
Copy link
Contributor Author

paulo-ferraz-oliveira commented Oct 29, 2020

This is a rather curious one.

I found

            LogLvlPrimary = proplists:get_value(logger_level, KernelCfg, all),
            {FilterDefault, Filters} =
              case lists:keyfind(filters, 1, KernelCfg) of
                  false -> {log, []};
                  {filters, FoundDef, FoundFilter} -> {FoundDef, FoundFilter}

in rebar_utils.

dialyzer complains about the fact that lists:keyfind will not return the expected tuple, since we're using Kernel config. as a proplist just before. In any case, what's intriguing is that filters doesn't appear to be a proper config. option for kernel but for logger instead.

Edit: is case lists:keyfind(filters, 1, KernelCfg) of supposed to be case lists:keyfind(filters, 1, LogCfg) of instead?
Edit: I tentatively "solved" this to my understanding of the code.

@paulo-ferraz-oliveira paulo-ferraz-oliveira force-pushed the fix/dialyzer_warnings_over_otp_23 branch 3 times, most recently from 31e967d to d44ed3a Compare October 29, 2020 22:34
@paulo-ferraz-oliveira
Copy link
Contributor Author

As for the use of -dialyzer({nowarn_... we can also segregate the least amount of code we want to "not warn" about to a separate function and only use the directive on top of that one.

src/rebar_state.erl Outdated Show resolved Hide resolved
@paulo-ferraz-oliveira paulo-ferraz-oliveira force-pushed the fix/dialyzer_warnings_over_otp_23 branch 8 times, most recently from cca4b3d to f2a972b Compare October 30, 2020 00:45
@ferd
Copy link
Collaborator

ferd commented Nov 17, 2020

So I merged the vendoring of 0.7.0; this does cause an incompatibility here since the vendoring file was switched (but the moving of files was desirable) which in turn makes a conflict with this branch this time around.

@paulo-ferraz-oliveira
Copy link
Contributor Author

@ferd, thanks. I'll rebase and use the new hex_core version when I have time. Probably tonight.

@paulo-ferraz-oliveira paulo-ferraz-oliveira force-pushed the fix/dialyzer_warnings_over_otp_23 branch 2 times, most recently from 81331b4 to 7b248a5 Compare November 17, 2020 22:10
@paulo-ferraz-oliveira paulo-ferraz-oliveira force-pushed the fix/dialyzer_warnings_over_otp_23 branch 2 times, most recently from e16bc0d to d2201fd Compare November 17, 2020 22:27
We adapt .gitignore to the new vendored folder, also
@paulo-ferraz-oliveira paulo-ferraz-oliveira force-pushed the fix/dialyzer_warnings_over_otp_23 branch from d2201fd to e871da9 Compare November 17, 2020 22:29
@paulo-ferraz-oliveira
Copy link
Contributor Author

paulo-ferraz-oliveira commented Nov 17, 2020

Is it acceptable to add dialyzer into GitHub Actions? (I'm not used to that system yet, though)
Edit: as it stands (current pull request status) there's no dialyzer issues.

@ferd
Copy link
Collaborator

ferd commented Nov 17, 2020

Yeah we could add dialyzer now that we've taken control of things, but it needs to only run on the last version because we're gonna have a hard time otherwise.

@ferd ferd merged commit 8adc139 into erlang:master Nov 17, 2020
@paulo-ferraz-oliveira paulo-ferraz-oliveira deleted the fix/dialyzer_warnings_over_otp_23 branch November 18, 2020 00:46
@nickelization
Copy link

This is a rather curious one.

I found

            LogLvlPrimary = proplists:get_value(logger_level, KernelCfg, all),
            {FilterDefault, Filters} =
              case lists:keyfind(filters, 1, KernelCfg) of
                  false -> {log, []};
                  {filters, FoundDef, FoundFilter} -> {FoundDef, FoundFilter}

in rebar_utils.

dialyzer complains about the fact that lists:keyfind will not return the expected tuple, since we're using Kernel config. as a proplist just before. In any case, what's intriguing is that filters doesn't appear to be a proper config. option for kernel but for logger instead.

Edit: is case lists:keyfind(filters, 1, KernelCfg) of supposed to be case lists:keyfind(filters, 1, LogCfg) of instead?
Edit: I tentatively "solved" this to my understanding of the code.

I ran into this behavior and interpreted it as a bug, because I agree that it should be getting filters from the logger config, not the kernel config. I was going to file a PR but am glad to see it's already been fixed -- thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dialyzer'ing rebar3
5 participants