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

Better support for documenting expressions with macros #12000

Merged
merged 2 commits into from
Jul 9, 2015
Merged

Conversation

yuyichao
Copy link
Contributor

@yuyichao yuyichao commented Jul 3, 2015

Fix #11993 (not sure if this is the best way though)

Original post:

  1. Disallow documenting macro with

    "<doc>"
    @macro
    

    syntax since it can be ambiguous with expanding and using the macro (which might have side effects)

  2. Expand the macro if the expression to be documented is a macro call in order to document the correct object.

Issues

  1. This is calling macroexpand in a macro. Not sure if this is the best thing to do but I couldn't come up with a better way to do what I want.

  2. Both "" @m and ""<\n>@m are is parsed as

    quote
        ""
        @id
    end

    The former should probably be reallowed,(edit: fixed) the latter this should probably not have the block (although I couldn't figure out if I can reset the input stream)

@yuyichao
Copy link
Contributor Author

yuyichao commented Jul 3, 2015

Updated and "" @m is parsed as doc string now. The extra block in ""<\n>@m is hopefully not a huge issue although I'm still not sure what to do with macroexpand.

@kshyatt kshyatt added the docsystem The documentation building system label Jul 3, 2015
@yuyichao
Copy link
Contributor Author

yuyichao commented Jul 3, 2015

I could also just document the object if the argument is a macro call but it will fail if this is a macro definition (which probably doesn't matter) and it won't be able to track function signature

julia> """
       (Float64)
       """
       @inline f(::Float64) = 1
f (generic function with 1 method)

julia> """
       (Int)
       """
       @inline f(::Int) = 1
f (generic function with 2 methods)

help?> f(1)
f(::Int64) at none:4

help?> f
search: f fd for fma fld fft full find filt fill fft! fdio frexp foldr foldl flush

  (Int)

@JeffBezanson
Copy link
Member

I'm uncomfortable with making zero-arg macro calls a special case like this. It's very fiddly to need to switch to "" @m syntax instead of a newline in this case.

@yuyichao
Copy link
Contributor Author

yuyichao commented Jul 3, 2015

making zero-arg macro calls a special case like this

So probably

  1. @doc "<doc string>" @macro should be kept.
  2. "<doc string>" <space or new line> @macro (<with or without arguments>) should expand the macro and apply the doc macro on the expansion?

Edit: and by kept I mean @doc "<doc string>" @macro should be documenting the macro @macro

@JeffBezanson
Copy link
Member

That sounds better.

@yuyichao
Copy link
Contributor Author

yuyichao commented Jul 3, 2015

There's still the inconsistency that "<doc string>"<new line or space>a will be documenting a while "<doc string>"<new line or space>@a will not document @a.

This is the inconsistency I'm trying to address with the 0-argument special case but I guess since it cannot be made fully consistent with everything anyway, I might as well make the implementation simpler....

@yuyichao
Copy link
Contributor Author

yuyichao commented Jul 3, 2015

Oh, and another issue is that I cannot distinguish @doc "" @macro and "" @macro in the @doc macro.

Now I remember why exactly I implement it this way...

  1. We want to make @doc "" @macro documenting @macro (or not?)
  2. We want to make ""<\n>@macro<with arguments> documenting the expansion of @macro

This itself will require a special case for 0-argument macrocall.

Furthermore, if there's no special case for this in the parser, "" @macro and ""<\n>@macro will both be lowered to @doc "" @macro and will be documenting the @macro itself instead of the expansion.

Another way out of this is to make bare string literal document to be lowered to a different internal macro that has different handling of macrocall. This also has the advantage that we and independently tweak the behavior of each one.

@yuyichao
Copy link
Contributor Author

yuyichao commented Jul 3, 2015

In short, in order to get the behavior I mentioned above #12000 (comment) (which I probably the most intuitive one I can think of) "<doc string>" @macro cannot be parsed to @doc "<doc string>" @macro. I'll try to parse it to a different macro and tweak the behavior to see how it looks.

@yuyichao
Copy link
Contributor Author

yuyichao commented Jul 3, 2015

The behavior should be the same with the I described above now.

Here's a compilation of the current behavior (with this PR)

julia> macro id(f)
       esc(f)
       end

julia> macro m(args...)
       println(args)
       end

# Documenting @id
julia> @doc """
       identity
       """ @id
(anonymous function)

# This is a normal macro call so having a new line does not work
julia> @doc """
       print
       """
       @m
()

help?> @id
  identity

# @m is not documented yet as can be seen from the printing of from @m and the error below
help?> @m
Symbol not found. Falling back on apropos search ...
No help information found.

# Still won't work because this document the expansion of @m, which is `nothing`
julia> """
       print
       """ @m
()

# As shown here
help?> @m
Symbol not found. Falling back on apropos search ...
No help information found.

# This is the only way to document a macro after it's definition (AFAICT)
julia> @doc """
       print
       """ @m
(anonymous function)

# And it works
help?> @m
  print

# Documenting function definition "decorated" with macros
julia> """
       (Int)
       """
       @inline f(::Int) = 1
f (generic function with 1 method)

julia> """
       (Float64)
       """
       @inline f(::Float64) = 1
f (generic function with 2 methods)

# And the methods are documented correctly
help?> f(1)
  (Int)

# Also for the whole function
help?> f
search: f fd for fma fld fft full find filt fill fft! fdio frexp foldr foldl flush floor float first findn filt! fill! fetch FFTW flipud fliplr fldmod findnz findin

  (Int)

  (Float64)

# Works as expected
julia> """
       """
       @id immutable bar
       x::Int
       end

julia> bar(1)
bar(1)

# Empty doc string. Ooops.
help?> bar
search: bar baremodule SubArray GlobalRef clipboard BitArray backtrace find_library mmap_bitarray BitMatrix takebuf_array catch_backtrace AbstractRNG AbstractArray


# Try again with a different type, now with non-empty doc string.
julia> """
       bar
       """
       @id immutable baz
       x::Int
       end

# And it works.
help?> baz
search: baz

  bar

@mauro3
Copy link
Contributor

mauro3 commented Jul 3, 2015

My use case would be to document types defined in macros:

julia> macro mkT(f)
       quote
       immutable $f end
       g() = 5 # just a bunch of other stuff also in the macro after the type declaration
       end
       end

julia> @mkT T
#123#g (generic function with 1 method)

so this should document the new type:

@doc """
My T
"""
@mkT T

would this work with this PR? (Sorry for being lazy and not just trying it out!) I.e. does it alway document the first item defined with the macro or something else?

@yuyichao
Copy link
Contributor Author

yuyichao commented Jul 3, 2015

(Sorry for being lazy and not just trying it out!)

NP. I don't always like to wait for ~10min to compile someone else's PR either.... :P

would this work with this PR?

Not directly and not on current master either because

  1. A normal macro call cannot use newline to separate arguments.
  2. The return value is not the type.

This works.

julia> macro mkT(f)
       quote
       immutable $f end
       g() = 5
       $f
       end
       end

julia> @doc """
       My T1
       """ @mkT T1
T1

julia> """
       My T2
       """
       @mkT T2
T2

help?> T1
search: T1 Int16 int16 Int128 rot180 int128 UInt16 uint16 Uint16 UInt128 uint128

  My T1

help?> T2
search: T2 hist2d hist2d! Int32 int32 Int128 int128 UInt32 uint32 Uint32 UInt128

  My T2

The expansion of the macro is not a single type definition so @doc or @s_doc will not recognize the structure. However, as long as the expression evaluates to the type, it can document the object with no problem.

P.S. The only exception to the above rules are macro definition (which doesn't return anything) and function definition (which returns the function and not the method, a behavior that is useful in many cases). In these cases, you would have to let @doc see the original syntax in order to document the correct stuff.

@StefanKarpinski
Copy link
Member

If the macro expander function were exposed in a more first-class way, then that would be a natural thing to attach the docstring to (in fact, I'm pretty sure this is how it actually works under the hood).

@yuyichao
Copy link
Contributor Author

yuyichao commented Jul 4, 2015

@StefanKarpinski Sorry I think I don't quite understand what you mean here.

What is "a more first-class way" (or why it is not first class now) and this is how what works under the hood?

@MichaelHatherly
Copy link
Member

I'm uncomfortable with making zero-arg macro calls a special case like this.

The zero-arg case was from when we couldn't add docstrings to most places in base prior to @one-more-minute's Base.DocBootstrap I think. Now that we can just write

"""
...
"""
macro m()
    # ...
end

anywhere wouldn't it be better to just remove the zero-arg case and require macros to be documented at the definition site? I realise that helpdb.jl basedocs.jl is using this syntax for macros, but hopefully we'll move away from that relatively soon?

That would remove the ambiguity and leave :macrocall syntax solely for cases such as @inline f(x) = ... etc.

@yuyichao
Copy link
Contributor Author

yuyichao commented Jul 4, 2015

So what about this (haven't pushed (clean up to follow the coding style and pushed), RFC),

  • No special case for macros with or without arguments
  • For most (all of) cases, macros should be documented where they are defined.
  • If for some reason, it is necessary to document macros after thay are defined, you should pass the symbol directly to @doc. This can be done with @eval as shown below. It's a bit complicated but should be really rare and doesn't need any special support

I'm using @eval for the two cases in Base mainly to show that this works and to make the change minimum. If everyone is fine with it, I can move them to the definition of the macro.

See commit 29a8eea

@one-more-minute @MichaelHatherly

@MichaelHatherly
Copy link
Member

It's a bit complicated but should be really rare and doesn't need any special support

+1

@yuyichao
Copy link
Contributor Author

yuyichao commented Jul 4, 2015

Only the docm(meta, def) needs macro expansion I think.

Fixed. Thanks.

Also fixed the tests...

@yuyichao yuyichao added the breaking This change will break code label Jul 4, 2015
@mauro3
Copy link
Contributor

mauro3 commented Jul 4, 2015

A bit slow to respond. So, if I wanted to document a method defined within a macro I would need to do:

macro m(f)
quote
$f(x) = 1
methods(f, (Any,))[1] # potentially tricky if there are many methods
end
end

@doc """
blah
"""
@m f

Right? (edited to reflect @yuyichao's correction)

@yuyichao
Copy link
Contributor Author

yuyichao commented Jul 4, 2015

@mauro3 I guess you meant two end after the methods line?

There still can't be a new line after the doc string if you use the @doc macro explicitly (and it's still just now macros are parsed).

I don't think there's currently a way to document a method after it's defined (although I might be wrong). It's probably possible to add support for that (and we should) but that's not in the scope of this PR.

@yuyichao
Copy link
Contributor Author

yuyichao commented Jul 4, 2015

potentially tricky if there are many methods

Well, if you defined multiple methods (or multiple objects to be documented) in a single macro, there isn't anything the doc system can help with that (unless you call @doc in your macro)

If you are just worrying about multiple methods of a single function, it shouldn't be too bad since which (and @which) should return the invoke compatible method now which should be the one that matches the signature precisely and you should be able to generate the correct signatrue in the macro.

@mauro3
Copy link
Contributor

mauro3 commented Jul 4, 2015

Thanks for the clarification. Works for me.

@yuyichao
Copy link
Contributor Author

yuyichao commented Jul 4, 2015

The document for the @doc seems to be on the wrong one. Fixed in this PR.

@MikeInnes MikeInnes self-assigned this Jul 5, 2015
@yuyichao
Copy link
Contributor Author

yuyichao commented Jul 8, 2015

Just realized that I can do manual TCO for unblock....... LOL
Will do that if this needs another revision (likely?).

@@ -201,7 +201,7 @@ end

doc(f, ::Method) = doc(f)

# Modules
# Modules
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 pretty weird, have we somehow ended up with a different kind of space 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.

It's a non-breaking space. We have a lot of them.

I have an old PR to clean them up but I still need to rebase it and check if any of them are used intentionally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ref #11605

Copy link
Member

Choose a reason for hiding this comment

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

I assume that must be my fault, although I have no idea why an editor would insert one of those.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Editor shortcut probably?

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, looks like Alt-Space does it on OS X – better watch out for that one.

@MikeInnes
Copy link
Member

I imagine there are many people who would want to see a manual implementation of TCO, but I'm definitely not one of them :) As a bonus we throw an error when people try to documenting infinite expressions, rather than hanging.

Anyway, LGTM, I'll wait on the tests.

@yuyichao
Copy link
Contributor Author

yuyichao commented Jul 8, 2015

@tkelman I guess the push failure is sth know and is fixed now?

@tkelman
Copy link
Contributor

tkelman commented Jul 8, 2015

yeah, fixed by 896cadf

@yuyichao
Copy link
Contributor Author

yuyichao commented Jul 8, 2015

Since this seems to be getting ready for merging, I created a @compat entry JuliaLang/Compat.jl#113 in case any packages is using the old syntax to document macros.

@mauro3
Copy link
Contributor

mauro3 commented Jul 9, 2015

Would this need documenting too?

@yuyichao
Copy link
Contributor Author

yuyichao commented Jul 9, 2015

I didn't bother add doc yet mainly because this wasn't a documented interface to begin with and originally this change distinguish between documenting with or without @doc macro and the new interface isn't documented yet either =(.

This isn't the case anymore so if everyone is happy about the current state I'll add some document about the change soon.

@yuyichao
Copy link
Contributor Author

yuyichao commented Jul 9, 2015

Added a section about documenting after an object is defined in the documentation of @doc. I'm pretty bad at writing documentation so suggestions welcome.

@MikeInnes
Copy link
Member

I'm wondering if it'd be a good idea to put ExpressionMatch.jl into Base. Especially if we start adding more syntax to @doc (and being more finicky about it), this code is only going to get more obscure, and we could make it a lot clearer what's going on:

julia> @match :(:@time) begin
         :(@f_) -> "macro $f"
         :(f_"") -> "string macro $f"
       end
"macro @time"

julia> @match :(:(r"")) begin
         :(@f_) -> "macro $f"
         :(f_"") -> "string macro $f"
       end
"string macro @r_str"

@yuyichao
Copy link
Contributor Author

yuyichao commented Jul 9, 2015

I'm wondering if it'd be a good idea to put ExpressionMatch.jl into Base.

I'm not an expert on what should go into base. But I'd say not this PR for sure.

Especially if we start adding more syntax to @doc (and being more finicky about it).

Well, OTOH, making it harder to add syntax also makes it harder to introduce breaking changes and obscure syntax. It took me a long time to convince myself that it is ok to add new syntax like this and I hope this should happen as infrequent as possible.

@MikeInnes
Copy link
Member

Sure, definitely not for this PR.

I just introduced a merge conflict (sorry), but if you can rebase this I'll merge then.

1. Always do macro expansion in @doc
2. Use `@doc "<doc string>" :@macro` to document macros after definition
3. Unblock recursively
@yuyichao
Copy link
Contributor Author

yuyichao commented Jul 9, 2015

I just introduced a merge conflict (sorry), but if you can rebase this I'll merge then.

I can tell you merge that PR just to post this comment 😆

It's a stupid end-of-test-file conflict. I'll push after it passes local tests. (edit: local tests pass)

@MikeInnes
Copy link
Member

Nothing interesting has changed since this passed the tests before, right? I'd really like to start working off these changes.

@yuyichao
Copy link
Contributor Author

yuyichao commented Jul 9, 2015

@one-more-minute There shouldn't be.

The AppVeyor test should be done in a few minutes so I would suggest waiting for a few more minutes rather than being ping'd by Tony if sth unexpected happened.

MikeInnes added a commit that referenced this pull request Jul 9, 2015
Better support for documenting expressions with macros
@MikeInnes MikeInnes merged commit 914aeb1 into master Jul 9, 2015
@MikeInnes
Copy link
Member

Awesome, really glad we have a better approach for this now.

@yuyichao yuyichao deleted the yyc/doc-macro branch July 9, 2015 16:56
@yuyichao
Copy link
Contributor Author

yuyichao commented Jul 9, 2015

@IainNZ Could you please check if this causes any breakage in packages? Depending on how bad it is we can either merge or discard JuliaLang/Compat.jl#113

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code docsystem The documentation building system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Doc string syntax does not mesh well with macros
8 participants