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

Tracking issue for RFC 1946 - intra-rustdoc links #43466

Open
nrc opened this Issue Jul 25, 2017 · 58 comments

Comments

Projects
None yet
@nrc
Copy link
Member

nrc commented Jul 25, 2017

RFC: https://github.com/rust-lang/rfcs/blob/master/text/1946-intra-rustdoc-links.md
RFC PR: rust-lang/rfcs#1946

Todo:

  • Land #47046
  • Make it work with cross-crate imports
  • Make it warn more on resolution errors (#49542)
  • Make warnings/errors use better spans than the first line of docs
    • first step: attribute (#51111)
    • point to actual link in doc comment (#51391)
    • Add lint for link resolution failure (#51382)
  • Implied Shortcut Reference Links (#48335)
  • Enum variants and UFCS/associated methods (#47701)
  • Primitives (#49459)
  • Use the right resolution scopes for inner doc comments on things that aren't modules (#55364)
  • Handle fields and variants (#49512)
    • Handle fields inside struct variants
  • Support Self (#49583)
  • Stabilization (current impl is gated to only run on nightly, but does not require a feature gate)

Other related issues:

  • provide option to turn intra-link resolution errors into proper errors (#50082)
@QuietMisdreavus

This comment has been minimized.

Copy link
Member

QuietMisdreavus commented Sep 19, 2017

Preliminary implementation notes:

Getting just the links from the markdown is relatively straightforward. You'll need to do this twice, to capture the behavior in both the Hoedown and Pulldown markdown renderers. Thankfully, each has a way of capturing links as they are parsed from the document. (While capturing the link you can also look for the "struct/enum/mod/macro/static/etc" marker to aid in resolution down the line.)

In Hoedown, the render function in src/librustdoc/html/markdown.rs captures all the events as they are captured. Hoedown uses a system of registering callbacks on the renderer configuration to capture each event separately. The link event and its related linkfn function pointer signature are already present in the file, so you would need to create and use that callback to capture and modify links as they appear.

For Pulldown, the parser is surfaced as an Iterator of events. There are already a handful of iterator wrappers present in html/markdown.rs, so adding another one (and using it in the impl fmt::Display for Markdown farther down) shouldn't be a big deal. I'm... honestly not that familiar with how Pulldown represents link definitions in its Event/Tag enums, so more research would be needed to effectively get link text for that.

Now, with the links in hand, you need to do two things, neither of which i know offhand since they involve interfacing with the compiler:

  • Attempt to parse the link as a path (if it's not a valid Rust path, just emit the URL as-is and skip the rest of this)
  • Resolve the path in the "current" module (emit a warning if resolution fails)

Presumably there's a way to make the resolution come up with a DefId? Even after cleaning the AST, rustdoc still deals in those, so if that's the case there are ways to map that to the proper relative link.

@Manishearth

This comment has been minimized.

Copy link
Member

Manishearth commented Dec 28, 2017

I've got a WIP going on in https://github.com/manishearth/rust/tree/correct-resolver-fail , which works off of Misdreavus' WIP (they did most of the work).

@Manishearth Manishearth referenced this issue Dec 28, 2017

Merged

Implement RFC 1946 - intra-rustdoc links #47046

6 of 12 tasks complete
@QuietMisdreavus

This comment has been minimized.

Copy link
Member

QuietMisdreavus commented Dec 28, 2017

Note that that branch is now live as #47046, which i'm polishing and finishing up.

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

QuietMisdreavus commented Jan 8, 2018

Note for people watching this thread but not #47046: We're trying to wind that PR down to get a minimal version landed, so an initial implementation is fairly close.

bors added a commit that referenced this issue Jan 22, 2018

Auto merge of #47046 - Manishearth:intra-doc-links, r=eddyb,Guillaume…
…Gomez,QuietMisdreavus,Manishearth

Implement RFC 1946 - intra-rustdoc links

rust-lang/rfcs#1946 #43466

Note for reviewers: The plain line counts are a little inflated because of how the markdown link parsing was done. [Read the file diff with "whitespace only" changes removed](https://github.com/rust-lang/rust/pull/47046/files?w=1) to get a better view of what actually changed there.

This pulls the name/path resolution mechanisms out of the compiler and runs it on the markdown in a crate's docs, so that links can be made to `SomeStruct` directly rather than finding the folder path to `struct.SomeStruct.html`. Check the `src/test/rustdoc/intra-paths.rs` test in this PR for a demo. The change was... a little invasive, but unlocks a really powerful mechanism for writing documentation that doesn't care about where an item was written to on the hard disk.

Items included:

 - [x] Make work with the hoedown renderer
 - [x] Handle relative paths
 - [x] Parse out the "path ambiguities" qualifiers (`[crate foo]`, `[struct Foo]`, `[foo()]`, `[static FOO]`, `[foo!]`, etc)
 - [x] Resolve foreign macros
 - [x] Resolve local macros
 - [x] Handle the use of inner/outer attributes giving different resolution scopes (handling for non-modules pushed to different PR)

Items not included:

 - [ ] Make sure cross-crate inlining works (blocked on refactor described in #47046 (comment))
 - [ ] Implied Shortcut Reference Links (where just doing `[::std::iter::Iterator][]` without a reference anchor will resolve using the reference name rather than the link target) (requires modifying the markdown parser - blocked on Hoedown/Pulldown switch and raphlinus/pulldown-cmark#121)
 - [ ] Handle enum variants and UFCS methods (Enum variants link to the enum page, associated methods don't link at all)
 - [ ] Emit more warnings/errors when things fail to resolve (linking to a value-namespaced item without a qualifier will emit an error, otherwise the link is just treated as a url, not a rust path)
 - [ ] Give better spans for resolution errors (currently the span for the first doc comment is used)
 - [ ] Check for inner doc comments on things that aren't modules

I'm making the PR, but it should be noted that most of the work was done by Misdreavus 😄

(Editor's note: This has become a lie, check that commit log, Manish did a ton of work after this PR was opened `>_>`)

bors added a commit that referenced this issue Jan 22, 2018

Auto merge of #47046 - Manishearth:intra-doc-links, r=eddyb,Guillaume…
…Gomez,QuietMisdreavus,Manishearth,killercup

Implement RFC 1946 - intra-rustdoc links

rust-lang/rfcs#1946 #43466

Note for reviewers: The plain line counts are a little inflated because of how the markdown link parsing was done. [Read the file diff with "whitespace only" changes removed](https://github.com/rust-lang/rust/pull/47046/files?w=1) to get a better view of what actually changed there.

This pulls the name/path resolution mechanisms out of the compiler and runs it on the markdown in a crate's docs, so that links can be made to `SomeStruct` directly rather than finding the folder path to `struct.SomeStruct.html`. Check the `src/test/rustdoc/intra-paths.rs` test in this PR for a demo. The change was... a little invasive, but unlocks a really powerful mechanism for writing documentation that doesn't care about where an item was written to on the hard disk.

Items included:

 - [x] Make work with the hoedown renderer
 - [x] Handle relative paths
 - [x] Parse out the "path ambiguities" qualifiers (`[crate foo]`, `[struct Foo]`, `[foo()]`, `[static FOO]`, `[foo!]`, etc)
 - [x] Resolve foreign macros
 - [x] Resolve local macros
 - [x] Handle the use of inner/outer attributes giving different resolution scopes (handling for non-modules pushed to different PR)

Items not included:

 - [ ] Make sure cross-crate inlining works (blocked on refactor described in #47046 (comment))
 - [ ] Implied Shortcut Reference Links (where just doing `[::std::iter::Iterator][]` without a reference anchor will resolve using the reference name rather than the link target) (requires modifying the markdown parser - blocked on Hoedown/Pulldown switch and raphlinus/pulldown-cmark#121)
 - [ ] Handle enum variants and UFCS methods (Enum variants link to the enum page, associated methods don't link at all)
 - [ ] Emit more warnings/errors when things fail to resolve (linking to a value-namespaced item without a qualifier will emit an error, otherwise the link is just treated as a url, not a rust path)
 - [ ] Give better spans for resolution errors (currently the span for the first doc comment is used)
 - [ ] Check for inner doc comments on things that aren't modules

I'm making the PR, but it should be noted that most of the work was done by Misdreavus 😄

(Editor's note: This has become a lie, check that commit log, Manish did a ton of work after this PR was opened `>_>`)

bors added a commit that referenced this issue Jan 23, 2018

Auto merge of #47046 - Manishearth:intra-doc-links, r=eddyb,Guillaume…
…Gomez,QuietMisdreavus,Manishearth

Implement RFC 1946 - intra-rustdoc links

rust-lang/rfcs#1946 #43466

Note for reviewers: The plain line counts are a little inflated because of how the markdown link parsing was done. [Read the file diff with "whitespace only" changes removed](https://github.com/rust-lang/rust/pull/47046/files?w=1) to get a better view of what actually changed there.

This pulls the name/path resolution mechanisms out of the compiler and runs it on the markdown in a crate's docs, so that links can be made to `SomeStruct` directly rather than finding the folder path to `struct.SomeStruct.html`. Check the `src/test/rustdoc/intra-paths.rs` test in this PR for a demo. The change was... a little invasive, but unlocks a really powerful mechanism for writing documentation that doesn't care about where an item was written to on the hard disk.

Items included:

 - [x] Make work with the hoedown renderer
 - [x] Handle relative paths
 - [x] Parse out the "path ambiguities" qualifiers (`[crate foo]`, `[struct Foo]`, `[foo()]`, `[static FOO]`, `[foo!]`, etc)
 - [x] Resolve foreign macros
 - [x] Resolve local macros
 - [x] Handle the use of inner/outer attributes giving different resolution scopes (handling for non-modules pushed to different PR)

Items not included:

 - [ ] Make sure cross-crate inlining works (blocked on refactor described in #47046 (comment))
 - [ ] Implied Shortcut Reference Links (where just doing `[::std::iter::Iterator][]` without a reference anchor will resolve using the reference name rather than the link target) (requires modifying the markdown parser - blocked on Hoedown/Pulldown switch and raphlinus/pulldown-cmark#121)
 - [ ] Handle enum variants and UFCS methods (Enum variants link to the enum page, associated methods don't link at all)
 - [ ] Emit more warnings/errors when things fail to resolve (linking to a value-namespaced item without a qualifier will emit an error, otherwise the link is just treated as a url, not a rust path)
 - [ ] Give better spans for resolution errors (currently the span for the first doc comment is used)
 - [ ] Check for inner doc comments on things that aren't modules

I'm making the PR, but it should be noted that most of the work was done by Misdreavus 😄

(Editor's note: This has become a lie, check that commit log, Manish did a ton of work after this PR was opened `>_>`)
@sgrif

This comment has been minimized.

Copy link
Contributor

sgrif commented Jan 23, 2018

One thing that appears to not work in #47046 (and I don't think is ever explicitly addressed in the RFC) is how to link to primitives like u32 or bool.

@Manishearth

This comment has been minimized.

Copy link
Member

Manishearth commented Jan 24, 2018

Yeah I have to investigate that

@Manishearth

This comment has been minimized.

Copy link
Member

Manishearth commented Jan 24, 2018

@killercup @QuietMisdreavus should we turn the error into a warning ? I don't like the idea of crate docs breaking because someone introduced a name into a glob import

@killercup

This comment has been minimized.

Copy link
Member

killercup commented Jan 24, 2018

@Manishearth does rustdoc have some idea of "lint levels"? What are the current deprecation warnings? Just println!s?

@Manishearth

This comment has been minimized.

Copy link
Member

Manishearth commented Jan 24, 2018

We can just struct_span_warn instead. rustc can emit warnings not tied to actual warning names. There are no lint levels.

@killercup

This comment has been minimized.

Copy link
Member

killercup commented Jan 24, 2018

@Manishearth

This comment has been minimized.

Copy link
Member

Manishearth commented Jan 24, 2018

lawliet89 added a commit to lawliet89/rocket_cors that referenced this issue Feb 14, 2018

@Manishearth

This comment has been minimized.

Copy link
Member

Manishearth commented Feb 18, 2018

Shortcut links being implemented at #48335

Manishearth added a commit to Manishearth/rust that referenced this issue Feb 19, 2018

Rollup merge of rust-lang#48335 - Manishearth:shortcut-links, r=Quiet…
…Misdreavus

Implement implied shortcut links for intra-rustdoc-links

cc rust-lang#43466

Needs raphlinus/pulldown-cmark#126

r? @QuietMisdreavus

Manishearth added a commit to Manishearth/rust that referenced this issue Feb 19, 2018

Rollup merge of rust-lang#48335 - Manishearth:shortcut-links, r=Quiet…
…Misdreavus

Implement implied shortcut links for intra-rustdoc-links

cc rust-lang#43466

Needs raphlinus/pulldown-cmark#126

r? @QuietMisdreavus

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Feb 21, 2018

Rollup merge of rust-lang#48335 - Manishearth:shortcut-links, r=Quiet…
…Misdreavus

Implement implied shortcut links for intra-rustdoc-links

cc rust-lang#43466

Needs raphlinus/pulldown-cmark#126

r? @QuietMisdreavus
@Nemo157

This comment has been minimized.

Copy link
Contributor

Nemo157 commented Oct 5, 2018

I assume that "Make it work with cross-crate imports" in the OP is referring to things like docs on externally-defined trait implementations failing to resolve and causing warnings to appear, even though the docs are valid in the crate that defined the trait? Is this still blocked on something else (and if so, is there somewhere with more details for tracking it than a comment on the old PR)?

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Oct 5, 2018

The issue @MajorBreakfast is hitting in #43466 (comment) is still happening and is a serious blocker for me-- even when building with --no-deps, I'm getting warnings and errors from upstream documentation resolution failures.

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Oct 5, 2018

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Oct 5, 2018

(am unblocked through the magic of RUSTDOC env var, shell scripts, and the --cap-lints setting, but still this should get fixed XD)

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

QuietMisdreavus commented Oct 5, 2018

[dependencies]
futures-preview = "0.3.0-alpha.7"
#![deny(warnings)]

extern crate futures;

This is enough to trigger a failure right now. (I'll need to narrow it down, but using a crate with some specific use of intra-links will work.) I thought i'd fixed this kind of erroneous error with #53162 (comment) but it turns out it wasn't good enough. I'm building/testing a fix right now that refuses to run link collection on non-local items. It may be a bit drastic (and breaks docs which actually work in this situation) but it finally silence this kind of issue until we can actually support them.

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

QuietMisdreavus commented Oct 5, 2018

Narrowed it down:

// inner crate

use std::fmt::Debug;

pub trait SomeTrait {}

/// this is an impl for [SomeTrait]
impl<T: Debug> SomeTrait for T {}

I bet the PR i linked doesn't touch this impl because it's generic, so it could apply to any number of local items.

@SergioBenitez

This comment has been minimized.

Copy link
Contributor

SergioBenitez commented Oct 6, 2018

In Rocket, we have two libraries in a workspace: a core library, rocket, and a contrib library rocket_contrib. rocket_contrib is not a dependency of rocket, but rocket refers to rocket_contrib in its documentation often.

Because rocket_contrib is not a dependency of rocket, cross-crate links are not resolving:

This is [rocket_contrib], a broken link.

Is there any workaround to this? I've tried adding --extern rocket_contrib=path/to.rmeta manually via RUSTDOCFLAGS, as well as using -Z unstable-options --extern-html-root-url rocket_contrib=https://rocket-contrib.docs, but neither work. Even if the latter worked, it isn't a great solution as it'll produce absolute links, partially defeating the purpose.

In general, I think cargo and/or rustdoc should automatically add all crates in the same workspace to the resolution domain for inter-crate links, making something like [rocket_contrb] work out of the box.

@killercup

This comment has been minimized.

Copy link
Member

killercup commented Oct 6, 2018

@SergioBenitez, have you tried adding a docs feature to the rocket lib that you set when building docs and that enables an optional dependency on rocket_contrib?

(Sorry to respond here without having followed this issue or impl much!)

@Nemo157

This comment has been minimized.

Copy link
Contributor

Nemo157 commented Oct 6, 2018

automatically add all crates in the same workspace to the resolution domain for inter-crate links, making something like [rocket_contrb] work out of the box.

This will cause issues with the docs when built outside the workspace, e.g. when someone runs cargo doc -p rocket --open to get locally built docs in their project. Changing RUSTDOCFLAGS or having an optional feature has similar problems.

Earlier I suggested including dev-dependencies during doc builds to solve the same issue, or maybe there could be an additional doc-dependencies section?

@SergioBenitez

This comment has been minimized.

Copy link
Contributor

SergioBenitez commented Oct 6, 2018

@killercup I considered that, but unfortunately that would introduce a cyclic dependency.

@Nemo157 I instinctively tried that - placing the dependency in [dev-dependencies] - actually. Although I secretly hoped it didn't work as I really don't want to build/check contrib when working on core. A special doc-dependencies section would resolve this particular issue if it allowed cyclic dependencies.

@Nemo157

This comment has been minimized.

Copy link
Contributor

Nemo157 commented Oct 25, 2018

Currently it's possible to link to fields of structs via a form of psuedo-path:

/// [`Foo::bar`]
pub struct Foo { pub bar: () }

Similarly it would be useful if it were possible to link to fields of a variant of an enum, probably via the same sort of psuedo-path:

/// [`Bar::Baz::quux`]
pub enum Bar {
    Baz { quux: () }
}
@dekellum

This comment has been minimized.

Copy link

dekellum commented Oct 25, 2018

I'm trying to ascertain the readiness of this feature for release use, and was hoping the tracking issue might help. While it is working great for me on recent nightly builds, on the just released stable rust 1.30.0 (da5f414 2018-10-24), I'm surprised to find that the feature still isn't complete: some links using rust paths aren't resolved to html links.

Is this feature intended to be nightly only? Or is there flags or feature's to be set in order to use it with the rust stable channel? Should I file a new issue for the missed links on 1.30?

@Manishearth

This comment has been minimized.

Copy link
Member

Manishearth commented Oct 25, 2018

@wagenet

This comment has been minimized.

Copy link
Contributor

wagenet commented Oct 25, 2018

@Manishearth I'm not privy to how Rust tends to handle these sorts of things, so I realize this might go against established practice, but it seems to me like it would be useful to have what currently exists in stable, but just not promoted as something that actually works. It's not dangerous and is useful in its current state. I guess the risk is that nobody ever finishes the support for re-exports and we're stuck with a half working feature?

@Manishearth

This comment has been minimized.

Copy link
Member

Manishearth commented Oct 25, 2018

@wagenet

This comment has been minimized.

Copy link
Contributor

wagenet commented Oct 25, 2018

@Manishearth I've found it super useful in its current form, but I can see the worry around things being subtly broken.

@dekellum

This comment has been minimized.

Copy link

dekellum commented Oct 25, 2018

I misinterpreted what I was initially seeing on 1.30: all, not just some, rust path style links (this feature) are not resolved on this latest (and prior) stable releases. Besides that not being explained anywhere I can find, the output is slightly confusing because the implicit type links are shown as obviously broken, e.g. "[SomeStruct]" where as links explicitly using a rust path are included verbatim, e.g. <a href="mod1::SomeStruct">, as would be expected without this feature being present.

As it seems now the intent for this was to be nightly-only until further along, shouldn't this tracking issue be edited to include the following, as it might save the next prospective user some confusion (@nrc)?

  • Stabilization PR (current implementation is nightly only, but not feature gated)

I agree that this is value add as found on current nightly. Authors can just avoid using the new link style for re-exports, as well as links to other crates which are not dependencies, until these are resolved.

But as it stands, its a bit difficult to justify using the feature when it will not generate correct links for a user-local cargo doc even on the latest stable, not to mention the minimum stable rust version. Please consider if keeping this off for stable, with no option or flag, is now more likely to cause trouble with broken links (as this starts getting used with nightly) vs any regressions stabilizing might otherwise cause, given that the new style link targets would never have worked without the feature?

@killercup

This comment has been minimized.

Copy link
Member

killercup commented Oct 25, 2018

Random idea: If it is planned to keep this semi-usable* for a few more months, it might interesting to see if we can make nightly rustdoc optionally print out the generated relative links (with obvious issues when docs are rendered in more than one place). I don't think we have precendence for this, but if you someone wants to have a fun project, they might even try to integrate this with rustfix (i.e., yield 'suggestion-like' output where you appen \n/// [$a]: $b for each link).

* You can use it right now, with the obvious missing features mentioned above. Since docs.rs uses a nightly compiler version where the intra doc links work, a lot of people reading the docs for your crate will see the correct links. People trying to build the docs will be disappointed, though.

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

QuietMisdreavus commented Oct 25, 2018

@dekellum You are correct in your observations: The feature is currently written so that links will only ever resolve when running on a nightly (or locally-compiled) rustdoc. In any other situation, they will be passed through in the way you observed.

The current implementation of intra-doc links is not only incomplete, as @Manishearth mentioned, but is also bug-ridden: There have long been issues of spurious "dead links" being found in crates which aren't using intra-doc links at all. This, combined with the fact that we emit a proper warning lint for these resolution errors, led to situations where innocent crates suddenly broke because they had #![deny(warnings)] set.

To be honest, running this without a proper feature gate is shaky, in terms of how unstable features work in the rest of the compiler and rustdoc, but until recently i was uncertain if we could properly introduce a #![feature] gate to the compiler without a matching use in rustc itself. (All the #![feature] attributes that rustdoc currently uses are tied to attributes, so their check code is also in the compiler.) I would be willing to introduce a feature gate for this, since it would also stamp down on the false positive rate across the ecosystem.

However, it leads to similar broken-link situations. Imagine crate A which uses intra-doc links, complete with having the feature set. Now imagine crate B, which re-exports items from crate A to inline them into its own documentation, but doesn't use intra-doc links itself. Because crate B doesn't have the feature set, now those links from the crate A items won't resolve, even if they were accidentally correct because those items were re-exported into the right positions!

(This situation exists today; these crates are futures and hyper, though the items from futures that hyper re-exports don't have any links on them.)

I'm very hesitant to "partially stabilize" these links, because of the subtleties with re-exports that @Manishearth mentioned. It's too easy to use any part of this feature and get it to a position where it doesn't work. And the major hurdle for making that work requires a deep compiler refactor that is hard to find a champion for. If it comes down to it, if there is another Rust All-Hands early next year like the one in Berlin earlier this year, i may try getting the right people in a room together to design this problem out. However, that won't be able to happen for another few months.

@euclio

This comment has been minimized.

Copy link
Contributor

euclio commented Nov 13, 2018

I wonder if the standard library crates will need special treatment by this feature? For example, the link to the REPLACEMENT_CHAR constant in String docs needs to resolve to std::char::REPLACEMENT_CHAR or core::char::REPLACEMENT_CHAR depending if the String docs are being rendered as part of the libstd or liballoc docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment