crystal-ameba / ameba

A static code analysis tool for Crystal
https://crystal-ameba.github.io
MIT License
521 stars 38 forks source link

Replace `Performance/AnyInsteadOfEmpty` with `Style/EmptyInsteadOfPresent` #408

Open devnote-dev opened 1 year ago

devnote-dev commented 1 year ago

With https://github.com/crystal-lang/crystal/pull/13866 going through, I believe that Performance/AnyInsteadOfEmpty should be replaced with Style/EmptyInsteadOfPresent which would check for double negation in if/unless statements and unnecessary ! operations in ternaries. For example:

if !{...}.empty? # should be rewritten to `unless {...}.empty?`
# or `if {...}.present?` if there is an `else`
unless !{...}.empty? # ditto

I'm placing this specifically as a Style rule because it has no bearing on performance, as proven by the forum thread for AnyInsteadOfEmpty. Alternatively it could be a Lint rule but I'm not sure what that group is for.

devnote-dev commented 1 year ago

It would help if I got the names right... 😅

Sija commented 1 year ago

I believe two things should happen, once the above mentioned PR is merged:

  1. Rule Performance/AnyInsteadOfEmpty should become Style/AnyInsteadOfPresent
  2. Rule Style/EmptyInsteadOfPresent should be added Side note: there are cases in which the negated form is more natural - like in unless, so this new rule should take that into account
devnote-dev commented 1 year ago

If Style/AnyInsteadOfPresent is disabled by default then I'm fine with that. I don't know if Enumerable#any? is used commonly but I'd imagine it's a pain for people that also use Ameba in their project, they either have to refactor their code or disable the rule because at present, no semantic analysis is being done to determine that .any? is what Ameba interprets it to be.

Sija commented 1 year ago

@devnote-dev If it wasn't a pain already, then why would it be now? I don't know the numbers, but just looking on the amount of issues re: that - not too many projects are affected - which I find reasonable, since it's not that common to have #any? method defined.

devnote-dev commented 1 year ago

True, I say this because I ran into this issue a couple months ago and just renamed the method to get around it, but I don't think it's something Ameba should do principally.

Sija commented 1 year ago

In such a case I'd advise to simply exclude this rule on the project level (.ameba.yml) instead - perfectly proper thing to do (see Lint/NotNil as the common, purposeful, offender) :)

straight-shoota commented 1 year ago

Rule Performance/AnyInsteadOfEmpty should become Style/AnyInsteadOfPresent

There's also an option for Enumerable#any? (without param or block) to become deprecated. At that point this rule wouldn't be necessary I suppose? Or maybe it could provide automatic migration?

Sija commented 1 year ago

Automatic migration is certainly possible.

HertzDevil commented 1 year ago

The blockless #any? being callable most certainly implies #empty? is also callable, but note that crystal-lang/crystal#13866 is restricted to Enumerable and there are quite a few stdlib types that define #empty? but aren't Enumerable, like String and IO::Memory. Also in String's case !str.empty? is not equivalent to str.present? if it gains a #present? that has the same semantics as ActiveSupport.