rubocop / rubocop-rails

A RuboCop extension focused on enforcing Rails best practices and coding conventions.
https://docs.rubocop.org/rubocop-rails
MIT License
819 stars 263 forks source link

Cop idea: AR use `none?`/`empty?` instead of `!exists?` without conditions #888

Open tagliala opened 1 year ago

tagliala commented 1 year ago

⚠️ Disclaimer: please double check if this proposal makes sense

Is your feature request related to a problem? Please describe.

A new cop to check for the usage of none? instead of !exists? when no conditions are given.

Describe the solution you'd like

The cop should suggest the following change:

-!Task.pending.exists?
+Task.pending.empty? # or .none?

This should not be triggered if exists? has parameters, because empty? and none? do not support arguments

Additional context

I've tried to read the source code of AR, and it appears that none? and !exists? are not the same, because empty? will avoid an extra SELECT 1 AS one query if the records are already loaded

I've tested if there are performance differences with:

class User < ActiveRecord::Base
  has_many :addresses
end

class Address < ActiveRecord::Base
  belongs_to :user
end
%i[ips memory].each do |benchmark|
  Benchmark.send(benchmark) do |x|
    x.report('empty?') { User.includes(:addresses).first.addresses.empty? }
    x.report('none?') { User.includes(:addresses).first.addresses.none? }
    x.report('!exists?') { !User.includes(:addresses).first.addresses.exists? }
    x.compare!
  end
end
Calculating -------------------------------------
              empty?    763.109  (± 1.4%) i/s -      3.825k in   5.013383s
               none?    758.504  (± 1.8%) i/s -      3.825k in   5.044711s
            !exists?    646.310  (± 2.9%) i/s -      3.234k in   5.008159s

Comparison:
              empty?:      763.1 i/s
               none?:      758.5 i/s - same-ish: difference falls within error
            !exists?:      646.3 i/s - 1.18x  (± 0.00) slower

Calculating -------------------------------------
              empty?   101.238k memsize (     0.000  retained)
                         1.053k objects (     0.000  retained)
                        19.000  strings (     0.000  retained)
               none?   101.238k memsize (     0.000  retained)
                         1.053k objects (     0.000  retained)
                        19.000  strings (     0.000  retained)
            !exists?   110.622k memsize (   392.000  retained)
                         1.219k objects (     5.000  retained)
                        25.000  strings (     1.000  retained)

Comparison:
              empty?:     101238 allocated
               none?:     101238 allocated - same
            !exists?:     110622 allocated - 1.09x more

In case the association is not preloaded (same scenario without includes(:addresses)), for this use case, the result is the same

Calculating -------------------------------------
              empty?      2.761k (± 1.3%) i/s -     13.974k in   5.062562s
               none?      2.743k (± 2.7%) i/s -     13.850k in   5.053566s
            !exists?      2.742k (± 1.3%) i/s -     13.800k in   5.033780s

Comparison:
              empty?:     2760.7 i/s
               none?:     2742.9 i/s - same-ish: difference falls within error
            !exists?:     2742.0 i/s - same-ish: difference falls within error

Calculating -------------------------------------
              empty?    17.478k memsize (   392.000  retained)
                       292.000  objects (     5.000  retained)
                        18.000  strings (     1.000  retained)
               none?    17.478k memsize (   392.000  retained)
                       292.000  objects (     5.000  retained)
                        18.000  strings (     1.000  retained)
            !exists?    17.758k memsize (   392.000  retained)
                       299.000  objects (     5.000  retained)
                        18.000  strings (     1.000  retained)

Comparison:
              empty?:      17478 allocated
               none?:      17478 allocated - same
            !exists?:      17758 allocated - 1.02x more
koic commented 1 year ago

Note, It can be changed from !Model.exists? to Model.none? when a direct method call is made to model class, but cannot be changed to Model.empty?.

Model.empty? #=> undefined method `empty?'
tagliala commented 1 year ago

Also:

user = User.new
user.posts.new

!user.posts.exists? => true user.posts.none? => false

tagliala commented 10 months ago

Heads-up

none? is not the same as !exists?. It is optimized and will not perform a query everytime