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

added @macroexpand #18660

Merged
merged 8 commits into from Sep 30, 2016
Merged

added @macroexpand #18660

merged 8 commits into from Sep 30, 2016

Conversation

jw3126
Copy link
Contributor

@jw3126 jw3126 commented Sep 24, 2016

Fix #18240

@@ -61,6 +61,16 @@ Takes the expression `x` and returns an equivalent expression with all macros re
"""
macroexpand(x::ANY) = ccall(:jl_macroexpand, Any, (Any,), x)

"""
@macroexpand
Copy link
Contributor

Choose a reason for hiding this comment

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

missing indent, and would have to be added to rst if this gets merged before #18588

Copy link
Contributor Author

Choose a reason for hiding this comment

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

where do I have to add it to rst?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@test (@macroexpand @doc "" f() = @x) == Expr(:error, UndefVarError(Symbol("@x")))
@test (@macroexpand @seven_dollar $bar) == 7
x = 2
@test (@macroexpand @seven_dollar 1+$x) == :(1 + 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this actually the correct behaviour? Shouldn't it throw an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right!

macro macroexpand(code)
    code_expanded = macroexpand(code)
    Expr(:quote, code_expanded)
end

code_expanded looks good the problem is the eval call on Expr(:quote, code_expanded) which does $ interpolation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So would the solution be to search and quote the dollars expressions in code_expanded?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm honestly not sure. This stuff always makes my head hurt.

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 think correct behaviour would be not to throw an error but return something like
1 + $(Expr(:$, :x))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah my head hurts, too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would be better to print the result, rather than return it, i.e. do:

macro macroprint(code)
    print(macroexpand(code))
end

After all, that's probably what you want in 99% of cases.

Copy link
Contributor

@yuyichao yuyichao Sep 24, 2016

Choose a reason for hiding this comment

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

👎 to print it.

macro macroexpand(code)
    QuoteNode(macroexpand(code))
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.

Is eval(QuoteNode(ex)) == ex for all expressions ex?

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be.

@simonbyrne
Copy link
Contributor

LGTM

@@ -0,0 +1,18 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this in a new file? This can probably go to parse or replutil or reflection.

"""
@macroexpand

Return equivalent expression with all macros removed (expanded).
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be documented that the expansion happens in the module where the @macroexpand macro is expanded (i.e. the same module if the expression is used directly) and not the current module when the returned code runs (which is the module if you call macroexpand) The difference is demonstrated below

julia> module M
       macro macroexpand(code)
           QuoteNode(macroexpand(code))
       end
       macro m()
           1
       end
       function f()
           (@macroexpand(@m), macroexpand(:(@m)))
       end
       end
M

julia> macro m()
           2
       end
@m (macro with 1 method)

julia> M.f()
(1,2)

This makes no difference when used in the REPL and as I said the current @macroexpand might be slightly better but this should still be documented.

Copy link
Contributor Author

@jw3126 jw3126 Sep 26, 2016

Choose a reason for hiding this comment

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

Can you give a usecase this would actually make a difference? And why is @macroexpand potentially better for interactive use?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you give a usecase this would actually make a difference?

That's exactly what the code above is showing.

And why is @macroexpand potentially better for interactive use?

I said

This makes no difference when used in the REPL

@yuyichao
Copy link
Contributor

LGTM other than the comments above.

@tkelman
Copy link
Contributor

tkelman commented Sep 26, 2016

commit messages could also be a bit more descriptive

@yuyichao
Copy link
Contributor

The PR title and the first commit message looks OK (more detail is fine but it's not bad). I assume the fix up commits will be squashed when merged.

@kshyatt
Copy link
Contributor

kshyatt commented Sep 26, 2016

So this needs a squash and then can be merged? Is that correct, @yuyichao?

@yuyichao
Copy link
Contributor

Also #18660 (review)

@kshyatt kshyatt added the status:needs docs Documentation for this change is required label Sep 26, 2016
julia> M.f()
(1,2)
```
With @macroexpand the expression expands where @macroexpand appears in the code (module M).
Copy link
Contributor

Choose a reason for hiding this comment

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

@macroexpand, macroexpand and M should be quoted. Otherwise 👍

Copy link
Contributor

@yuyichao yuyichao left a comment

Choose a reason for hiding this comment

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

LGTM. Will merge later after the CI finishes. Thx for the contribution.

@jw3126
Copy link
Contributor Author

jw3126 commented Sep 29, 2016

Cool!

@yuyichao yuyichao removed the status:needs docs Documentation for this change is required label Sep 30, 2016
@yuyichao yuyichao merged commit 759ac10 into JuliaLang:master Sep 30, 2016
With macroexpand the expressions expands in the current module where the code was finally called.
Note that when calling macroexpand or @macroexpand directly from the REPL, both of these contexts coincide, hence there is no difference.
With `@macroexpand` the expression expands where `@macroexpand` appears in the code (module `M`).
With `macroexpand` the expressions expands in the current module where the code was finally called (REPL).
Copy link
Contributor

Choose a reason for hiding this comment

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

this isn't the repl if the function is run in a script, is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me explain it differently. If you call macroexpand, the expansion takes place at runtime. If you call @macroexpand the expansion takes place at compile time (when @macroexpand is expanded to be more precise.). So the context of @macroexpand is where it appears in the code, while the context of macroexpand is the current environment. So if you run M.f() from a script, the answer will be
(1, whatever @m() currently means in your script)
Does this answer the question?

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense. I'm referring to the "where the code was finally called (REPL)." here. I don't see why "(REPL)" is accurate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But in the example it is the REPL?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're making a general statement here "with macroexpand the expression expands` ... followed by a very not-general parenthetical that doesn't serve much purpose

Copy link
Member

Choose a reason for hiding this comment

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

The julia> in the example implies that it runs in the REPL, doesn't it? And the "(module M)" in the sentence before also makes sense only in context of the example. So I'd say either leave as is or rephrase as "(module M in the example)" and "(REPL in the example)" (which I could well do in #18784).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@martinholters I am happy with either formulation, so rephrase it as you see fit.

Copy link
Contributor

Choose a reason for hiding this comment

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

a few words saying "in the example" sounds like a good clarification, thanks

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

Successfully merging this pull request may close these issues.

None yet

6 participants