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

Fix docstring search by signatures revisited #54324

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

projekter
Copy link

This is an extension of #53824 (therefore also fixing #52669), where details on the issue can be found (and, related to this, a different area where the issue shows, namely when you try to include documentation for a particular method of a function, which is similarly impossible when type parameters are involved).
The fix proposed there had two drawbacks:

  • with it, it is no longer possible to discriminate between two constructor functions that differ only in the type parameters. This is remedied with this PR by adding the parameters of the constructor as a unique type to the front of the method signature.
  • with it, constructors must specify type parameters explicitly. The previous behavior is restored with this PR, which allows to have the type parameters unspecified.
    However, I completely agree with @matthias314's analysis that in this way, it is impossible to make the distinction between existing types and parametric types (which cannot be done well during macro deconstruction). This is also an issue in the code currently in Base; any type parameter will always be seen as parametric. For this reason, I'd rather see this whole behavior disappear; in the end, you cannot write Julia code like this. Removing the if isempty would prevent such a usage. A kind of middle ground would be to allow the current behavior, but only if the type is constrained (because this would not be possible for existing types).

@LilithHafner LilithHafner added the needs tests Unit tests are required for this change label May 10, 2024
Copy link
Member

@LilithHafner LilithHafner left a comment

Choose a reason for hiding this comment

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

Thanks for making this PR! I can't do a full review right now, but hopefully we can find someone who is willing to.

Base.unsafe_copyto!{T}(::Ptr{T}, ::Ptr{T}, ::Any)
Base.unsafe_copyto!(::Ptr{T}, ::Ptr{T}, ::Any) where T
Copy link
Member

Choose a reason for hiding this comment

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

These changes seem reasonable/good/bugfixes, but I'm worried how breaking they might be to docsystem building across the ecosystem.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to figure this out? From some JuliaCon talks for example I get the impression that it is possible to test many or all packages with a modified Julia version and see what happens. If the documentation of a package would require changes, then this would result in an error during precompilation or when building the docs.

Copy link
Member

Choose a reason for hiding this comment

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

There is nanosoldier, but that only runs testsuites which often do not include building docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

In Base, most changes occur in docstrings (as opposed to separate documentation files), and a missing change gives an error during precompilation. So with nanosoldier we might still get an idea of how big the problem really is.

Copy link
Author

Choose a reason for hiding this comment

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

I would also have thought that this issue should be detected by Nanosoldier.
The only way to circumvent this possible incompatibility is if we decide to formally allow this syntax for functions. (But then, it should be documented somewhere.)
Then, it should also formally be defined show this syntax is interpreted. If we just say that in the documentation, fun{X}(args) is exactly equivalent to fun(args) where {X}, then it is clear that everything in X is a type variable (with possible subtyping). Apart from the case in which fun is the name of a type/constructor, as then it could also be a concrete type (or some other bitstype). I previously thought that it is not easily possible for the macro-called function determining which is the case, but as I now think of it, I'm no longer so sure. For outer constructors, for sure when the macro is called, the type must already exist. And in order for the Julia compiler to accept fun{X} without X being in the where clause, X must also be well defined (in the context of the calling module, which is available to the macro). I don't know how it is when the docstring is attached to an inner constructor, though.
Then, in the specification of what fun{X}(args) means when fun is a constructor, should we allow mixing actual generic types (coming first?) with parametric types from the arguments (pretty clumsy IMHO)? Or just disallow this shorthand notation?

@matthias314
Copy link
Contributor

[I'm moving the discussion out of the code review because it has a wider scope]

In my opinion, we should not allow the syntax f{T}(x::T) for docstrings if f is a function (as opposed to a constructor). For constructors, I also find it bad syntax because it cannot distinguish between actual types and type parameters. If T is a parameter, then one should write f{T}(x::T) where T. However, the currently allowed syntax might be so widespread in the existing documentation that it might be better not to change it. After all, replacing a type parameter by an actual type should not happen often. I still think that it would be helpful to run nanosoldier (or whatever tool is appropriate) over a large collection of packages to see how much would break with disallowing this syntax.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests Unit tests are required for this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants