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

Fix a dialyzer-issued warning #223

Merged
merged 1 commit into from
Mar 3, 2021
Merged

Fix a dialyzer-issued warning #223

merged 1 commit into from
Mar 3, 2021

Conversation

paulo-ferraz-oliveira
Copy link
Contributor

I'm not exactly sure how this got by GitHub Actions without triggering a fail. (I got it while preparing for OTP 24)

@paulo-ferraz-oliveira paulo-ferraz-oliveira mentioned this pull request Feb 7, 2021
1 task
@eproxus
Copy link
Owner

eproxus commented Feb 17, 2021

Are you using different Dialyzer flags perhaps?

@eproxus
Copy link
Owner

eproxus commented Feb 17, 2021

Just did some other changes and I'm not getting warnings on GitHub Actions without this PR.

@paulo-ferraz-oliveira
Copy link
Contributor Author

paulo-ferraz-oliveira commented Feb 17, 2021

No different flags (using those from the project). But I was using OTP 24. Let me recheck this with a more recent OTP 24 version...

The flag that controls this warning is unmatched_returns (already present in meck), by the way.

Yup, still present (at least in OTP 24); let me recheck this with e.g. 23.2... no, not there. (this is where the PR must have been misleading, since I was under the impression I'd found this issue in a pre-23 version)

Recap:

  • will start getting a warning from OTP 24 (it the analysis remains as-is),
  • does not affect currently deployed CI for meck.

You decide if it's important to consider for now or leave it to handle later when we add 24 to CI.

@paulo-ferraz-oliveira
Copy link
Contributor Author

For reference, this is what I'm getting

src/meck_proc.erl
Line 599 Column 5: Expression produces a value of type [{'error','non_existing' | 
'not_main_node' | string() | {'encrypted_abstract_code',string()} |
{'no_abstract_code',string()} | {'no_file_attribute',string()} |
{'already_cover_compiled','no_beam_found',atom()}} | {'ok',atom()}] |
{'error','non_existing' | 'not_main_node' | string() | {'encrypted_abstract_code',string()} |
{'no_abstract_code',string()} | {'no_file_attribute',string()} |
{'already_cover_compiled','no_beam_found',atom()}} |
{'ok',atom()}, but this value is unmatched

(there be newlines here, for readability)


Line 599 Column 5 is from using a rebar3 version compatible with OTP 24's new {error_location, _}.

@paulo-ferraz-oliveira
Copy link
Contributor Author

@eproxus, don't close or merge this just yet. Let me confirm some stuff and report back here.

@paulo-ferraz-oliveira
Copy link
Contributor Author

(I'm thinking that either this was a hidden bug, now fixed, or the matching with Mod has some influence in the result - which would be fairly odd)

@paulo-ferraz-oliveira
Copy link
Contributor Author

Mod has no influence (oddity avoided). Removing unmatched_returns also removes the warning (as expected).

@paulo-ferraz-oliveira
Copy link
Contributor Author

I'm going to test my patience with git bisecting OTP to check where the bug was eventually fixed. If I don't reply back and/or you feel like making a move, I'm OK with whatever you decide. Thanks.

@paulo-ferraz-oliveira
Copy link
Contributor Author

Well, I couldn't find anything conclusive, but still get the warning on OTP 24. (compiling OTP locally, on a Mac, is sometimes painful 😢)

@eproxus
Copy link
Owner

eproxus commented Feb 19, 2021

Ok, let's leave this PR open and then if the warning shows up when OTP 24 is released we'll merge it?

@paulo-ferraz-oliveira
Copy link
Contributor Author

@eproxus, I'm OK with that (though it mostly postpones all our releases also, since we depend on meck and want a clean compilation flow - we can wait). I have been trying to compile/bisect/compile to figure where this comes from, but still haven't found it (keep running into compilation issues - now using Ubuntu). Just looking at the code it seems the warning must've been present before, but it isn't there (which is why I'm confused 😄).

@paulo-ferraz-oliveira
Copy link
Contributor Author

After a short offline conversation with @eproxus he actually realised this might have surfaced a bug, so I adapted "my" solution to:

  1. not be a "simple" dialyzer -based fix,
  2. make it more robust, fixing the bug.

@paulo-ferraz-oliveira
Copy link
Contributor Author

@eproxus, if you're OK with this would a new release be possible, to incorporate this change? Thanks much.

@paulo-ferraz-oliveira
Copy link
Contributor Author

As per @michalmuskala' help to find where the bug was fixed, here it is: erlang/otp@5040a7a.

cover got new -spec(_).s. 😄

@paulo-ferraz-oliveira
Copy link
Contributor Author

@eproxus, since this is no longer an OTP-24 -related issue, do you think it possible to merge and release? Thanks.

@eproxus eproxus merged commit b939af9 into eproxus:master Mar 3, 2021
@paulo-ferraz-oliveira paulo-ferraz-oliveira deleted the fix/static_analysis_warning branch March 4, 2021 03:00
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.

2 participants