Docs.jl: Fix documenting parametric functor calls

[mgkurtz]

Differentiate signature of constructors, functors and normal function calls, thus improving upon PR #25626 and fixing #44889. Also allows fixing JuliaDocs/Documenter.jl#558.

[mortenpi]

It looks like Julia fails to bootstrap itself with these changes:

error during bootstrap:
LoadError("sysimg.jl", 3, LoadError("Base.jl", 418, UndefVarError(:T)))
ijl_undefined_var_error at /home/mortenpi/Julia/julia-45367/src/rtutils.c:132
jl_eval_global_var at /home/mortenpi/Julia/julia-45367/src/interpreter.c:150 [inlined]
eval_value at /home/mortenpi/Julia/julia-45367/src/interpreter.c:196

I'd be happy to review this, but would you mind elaborating what exactly the intended change here is? The Docs module can be pretty confusing and it's not immediately obvious what this actually changes.

As far as I understand, the main issues related to the functors are:

1. For parametric structs, the docstring for functor syntax get attached to the symbol of the variable name, rather than the module (i.e. to f for function (f::Parametric{T})(x) where {T} end, rather than Parametric; this causes Bug when documenting callable parametric struct #44889).

2. You can't distinguish the docstrings for functions/constructors and functors, because signature yields the same for both, e.g. from current Julia:

   julia> Docs.signature(:(
            (::Foo{T})(x::T, y::Vector{T}) where T
          ))
   :(Union{Tuple{T, Vector{T}}, Tuple{T}} where T <: Any)
   
   julia> Docs.signature(:(
            Foo{T}(x::T, y::Vector{T}) where T
          ))
   :(Union{Tuple{T, Vector{T}}, Tuple{T}} where T <: Any)
   

For (1), as far as I can tell though, determining the binding is not the job of the signature function, so is that behavior changed here at all?

For (2), I presume you are storing a Functor instance now as the signature? However, is a separate type (ParametricConstructor) actually necessary for constructors? Why couldn't those just be the standard Union types that we use for parametric methods?

[mgkurtz]

Thank you very much for reviewing ♥ It seems like I have to fix that bootstrapping error first, so I convert this to a draft for the meantime.

I'd be happy to review this, but would you mind elaborating what exactly the intended change here is? The Docs module can be pretty confusing and it's not immediately obvious what this actually changes.

Oh, I happily elaborate on this (as long as anyone wants to read it :D ). That Docs module also confused me. Especially the part with namifying and finding the signature, which try to mimic Julia’s parsing. So they must either be as complicated as parts of the parser or produce bugs. This PR simply shifts this balance a bit (complexity++ and bugs--).

After spending some time to make the changes in this PR, I very much wished for a way to interact with Julia’s internals to precisely find out, what Julia really defines instead of all this mimicry parsing. This would naturally remove all the bugs and complexity stemming from this mimicry (while hopefully introducing less bugs and less complexity). Do you think this could be viable and anyone could do this? Or anyone could tell me, how to do this?

As far as I understand, the main issues related to the functors are:
[...]

That is my understanding, too.
There are also minor issues with typevars: A{Int}() gets signature Union{Tuple{}, Tuple{Int}} where Int <: Any and f() where T gets Union{Tuple{}, Tuple{T}} where T <: Any. Both seem to be wrong and I fix them with my rewrites.

For (1), as far as I can tell though, determining the binding is not the job of the signature function, so is that behavior changed here at all?

Sorry, I forgot to mention this (as it was a rather minor change in astname called by namify). Indeed I get

julia> Docs.namify(:(function (f::Parametric{T})(x) where {T} end))
:Parametric

as you wished for.

For (2), I presume you are storing a Functor instance now as the signature? However, is a separate type (ParametricConstructor) actually necessary for constructors? Why couldn't those just be the standard Union types that we use for parametric methods?

Mainly, because I first did not find any way, how using a Union of Tuples can uniquely (not to speak of comprehensibly) differentiate all kinds of functions with optional arguments and parametric constructors. Some examples:

# option 1 (extra tuple)
A{Int8, Int}(::Int8, ::Int) → Union{Tuple{Int8, Int}, ???} # Using `Tuple{Int8, Int}` as ??? would clash with
A(::Int8, ::Int) → Union{Tuple{Int8, Int}}
# option 2 (prepend to tuple)
A{Int8, Int}(::String) → Union{Tuple{???, String}} # Using `Int8, Int` as ??? would clash with
A(::Int8, ::Int, ::String) → Union{Tuple{Int8, Int, String}}

Of course, we could also add some other type to the Tuple. Adding A itself would not work, as A can also be the type of some argument. So what shall we do? If we add some new own type Base.Docs.Foo to our Tuple, it seems like than we can also put that new type around everything?

Afterthought: There seems to be a solution exploiting, that Union{} is an insensible function argument:

# option 1 (extra tuple)
A{Int8, Int}(::Int8, ::Int) → Union{Tuple{Int8, Int}, Tuple{Union{}, Int8, Int}}
# option 2 (prepend to tuple)
A{Int8, Int}(::String) → Union{Tuple{Union{}, Int8, Int, String}}

We could actually use, say option 1 for functors and option 2 for constructors. Do you think, this would be better? If you would prefer this approach, I could change my code rather easily.

Remaining bugs

Julia itself takes semantic information into account, when defining a function. Docs.jl does not. Hence we get bugs whenever we add methods to some kind of alias. Also refer to my wish above of letting Julia tell us, what it does instead of emulating Julia’s internal behavior.

f() = 0
const g = f
"""
    f(x)

but documented as `g(x)`.
"""
g(x) = x

f() = 0
g = f
"""
    f(x)

but documented as `g(x)`.
"""
Main.g(x) = x
g = "My documentation lies!"

"""
    Int8(x)

but documented as `Union{Int8}(x)`.
"""
Union{Int8}(x) = x

struct A end
const f = A()
"""
    (::A)(x)

but documented as `f(x)`.
"""
f(x) = x

---

Does that elaboration help? Btw. I actually removed my rather elaborative comments in the code, hoping that the code could finally speak for itself. Maybe I shall add short comments again. Shall I?

[mortenpi]

That's terrific, thank you for writing this all up!

Sorry, I forgot to mention this (as it was a rather minor change in astname called by namify).

Ah, cool. If this is orthogonal to the signature changes, do you think we could have this + associated tests in a small separate PR? It looks like a straightforward bugfix and no need for it to get held back by the functor stuff.

I very much wished for a way to interact with Julia’s internals to precisely find out, what Julia really defines instead of all this mimicry parsing. [..] Do you think this could be viable and anyone could do this?

Hmm.. do you basically mean that, instead of passing the AST to docm, you'd pass something that is already evaluated by Julia? E.g. the evaluated macro expansion, like a Function object?

I guess one issue is that, for example, a function definition returns the general Function object, so you actually no longer know which method it was that got defined. So you would need to hook into the process somewhere inbetween, while the Julia internals are doing their thing.

I have no idea how doable something like this would be though, and have to defer to somebody else for this.

Hence we get bugs whenever we add methods to some kind of alias.

I think one can argue whether the alias behaviour is a bug or not. It might be useful if you're defining a custom binding for a specialization (e.g. Vector):

const MyPackageSpecializationType = ParentType{Foo}

You probably want to attach docs to this custom name, rather than ParentType, if you expect the users to use that special name in their code. I think in this case the docsystem gives you some power to arrange the docs in whatever way you see fit, even if you can also create nonsense that way.

Btw. I actually removed my rather elaborative comments in the code, hoping that the code could finally speak for itself. Maybe I shall add short comments again. Shall I?

I think comments are always good!

[mortenpi]

As for the ParametricConstructor: ohh.. I think the reason e.g. A{Int}() gets the weird nonsense where signature right now is that this used to be the parametric method syntax back in the day. So Docs doesn't realize that this is a parametric struct, but thinks that this is just a normal function with parametric types.

It didn't occur to me that you can have the same argument signature for constructors with different type parameters (e.g. Foo{Int}(::String) and Foo{String}(::String); right?). And the parametric method stuff wouldn't help in that case, because you also need to store the type parameters somewhere.

With that in mind, I think having this special ParametricConstructor type might be the way to go after all. I'm sure we could invent a Union{} thingy to also do the job, but I think a special type would be far clearer to anyone introspecting this in the future.

---

One last thing I am slightly concerned about here is breaking packages / code that are already diving into the docsystem metadata (like Documenter; although I am more than happy to just patch Documenter right away, with version guards if need be). But I wonder how much others rely on these internal details, like maybe even Revise etc.

[mgkurtz]

Thanks for your replies. I have just now taken the time to look into that bootstrapping error and will come back to your replies tomorrow.

The bootstrapping error arose from

"""
    Vector{T}(undef, n)
"""
Vector{T}(::UndefInitializer, n)

without specifying T (as the T got ignored anyway). This now does not work anymore. Instead we need to add where T there. Appropriately, in DocBase.docm we need something like Docs.iscallexpr (okay, actually I should just move it there, I guess).

Symmetrically, if we want to get the documentation for Vector{T}(undef, n) (e.g. in the REPL) we now need to add the where T or use Vector{Int}(undef, n). Perhaps that is somewhat clumsy and the old way of writing Vector(undef, n) would actually be nice there?

As it seems, some tests that want to document Core.Array(::UndefInitializer, ::Any) and the like break now. So, backwards compatibility in the interface may be nice, to not break lots of documentation out there, which also relies on this (mis)behavior. Any ideas how to get such compatibility non-ugly? Do we actually want/need it? Or do we want to produce more meaningful errors instead?

[mgkurtz]

Ah, cool. If this is orthogonal to the signature changes, do you think we could have this + associated tests in a small separate PR? It looks like a straightforward bugfix and no need for it to get held back by the functor stuff.

I have put that in #45529.

I think one can argue whether the alias behaviour is a bug or not.

Yes, probably. So I would also just keep the behavior as it is.

I think comments are always good!

I will see to that.

One last thing I am slightly concerned about here is breaking packages / code that are already diving into the docsystem metadata (like Documenter; although I am more than happy to just patch Documenter right away, with version guards if need be). But I wonder how much others rely on these internal details, like maybe even Revise etc.

That could indeed be problematic, but Documenter only relies on the ability to compare signatures via <: and Revise does not use the internals of Base.Docs at all as it seems. So at least those packages behave nicely.

Regarding Revise, I have seen, that it computes signatures using lowered code. This seems to be kind of the solution I was hoping for, but it depends on LoweredCodeUtils.jl and JuliaInterpreter.jl. So perhaps, we do not want that overkill?

Still, I like the way Revise signatures look like way more than how they look like here. Revise’s signatures result in a much cleaner interface without the extra types I introduced. The cost would be to change all normal function signatures (prepending them with typeof(f) or the like) with whatever effect to existing code bases. Hopefully the effect should be small, as there seems to be no reason for anyone, to inspect the result of Base.Docs.signature. Do you think such a change could be viable?

As the failing tests reflect, it could potentially be wise to add better error messages or some fallback behavior in Documenter.jl. As for now, interpolating a docstring with missing type parameters or where clauses results in error.