Add macro methods to ProcNotation and ProcLiteral #5206

willhbr · 2017-10-29T05:30:45Z

I can't see where tests for macros live, so I've only briefly tested this manually. I will happily write some real tests before merging.

Also unsure about whether NilLiteral is the best thing to return in the case of no return type - is there already a convention for this?

Fix #4485

yxhuvud · 2017-10-29T08:21:14Z

Some tests would be nice.

RX14 · 2017-10-29T09:30:14Z

Macro specs are here: https://github.com/crystal-lang/crystal/blob/master/spec/compiler/macro/macro_methods_spec.cr

bew · 2017-10-29T10:40:11Z

spec/compiler/macro/macro_methods_spec.cr

@@ -1197,6 +1197,30 @@ describe "macro methods" do
    end
  end

+  describe "proc notation tests" do


I think that tests is redundant here

Changed to "methods" to fit in with other tests.

Sija · 2017-10-29T23:32:29Z

spec/compiler/macro/macro_methods_spec.cr

@@ -1197,6 +1197,30 @@ describe "macro methods" do
    end
  end

+  describe "proc notation methods" do
+    it "gets single input" do
+      assert_macro "x", %({{x.inputs}}), [ProcNotation.new([Path.new("SomeType")] of ASTNode, Path.new("SomeResut"))] of ASTNode, "[SomeType]"


typo: SomeResut -> SomeResult

Oops, fixed.

asterite · 2017-10-30T17:12:42Z

src/compiler/crystal/macros.cr

+  class ProcLiteral < ASTNode
+    # Return the function declaration for this proc.
+    # The name of the function will be "->"
+    def method : Def


This is actually the way a ProcLiteral is implemented. This shouldn't leak to the macros API. A ProcLiteral should have the usual methods a def has. The macro logic just needs to forward some methods to the internal def.

What I mean is, ProcLiteral should have a body, args, etc. methods. It shouldn't have a name, though, because proc literals don't have a name.

Is the way I have done that now ok, or should I do every method explicitly?

(Is there some consequence of the way that I've done it that isn't desirable?)

I'm not sure. There are many methods in the base ASTNode class. The safest thing to do is to only forward the names that Def also handles.

I've altered that to pass through all the current methods on Def, otherwise call super.

asterite · 2017-10-30T23:48:38Z

src/compiler/crystal/macros/methods.cr

+        "return_type",
+        "body",
+        "receiver",
+        "visibility"


Now that I see the list, I'm pretty sure all of these don't make sense in a ProcLiteral: splat_index, double_splat, block_arg, return_type, visibility. So it seems only receiver, args and body make sense. And please use case ... when instead of {..}.includes?(...). Thank you!

Amended to just include those methods. I'm not super familiar with Crystal yet so didn't catch that those didn't make sense!

Sija · 2017-10-31T00:42:38Z

src/compiler/crystal/macros/methods.cr

+  class ProcLiteral
+    def interpret(method, args, block, interpreter)
+      case method
+      when "args"


you can simplify it to:

case method when "args", "receiver", "body" @def.interpret(method, args, block, interpreter) else super end

Aha, thanks!

willhbr · 2017-10-31T10:30:18Z

Not sure why Travis is failng, the tests are passing on my machine.

asterite · 2017-10-31T11:13:52Z

@javanut13 Probably some hiccup.

In any case, I'm sorry but I confused ProcNotation with ProcLiteral. Now I remember there's no receiver for ProcLiteral. So only body and args. Could you make that last change? Maybe travis will also work after that change (if not, I'll restart it).

bew · 2017-10-31T11:17:41Z

I remembered there is also ProcPointer (e.g: ->some_func(String)) that don't have macro methods, maybe it would be the right PR to add them?

willhbr · 2017-10-31T22:19:46Z

@asterite No worries, I'll change that.

@bew this would probably be a reasonable place to do so - what methods should it have? Looking at the definition in ast.cr there is obj, name, and args. I assume name and args are definite, but what is obj?

bew · 2017-10-31T22:30:15Z

obj is the receiver of the method.

For example: ->my_instance.some_method(String). Here obj is my_instance, name is some_method and args is String.

Also add documentation to proc literal and proc notation macro methods

willhbr · 2017-11-07T22:27:26Z

Is there anything else that needs attention here?

bew · 2017-12-12T16:58:02Z

Bump, could we have this soon?
I know you requested a review from @asterite but I'm not sure we'll ever get it.. (please correct me if I'm wrong!)

edit: I was wrong :)

bew · 2018-01-03T12:26:43Z

@asterite ping for review 😃

Add macro methods to ProcNotation and ProcLiteral

2739f90

Fix #4485

bew reviewed Oct 29, 2017

View reviewed changes

RX14 approved these changes Oct 29, 2017

View reviewed changes

Sija reviewed Oct 29, 2017

View reviewed changes

Add tests for proc notation and literal

792deb2

asterite requested changes Oct 30, 2017

View reviewed changes

Sija reviewed Oct 31, 2017

View reviewed changes

willhbr added 2 commits November 1, 2017 12:13

ProcLiteral includes args and body macro methods

9b1a9e7

Add macro method to proc pointer

36c1f75

Also add documentation to proc literal and proc notation macro methods

RX14 requested a review from asterite November 7, 2017 23:55

bew mentioned this pull request Dec 12, 2017

Crystal callback veelenga/lua.cr#15

Open

RX14 approved these changes Dec 12, 2017

View reviewed changes

asterite approved these changes Jan 3, 2018

View reviewed changes

RX14 added kind:feature topic:compiler topic:stdlib:macros labels Jan 3, 2018

RX14 added this to the Next milestone Jan 3, 2018

RX14 merged commit 8bc0dbc into crystal-lang:master Jan 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add macro methods to ProcNotation and ProcLiteral #5206

Add macro methods to ProcNotation and ProcLiteral #5206

willhbr commented Oct 29, 2017

yxhuvud commented Oct 29, 2017

RX14 commented Oct 29, 2017

bew Oct 29, 2017

willhbr Oct 29, 2017

Sija Oct 29, 2017

willhbr Oct 30, 2017

asterite Oct 30, 2017

willhbr Oct 30, 2017

asterite Oct 30, 2017

willhbr Oct 30, 2017

asterite Oct 30, 2017

willhbr Oct 31, 2017

Sija Oct 31, 2017

willhbr Oct 31, 2017

willhbr commented Oct 31, 2017

asterite commented Oct 31, 2017

bew commented Oct 31, 2017

willhbr commented Oct 31, 2017

bew commented Oct 31, 2017

willhbr commented Nov 7, 2017

bew commented Dec 12, 2017 •

edited

bew commented Jan 3, 2018

Add macro methods to ProcNotation and ProcLiteral #5206

Add macro methods to ProcNotation and ProcLiteral #5206

Conversation

willhbr commented Oct 29, 2017

yxhuvud commented Oct 29, 2017

RX14 commented Oct 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willhbr commented Oct 31, 2017

asterite commented Oct 31, 2017

bew commented Oct 31, 2017

willhbr commented Oct 31, 2017

bew commented Oct 31, 2017

willhbr commented Nov 7, 2017

bew commented Dec 12, 2017 • edited

bew commented Jan 3, 2018

bew commented Dec 12, 2017 •

edited