Block bodies in macros should always be Expressions

[willhbr]

Currently in macros, bodies of blocks of code can either be Expressions, Nop, or any other type of ASTNode - depending on how many expressions/ statements there are. This is the case for control flow, blocks, methods, etc. Iterating over the expressions is difficult as you have to check what type the body is before using it.

macro print_methods(&block)
  {% for method in block.body.expressions %}
    {% puts method.name %}
  {% end %}
end

print_methods do
  def foo
  end
  def bar
  end
end
# Prints:
# foo
# bar

print_methods do
  def foo
  end
end
# Undefined macro method 'Def#expressions'

This can be worked around, but the workaround gets tedious:

macro print_methods(&block)
  {% expressions = block.body.is_a?(Expressions) ? block.body.expressions : [block.body] %}
  {% for method in expressions %}
    {% puts method.name %}
  {% end %}
end

I think that the simplest / cleanest solution would be for the body to be an Expressions object, containing some number of nodes (zero or more). There could be a helper method to check if it is a no-op (ie it contains no elements in the list) which could replace the use of .is_a?(Nop).

Crystal version (pretty close to master)

$ crystal --version
Crystal 0.24.0+3 [9b1a9e7] (2017-11-01) 
LLVM: 3.8.0
Default target: x86_64-pc-linux-gnu
$ uname -a
Linux ed 4.4.0-92-generic #115-Ubuntu SMP Thu Aug 10 09:04:33 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux

[asterite]

If you want to pass one or many arguments to a macro, use a splat:

print_methods(
  def foo
  end,
  def bar
  end
)

I don't think we'll change macros to do what you want, it's counter-intuitive and expensive, and it changes what you send and what you print.

[RX14]

Why is it counter-intuitive for a block body to ever be an expressions of size 1?

[asterite]

macro foo(&block)
  {{ block.body.is_a?(If) }}
end

foo do
  if 1; 2; end
end

Wouldn't it be counter-intuitive that the above gives false?

[bew]

In you example @asterite I think it is counter-intuitive to have:

foo do
  if 1; 2; end
end
# Gives true


foo do
  if 1; 2; end
  if 3; 4; end
end
# Gives false

[RX14]

@asterite Absolutely not, it's counter-intuitive to me that that does work. And certainly less powerful.

"A block body is an Expressions unless it's only one expression" is a lot more complex and introduces a lot more complexity (when you don't have the power of visitor classes) then "A block body is always an Expressions".

[straight-shoota]

I agree. It seems only reasonable that a macro body is always Expressions. This keeps the API simple and concise. And you don't suddenly get funny behaviour just because you remove/add an unrelated line of code in a macro block.

[HertzDevil]

It would certainly break everything if we change methods like Def#body to return an Expressions at this moment; besides, these methods more or less reflect the internal structure of the AST node, and the parser doesn't wrap everything in Expressions nodes, for performance reasons.

Instead I think we should define ASTNode#expressions that has the same behaviour as the given workaround: continue to return the same thing in Expressions#expressions, return an empty ArrayLiteral in Nop#expressions, and return a 1-element ArrayLiteral containing the receiver for the other node types.

[straight-shoota]

@HertzDevil Sounds reasonable. However, I'm wondering if we really need this on every ASTNode type, or if it could be a method on nodes that have body expressions, i.e. Def#body_expressions. Conceptually, this might make more sense because it's in the context of its use case. I doubt there's much need for a generic method to wrap arbitrary nodes in Expressions.