New Rule: Consistent Generic Type

[jackyhui96]

Closes #258

[paulo-ferraz-oliveira]

@jackyhui96, if you change your description to e.g. Closes #258, it automatically links to that issue.

[paulo-ferraz-oliveira]

I ask for a few tweaks but I like the overall direction and implementation.

[paulo-ferraz-oliveira]

I'm Ok with all the recent changes. I'll approve, assuming you'll eventually accept the only new suggestion I've added. I'll also add @elbrujohalcon as a reviewer so he can give this a look. Many thanks.

[elbrujohalcon]

I like the idea.
I think this rule is very similar to Behaviour Spelling and so I would expect its implementation, documentation, tests, etc. to be more similar to it..
That's the goal of all the suggestions I made.

Besides that, can you add more tests to showcase the different places where the type can be, like…

% Type definitions when this is alone or combined
-type my_type() :: any().
-type combined() :: term() | my_type().

% Callback definitions (with or without when)
-callback my_callback(term()) -> any().
-callback my_callback(X) -> X when X :: term().

% Specs with when
-spec my_spec(X) -> X when X :: any().

% Record definitions
-record(my_record, {t :: term(), a :: any()}).

…and can you add tests where this should not be validated, like these…

% A parametric type called any
-type any(Thing) :: Thing.

% A function called any
-spec any() -> term().
any() -> any.

% A function that calls the function called any
my_function() -> any().

[elbrujohalcon]

Sorry for the back and forth, @jackyhui96 … Multiple maintainers, multiple views on the project… 🤷🏻
If you want, before considering my suggestions, wait for @paulo-ferraz-oliveira to reply to my comments so we can agree on what our preference is.

[paulo-ferraz-oliveira]

In any case, the only meaningful request to change I see is the fact that the rule should not be restricted to term() or any(), everything else are details, for me, that I have no strong opinions on:

1. whether the rule is active by default or not (in which case it would need a default),
2. whatever the output message is, as long as it's easy to act on.

I agree with adding more tests.

[elbrujohalcon]

1. whether the rule is active by default or not (in which case it would need a default),

I agree not to activate the rule by default (i.e. not to put it in the default ruleset erl_files) but, at the same time, it can totally have a default so users can add it just like…

rules => [{elvis_style, whatever_name_it_ends_up_with, #{}}]

…without the need to specify a preferred value explicitly.

2. whatever the output message is, as long as it's easy to act on.

Yeah… easy to act on is fine, but consistent with other messages is important to me, too.

I agree with adding more tests.

🤝

[elbrujohalcon]

Alright @jackyhui96 … You have green light to move forward with the changes. Sorry again for the back-and-forth.

[paulo-ferraz-oliveira]

@elbrujohalcon, is the rule to be renamed as consistent_generic_type?

[paulo-ferraz-oliveira]

I'm summarising the requested changes below:

• rename the field prefer as preferred_type
• add a default value for that field (and update the doc. accordingly): the value should be term
• change message to Found usage of type ~p/0 on line ~p. Please use ~p/0, instead.
• add more tests as per New Rule: Consistent Generic Type #259 (review)
• doc. should state rule as "Consistent Generic Type": the file should be named consistent_generic_type.md and the rule consistent_generic_type for use.

[elbrujohalcon]

comment updated, @paulo-ferraz-oliveira ;)

[jackyhui96]

Currently it looks like the rule doesn’t apply to callbacks

[elbrujohalcon]

@jackyhui96 you mean that elvis_code:find/3 doesn't traverse callbacks?
If that's so, it's a bug and it's worth reporting it as such.

Or maybe elements within callbacks are not categorized as types… 🤔 … If that's so, same thing: it merits a bug report.

[elbrujohalcon]

@paulo-ferraz-oliveira I already approved the PR. I leave it up to you to merge it, if you're comfortable with it :)

[jackyhui96]

@jackyhui96 you mean that elvis_code:find/3 doesn't traverse callbacks? If that's so, it's a bug and it's worth reporting it as such.

Or maybe elements within callbacks are not categorized as types… 🤔 … If that's so, same thing: it merits a bug report.

Yeah it doesn't categorise them as types 😞 this is what the nodes look like for a callback:

#{attrs => #{location => {13,1},  text => "-"},           type => '-'},
#{attrs => #{location => {13,2},  text => "callback"},    type => atom, value => callback},
#{attrs => #{location => {13,11}, text => "my_callback"}, type => atom, value => my_callback},
#{attrs => #{location => {13,22}, text => "("},           type => '('},
#{attrs => #{location => {13,23}, text => "term"},        type => atom, value => term},
#{attrs => #{location => {13,27}, text => "("},           type => '('},
#{attrs => #{location => {13,28}, text => ")"},           type => ')'},
#{attrs => #{location => {13,29}, text => ")"},           type => ')'},
#{attrs => #{location => {13,31}, text => "->"},          type => '->'},
#{attrs => #{location => {13,34}, text => "term"},        type => atom, value => term},
#{attrs => #{location => {13,38}, text => "("},           type => '('},
#{attrs => #{location => {13,39}, text => ")"},           type => ')'},
#{attrs => #{location => {13,40}, text => ".\n"},         type => dot},

[elbrujohalcon]

Oooooh… that's bad. Do you mind filing a bug for that, @jackyhui96 ?

[jackyhui96]

@elbrujohalcon sure thing, is this a bug with elvis or with ktn_code?

[elbrujohalcon]

I would start with elvis_core… if upon further investigation it turns out it comes from ktn_code… we can then report a bug there.

[paulo-ferraz-oliveira]

@elbrujohalcon, I left a minor comment (hopefully the last 😄).

[paulo-ferraz-oliveira]

@jackyhui96, I'm merging... many thanks for this.