memoize fails to handle default parameter = false

Is anyone else experiencing this behavior?

We've used patterns like the following to allow caching of computed nil and false results:

def cached_value

(@cached_value ||= [ computed_value ]).first

end def computed_value

something.that.might.return.false

end

Does memoize use false (and/or nil) as the "uncached" sentinel value? If it does, it's very broken.

Gah.. that should have been formatted as:

def cached_value
  (@cached_value ||= [ computed_value ]).first
end
def computed_value
  something.that.might.return.false
end

Here's fix for activesupport/lib/active_support/memoizable.rb:

module ActiveSupport
  module SafelyMemoizable
    def safely_memoize(*symbols)
      symbols.each do |symbol|
        class_eval <<-RUBY, __FILE__, __LINE__ + 1
          def #{symbol}(*args)
            memoized = @_memoized_#{symbol} || ::ActiveSupport::ConcurrentHash.new
            (memoized[args] ||= [ memoized_#{symbol}(*args) ]).first
          end
        RUBY
      end
    end
  end

Also, should:

memoized = @_memoized_#{symbol} || ::ActiveSupport::ConcurrentHash.new

be:

memoized = (@_memoized_#{symbol} ||= ::ActiveSupport::ConcurrentHash.new)

???

There also a problem if method being memoized is named "memoized". :) The "memoized" local variable in the code template should be gensym'ed.

The elements in args are not protected from side-effects either so something like:

  a = [ 1, 2 ]
  puts memoized_method(a)
  a[0] = 5
  puts memoized_method(a)

will likely have undetermined effects. Cache keys must be immutable, perhaps use:

  memoized[Marshal.dump(args)] ||= ...

at a great performance cost? It's a tough problem to solve.

Caveat Emptor...

Well, actually I don't think my problem has to do with the result of the memoized method necessary, but more with the fact that the value of the default parameter is 'false'. It seems to be ignoring if I pass in 'true' as this parameter. It will always just default it to false. Here's a better example that doesn't use a false return value:

def some_method(param1, param2 = false)
  return param2 ? param1 : "FALSE"
end
memoize :some_method

Then when you call the method:

>> some_method("TRUE", false)
=> "FALSE"

>> some_method("TRUE", true)
=> "FALSE"

You get "FALSE" both times, when the second time should have resulted in "TRUE". Does this make more sense?

This is why:

            def #{symbol}(*args)                                                   #   def mime_type(*args)
              #{memoized_ivar} ||= {} unless frozen?                               #     @_memoized_mime_type ||= {} unless frozen?
              reload = args.pop if args.last == true || args.last == :reload       #     reload = args.pop if args.last == true || args.last == :reload
                                                                                   #

The memoized wrapper assumes that a trailing true or :reload means "reload". Having the memoized wrapper try to handle a "meta argument" is a bad idea.

There should probably be a separate method to handle reload requests.

Not sure why methods of different aritys have different caching code.

To fix this for your calling sequence would require removal of the "reload" magic. IMO, the reload magic is a really bad API idea, but it would require everyone else to stop using it.

Kurt, Thanks for the explanation. I'll try to work around it for now.