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

Figure out a way to link to downstream crates #74481

Open
jyn514 opened this issue Jul 18, 2020 · 42 comments
Open

Figure out a way to link to downstream crates #74481

jyn514 opened this issue Jul 18, 2020 · 42 comments
Labels
A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name C-enhancement Category: An issue proposing an enhancement or a PR with one. E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@jyn514
Copy link
Member

jyn514 commented Jul 18, 2020

After #73101 there is no way to link to items in downstream crates. This is fairly common to try; futures does this extensively (#64193), as does core/std (#73423).

On one hand, this clearly breaks rust's scoping rules: inner crates can't refer to the types of their dependencies because it causes a cyclic dependency. So there's an argument to be made for saying this is just not supported.

On the other hand, this seems really useful. In fact it used to work for re-exports from the inner to outer crate, but only when you documented both crates at the same time, and the outer crate after the inner crate (this was #73699). So it would be great to find a way to make it work.

cc @Manishearth, @Nemo157

@jyn514 jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. C-enhancement Category: An issue proposing an enhancement or a PR with one. A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name labels Jul 18, 2020
@jyn514
Copy link
Member Author

jyn514 commented Jul 18, 2020

@Nemo157 also suggested that this be supported through cargo:

nemo157: One thing I mentioned a long time ago was that a concept of doc-dependencies would be useful, so that you could link to other crates just while building your documentation. My original use case being for futures-util to be able to link up to the futures facade in places
jynelson: how would that work with cyclic dependencies? does cargo allow that?
nemo157: they wouldn't have to be necessarily cyclic, futures-util(doc) would depend on futures(lib) would depend on futures-util(lib)
nemo157: similar to how you can have what looks like cyclic dependencies with dev-dependencies
jynelson: isn't futures-util(doc) the same crate as futures-util(lib) from the perspective of rustc?
or does it not matter about rustc and we only care about cargo?
nemo157: it might be now, but I think it could be setup that the current crate is built into metadata, then that metadata is passed to rustdoc when building the doc

@Manishearth
Copy link
Member

I don't think cargo would support something like that. That still introduces a cyclic dependency, even if it's technically feasible. Cargo doesn't really distinguish in the crate graph

@Nemo157
Copy link
Member

Nemo157 commented Jul 18, 2020

It would be good to clarify "upstream", my assumption would be that an "upstream crate" is a dependency, which should work with intradoc links?

@jyn514
Copy link
Member Author

jyn514 commented Jul 18, 2020

Oops, yeah I got upstream and downstream mixed up.

@jyn514 jyn514 changed the title Figure out a way to link to upstream crates Figure out a way to link to downstream crates Jul 18, 2020
@Nemo157
Copy link
Member

Nemo157 commented Jul 18, 2020

@Manishearth cargo already allows "cyclic" dependencies for dev-dependencies, since they're not really cyclic in its build graph. Changing it to pass the current crate and dependents of it in as metadata when it calls rustdoc seems relatively trivial. The rustdoc side changes to support a sort of mixed source-files+metadata for the crate being documented is more difficult (especially with cfg(doc)), and after thinking about it more since the above discussion I'm not sure if it's actually feasible.

@Manishearth
Copy link
Member

@Nemo157 it's not trivial: dev-dependencies are dependencies of the test crates, which are independent crates. That's not the case for docs.

even if we somehow solve this in cargo, the rustc infrastructure rustdoc relies on would not work.

@Manishearth
Copy link
Member

So another thought I had: Perhaps the solution for this is not in intra doc links. I'm not a fan of making intra doc links behave outside of the scope of paths and having forward decls: it's a major infrastructure change for rather minor benefit. The only main place that benefits is std which is designed as a facade crate.

An alternate idea is to have a $DOC_ROOT, i.e. you can define links as [foo]: $DOC_ROOT/std/blah. These are only for forward decls, and they require a feature gate that we do not plan to stabilize.

Thoughts? @rust-lang/rustdoc

@jyn514
Copy link
Member Author

jyn514 commented Jul 18, 2020

This does not solve the more general case of linking to downstream crates :/ I'm ok with working around this for std but I'd like to eventually have something that works for futures and other non-std crates.

@Manishearth
Copy link
Member

I kind of feel like there shouldn't be such a solution for other crates.

Though I'd be open to eventually stabilizing DOC_ROOT

@GuillaumeGomez
Copy link
Member

I'm not much in favor for this and I really don't like the DOC_ROOT idea (not a fan of "environment variables", or variables more generally for intra-doc links). If someone wants to link to a downstream crate, why not reexporting it and then use this reexport?

@Nemo157
Copy link
Member

Nemo157 commented Jul 20, 2020

DOC_ROOT won't work for docs.rs, each release has an independent DOC_ROOT.

I don't actually remember what the usecase for this in futures was, the overwhelming majority of its links are within-crate or to dependencies which appear to work ok with the cross-crate inlining fixes. I'll try and find some of the places where we did want to link up to the facade from the components.

@Manishearth
Copy link
Member

@GuillaumeGomez I don't see how reexporting works? If libcore wants to link to std::String it can't, right now.

DOC_ROOT is not an environment variable, it's just a signal to rustdoc to replace it.

@Manishearth
Copy link
Member

@Nemo157 oh, hmm, that's a fair point.

@GuillaumeGomez
Copy link
Member

Ah I see, for me it's upstream looking "up" this way. This explains my confusion. I still don't think this is a good idea, a crate shouldn't know about its users. And there is also the issue @Nemo157 brought up too. :)

@jyn514
Copy link
Member Author

jyn514 commented Jul 20, 2020

I don't see how reexporting works? If libcore wants to link to std::String it can't, right now.

One possibility is to resolve links on the extra docs on the re-export relative to the current crate. So

/// Link to [`crate::string::String`]
pub use core::str::str;

Would work correctly from std and the docs just wouldn't be present in libcore. That's really annoying for the end user though and also requires we fix the bug that it's currently resolved relative to the original crate.

@Manishearth
Copy link
Member

@GuillaumeGomez Yeah, I agree. I would rather we didn't support this.

This ties in a bit to a wishlist item i've had for ages where Rust understands the concept of a "package": multiple rust crates that are friends and released together. Like all the futures-foo crates, or the regex-foo crates. They'd have special dispensation when it comes to breaking coherence rules, but you'd still be able to pull in a fraction of a package if you wish. These could be documented together and would also fix the docs.rs problem.

However, that's waaay out of scope for this.

I still think using DOC_ROOT for libcore and libstd (which never goes on docs.rs) should be okay. After @Nemo157's comment I am against ever stabilizing that.

@jyn514
Copy link
Member Author

jyn514 commented Jul 28, 2020

An idea @Mark-Simulacrum came up with is to have 'unchecked' intra-doc links: link to a fully-specified type like struct@::some_crate::path::to::Foo and have rustdoc 'optimistically' generate the link, without checking whether it exists.

One possible issue here is how to determine the doc root for the crate - docs.rs uses --extern-html-root extensively, but it can only pass that flag for crates it knows about, i.e. dependencies.

@jyn514
Copy link
Member Author

jyn514 commented Jul 28, 2020

I guess since docs.rs allows custom flags crate authors could add rustdoc-args = ["--extern-html-root=docs.rs/futures/0.x"]. But that seems like a really giant hack.

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Jul 28, 2020

I think to start we could aim to support local usage, and optimistically match the layout generated by Cargo's normal invocation in the same working directory of rustdoc.

Supporting docs.rs I would do via doc-dependencies (in a metadata section, like docs.rs already does), so something like:

[package.metadata.docs-rs]
links-to = ["futures:0.3", "tokio:0.2"]

That can then generate something like --extern-html-root futures=docs.rs/futures/0.x. We could also support renaming like Cargo does, in theory.

I would personally still feel like we could formalize this idea of doc-dependencies and have Cargo (or whoever) generate similar html-roots for rustdoc.

@Manishearth
Copy link
Member

I think the problem with doc-dependencies is that making that kind of thing work really requires completely changing the model of how rustdoc builds things, and it's really not worth it to do that just to get facade intra-doc-links working,

@Mark-Simulacrum
Copy link
Member

I feel like that's only true if these links are to be checked? That was my impression of our discussion yesterday - that we could indeed generate links if given a fully qualified path and namespace. Larger projects like rust-lang/rust already run linkcheck and similar, so there we would get most of the benefits already.

saona-raimundo added a commit to saona-raimundo/fluent-rs that referenced this issue Jun 13, 2022
There are many links that I could not fix in the luent crate, but they are not a problem in the luent-bundle crate. I do not understand why these problems arise.

Relevant readings on Rust docs:
- Last addition to rustdoc capacities: https://rust-lang.github.io/rfcs/1946-intra-rustdoc-links.html
- Frontend/backend crates pattern: https://users.rust-lang.org/t/cross-crate-documentation-links-in-a-workspace/67588
- Help linking to an external crate (for which we want to link only on the documentation level): rust-lang/api-guidelines#186
- Open issue on how to do that: rust-lang/rust#74481
zbraniecki pushed a commit to projectfluent/fluent-rs that referenced this issue Jun 13, 2022
There are many links that I could not fix in the luent crate, but they are not a problem in the luent-bundle crate. I do not understand why these problems arise.

Relevant readings on Rust docs:
- Last addition to rustdoc capacities: https://rust-lang.github.io/rfcs/1946-intra-rustdoc-links.html
- Frontend/backend crates pattern: https://users.rust-lang.org/t/cross-crate-documentation-links-in-a-workspace/67588
- Help linking to an external crate (for which we want to link only on the documentation level): rust-lang/api-guidelines#186
- Open issue on how to do that: rust-lang/rust#74481
eggyal pushed a commit to eggyal/copse that referenced this issue Jan 9, 2023
Previously, `BTreeMap` tried to link to `crate::collections`, intending
for the link to go to `std/collections/index.html`. But `BTreeMap` is
defined in `alloc`, so after the fix in the previous commit, the links
instead went to `alloc/collections/index.html`, which has almost no
information.

This changes it to link to `index.html`, which only works when viewing
from `std::collections::BTreeMap`, the most common place to visit the
docs. Fixing it to work from anywhere would require the docs for
`std::collections` to be duplicated in `alloc::collections`, which in
turn would require HashMap to be `alloc` for intra-doc links to work
(rust-lang/rust#74481).
github-merge-queue bot pushed a commit to bevyengine/bevy that referenced this issue Jul 31, 2023
# Objective

This PR continues #8885

It aims to improve the `Mesh` documentation in the following ways:
- Put everything at the "top level" instead of the "impl".
- Explain better what is a Mesh, how it can be created, and that it can
be edited.
- Explain it can be used with a `Material`, and mention
`StandardMaterial`, `PbrBundle`, `ColorMaterial`, and
`ColorMesh2dBundle` since those cover most cases
- Mention the glTF/Bevy vocabulary discrepancy for "Mesh"
- Add an image for the example
- Various nitpicky modifications

## Note

- The image I added is 90.3ko which I think is small enough?
- Since rustdoc doesn't allow cross-reference not in dependencies of a
subcrate [yet](rust-lang/rust#74481), I have a
lot of backtick references that are not links :(
- Since rustdoc doesn't allow linking to code in the crate (?) I put
link to github directly.
- Since rustdoc doesn't allow embed images in doc
[yet](rust-lang/rust#32104), maybe
[soon](rust-lang/rfcs#3397), I had to put only a
link to the image. I don't think it's worth adding
[embed_doc_image](https://docs.rs/embed-doc-image/latest/embed_doc_image/)
as a dependency for this.
cart pushed a commit to bevyengine/bevy that referenced this issue Aug 10, 2023
# Objective

This PR continues #8885

It aims to improve the `Mesh` documentation in the following ways:
- Put everything at the "top level" instead of the "impl".
- Explain better what is a Mesh, how it can be created, and that it can
be edited.
- Explain it can be used with a `Material`, and mention
`StandardMaterial`, `PbrBundle`, `ColorMaterial`, and
`ColorMesh2dBundle` since those cover most cases
- Mention the glTF/Bevy vocabulary discrepancy for "Mesh"
- Add an image for the example
- Various nitpicky modifications

## Note

- The image I added is 90.3ko which I think is small enough?
- Since rustdoc doesn't allow cross-reference not in dependencies of a
subcrate [yet](rust-lang/rust#74481), I have a
lot of backtick references that are not links :(
- Since rustdoc doesn't allow linking to code in the crate (?) I put
link to github directly.
- Since rustdoc doesn't allow embed images in doc
[yet](rust-lang/rust#32104), maybe
[soon](rust-lang/rfcs#3397), I had to put only a
link to the image. I don't think it's worth adding
[embed_doc_image](https://docs.rs/embed-doc-image/latest/embed_doc_image/)
as a dependency for this.
@sam0x17
Copy link

sam0x17 commented Aug 28, 2023

I do think the real solution to problems like this is to allow resolving links within dev-dependencies as a fallback for when the normal link resolution behavior cannot find an item, and the same could apply to #[cfg(doc)] code referencing dependencies in general -- if dev-dependencies is currently available, it should be checked for the missing item when running cargo doc that can't be found in dependencies.

Consider this issue from docify:

If you try to build something like this with docify as only a dev-dependency, build will fail:

[dev-dependencies]
docify = "0.2"
//! ### Interned Example
#![cfg_attr(doc, doc = docify::embed_run!("tests/tests.rs", test_interned_showcase))]
error[E0433]: failed to resolve: use of undeclared crate or module `docify`
  --> src/lib.rs:38:24
   |
38 | #![cfg_attr(doc, doc = docify::embed_run!("tests/tests.rs", test_interned_showcase))]
   |                        ^^^^^^ use of undeclared crate or module `docify`

If we've decided adding doc-dependencies would increase complexity too much, we should at least make cargo doc respect dev-dependencies the way it is supposed to. It is a bit silly to have #[cfg(doc)] if there is no way to have dependencies that correspond with this cfg switch. Frankly I would consider the inability to do this to be a bug.

(originally mentioned in rust-lang/cargo#8905 (comment))

@Manishearth
Copy link
Member

I think that's got the same architectural challenges as doc-dependencies, really, because doc targets currently work by building the crate itself and then producing a different artifact, unlike dev targets which are effectively new crates that depend on the main crate (and thus can depend on things that depend on the crate itself).

The path to fixing this would be to explicitly make rustdoc consume crate metadata of an already compiled crate rather than intercepting compilation. This ought to be doable; and might even just be good practice, but I suspect it'll be quite hard.

If we're going to making this architectural change anyway I'd prefer to do it with some separation (it's already frustrating that you can't do dev-dependencies at a per target level).

cargo doc respect dev-dependencies the way it is supposed to

I think this is a pretty subjective judgement, it definitely is not intended to work that way

@sam0x17
Copy link

sam0x17 commented Aug 28, 2023

I think this is a pretty subjective judgement, it definitely is not intended to work that way

That's fair. The lore I had encountered when asking around the community is "cargo doc is supposed to be under the auspices of dev-dependencies because doc tests", but that definitely isn't true in reality.

I would push back a bit by saying it is wild that we have #[cfg(doc)] as a top-level / first-class cfg option but no way to gate dependencies based on it.

An alternative solution would be greater flexibility in Cargo.toml when specifying optional dependencies, like if instead of just feature = we could specify a full cfg expression, then we could specify doc and that would also solve this without having to add a whole [doc-dependencies] table, though I suspect this might be just as complex as any solution based on what you're saying?

@Manishearth
Copy link
Member

Manishearth commented Aug 28, 2023

That's fair. The lore I had encountered when asking around the community is "cargo doc is supposed to be under the auspices of dev-dependencies because doc tests", but that definitely isn't true in reality.

Yeah, the actual thing is that doc tests are under the auspices of dev-dependencies because they are tests. Docs themselves are not.

I would push back a bit by saying it is wild that we have #[cfg(doc)] as a top-level / first-class cfg option but no way to gate dependencies based on it.

Yes, but either way that would not let you do cyclic dependencies.

though I suspect this might be just as complex as any solution based on what you're saying?

More complex and a violation of separation of responsibilities because a lot of cfg things are a compiler-specific thing.

And also it would not solve this problem.

I would in general not be too excited about solutions that don't solve the particular problem of linking downstream because in my experience that is the number one reason people typically need doc dependencies.

@sam0x17
Copy link

sam0x17 commented Aug 28, 2023

I would in general not be too excited about solutions that don't solve the particular problem of linking downstream because in my experience that is the number one reason people typically need doc dependencies.

I definitely agree and I would add it's not always downstream, there is very often the desire to link to items in associated crates like crates often used with your crate that you have no dependency relationship with (in either direction). Right now talking about items like these is basically discouraged because there is no easy way to link to them without them being a direct dependency.

Ideally we should be able to link to any item in any crate without affecting the dependency tree other than when docs are compiling.

Annoyingly, we can use items from "associated" crates in doc tests via dev-dependencies, but then we can't talk about these same items in the text surrounding the doc test! It would make a lot of sense to be able to link to all items mentioned in a doc test in the surrounding docs.

@Manishearth
Copy link
Member

there is very often the desire to link to items in associated crates like crates often used with your crate that you have no dependency relationship with (in either direction).

Yes, but I highlight downstream because it's a crucial component, and I think work in this feature space in a way that does not support that will eventually lead to two ways of doing things, so in general I would like to focus on solutions that do allow for downstream stuff; which does seem to be the majority use case and also the hardest one.

@est31
Copy link
Member

est31 commented Aug 29, 2023

The path to fixing this would be to explicitly make rustdoc consume crate metadata of an already compiled crate rather than intercepting compilation.

FTR one doesn't have to switch all of rustdoc to such a scheme, it's enough to add either an entirely new driver, or something that uses the rustdoc or rustc drivers to, for a given input path and metadata file, resolve that path, and outputs a relative html url (or a json diagnostic if the resolution failed). As in, one doesn't have to touch the main rustdoc path that generate the html. That path would shell out to such a command. It would be a flag like rustdoc -C resolve-path=hello::world::Foo, plus some span info that the diagnostic would need.

IMO a "link to downstream/external" feature should only be weakly coupled into the existing resolution and metadata system, precisely for the architectural concerns.

No changes to the fundamental nature of rustdoc would be needed for the normal non-resolution mode of rustdoc. Of course, this still requires implementing a mode that reads the metadata of the specific crate. One would also need cargo integration of doc-dependencies.

Usually there aren't that many links to downstream crates, but if you want to make it faster, you could pass multiple paths you are interested in e.g. via json.

Similar things are already done with the doctests where rustdoc create a fully fledged compiler environment, it instead spawns the rustc process. Here too, a .rs file is created on disk because passing all of the test on the command line isn't really great :).

Regarding the points on the mental model, I think the point of view that is the most important for documentation is that of the readers. Readers generally have the entire problem domain in mind, including downstream or unrelated crates. E.g. when I read the core docs, I want to be able to see links to alloc or std, maybe so that I know that these things I really want to hide behind an alloc cargo feature. Even though I'm reading the core docs, my mental model includes the alloc and std crates as well.

@Manishearth
Copy link
Member

Manishearth commented Aug 29, 2023

Yep, such a design might work! It's still a fair amount of work to get all that working, though.

Honestly, I feel that making the model more like test targets where there's a separate target that is allowed to depend on anything is probably better in the long run over doing this kind of frankenbuild. But it depends.

@sam0x17
Copy link

sam0x17 commented Aug 31, 2023

I feel that making the model more like test targets where there's a separate target that is allowed to depend on anything is probably better in the long run over doing this kind of frankenbuild. But it depends.

I think this matches a lot of people's mental models for how they think it already works anyway

@clarfonthey
Copy link
Contributor

Just going to add my comment from #121436 (which I filed without noticing this issue): I do agree that the test target being its own thing is a good argument for supporting this, but I think it's also worth reiterating that doc tests also fall into this category, and those themselves are managed by rustdoc. So, it feels appropriate that rustdoc would leverage something similar to what it does for doctests when doing intra-doc links, being able to access dev-dependencies crates or something like that.

Additionally, at least in libstd, it manages to have access to the normal std prelude in doctests, seemingly as some kind of weird hack. I assume that some kind of hack could be done to make linking std items work, but I also know nothing and am essentially just guessing.

@RalfJung
Copy link
Member

RalfJung commented Mar 8, 2024

Doctests simply shell out to a new rustc instance (basically rustdoc dumps the doctest code into a file after adding fn main() { ... } around it, then launches rustc), which then has access to the regular sysroot -- no hack required. rusdoc doesn't actually do any name resolution in the doctest itself. So the way a doctest gets interpreted has nothing to do with the way docs get interpreted. There's a reason cfg(doc) and cfg(doctest) are separate things.

But yeah not being a able to link to std items from core is repeatedly a pain and sometimes leads to us writing worse docs since we can't add the links we need to add.

dekirisu pushed a commit to dekirisu/bevy_gltf_trait that referenced this issue Jul 7, 2024
# Objective

This PR continues bevyengine/bevy#8885

It aims to improve the `Mesh` documentation in the following ways:
- Put everything at the "top level" instead of the "impl".
- Explain better what is a Mesh, how it can be created, and that it can
be edited.
- Explain it can be used with a `Material`, and mention
`StandardMaterial`, `PbrBundle`, `ColorMaterial`, and
`ColorMesh2dBundle` since those cover most cases
- Mention the glTF/Bevy vocabulary discrepancy for "Mesh"
- Add an image for the example
- Various nitpicky modifications

## Note

- The image I added is 90.3ko which I think is small enough?
- Since rustdoc doesn't allow cross-reference not in dependencies of a
subcrate [yet](rust-lang/rust#74481), I have a
lot of backtick references that are not links :(
- Since rustdoc doesn't allow linking to code in the crate (?) I put
link to github directly.
- Since rustdoc doesn't allow embed images in doc
[yet](rust-lang/rust#32104), maybe
[soon](rust-lang/rfcs#3397), I had to put only a
link to the image. I don't think it's worth adding
[embed_doc_image](https://docs.rs/embed-doc-image/latest/embed_doc_image/)
as a dependency for this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name C-enhancement Category: An issue proposing an enhancement or a PR with one. E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants