change `singleton_class?` to a class method to allow compatibility with zeitwerk

kennyfrc commented 1 year ago

Hi there, thanks for this gem! As I was testing this, I bumped into a bug when it's used with zeitwerk.

Problem

When zeitwerk loads the Busy Class, it invokes singleton_class? method (see here. Because of the singleton_class monkey patch in core_ext.rb, it raises the below error.

Error Trace

e lockfile by running `gem install bundler:2.4.3`.
Traceback (most recent call last):
    39: from /Users/{$whoami}/.rvm/gems/ruby-2.7.5/bin/ruby_executable_hooks:22:in `<main>'
    38: from /Users/{$whoami}/.rvm/gems/ruby-2.7.5/bin/ruby_executable_hooks:22:in `eval'
    37: from /Users/{$whoami}/.rvm/gems/ruby-2.7.5/bin/rackup:23:in `<main>'
    36: from /Users/{$whoami}/.rvm/gems/ruby-2.7.5/bin/rackup:23:in `load'
    35: from /Users/{$whoami}/.rvm/gems/ruby-2.7.5/gems/rack-2.2.6.2/bin/rackup:5:in `<top (required)>'
    34: from /Users/{$whoami}/.rvm/gems/ruby-2.7.5/gems/rack-2.2.6.2/lib/rack/server.rb:168:in `start'
    33: from /Users/{$whoami}/.rvm/gems/ruby-2.7.5/gems/rack-2.2.6.2/lib/rack/server.rb:311:in `start'
    32: from /Users/{$whoami}/.rvm/gems/ruby-2.7.5/gems/rack-2.2.6.2/lib/rack/server.rb:379:in `handle_profiling'
    31: from /Users/{$whoami}/.rvm/gems/ruby-2.7.5/gems/rack-2.2.6.2/lib/rack/server.rb:312:in `block in start'
    30: from /Users/{$whoami}/.rvm/gems/ruby-2.7.5/gems/rack-2.2.6.2/lib/rack/server.rb:422:in `wrapped_app'
    29: from /Users/{$whoami}/.rvm/gems/ruby-2.7.5/gems/rack-2.2.6.2/lib/rack/server.rb:249:in `app'
    28: from /Users/{$whoami}/.rvm/gems/ruby-2.7.5/gems/rack-2.2.6.2/lib/rack/server.rb:349:in `build_app_and_options_from_config'
    27: from /Users/{$whoami}/.rvm/gems/ruby-2.7.5/gems/rack-2.2.6.2/lib/rack/builder.rb:66:in `parse_file'
    26: from /Users/{$whoami}/.rvm/gems/ruby-2.7.5/gems/rack-2.2.6.2/lib/rack/builder.rb:105:in `load_file'
    25: from /Users/{$whoami}/.rvm/gems/ruby-2.7.5/gems/rack-2.2.6.2/lib/rack/builder.rb:116:in `new_from_string'
    24: from /Users/{$whoami}/.rvm/gems/ruby-2.7.5/gems/rack-2.2.6.2/lib/rack/builder.rb:116:in `eval'
    23: from /Users/{$whoami}/Documents/code/work/{$path-to-demo}/shrine/demo/config.ru:1:in `block in <main>'
    22: from /Users/{$whoami}/Documents/code/work/{$path-to-demo}/shrine/demo/config.ru:1:in `require'
    21: from /Users/{$whoami}/Documents/code/work/{$path-to-demo}/shrine/demo/app.rb:3:in `<top (required)>'
    20: from /Users/{$whoami}/Documents/code/work/{$path-to-demo}/shrine/demo/app.rb:3:in `require'
    19: from /Users/{$whoami}/Documents/code/work/{$path-to-demo}/shrine/demo/routes/albums.rb:2:in `<top (required)>'
    18: from /Users/{$whoami}/Documents/code/work/{$path-to-demo}/shrine/demo/routes/albums.rb:2:in `require'
    17: from /Users/{$whoami}/Documents/code/work/{$path-to-demo}/shrine/demo/models/album.rb:2:in `<top (required)>'
    16: from /Users/{$whoami}/Documents/code/work/{$path-to-demo}/shrine/demo/models/album.rb:2:in `require'
    15: from /Users/{$whoami}/Documents/code/work/{$path-to-demo}/shrine/demo/uploaders/image_uploader.rb:4:in `<top (required)>'
    14: from /Users/{$whoami}/Documents/code/work/{$path-to-demo}/shrine/demo/uploaders/image_uploader.rb:4:in `require'
    13: from /Users/{$whoami}/Documents/code/work/{$path-to-demo}/shrine/demo/config/shrine.rb:9:in `<top (required)>'
    12: from /Users/{$whoami}/.rvm/gems/ruby-2.7.5/gems/zeitwerk-2.6.7/lib/zeitwerk/kernel.rb:38:in `require'
    11: from /Users/{$whoami}/.rvm/gems/ruby-2.7.5/gems/zeitwerk-2.6.7/lib/zeitwerk/kernel.rb:38:in `require'
    10: from /Users/{$whoami}/Documents/code/work/{$path-to-demo}/shrine/demo/jobs/attachment/promote_job.rb:1:in `<top (required)>'
     9: from /Users/{$whoami}/.rvm/gems/ruby-2.7.5/gems/zeitwerk-2.6.7/lib/zeitwerk/kernel.rb:38:in `require'
     8: from /Users/{$whoami}/.rvm/gems/ruby-2.7.5/gems/zeitwerk-2.6.7/lib/zeitwerk/kernel.rb:38:in `require'
     7: from /Users/{$whoami}/.rvm/gems/ruby-2.7.5/gems/sucker_punch-2.1.2/lib/sucker_punch.rb:3:in `<top (required)>'
     6: from /Users/{$whoami}/.rvm/gems/ruby-2.7.5/gems/zeitwerk-2.6.7/lib/zeitwerk/kernel.rb:38:in `require'
     5: from /Users/{$whoami}/.rvm/gems/ruby-2.7.5/gems/zeitwerk-2.6.7/lib/zeitwerk/kernel.rb:38:in `require'
     4: from /Users/{$whoami}/.rvm/gems/ruby-2.7.5/gems/sucker_punch-2.1.2/lib/sucker_punch/counter.rb:1:in `<top (required)>'
     3: from /Users/{$whoami}/.rvm/gems/ruby-2.7.5/gems/sucker_punch-2.1.2/lib/sucker_punch/counter.rb:2:in `<module:SuckerPunch>'
     2: from /Users/{$whoami}/.rvm/gems/ruby-2.7.5/gems/sucker_punch-2.1.2/lib/sucker_punch/counter.rb:17:in `<module:Counter>'
     1: from /Users/{$whoami}/.rvm/gems/ruby-2.7.5/gems/sucker_punch-2.1.2/lib/sucker_punch/counter.rb:18:in `<class:Busy>'
/Users/{$whoami}/.rvm/gems/ruby-2.7.5/gems/zeitwerk-2.6.7/lib/zeitwerk/explicit_namespace.rb:69:in `tracepoint_class_callback': private method `singleton_class?' called for SuckerPunch::Counter::Busy:Class (NoMethodError)
Did you mean?  singleton_class

Steps to reproduce with Zeitwerk

You can use the demo application located here and follow the steps. Running bundle exec rackup should throw the above error.

Proposed Fix

To avoid this from clashing with the instance method singleton_class? I propose to to move this to a class method.

Tests

All tests pass

bethesque commented 1 year ago

I came here with exactly the same request! This is a blocker for a large piece of work to upgrade to Ruby 3.0, and I would love to have this merged.

bethesque commented 1 year ago

I'm just having a look at what the code is meant to be doing. It's a simpler version of https://github.com/rails/rails/blob/94ca3e0a571dba0fe41ca18d61634c5f3aa11209/activesupport/lib/active_support/core_ext/class/attribute.rb

I've forked my own copy of sucker_punch, and unfortunately, there is no test that covers a positive use case for if singleton_class?, so I can't see how/if it is used in real life. My gut feel is that it's not used, give that the class_attribute method is only expected to be called in a class definition, but I can't say that with any authority!

Without having a test to show any failures, I would recommend renaming or inlining it, rather than making it a class method. This is because singleton_class? is a core ruby method (it probably wasn't when sucker_punch was first written - it isn't there in Ruby 2.4) and we really don't want to be overwriting core ruby methods.

bethesque commented 1 year ago

Btw, I'm not even sure if the singleton_class? method does what it is expected to. When I run the demo code from the core ruby singleton_class? example with the definition of singleton_class? from sucker_punch in Ruby 2.4 this is what I get:

class Class
  def singleton_class?
    ancestors.first != self
  end
end

class C
end
C.singleton_class?                  #=> false
C.singleton_class.singleton_class?  #=> should be true according to the demo, but is false