seattlerb / debride

Analyze code for potentially uncalled / dead methods, now with auto-removal.
https://www.zenspider.com/projects/debride.html
720 stars 19 forks source link

Exception thrown on class methods #29

Closed marcinruszkiewicz closed 7 years ago

marcinruszkiewicz commented 7 years ago

So we have a class that contains a class method.

class Test
  def self.testme
    puts "boom"
  end
end

Parsing this file with newest debride (1.7.0) returns the following:

/Volumes/Projects/debride/lib/debride.rb:402:in `block (2 levels) in report': undefined method `[]' for nil:NilClass (NoMethodError)
    from /Volumes/Projects/debride/lib/debride.rb:400:in `map'
    from /Volumes/Projects/debride/lib/debride.rb:400:in `block in report'
    from /Volumes/Projects/debride/lib/debride.rb:399:in `each'
    from /Volumes/Projects/debride/lib/debride.rb:399:in `report'
    from bin/debride:5:in `<main>'

This particular bit of a code searches for the method name in the method_locations hash, but our class method is saved there as {"Test::testme"=>"./test.rb:2"} and the code in line 402 looks for Test#testme and obviously doesn't find anything.

etagwerker commented 7 years ago

I got the same backtrace as @marcinruszkiewicz:

$ debride --rails app/models/user.rb
These methods MIGHT not be called:
/Users/etagwerker/.rvm/gems/ruby-2.3.1@ombu/gems/debride-1.7.0/lib/debride.rb:402:in `block (2 levels) in report': undefined local variable or method `byebug' for #<Debride:0x007fe97c767650> (NameError)
    from /Users/etagwerker/.rvm/gems/ruby-2.3.1@ombu/gems/debride-1.7.0/lib/debride.rb:400:in `map'
    from /Users/etagwerker/.rvm/gems/ruby-2.3.1@ombu/gems/debride-1.7.0/lib/debride.rb:400:in `block in report'
    from /Users/etagwerker/.rvm/gems/ruby-2.3.1@ombu/gems/debride-1.7.0/lib/debride.rb:399:in `each'
    from /Users/etagwerker/.rvm/gems/ruby-2.3.1@ombu/gems/debride-1.7.0/lib/debride.rb:399:in `report'
    from /Users/etagwerker/.rvm/gems/ruby-2.3.1@ombu/gems/debride-1.7.0/bin/debride:5:in `<top (required)>'
    from /Users/etagwerker/.rvm/gems/ruby-2.3.1@ombu/bin/debride:23:in `load'
    from /Users/etagwerker/.rvm/gems/ruby-2.3.1@ombu/bin/debride:23:in `<main>'
    from /Users/etagwerker/.rvm/gems/ruby-2.3.1@ombu/bin/ruby_executable_hooks:15:in `eval'
    from /Users/etagwerker/.rvm/gems/ruby-2.3.1@ombu/bin/ruby_executable_hooks:15:in `<main>'

I submitted https://github.com/seattlerb/debride/pull/30 in order to fix this problem. With those changes, I can see this output:

These methods MIGHT not be called:

User
  activate_user!                      app/models/user.rb:60
  active_for_authentication?          app/models/user.rb:66
  admin_created?                      app/models/user.rb:235
  ... 
  user_type                           app/models/user.rb:215
  wholesale=                          app/models/user.rb:58
zenspider commented 7 years ago

Odd... because I use this code on my client's codebase all the time and they're rife with class methods. I wonder how they differ

zenspider commented 7 years ago

Nope. Figured it out. My fault.

zenspider commented 7 years ago

Fixed. Thanks everyone!

marcinruszkiewicz commented 7 years ago

@zenspider unfortunately I'm afraid your small change to etagwerker's PR kind of broke it back again ;)

You do location = "klass#method" || "klass::method" but the first one will be truthy as it is a string, so the class method version won't be added

zenspider commented 7 years ago

@marcinruszkiewicz I'm not following. Here's @etagwerker's code in question:

        location = method_locations["#{klass}##{meth}"]
        location ||= method_locations["#{klass}::#{meth}"]

and here's mine:

        location =
          method_locations["#{klass}##{meth}"] ||
          method_locations["#{klass}::#{meth}"]

Where's the problem? Do you have a repro?

marcinruszkiewicz commented 7 years ago

Ah, sorry, I know what it is - the tests in the repo used 'require "debride"' so on my machine it loaded the old version still, not the updated one. And I'm not used to minitest, so I missed the -Ilib:test part of the command.

Sorry for my confusion before the first coffee of the day :)

zenspider commented 7 years ago

Hah. No worries. I frequently suffer from the same malady w/o coffee. 😄