Local properties are not propagated

[akimd]

Hi, There's a pattern that the library does not seem to support well: processors that label some nodes with properties. Indeed, when a parent node is updated with children equal in value, but different in properties, then we still use the original parent node instead of building it again with the new children. As a consequence we drop the property from the children and go back to the original node. That's the behavior of Node#updated

https://github.com/whitequark/ast/blob/2a878a2b5fb33524b03e9d5cd176a4980ed21a8a/lib/ast/node.rb#L138

which relies on @children == new_children, which does not take the properties into account.

The following toy example demonstrates the problem.

require 'ast'

class ArithmeticsProcessor
  include AST::Processor::Mixin
  def on_integer(node)
    node.updated(nil, nil, {line: 42})
  end

  def on_add(node)
    node.updated(nil, process_all(node))
  end
end

include AST::Sexp
expr = s(:add, s(:integer, 2), s(:integer, 2))
res = ArithmeticsProcessor.new.process(expr)
p res.children[0].instance_variables

When run, we get [:@children, :@type, :@hash], i.e., the line property of the integer was dropped.

Is this the intended behavior?

[iliabylich]

Right, that's because @children == new_children compares items (AST::Nodes in your case) using == that doesn't know about your custom attribute:

    def ==(other)
      if equal?(other)
        true
      elsif other.respond_to? :to_ast
        other = other.to_ast
        other.type == self.type &&
          other.children == self.children
      else
        false
      end
    end

Your should also check for line too:

require 'ast'

module LineAttribute
  attr_reader :line

  def ==(other)
    super && (self.line == other.line)
  end
end

class AST::Node
  prepend LineAttribute
end

class ArithmeticsProcessor
  include AST::Processor::Mixin
  def on_integer(node)
    node.updated(nil, nil, {line: 42})
  end

  def on_add(node)
    node.updated(nil, process_all(node))
  end
end

include AST::Sexp
expr = s(:add, s(:integer, 2), s(:integer, 2))
res = ArithmeticsProcessor.new.process(expr)
lhs, rhs = *res

p [lhs, rhs]
p [lhs.line, rhs.line]

prints

[s(:integer, 2), s(:integer, 2)]
[42, 42]

I think the reason why it doesn't check for all instance variables is that they are user-defined, and so users should specify which attributes participate in comparison. @whitequark is it true?

[akimd]

Yes, I can see that I should redefine the equality. My question is whether that was really intended. If it was, IMHO, the documentation should highlight it.