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

rustdoc: Can't use anchors in links to primitive types #83083

Closed
taylordotfish opened this issue Mar 13, 2021 · 4 comments · Fixed by #87073
Closed

rustdoc: Can't use anchors in links to primitive types #83083

taylordotfish opened this issue Mar 13, 2021 · 4 comments · Fixed by #87073
Labels
A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name C-bug Category: This is a bug. E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@taylordotfish
Copy link

rustdoc doesn't allow anchors in links to primitive types. For example, given this code:

//! [test](str#examples)

cargo doc produces this warning:

warning: `str#examples` contains an anchor, but links to builtin types are already anchored
 --> src/lib.rs:1:12
  |
1 | //! [test](str#examples)
  |            ^^^^^^^^^^^^ contains invalid anchor
  |
  = note: `#[warn(rustdoc::broken_intra_doc_links)]` on by default

and the resulting link doesn't point to any part of str's documentation, but is rather treated as a normal URL relative to the current page.

The warning claims that “links to builtin types are already anchored”, but this does not appear to be correct. For example, this code:

//! [test](str)

produces a link to https://doc.rust-lang.org/nightly/std/primitive.str.html, which does not contain an anchor.

Meta

rustc 1.52.0-nightly (b3e19a221 2021-03-12)
binary: rustc
commit-hash: b3e19a221e63dcffdef87e12eadf1f36a8b90295
commit-date: 2021-03-12
host: powerpc64le-unknown-linux-gnu
release: 1.52.0-nightly
LLVM version: 12.0.0
@taylordotfish taylordotfish added the C-bug Category: This is a bug. label Mar 13, 2021
@jyn514 jyn514 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Mar 13, 2021
@jyn514
Copy link
Member

jyn514 commented Mar 13, 2021

This was first made an error in 454d13b. At that point there was a comment saying "TODO: give an error", which was first added in 09293be.

@GuillaumeGomez do you remember why you made this an error? #66675 was released in 1.41.0, which still uses the primitive.str.html URL scheme.

@jyn514 jyn514 added the A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name label Mar 13, 2021
@GuillaumeGomez
Copy link
Member

I think I had in mind that the anchors weren't stable and that it wasn't "safe" to use anchors on primitive for this reason. Not sure if it makes much sense though considering we allow it for foreign types...

@jyn514
Copy link
Member

jyn514 commented Mar 14, 2021

Oh, I found why this restriction is here:

/// Most of the time this is fine, because you can just link to the page of
/// the item if you want to provide your own anchor. For primitives, though,
/// rustdoc uses the anchor as a side channel to know which page to link to;
/// it doesn't show up in the generated link. Ideally, rustdoc would remove
/// this limitation, allowing you to link to subheaders on primitives.
RustdocAnchorConflict(Res),

I'm ambivalent whether this should be considered a bug or a feature request; happy for anyone to try and fix it but it won't be trivial. The fragment is used here:
Some(ItemLink { link: ori_link.link, link_text, did: None, fragment })
and later here:
if let Some(ref fragment) = *fragment {
let url = match cache.extern_locations.get(&krate) {
Some(&(_, _, ExternalLocation::Local)) => {
let depth = CURRENT_DEPTH.with(|l| l.get());
"../".repeat(depth)
}
Some(&(_, _, ExternalLocation::Remote(ref s))) => s.to_string(),
Some(&(_, _, ExternalLocation::Unknown)) | None => String::from(
// NOTE: intentionally doesn't pass crate name to avoid having
// different primitive links between crates
if UnstableFeatures::from_environment(None).is_nightly_build() {
"https://doc.rust-lang.org/nightly"
} else {
"https://doc.rust-lang.org"
},
),
};
// This is a primitive so the url is done "by hand".
let tail = fragment.find('#').unwrap_or_else(|| fragment.len());
Some(RenderedLink {
original_text: s.clone(),
new_text: link_text.clone(),
href: format!(
"{}{}std/primitive.{}.html{}",
url,
if !url.ends_with('/') { "/" } else { "" },
&fragment[..tail],
&fragment[tail..]
),
})

@jyn514
Copy link
Member

jyn514 commented Mar 14, 2021

@jyn514 jyn514 added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Mar 14, 2021
@jyn514 jyn514 added the E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. label Mar 19, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Apr 14, 2021
rustdoc: Note that forbidding anchors in links to primitives is a bug

cc rust-lang#83083, rust-lang#84147 (comment)

r? `@cuviper`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Aug 26, 2021
…omez

Fix rustdoc handling of primitive items

This is a complicated PR and does a lot of things. I'm willing to split it up a little more if it would help reviewing, but it would be tricky and I'd rather not unless it's necessary.

 ## What does this do?

- Fixes rust-lang#73423.
- Fixes rust-lang#79630. I'm not sure how to test this for the standard library explicitly, but you can see from some of the diffs from the `no_std` tests. I also tested it locally and it works correctly: ![image](https://user-images.githubusercontent.com/23638587/125214383-e1fdd000-e284-11eb-8048-76b5df958aad.png)
- Fixes rust-lang#83083.

## Why are these changes interconnected?

- Allowing anchors (rust-lang#83083) without fixing the online/offline problem (rust-lang#79630) will actually just silently discard the anchors, that's not a fix. The online/offline problem is directly related to the fragment hack; links need to go through `fn href()` to be fixed.
- Technically I could fix the online/offline problem without removing the error on anchors; I am willing to separate that out if it would be helpful for reviewing. However I can't fix the anchor problem without adding docs to core, since rustdoc needs all those primitives to have docs to avoid a fallback, and currently `#![no_std]` crates don't have docs for primitives. I also can't fix the online/offline problem without removing the fragment hack, since otherwise diffs like this will be wrong for some primitives but not others:
```diff
`@@` -385,7 +381,7 `@@` fn resolve_primitive_associated_item(
                         ty::AssocKind::Const => "associatedconstant",
                         ty::AssocKind::Type => "associatedtype",
                     };
-                    let fragment = format!("{}#{}.{}", prim_ty.as_sym(), out, item_name);
+                    let fragment = format!("{}.{}", out, item_name);
                     (Res::Primitive(prim_ty), fragment, Some((kind.as_def_kind(), item.def_id)))
                 })
         })
```
- Adding primitive docs to core without making any other change will cause links to go to `core` instead of `std`, even for crates with `extern crate std`. See "Breaking changes to doc(primitive)" below for why this is the case. That said, I could add some special casing to rustdoc at the same time that would let me separate this change from the others (it would fix rust-lang#73423 but still special-case intra-doc links). I'm willing to separate that out if helpful for reviewing.

### Add primitive documentation to libcore

This works by reusing the same `include!("primitive_docs.rs")` file in both core and std, and then special-casing links in core to use relative links instead of intra-doc links. This doesn't use purely intra-doc links because some of the primitive docs links to items only in std; this doesn't use purely relative links because that introduces new broken links when the docs are re-exported (e.g. String's `&str` deref impl, or Vec's slice deref impl).

### Fix inconsistent online/offline primitive docs

This does four things:
- Records modules with `doc(primitive)` in `cache.external_paths`. This is necessary for `href()` to find them later.
- Makes `cache.primitive_locations` available to the intra-doc link pass, by refactoring out a `PrimitiveType::primitive_locations` function that only uses `TyCtxt`.
- Special cases modules with `doc(primitive)` to be treated as always public for the purpose of links.
- Removes the fragment hack. cc `@notriddle,` I know you added some comments about this in the code (thank you for that!)

### Breaking changes to `doc(primitive)`

"Breaking" is a little misleading here - these are changes in behavior, none of them will cause code to fail to compile.

Let me preface this by saying I think stabilizing `doc(primitive)` was a uniquely terrible idea. As far as I can tell, it was stabilized by oversight; it's been stable since 1.0. No one should have need to use it except the standard library, and a crater run shows that in fact no one is using it: rust-lang#87050 (comment). I hope to actually make `doc(primitive)` a no-op unless you opt-in with a nightly feature, which will keep crates compiling without forcing rustdoc into trying to keep somewhat arbitrary behavior guarantees; but for now, this just subtly changes some of the behavior if you use `doc(primitive)` in a dependency.

That said, here are the changes:
-  Refactoring out `primitive_locations()` is technically a change in behavior, since it no longer looks for primitives in crates that were passed through `--extern`, but not used by the crate; however, that seems like such an unlikely edge case it's not worth dealing with.
- The precedence given to primitive locations is no longer just arbitrary, it can also be inconsistent from run to run. Let me explain that more: previously, primitive locations were sorted by the `CrateNum`; the comment on that sort said "Favor linking to as local extern as possible, so iterate all crates in reverse topological order." Unfortunately, that's not actually what CrateNum tracks: it measures the order crates are loaded, not the number of intermediate crates between that dependency and the root crate. It happened to work as intended before because the compiler injects `extern crate std;` at the top of every crate, which ensured it would have the first CrateNum other than the current, but every other CrateNum was completely arbitrary (for example, `core` often had a later CrateNum than `std`). This now removes the sort on CrateNum completely and special-cases core instead. In particular, if you depend on both `std` and a crate which defines a `doc(primitive)` module, it's arbitrary whether rustdoc will use the docs from std or the ones from the other crate. cc `@alexcrichton,` you wrote this originally.

cc `@rust-lang/rustdoc`
cc `@rust-lang/libs` for the addition to `core` (the commit you're interested in is rust-lang@36729b0)
@bors bors closed this as completed in 0273e3b Sep 12, 2021
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-bug Category: This is a bug. E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants