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

no_std crates do not link to primitives #73423

Closed
Manishearth opened this issue Jun 16, 2020 · 10 comments · Fixed by #87073
Closed

no_std crates do not link to primitives #73423

Manishearth opened this issue Jun 16, 2020 · 10 comments · Fixed by #87073
Labels
C-bug Category: This is a bug. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@Manishearth
Copy link
Member

#![no_std]

// Link to [u8]
pub fn foo() -> u8 {}

The link in x()'s return type does not work. The intra-doc link on Foo does work, but that's because we hardcode the std in it, which should be fixed once this is.

Turns out that the "primitive type u8" only lives in std, see https://doc.rust-lang.org/nightly/std/index.html?search=u8

@Manishearth Manishearth added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. C-bug Category: This is a bug. labels Jun 16, 2020
@Manishearth
Copy link
Member Author

We should first get these things to link to std at least. If we decide to add a separate primitive page for libcore as well (I'm not sure if we should), then the following patch makes it work for intra doc links:

diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs
index 5c76c840b1d..1eb52c8f7bb 100644
--- a/src/librustdoc/clean/types.rs
+++ b/src/librustdoc/clean/types.rs
@@ -55,6 +55,7 @@ pub struct Crate {
     pub external_traits: Rc<RefCell<FxHashMap<DefId, Trait>>>,
     pub masked_crates: FxHashSet<CrateNum>,
     pub collapsed: bool,
+    pub is_no_std: bool,
 }
 
 #[derive(Clone, Debug)]
@@ -114,8 +115,8 @@ impl Item {
         self.attrs.collapsed_doc_value()
     }
 
-    pub fn links(&self) -> Vec<(String, String)> {
-        self.attrs.links(&self.def_id.krate)
+    pub fn links(&self, is_no_std: bool) -> Vec<(String, String)> {
+        self.attrs.links(&self.def_id.krate, is_no_std)
     }
 
     pub fn is_crate(&self) -> bool {
@@ -598,7 +599,7 @@ impl Attributes {
     /// Gets links as a vector
     ///
     /// Cache must be populated before call
-    pub fn links(&self, krate: &CrateNum) -> Vec<(String, String)> {
+    pub fn links(&self, krate: &CrateNum, is_no_std: bool) -> Vec<(String, String)> {
         use crate::html::format::href;
 
         self.links
@@ -630,12 +631,14 @@ impl Attributes {
                             };
                             // This is a primitive so the url is done "by hand".
                             let tail = fragment.find('#').unwrap_or_else(|| fragment.len());
+                            let std = if is_no_std {"core"} else {"std"};
                             Some((
                                 s.clone(),
                                 format!(
-                                    "{}{}std/primitive.{}.html{}",
+                                    "{}{}{}/primitive.{}.html{}",
                                     url,
                                     if !url.ends_with('/') { "/" } else { "" },
+                                    std,
                                     &fragment[..tail],
                                     &fragment[tail..]
                                 ),
diff --git a/src/librustdoc/clean/utils.rs b/src/librustdoc/clean/utils.rs
index c4e4802db6c..cb6b7d26f7d 100644
--- a/src/librustdoc/clean/utils.rs
+++ b/src/librustdoc/clean/utils.rs
@@ -10,6 +10,7 @@ use crate::core::DocContext;
 
 use itertools::Itertools;
 use rustc_data_structures::fx::FxHashSet;
+use rustc_ast::attr;
 use rustc_hir as hir;
 use rustc_hir::def::{DefKind, Res};
 use rustc_hir::def_id::{DefId, LOCAL_CRATE};
@@ -87,6 +88,7 @@ pub fn krate(mut cx: &mut DocContext<'_>) -> Crate {
         }));
     }
 
+    let is_no_std = attr::contains_name(&krate.item.attrs, sym::no_std);
     Crate {
         name,
         version: None,
@@ -97,6 +99,7 @@ pub fn krate(mut cx: &mut DocContext<'_>) -> Crate {
         external_traits: cx.external_traits.clone(),
         masked_crates,
         collapsed: false,
+        is_no_std,
     }
 }
 
diff --git a/src/librustdoc/html/render.rs b/src/librustdoc/html/render.rs
index 07631093edd..abc6ee5de17 100644
--- a/src/librustdoc/html/render.rs
+++ b/src/librustdoc/html/render.rs
@@ -161,6 +161,7 @@ struct Context {
     id_map: Rc<RefCell<IdMap>>,
     pub shared: Arc<SharedContext>,
     pub cache: Arc<Cache>,
+    pub is_no_std: bool,
 }
 
 crate struct SharedContext {
@@ -556,6 +557,7 @@ pub fn run(
         id_map: Rc::new(RefCell::new(id_map)),
         shared: Arc::new(scx),
         cache: cache.clone(),
+        is_no_std: krate.is_no_std,
     };
 
     // Freeze the cache now that the index has been built. Put an Arc into TLS
@@ -1890,7 +1892,7 @@ fn document_short(
         } else {
             plain_summary_line(Some(s))
         };
-        render_markdown(w, cx, &markdown, item.links(), prefix, is_hidden);
+        render_markdown(w, cx, &markdown, item.links(cx.is_no_std), prefix, is_hidden);
     } else if !prefix.is_empty() {
         write!(
             w,
@@ -1904,7 +1906,7 @@ fn document_short(
 fn document_full(w: &mut Buffer, item: &clean::Item, cx: &Context, prefix: &str, is_hidden: bool) {
     if let Some(s) = cx.shared.maybe_collapsed_doc_value(item) {
         debug!("Doc block: =====\n{}\n=====", s);
-        render_markdown(w, cx, &*s, item.links(), prefix, is_hidden);
+        render_markdown(w, cx, &*s, item.links(cx.is_no_std), prefix, is_hidden);
     } else if !prefix.is_empty() {
         write!(
             w,
@@ -2158,7 +2160,7 @@ fn item_module(w: &mut Buffer, cx: &Context, item: &clean::Item, items: &[clean:
                        </tr>",
                     name = *myitem.name.as_ref().unwrap(),
                     stab_tags = stability_tags(myitem),
-                    docs = MarkdownSummaryLine(doc_value, &myitem.links()).to_string(),
+                    docs = MarkdownSummaryLine(doc_value, &myitem.links(cx.is_no_std)).to_string(),
                     class = myitem.type_(),
                     add = add,
                     stab = stab.unwrap_or_else(String::new),
@@ -3616,7 +3618,7 @@ fn render_impl(
                 "<div class='docblock'>{}</div>",
                 Markdown(
                     &*dox,
-                    &i.impl_item.links(),
+                    &i.impl_item.links(cx.is_no_std),
                     &mut ids,
                     cx.shared.codes,
                     cx.shared.edition,

@GuillaumeGomez
Copy link
Member

Linked to #43466

@Manishearth
Copy link
Member Author

It's not linked to that issue, it's an independent bug

@jyn514
Copy link
Member

jyn514 commented Jul 18, 2020

I think we should consider moving primitives into libcore. Otherwise there's not a way to fix this without allowing links to upstream types (#73699), which is another can of worms and I don't think should be tied to this issue.

@Manishearth was concerned that this wouldn't show libstd implementations of libcore traits. A possible workaround here is to duplicate primitives in both libstd and libcore; in libstd they would show the trait implementations, in libcore they wouldn't but links to primitives would still work.

Another thing to consider is that it's not clear this is under rustdoc's control: the metric for whether a primitive type is local or not is

match m.primitive_locations.get(&prim) {
Some(&def_id) if def_id.is_local() => {
but that seems independent of anything rustdoc does. So we should figure out why primitives are currently being seen as local to libstd before we go much further.

@jyn514
Copy link
Member

jyn514 commented Jul 18, 2020

we should figure out why primitives are currently being seen as local to libstd before we go much further

Oh, I think it might be as simple as primitive_docs.rs being in libstd, not libcore. https://github.com/rust-lang/rust/blob/98450757e5fa18ee0be9213d2830c9363b0f5fd3/src/libstd/primitive_docs.rs

@jyn514
Copy link
Member

jyn514 commented Jul 18, 2020

I think we should consider moving primitives into libcore.

Doing this means that intra-doc links to String, etc. won't resolve in either libstd or libcore.

@poliorcetics
Copy link
Contributor

I think I'm hitting this case in #77875.

@jyn514
Copy link
Member

jyn514 commented Mar 29, 2021

Note that because the standard library uses lang items to add impls to primitives in alloc and std, this means the documentation in core will be missing some methods (e.g. #83577)

@camelid
Copy link
Member

camelid commented Mar 29, 2021

I think we should consider moving primitives into libcore.

Doing this means that intra-doc links to String, etc. won't resolve in either libstd or libcore.

Hmm, why would that be? IIUC, String wouldn't resolve anyway in core because it's defined in alloc, and it's not a primitive. Why would this change cause links to String to not resolve?

@jyn514
Copy link
Member

jyn514 commented Mar 29, 2021

Hmm, why would that be? IIUC, String wouldn't resolve anyway in core because it's defined in alloc, and it's not a primitive. Why would this change cause links to String to not resolve?

Because right now, the links are written in libstd, which can link to alloc fine. If we changed it to be in libcore, then the links would break, because core doesn't depend on alloc. That's true even if the docs are re-exported into libstd, rustdoc resolves the links relative to the original module.

bors added a commit to rust-lang-ci/rust that referenced this issue Aug 16, 2021
Add future-incompat lint for `doc(primitive)`

## What is `doc(primitive)`?

`doc(primitive)` is an attribute recognized by rustdoc which adds documentation for the built-in primitive types, such as `usize` and `()`. It has been stable since Rust 1.0.

## Why change anything?

`doc(primitive)` is useless for anyone outside the standard library. Since rustdoc provides no way to combine the documentation on two different primitive items, you can only replace the docs, and since the standard library already provides extensive documentation there is no reason to do so.

While fixing rustdoc's handling of primitive items (rust-lang#87073) I discovered that even rustdoc's existing handling of primitive items was broken if you had more than two crates using it (it would pick randomly between them). That meant both:
- Keeping rustdoc's existing treatment was nigh-impossible, because it was random.
- doc(primitive) was even more useless than it would otherwise be.

The only use-case for this outside the standard library is for no-std libraries which want to link to primitives (rust-lang#73423) which is being fixed in rust-lang#87073.

rust-lang#87073 makes various breaking changes to `doc(primitive)` (breaking in the sense that they change the semantics, not in that they cause code to fail to compile). It's not possible to avoid these and still fix rustdoc's issues.

## What can we do about it?

As shown by the crater run (rust-lang#87050 (comment)), no one is actually using doc(primitive), there wasn't a single true regression in the whole run. We can either:
1. Feature gate it completely, breaking anyone who crater missed. They can easily fix the breakage just by removing the attribute.
2. add it to the `INVALID_DOC_ATTRIBUTES` future-incompat lint, and at the same time make it a no-op unless you add a feature gate. That would mean rustdoc has to look at the features of dependent crates, because it needs to know where primitives are defined in order to link to them.
3. add it to `INVALID_DOC_ATTRIBUTES`, but still use it to determine where primitives come from
4. do nothing; the behavior will silently change in rust-lang#87073.

My preference is for 2, but I would also be happy with 1 or 3. I don't think we should silently change the behavior.

This PR currently implements 3.
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
camelid added a commit to camelid/rust that referenced this issue Oct 30, 2021
Now that rust-lang#73423 is fixed, sorting should no longer be necessary.
See also this discussion [1].

[1]: rust-lang#86889 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. 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.

5 participants