Prevent dialyzer warnings on OTP 23

[paulo-ferraz-oliveira]

Found these while dialyzer'ing rebar3.

Opening as a draft pull request, since I don't know when this'll be ready for review, but it's not now.

[paulo-ferraz-oliveira]

I'm having a hard time getting rid of these

src/hex_pb_package.erl
1395: The pattern <X, Path> can never match since previous clauses completely covered the type <integer(),['Package' | 'Release' | 'RetirementStatus' | 'reason' | 'releases' | 'retired',...]>

src/hex_registry.erl
  83: Function sign_protobuf/2 has no local return
  85: The call hex_pb_signed:encode_msg(#{'payload':=binary(), 'signature':=binary()}, 'Signed') will never return since the success typing is (#{'payload':=binary() | maybe_improper_list(binary() | maybe_improper_list(any(),binary() | []) | byte(),binary() | [])}, 'Signed') -> binary() and the contract is ('Signed'(), atom()) -> binary()

I bumped rebar3_gpb_plugin (to latest) and updated the generated files, but it made no difference.

The first issue relates to an internal function, so it seems like a generation issue.
The second issue relates to the fact we're using optional and not required in the proto message (does the message name make sense?)

[paulo-ferraz-oliveira]

I opened tomas-abrahamsson/gpb#188 to get more info. and figure out if there's a better way to proceed.

[paulo-ferraz-oliveira]

According to tomas-abrahamsson/gpb#188 (comment), hex_core would benefit from changing 17 to 19 in https://github.com/hexpm/hex_core/blob/master/rebar.config#L14 (in terms of dialyzer -related warnings). I don't know, however, if this is desired by maintainers (e.g. there's no minimum_otp_vsn in rebar.config), since I found no indication of the support versions you're aiming for.

Note: there's still a potential issue there, regarding the generation of specs for maps, though, so this would also benefit from waiting for that "fix".

[wojtekmach]

hex_core needs to support OTP 17. This is because it needs to be usable by Hex which in turns needs to be usable by Elixir 1.0.x. At some point Hex will no longer support old Elixir versions but not quite yet.

[wojtekmach]

What are other options to suppress the warnings, is there anything we can do in rebar.config? E.g. strip the specs (tell gpb not to generate them)? Tell dialyzer to ignore these files?

[paulo-ferraz-oliveira]

I'm not sure it's possible to tell dialyzer to not issue warnings on specific files (or do anything with rebar.config, in that sense). I know rebar3 has an option that's kinda for that (exclude_mods - ?) but haven't tried it with hex_core yet (IIRC, the last time I used it it seemed to remove the module from the analysis altogether which would mean that calls to it would provoke undefined_function_calls).

I don't think stripping the specs would help here either, since at least one of the errors has nothing to do with the specs themselves, but with the fact that the dialyzer detects certain parts of the code will never be called. Since the issue is in the vendored modules (and since you have to support OTP 17+), is there any possibility that they're generated with a different target_erlang_version, for example?

[wojtekmach]

is there any possibility that they're generated with a different target_erlang_version, for example?

I'm not sure. Do you have a proposal how this would work while keeping the compatibility?

I can think of two workarounds:

1. we generate two sets of modules, one compatible with OTP 17 and other with something more modern, and when using the library we somehow pick one or the other, but not sure how exactly
2. each *.pb.erl contains both versions of the generated module guarded by an -ifdef. (perhaps an old compiler wouldn't accept that anyway)

neither approach seems good, it adds bloat to the package and may increase maintenance burden, so I don't think I'd pursue that.

[paulo-ferraz-oliveira]

I'm not sure. Do you have a proposal how this would work while keeping the compatibility?

Not yet, no. I'll try to find the time to cook something up. In any case, I'm specifically talking about the vendored modules, which are simply generated and included somewhere else (much like rebar3 does). So my proposal goes to simply try and adding an input arg. to vendor.sh and work from there.

[paulo-ferraz-oliveira]

72803c6 adds the possibility to generate different Erlang-targeted gpb consumables without breaking interface. This will allow vendored modules to be created for a given target version (when included in rebar3 they'll prevent certain warnings from happening, for example).

Note: this would effectively conclude this pull request, since I don't think it'll be possible to reach 0-warnings given the current expected constraints.

[paulo-ferraz-oliveira]

Do you think anything's still missing? Could I propose a merge and release, if all's OK with you?

[wojtekmach]

we'll be changing this type in the future (#77) but it's probably ok to export it since we use it elsewhere, otherwise we get the warnings.

[wojtekmach]

Thank you!

[paulo-ferraz-oliveira]

Thank you. Any change this gets a release soon, so it's vendored into rebar3?