inaka / elvis_core

The core of an Erlang linter
Other
61 stars 56 forks source link

`state_record_and_type` not considering `opaque` as a type declaration #349

Closed paulo-ferraz-oliveira closed 1 month ago

paulo-ferraz-oliveira commented 3 months ago

Bug Description

I have a state, in a gen_server declared as a record.

I have to export it for export_used_types (it's in the public interface).

If I try to make it opaque, I get This module implements an OTP behavior and has a 'state' record but is missing a 'state()' type.

Should it be "wrong" (for Elvis) to have

-record(state, {
}).
-opaque state() :: #state{}.
-export_type([state/0]).

?

To Reproduce

Check Description.

Expected Behavior

opaque should be handled the same as type.

rebar3 Logs

Not relevant.

Additional Context

Not relevant.

elbrujohalcon commented 3 months ago

I have to export it for export_used_types (it's in the public interface).

This is weird. Why do you need to export a gen_server internal state? 🤔

I think this all comes down to this ☝🏻 . It's not usual to export the internal state of a behaviour implementation. With that in mind, if there is a good reason to do so… That (in my mind) would be the perfect example for when -elvis is needed. It's not that the rules are wrong, it's just that this is a very very uncommon situation.

paulo-ferraz-oliveira commented 3 months ago

Why do you need to export a gen_server internal state?

I don't, but https://github.com/inaka/elvis_core/blob/main/doc_rules/elvis_style/export_used_types.md does 😄 So the bug is basically that the rules are "incompatible" if applied at the same time.

elbrujohalcon commented 3 months ago

I see… The type needs to be exported because it's used in the spec of a callback function. So, yeah… I agree… state_record_and_type should not warn you if you declare an opaque type, instead of a public one.