matthewrudy / memoist

ActiveSupport::Memoizable with a few enhancements
MIT License
920 stars 99 forks source link

0.13 - 0.14 upgrade breaks when calling memoized_methods from a child class #51

Closed januszm closed 7 years ago

januszm commented 8 years ago

After upgrading from 0.13 to 0.14 on Rails 4.2.7.1 app I get those errors from a child class of:

class AbstractService
  extend Memoist

module:

undefined method 'memoized_methods' for AbstractService:Class

adding require 'memoist' at the top of the file does not resolve the issue.

matthewrudy commented 8 years ago

@januszm can you explain what happens?

The basics definitely work for me.

januszm commented 8 years ago

Funny, on Rails 5.0.0.1 it doesn't even know the Memoist module and I can't require 'memoist' , it says no such file.

Anyway, on Rails 4.2.7.1 I get this in a call to flush_cache method.

class AbstractService
  extend Memoist

  def run
    flush_cache if respond_to?(:flush_cache)
matthewrudy commented 8 years ago

I think something else must be at play.

Can you try turning off backtrace silencers

# /config/initializers/backtrace_silencers.rb
Rails.backtrace_cleaner.remove_silencers!

And see where the actual error comes from

januszm commented 8 years ago

Now it shows line 102 in lib/memoist.rb

# an ancestor method.
ancestors.grep(Memoist).each do |ancestor|
  ancestor.memoized_methods.each do |m|
    structs << m unless structs.any? {|am| am.memoized_method == m.memoized_method }
  end
end
januszm commented 8 years ago

hmm, yeah this piece of code was added between v0.13.0 and v0.14.0 in 29fa0f01, how about this dirty quick fix?

ancestor.memoized_methods.each do |m|
  structs << m unless structs.any? {|am| am.memoized_method == m.memoized_method }
end if ancestor.respond_to?(:memoized_methods)
januszm commented 8 years ago

@matthewrudy just to be clear, I ommitted this before. I get this error from a child (SomeService) class that inherits from the AbstractService. Maybe this is what's happening:

  1. Parent class extends Memoist module but doesn't make a call to memoize
  2. Child class calls memoize_methods on its ancestor (which is Parent class)

And here we get the undefined method error because this portion of code wasn't executed:

def memoize(*method_names)
  ...
  Memoist.memoist_eval(self) do
    def self.memoized_methods

because there's no call to memoize in the Parent, AbstractService class in my case.

Anyway, the current implementation makes using inheritance harder. I don't have a need to memoize any method in the Parent class but I do want to DRY-up my code by calling extend Memoist once.

matthewrudy commented 8 years ago

@januszm that makes sense.

So I guess we can reproduce with a test.

matthewrudy commented 8 years ago

I tried adding this test and its passes

def test_memoization_where_superclass_doesnt_call_memoize
    a = Class.new
    a.extend Memoist
    b = Class.new(a) do
      def foo
        @foo_count ||= 0
        @foo_count += 1
        "foo #{@foo_count}"
      end
      memoize :foo
    end

    obj = b.new
    2.times do
      assert_equal "foo 1", obj.foo
    end
  end
januszm commented 8 years ago

Here's my example that fails:

class Abb
  extend Memoist

  def run(*args)
    flush_cache if respond_to?(:flush_cache)
    execute
  end

  def some_method
    # Override this
  end
end  

class Bbb < Abb

  def some_method
    :foo
  end
  memoize :some_method
end

x = Bbb.new
x.run

NoMethodError: undefined method `memoized_methods' for Abb:Class
from ...gems/memoist-0.14.0/lib/memoist.rb:100
januszm commented 8 years ago

It works if I remove the call to flush_cache so this is the place where problems arise

matthewrudy commented 8 years ago

@januszm I get it.

Actually I guess it's enough to say

class Foo
  extend Memoist
end

Foo.new.flush_cache

Let me work out why we don't define memoized_methods when we extend

ccmcbeck commented 7 years ago

@matthewrudy - any progress? I have several class methods on a Device model that I memoize then use Device.flush_cache in an RSpec config.before block that produces undefined method 'memoized_methods'

PikachuEXE commented 7 years ago

I got the same issue too

PikachuEXE commented 7 years ago

Submitted a fix in #68