Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add macro verbatim blocks #6108

Merged
merged 1 commit into from
May 20, 2018
Merged

Add macro verbatim blocks #6108

merged 1 commit into from
May 20, 2018

Conversation

asterite
Copy link
Member

(correct implementation of #6105... I should have not closed the other PR so fast :-P)

What

This adds the ability to specify verbatim blocks in macros, so macro code isn't immediately execute.

Why

Take a look at how the macro def_clone is implemented:

  macro def_clone
    # Returns a copy of `self` with all instance variables cloned.
    def clone
      clone = \{{@type}}.allocate
      clone.initialize_copy(self)
      GC.add_finalizer(clone) if clone.responds_to?(:finalize)
      clone
    end

    protected def initialize_copy(other)
      \{% for ivar in @type.instance_vars %}
        @\{{ivar.id}} = other.@\{{ivar.id}}.clone
      \{% end %}
    end
  end

Note all the \{% and \{{ in there. It's needed because we don't want to execute @type right away: we want to defer it until the method is executed, so it works like a macro method. The \ is ugly, but it works.

With this PR, we can write it like this:

  macro def_clone
    {% verbatim do %}
      # Returns a copy of `self` with all instance variables cloned.
      def clone
        clone = {{@type}}.allocate
        clone.initialize_copy(self)
        GC.add_finalizer(clone) if clone.responds_to?(:finalize)
        clone
      end

      protected def initialize_copy(other)
        {% for ivar in @type.instance_vars %}
          @{{ivar.id}} = other.@{{ivar.id}}.clone
        {% end %}
      end
    {% end %}
  end

verbatim basically won't treat the block inside it as macro code, but instead as just a string that's being pasted. Then when the clone and initialize_copy methods are invoked, the pasted code that's actually macro is executed.

So this is just a better way to write and do what we can already do.

I also need this for a complete implementation of #6082 . The JSON::Serialize module must inject an initialize in the included type so that types referenced in attributes (like converter) work well. For example:

class Foo
  class MyConverter
  end

  @[JSON::Field(converter: MyConverter)]
  property x : Int32
end

If MyConverter is pasted in the included JSON::Serialize module, it will give an error because MyConverter can't be reached from that location (that's the reason why I had to remove all the private types in the specs in that PR). If initalize is injected with an included macro, MyConverter can be accessed.

But since that injected initialize method already uses @type in macros, I'll have to escape with \ all of that. But it's so much simpler to just surround everything with a verbatim block.

@RX14 RX14 added this to the Next milestone May 20, 2018
@RX14 RX14 merged commit 06e763b into master May 20, 2018
@paulcsmith
Copy link
Contributor

This will be very helpful!

@asterite
Copy link
Member Author

@paulcsmith Cool! Will you use it in lucky? Where?

@paulcsmith
Copy link
Contributor

paulcsmith commented May 23, 2018

@asterite
Copy link
Member Author

@paulcsmith Yeah, I now think this feature might not be that useful after all... in the first snippet you have {{http_method}}, so because of that you won't be able to use verbatim. The second snippet is good, though.

I'm starting to think this is all very similar to quote/unquote, though in a different way... but here you can't go out of verbatim for a bit to inject a value :-(

@paulcsmith
Copy link
Contributor

After looking at more of the Lucky code I think you're right. I might not actually use it much because as soon as I need something without a \ I'd have to rewrite the whole thing without verbatim. That makes me nervous because that's more painful than just using \ from the start

I much prefer Crystal's approach over quote/unquote in Elixir and I'd rather not add new special keywords just for an escape hatch in verbatim. I'd be happy with keeping this for those limited use-cases, but I'd also be happy with it being removed since it hasn't actually been released and the use-cases are more limited than I'd realized

@straight-shoota
Copy link
Member

verbatim adds another syntax feature but can't replace \ escape. It is only applicable in places where you don't need to expand any macro expressions directly. So it's very limited. And switching between verbatim and \ creates much noise which could be avoided if \ was used everywhere.

@asterite
Copy link
Member Author

The problem is that macros inside methods are only expanded on instantiation, which is a bit weird and inconsistent, but it's what makes macro methods work. And then if you need to define such method inside a macro, like inherited or ìncluded, verbatim` can be handy. But yes, it's an incomplete feature. Maybe if we had a way to insert a value inside a verbatim block... but that would mean more syntax.

I think \{{ and \{% prove that macros are a kind of hack, and not well planned. Macros should be like elixir, with the quote/unquote stuff, which would also make macros be nicely formatted by the formatter. But it would be a big language change, and it's also not trivial to implement.

I guess this is what we got and this is what we have to stick to... unfortunately.

@asterite
Copy link
Member Author

@paulcsmith Could you explain why you prefer Crystal's approach over Elixir's? Elixir is super consistent in this regard.

@paulcsmith
Copy link
Contributor

paulcsmith commented May 23, 2018

@asterite It may be a purely syntactic thing and nothing to do with how quote/unquote works. I just find it extremely hard to mentally parse because it looks like any other method call, but it isn't. I've written a lot of Elixir so it's not just something that comes with time (at least for me). Using {{ and {% make it look like I'm writing a template in ECR/ERB and it just clicked immediately. I also like that in Crystal macros I'm not dealing directly with the AST. I much prefer that there are classes that represent the different nodes and I can call methods on them.

Overall, I don't think that \{{ is that big of a deal as it's something that I have never done from user-generated code. In fact, I try to avoid macros at all costs in my own code. It's always in a library and even then it is somewhat rare. I suppose the macros could be improved, but it hasn't been a big hurdle.

With that said, if macros could keep the same {{ and {% syntax but not need \ then that would be great

@asterite
Copy link
Member Author

@paulcsmith Cool, thanks for the reply!

Yes, the idea of using {%...%} and {{...}} for macros is that they look like ERB templates. In fact, when we chose the syntax we finally settles to what Django uses.

Then using macros as templates make it really easy to conditionally insert an "if" depending on some condition. With Elixir I guess you'd have to build an AST, conditionally wrapping stuff with another AST node, etc, which would make the code harder to follow. But then again, consistency is a really good thing in Elixir.

I wish we could somehow get rid of \{{. For example, this is pretty convulted in my opinion. Or maybe not...? I don't know :-P

@paulcsmith
Copy link
Contributor

paulcsmith commented May 23, 2018

Yeah, that is pretty hairy. I've got a few like that myself.

This may or may not be off-topic, but I think it's relevant to deciding what to prioritize. I think being able to call other macros inside {{ }} would make it much easier to extract nicely named concepts so complex macros make more sense (I want a way to express intent). This would probably help much more than figuring out \{{. Here's what I was thinking:

https://gist.github.com/paulcsmith/7973eae4f6fa5584e7e26f4163d3b026

# Here is what I have now
def self.create{% if with_bang %}!{% end %}(
  # Can't call another macro in here because it won't be expanded by the compiler
  {% if with_params %}params,{% end %}
  {% for type_declaration in (NEEDS_ON_CREATE + NEEDS_ON_INITIALIZE) %}
    {{ type_declaration }},
  {% end %}
  {% if @type.constant :FIELDS %}
    {% for field in FIELDS %}
      {{ field[:name] }} : {{ field[:type] }} | Nothing{% if field[:nilable] %} | Nil{% end %} = Nothing.new,
    {% end %}
  {% end %}
  {% for field in VIRTUAL_FIELDS %}
    {{ field.var }} : {{ field.type }} | Nothing = Nothing.new,
  {% end %}
)
  
# Here's what I'd *love* to do
def self.create{% if with_bang %}!{% end %}(
  {% if with_params %}params,{% end %}
  {{ args_for_needs() }}
  {{ args_for_fields() }}
  {{ args_for_virtual_fields() }}
  # Or maybe a method like this
  {{ run_macro args_for_fields() }}
)

macro args_for_needs
  {% for type_declaration in (NEEDS_ON_CREATE + NEEDS_ON_INITIALIZE) %}
    {{ type_declaration }},
  {% end %}
end

macro args_for_fields
  {% if @type.constant :FIELDS %}
    {% for field in FIELDS %}
      {{ field[:name] }} : {{ field[:type] }} | Nothing{% if field[:nilable] %} | Nil{% end %} = Nothing.new,
    {% end %}
  {% end %}
end

macro args_for_virtual_fields
  {% for field in VIRTUAL_FIELDS %}
    {{ field.var }} : {{ field.type }} | Nothing = Nothing.new,
  {% end %}
end

It would also be great to do this with {% so that you can have nicely named conditional macros `{% if has_inherited_settings?() %}. That's the biggest thing missing in terms of making readable macros IMO. Not sure how hard this would be, but it sure would be awesome! Hopefully this makes sense/helps. Feel free to disregard this if it isn't helpful, it was just a thought I've had while writing some macros and wishing there was a way to clarify my intent.

EDIT: Maybe this would be hard to figure out scoping so possible fully qualifying things would work: {{ @type.args_for_fields() }} or {{ MyModule.args_for_fields() }}?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants