whitequark / ast

A library for working with Abstract Syntax Trees.
MIT License
195 stars 25 forks source link

Adding/ removing child nodes in existing AST #4

Closed dblommesteijn closed 10 years ago

dblommesteijn commented 10 years ago

For a project I need some code duplication for a ruby file as described below. I'm using the parser project where it's using code from this repo. I seem to run into an issue with appending to an existing children array/ list. Perhaps someone can help me clarify how the AST::Node function updated works. Because I cannot seem to get the children list appended.

def some_function
  @a = "a"
end
def here_s_another_one
  @b = "b"
end
# result of duplicating child (what I want to achieve):
def some_function
  @a = "a"
end
def here_s_another_one
  @b = "b"
end
def some_function
  @a = "a"
end

I have found the AST::Node def updated() on line 121, but when I try to insert a copy of the example module A, It is not appended/ concatenated. Code snippet below:

file_contents = File.read(filename)
p = Parser::CurrentRuby.parse(file_contents)
src = p.children[0]
p.concat(src)
file_contents = Unparser.unparse(p)
File.write(filename, file_contents)

However, when I remove the children.to_a.freeze in the AST::Node def initialize() found here, I'm able to mutate the children list directly. Like shown below, without the modification it prints an error: can't modify frozen Array.

file_contents = File.read(filename)
p = Parser::CurrentRuby.parse(file_contents)
src = p.children[0]
p.children << src
file_contents = Unparser.unparse(p)
File.write(filename, file_contents)

PS. Without the freeze, the tests for AST seem to run perfectly (apart from should be frozen). So I wonder why you've decided to implement it like this.

mbj commented 10 years ago

AST::Node#updated returns an updated node. But does NOT modify in-place.

whitequark commented 10 years ago

Immutability of AST::Node is a deliberate design choice. Please see the documentation, as it answers the very questions you have.

I will close the issue, as freeze will stay there. If the documentation does not answer your questions, please comment further on this issue.

dblommesteijn commented 10 years ago

Thanks for answering my question. @whitequark is making freeze configurable an option?

whitequark commented 10 years ago

Out of question.

mbj commented 10 years ago

@whitequark full ack.

@dblommesteijn Its possible to implement all AST transforms that require mutable AST nodes with ones that work on immutable ones also. See the way the buildin AST processor works.

dblommesteijn commented 10 years ago

@mbj thanks, ill look into that.