Fix DelayedEvaluator's with negative arity

[mal]

Description

DelayedEvaluator procs utilising a *rest param get a negative arity value, this causes them to be evaluated in the wrong context, checking against negative arity fixes this.

This functionality is useful when extending the DSL in cookbooks to support other kinds of DelayedEvaluators. I've included the real world example that lead to discovering this case under the fold in case there's an error in approach that invalidates this use case.

Custom DelayedEvaluator example

class MemoizedDelayedEvaluator < Chef::DelayedEvaluator
  Empty = BasicObject.new

  def self.new(&proc)
    memo = Empty
    super() do |resource, *args|
      if memo == Empty
        memo = if proc.arity > args.size || ~proc.arity > args.size
          proc.call(resource, *args)
        else
          resource.instance_exec(*args, &proc)
        end
      end
      memo
    end
  end
end

Reproduction

log 'this works now' do
  message lazy { |a, b| a.level.to_s }
  level :warn
end
# [2018-07-07T19:12:37+01:00] WARN: warn

log 'this is fixed by this pr' do
  message lazy { |a, *b| a.level.to_s }
  level :warn
end
# [2018-07-07T19:12:47+01:00] FATAL: NoMethodError: undefined method `level' for nil:NilClass

[mal]

Travis failure appears unrelated.

[coderanger]

I'm not really seeing the use case here, I'm not even sure why we support the non-instance_eval path there at all. Can you be more specific?

[mal]

Based on my reading of the tests the non-instance_eval path is to support cases where the Proc needs to access both resource methods/properties and those from the scope it was constructed in. As to the specifics of why that's supported, I'm not sure, but it kinda makes sense.

The use case for this change is shown by example in the MemoizedDelayedEvaluator code I provided. In order to maintain sanity and have it keep API compatibility with DelayedEvaluator while still inserting the intermediate memoization step, it builds a Proc mirroring the logic inside Property#exec_in_resource (so it can execute the internal DelayedEvaluator in the correct context, before memoization) and as a result finds itself needing to handle an arbitrary number of params as that code does, which is not currently possible due to the incomplete arity check this PR seeks to remedy.

[coderanger]

So I wouldn't implement your feature as a modification on the DelayedEvaluator, I would make a Property subclass and implement the logic there, since that has complete control over the system. That's worked nicely into the DSL, property :foo, MemoizedProp or similar I think should work (I can double check the code later, but it's something like that).

I'm also not sure we really need explicit support for funky context spanning stuff when you can just as easily do this = self and close over that.

[mal]

So I wouldn't implement your feature as a modification on the DelayedEvaluator, I would make a Property subclass and implement the logic there, since that has complete control over the system.

I'm not sure (or at least don't understand) if that would achieve the same thing I'm aiming for, the benefit of it being a subclass of DelayedEvaluator is that it can be used as a drop in replacement in any resource without needing to customise the resource to take a special property i.e.

class Chef::Resource
  def late(&block)
    MemoizedDelayedEvaluator.new(&block)
  end
end

[...]

file '/tmp/example' do
  content late { non_trivial_workload_that_will_only_run_once }
end

I'm also not sure we really need explicit support for funky context spanning stuff when you can just as easily do this = self and close over that.

I don't necessarily disagree, I'm just trying to support the behaviour that already existed.

[mal]

I still think this is technically a bug, but I've worked around it now by convincing myself that the *args passed to Property#exec_in_resource is only used during Property#coerce calls and thus can be ignored in the small recreation of it I need to correctly bind the block.

Obviously this is going to be somewhat flimsy since it relies on duplicating a small about of logic from a private method inside the Property class, but I figured I'd put it here in case some else finds themselves trying to do the same thing:

class Chef::Resource
  def late(&block)
    memo = Chef::NOT_PASSED
    lazy do |r|
      if memo == Chef::NOT_PASSED
        memo = block.arity > 0 ? block.call(r) : r.instance_exec(&block)
      else
        memo
      end
    end
  end
end

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.