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.

Sign up for GitHub

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

Jump to bottom

Add macro methods to ProcNotation and ProcLiteral #5206

Merged
merged 4 commits into from
Jan 3, 2018
Merged

Add macro methods to ProcNotation and ProcLiteral #5206

merged 4 commits into from
Jan 3, 2018

Conversation

willhbr
Copy link
Contributor

@willhbr willhbr commented Oct 29, 2017

Fixes #4485.

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?

@yxhuvud
Copy link
Contributor

yxhuvud commented Oct 29, 2017

Some tests would be nice.

@RX14
Copy link
Contributor

RX14 commented Oct 29, 2017

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

bew
bew reviewed Oct 29, 2017
View reviewed changes
spec/compiler/macro/macro_methods_spec.cr Outdated
@@ -1197,6 +1197,30 @@ describe "macro methods" do
end
end

describe "proc notation tests" do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that tests is redundant here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sija
Sija reviewed Oct 29, 2017
View reviewed changes
spec/compiler/macro/macro_methods_spec.cr Outdated
@@ -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]"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: SomeResut -> SomeResult

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, fixed.

asterite
asterite requested changes Oct 30, 2017
View reviewed changes
src/compiler/crystal/macros.cr Outdated
class ProcLiteral < ASTNode
# Return the function declaration for this proc.
# The name of the function will be "->"
def method : Def
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

asterite
asterite requested changes Oct 30, 2017
View reviewed changes
src/compiler/crystal/macros/methods.cr Outdated
"return_type",
"body",
"receiver",
"visibility"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Sija reviewed Oct 31, 2017
View reviewed changes
src/compiler/crystal/macros/methods.cr Outdated
class ProcLiteral
def interpret(method, args, block, interpreter)
case method
when "args"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can simplify it to:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, thanks!

@willhbr
Copy link
Contributor Author

willhbr commented Oct 31, 2017

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

@asterite
Copy link
Member

@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
Copy link
Contributor

bew commented Oct 31, 2017

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
Copy link
Contributor Author

willhbr commented Oct 31, 2017

@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
Copy link
Contributor

bew commented Oct 31, 2017

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.

@willhbr
ProcLiteral includes args and body macro methods
9b1a9e7
@willhbr
Add macro method to proc pointer
36c1f75 
Also add documentation to proc literal and proc notation macro methods
@willhbr
Copy link
Contributor Author

willhbr commented Nov 7, 2017

Is there anything else that needs attention here?

@RX14 RX14 requested a review from asterite November 7, 2017 23:55
@bew bew mentioned this pull request Dec 12, 2017
Open
@bew
Copy link
Contributor

bew commented Dec 12, 2017 •
edited
Loading

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
Copy link
Contributor

bew commented Jan 3, 2018

@asterite ping for review 😃

@RX14 RX14 added this to the Next milestone Jan 3, 2018
@RX14 RX14 merged commit 8bc0dbc into crystal-lang:master Jan 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bew bew bew left review comments

@Sija Sija Sija left review comments

@RX14 RX14 RX14 approved these changes

@asterite asterite asterite approved these changes

Assignees
No one assigned
Labels
kind:feature topic:compiler topic:stdlib:macros
Projects
None yet
Milestone
0.25.0
Development

Successfully merging this pull request may close these issues.
6 participants
@willhbr @yxhuvud @RX14 @asterite @bew @Sija