-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
WIP: Docstring refactoring #58747
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
base: master
Are you sure you want to change the base?
WIP: Docstring refactoring #58747
Conversation
Nice. This implementation makes me much happier. Whenever things guess at what lowering might do, trouble starts happening.
I think I'd be happy for this syntax to always just do the dumb thing of lowering |
A bit of a side quest, but I've always hated this type. Can we just use regular |
We'd need to require
Would be nice. I'll see if it's feasible (and reasonable to do in this PR). |
That is more of an implementation bug though (quite a few open issues about it), since it is not a Binding, but rather a StructFieldPath (which might sometimes happen to alias some globals) |
What are you referring to, Docs.Binding? Also, what is a StructFieldPath? |
e57fb83
to
87b1cb9
Compare
# Temporary storage until the full docsystem is available | ||
struct DocLinkedList | ||
doc::SimpleVector | ||
args::Tuple |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this have implications for forcing the generation of a lot of unnecessary Tuple
types? I assume that's why SimpleVector
was originally used here?
This change moves the construction of docstring registration calls into lowering
instead of doing so in a macro. The original intent was just to fix #58630,
but some refactoring is necessary to do this in a good way.
Not all of the following is implemented yet. Feedback on the design is welcome!
Current design
Docstrings are stored in a module-scoped IdDict mapping Base.Docs.Binding to
Base.Docs.MultiDoc, which either contains a single docstring, or a map from
method signature to docstring.
Docstrings are parsed to
@doc str x
calls (this macro is also public andused).
@doc
defers toCore.atdoc!
, which figures out if it's one of themany things we can document, tries to guess the signature of any methods if
x
is a function, and returns a quoted call to
Docs.doc!
, which registers thedocstring.
A few things make our current design complex:
trying to figure out what we're documenting purely by Expr. We need to dig
through struct contents and call expressions to find field docstrings and
signatures.
@doc
is sometimes responsible for evaluating the expression it's given, andsometimes not.
Complicating matters further are a few "doc-only" special cases where adding a
docstring changes the semantics of the following expression
This change
Structures and the information we store (other than including signatures'
first arguments in keys) are both unchanged.
Change two-arg
@doc
to produceExpr(:doc, ...)
instead of callingCore.atdoc
New
Expr(:doc, str, x, linenode)
form. Generally, this lowersx
as usual followed by a call like
and returns the value of
x
if we're not in a doc-only case. Pending parserchanges, this lets us prevent docstrings from changing what an expression
returns (modulo doc-only cases).
Unlike
Core.atdoc
, the newCore.setdoc
gets the answers from lowering, andcan query the runtime about the object to document. The "signature" key we
use for method
x
is now exactlyx.sig
.Misc
@doc x
("get docs forx") and
@doc str x
(set docstringstr
forx
) code paths, which arepretty disjoint.
@doc "foo" -> bar
syntax, which appears to be an undocumented specialsyntax. I'll put it back if people use it.
@doc
used to take all arguments and pass them toatdoc!
, so had at leastone secret undocumented argument
define
. Remove this option.Potential issues
expressions where there's nothing to lower. In its current form, this PR
breaks these.
to replace it.
don't mind being the one to add this to JuliaLowering too).
Todo
"str" x
asExpr(:doc, ...)
instead of a call to@doc
(JuliaSyntaxalready does this internally before Expr-converting it)
Expr(:doc)
; so far I'vetried to stay faithful to existing behaviour.
https://github.com/mlechu/julia/blob/docsystem-wrangling/src/julia-syntax.scm#L2604-L2617
(::T)(x) = x
T
@doc
should ideally return the same thing (Base.Docs.Binding) asbefore, probably by just calling one-arg
@doc
@__doc__
and quoted macro calls (should be easy)