dry-auto_inject 0.4.6 can lose values imported into both base and derived classes

[dmaze]

We have a project with a complex class hierarchy that uses a dry-auto_inject importer in a base class, and also uses it in a derived class. If the base class (or anything else in the class hierarchy) has a non-default initialize method, and both the base and derived class try to import the same names, the imported value will get set to nil.

class Base
  include Import[:name]

  def initialize(*args)
    super
  end
end

class Derived < Base
  include Import[:name]
end

puts Derived.new.name

https://gist.github.com/dmaze/1dff0e3db045157b5fab9dd90437f2b9 is a more complete reproduction.

This was not a problem in dry-auto_inject 0.4.5; we hit it on an upgrade. It is a very reasonable workaround to just remove the duplicate import from the derived class.

If I'm reading the kwargs strategy code correctly, the generated .new fills in keyword parameters from the container, then that gets passed to #initialize. At https://github.com/dry-rb/dry-auto_inject/blob/master/lib/dry/auto_inject/strategies/kwargs.rb#L63 any kwarg that's consumed by this class gets stripped from the parameter list before getting passed on to the base class, so when Base#initialize finally gets run the parameter that was injected in the derived class now isn't in the parameters and the instance variable gets set to nil. In commit d330299 the order of "set the injected variables" and "call super" got swapped, so before that commit the base class would set the variable to nil and then the derived class would set it to the correct value.

[timriley]

Thanks for reporting this, @dmaze, and for taking the time to dive into the cause.

I definitely consider this a "bug" and want to make sure we fix it (hopefully while keeping the extra flexibility that d330299 provided).

I might not get time to look at this right away, but I just wanted to let you know that it will get taken care of.