Question about inheritance

[solnic]

Question about inheritance

[dkubb]

Based on a quick review of the code it doesn't look like attributes defined in a superclass are propagated down to descendant classes.

The behaviour that DataMapper has is that at the time the class is inherited the attributes are copied to the subclass (via the inherited hook). Attributes defined in the descendant are not propagated up to the superclass. However, attributes added to the superclass after inheritance are propagated down to all it's descendant classes. The reason we do this is to handle cases where the class is reopened and new attributes added directly or through mixins.

I'm not sure if @solnic plans to maintain this behaviour, but I wanted to bring it up just in case.

[trans]

Ah, ok. So you do propagate the the attributes to the descendants even after inheritance. That's great! I was looking for the code for that and didn't see it, so that's why I was asking. This works just as it should then.

In that case, the only thing that might be missing is propagation through modules. True? If so, see Ruby Facets class_extend method.

[trans]

Woops... I misread. So Virtus does NOT have it, but DataMapper does. Ok, that's a sure fixer-upper then.

[solnic]

Yes, I simply forgot to port that functionality. I'll think about how to approach that and eventually come up with an implementation. Ideas / suggestions / pull requests are welcomed :)

[dkubb]

@solnic well you'd have to track descendants for the information to be propagated down. We do that in DM, but our approach is probably overlkill. At this point we're trying to keep Virtus as small and simple as possible. In Veritas I have a really simple approach that would work fine for now: https://github.com/dkubb/veritas/blob/master/lib/veritas/attribute.rb#L32-46

Next it would be a matter of having the the inherited hook copy things to the descendant. After that you'd have attribute method iterate over the list of descendants and append the attribute object to the descendants.attributes list.

[trans]

I think it can be done in a more dynamic fashion. Anise works by always having the descendant look up their parent's info on the fly, so it's always in sync, but as I mentioned before that's kind of slow, b/c every time someone goes to access that information it has to re-collate it. However, it might be possible to store a checksum that the descendant gets a copy of from the parent when it is created. Then it only needs to check that checksum each time to see if it's parent has changed --and only then recalc.

[emmanuel]

I'm a little confused, what would be the advantage of defining attributes in descendants dynamically vs propagating them statically via inherited the way DataMapper does it now?

[trans]

Usually it won't matter. But since Ruby is a dynamic language, it is possible for a parent class to change after a subclass has been created.

[trans]

Just occurred to me, btw, that the checksum is already in place, just use the #hash value of the parent, i.e. parent.attributes.hash.

[dkubb]

@trans I'm still unclear why a checksum is even needed. It seems much more straight forward to simply copy the attribute to each descendant in Virtus::ClassMethods#attribute by doing:

descendants.each { |descendant| descendant.attributes[name] = attribute }

Of course we'd need a Virtus::ClassMethods#inherited hook that copies the attributes at the point of inheritance, like this:

def descendants
  @descendants ||= []
end

def inherited(descendant)
  # track the descendant
  superclass.inherited(descendant) unless superclass.equal?(::Object)
  descendants.unshift(descendant)

  # copies the attributes into the descendant
  descendant.attributes.update(attributes)
end

And that's pretty much it I think. Is there a simpler way to propagate the Attribute object I'm not seeing?

[emmanuel]

One thing that 'statically' propagating (via inherited) makes more complicated, perhaps, is removal of an attribute from the parent. That really depends on whether it's desirable for such removal to be reflected in the children, or not. It could still be handled via descendant-tracking, but might add a little more complexity.

That said, dynamically reflecting on the parent seems more complex, and I'm not sure what it offers that descendant tracking and propagation lacks.

[dkubb]

I would think removing an attribute from a descendant is a bad idea since it would break the Liskov substitution principle because a descendant would then have a different interface as an ancestor.

EDIT changed "behaviour" to "interface" to make it more clear what the true problem is.

Technically you can still remove attributes using Hash#delete although that ability is only a side effect of the implementation. I don't know if it's part of the contract, but I assume it is not. It's conceivable that another object could track attributes (like DataMapper::Property does) that only provides a subset of methods and does not have a #delete equivalent.

[trans]

Yeah, descendant tracking might be the way to go. I'm not sure which is best overall. I shied away from tracking descendants just b/c I wan't sure that was something that should be done. But really is it a big deal? Actually, why doesn't Ruby just track descendants out of the box?

But to clarify and contrast my idea is something along the lines of:

def self.inherited(descendant)
  descendant.attributes_hash = attributes.hash
  descendant.attributes = attributes.dup
end

def self.attributes
  if attributes_hash != superclass.attributes.hash
    @attributes.update(superclass.attributes)
  else
    @attributes
  end
end

Of course, not quite that simple, but it conveys the overall idea.

[trans]

There is also another important issue to consider in this: how to keep separation between the local attributes (of the descendant) and the superclass' attributes. If they are just being copied into the same local hash then they are getting mixed together and it's probably not possible to separate them. This could pose a problem if a descendant changes an attribute that it originally picked up from it's parent and then the parent subsequently changes it. Does the parent's override what the descendant changed? Probably not. So the descendants' must stay intact. But how to keep track of which is which? Moreover, might the parents' change be a kind that can be merged with the descendants at a finer grain level?

To avoid the problem some means of separation has to be maintained. That in turn leads to a tricky situation where reading and writing to the attributes doesn't simply act on the same hash. Reading must come form the combination of the local and parent attributes, while writing should only effect the local attributes, using COW from parent to local if a parent attribute is changed. A special "AttributesContainer" class might be needed to handle this.

Hope I explained that well enough.

[dkubb]

@trans I really am not sure why ruby doesn't track descendants. I think it should. When I originally wrote the code in DataMapper to do this (for the very same reason as we're discussing) I looked at how Module#ancestors worked, and I tried to write something that would work the same way. I even chose the method name "descendants" to be it's natural inverse.

I think your idea of using the superclass attributes could be a viable alternative.

The idea of the superclass changing an already defined attribute is interesting. I hadn't thought of that use case. It could certainly happen, especially in cases where people have moved attribute configuration out to several modules, which they include and their #included hooks add different attributes to the class (we encourage this kind of organization in DM actually). I suppose it's conceivable someone would have an attribute that is same named as another module, but perhaps with an extra distinction or tighter constraint.

I do think that ultimately we're going to end up having to have an object that handles tracking the list of attributes. A simple Hash won't cut it, and IMHO it exposes an interface that is too wide anyway.

In this scenario I think the only thing you could do is have the ancestor send a message to it's descendants telling them the old and new attribute objects, and letting them decide whether they want to update their attributes or not. They could check to see if their same-named attribute matches the old attribute, then they could replace it with the new object.

[emmanuel]

(not adding much to the discussion here, but...) in DM, this is currently handled by PropertySet, which has semantics and interface similar to Set from the standard lib, but which preserves order, and in PropertySet uniqueness is enforced by #name, rather than #hash.

[solnic]

I started with a hash of attributes but I do have in mind that DM uses PropertySet. I just didn't want to copy that code over here. We might want to come up with a common library for this / cc @snusnu

[trans]

@solnic So a new AttributeSet class then?

@dkubb Its not clear in my mind yet, but might the Attribute class have a #update_from_parent method to handle it, which could be overridden for special cases? So if an attribute exists by the same name in the descendant when a parent updates it, it's up to the attribute itself to decide how to integrate the change via this method. But it also needs to know whether it's changed since it was copied from the parent.

Taking a step back, I start to think dynamic lookup is less of a headache despite it being slower. Sure would have a much smaller memory foot print.

[dkubb]

@trans I was more thinking along the lines of having the AttributeSet object be the receiver of the message, and handling swapping the Attribute out if the "old" parameter matches something in the set.

There's another mechanism that could work too that is sort of inspired by @trans' suggestion:

Imagine we have an AttributeSet object, and it holds a reference to it's superclass' AttributeSet, all the way up to the base class which doesn't have a reference at all. When you add an attribute you only add it to the AttributeSet for that class. In a descendant, when someone asks for the attribute called :name, it first looks for it locally, then asks it's superclass for it if it can't find it. The superclass does the same kind of lookup if it can't find it, all the way up the chain. I guess this sort of mirrors inheritance, but I can't figure out a way to cleanly leverage ruby's own inheritance system to get the same behaviour (which would be my preference).

One thing we could do with this scheme is memoize the attribute when we find it, so we don't have to hunt all the way up the chain everytime a descendant has to do a lookup for a ancestor's attribute. We'd then need to have a way for the memoization to be cleared though in case a parent AttributeSet is updated.

While this may sound complex, I think this would be relatively simple to implement. It'll give us consistent behaviour, and I think it will handle all the edge cases we've talked about so far. The performance should be pretty good, except on a cache miss, but that'll be fairly limited -- once all the class loading is done and the mixins are applied, the performance should be consistent.

[trans]

Goldilocks (aka BGITYGI)

[dkubb]

@trans I have no idea what you're referring to :)

[dkubb]

This is what I was describing in my comment a few hours ago: https://gist.github.com/1047265

I hope it's relatively clear what it's doing from the code. If not please let me know and I'd be happy to either explain it, add some comments, or rename methods/refactor it until it makes sense.

EDIT: I tried to stick close to Set semantics with #<< and #merge, while also allowing the name based lookup using #[]. Hopefully it's not confusing with both approaches.

[trans]

By God I Think You Got It :)

Code is clear and a good start. I'll make some comments on the gist.

[dkubb]

Based on the comments here and in the gist I've created a pull request to add an AttributeSet: #8

A tiny bit more work will be necessary to add descendant tracking, and then use that inside AttributeSet and Attribute. Once that's done then a one liner will need to be added to Virtus::ClassMethods#attribute:

descendants.each { |descendant| descendant.attributes.reset }

[dkubb]

@trans your comments on code organization were really good. I've implemented most of your ideas on that pull request, but if you have other comments feel free to let me know.

[solnic]

I guess we can close this issue, right?

[dkubb]

@solnic yeah everything here is now handled in edge.