eproxus / meck

A mocking library for Erlang
http://eproxus.github.io/meck
Apache License 2.0
815 stars 231 forks source link

Fix a dialyzer-issued warning #223

Closed paulo-ferraz-oliveira closed 3 years ago

paulo-ferraz-oliveira commented 3 years ago

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

eproxus commented 3 years ago

Are you using different Dialyzer flags perhaps?

eproxus commented 3 years ago

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

paulo-ferraz-oliveira commented 3 years ago

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:

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 commented 3 years ago

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 commented 3 years ago

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

paulo-ferraz-oliveira commented 3 years ago

(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 commented 3 years ago

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

paulo-ferraz-oliveira commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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

paulo-ferraz-oliveira commented 3 years ago

As per @michalmuskala' help to find where the bug was fixed, here it is: https://github.com/erlang/otp/commit/5040a7ad7fa41fa616add4c983c1677fcd93bc7c.

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

paulo-ferraz-oliveira commented 3 years ago

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