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 rustdoc handling of primitive items #87073

Merged
merged 5 commits into from
Sep 12, 2021
Merged

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Jul 12, 2021

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?

Why are these changes interconnected?

  • Allowing anchors (rustdoc: Can't use anchors in links to primitive types #83083) without fixing the online/offline problem ([intra-doc-links] primitives inconsistently point offline or online in std docs #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:
@@ -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 no_std crates do not link to primitives #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).

Note that this copies the whole file to core, to avoid anyone compiling core to have to set CARGO_PKG_NAME. See https://rust-lang.zulipchat.com/#narrow/stream/122651-general/topic/Who.20should.20review.20changes.20to.20linkchecker.3F/near/249939598 for more context. It also adds a tidy check to make sure the two files are kept in sync.

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: #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 91346c8)

@jyn514 jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 12, 2021
@rust-highfive

This comment has been minimized.

@rust-highfive
Copy link
Collaborator

r? @CraftSpider

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 12, 2021
@rust-log-analyzer

This comment has been minimized.

@alexcrichton
Copy link
Member

cc @alexcrichton, you wrote this originally

Sorry, I don't really remember anything in this area so I don't think I'll be of much help

@jyn514
Copy link
Member Author

jyn514 commented Jul 12, 2021

@alexcrichton no worries, I got it working with roughly the same behavior :)

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors

This comment has been minimized.

@bors

This comment has been minimized.

Res::Def(_, id) => Some(id),
Res::Primitive(_) => None,
Res::Def(_, id) => id,
Res::Primitive(prim) => *PrimitiveType::primitive_locations(tcx).get(&prim).unwrap(),
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure we always have all the primitives? I guess unless new primitives are added we're fine, but I'm still feeling a bit uneasy... Well, not a blocker in any case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can go through them one by one this afternoon, but they should all be there now that primitives are documented in core. This will cause an ICE if you use no_core and try to link to a primitive, but no_core isn't really supported anyway.

@bors

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Sep 11, 2021

📌 Commit 9ec79b92fcdd9679b61b6eae59372cab47b3cf4e has been approved by GuillaumeGomez,jyn514

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 11, 2021
@bors

This comment has been minimized.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 11, 2021
jyn514 and others added 5 commits September 12, 2021 02:23
This works by doing two things:
- Adding links that are specific to the crate. Since not all primitive
  items are defined in `core` (due to lang_items), these need to use
  relative links and not intra-doc links.
- Duplicating `primitive_docs` in both core and std. This allows not needing CARGO_PKG_NAME to build the standard library. It also adds a tidy check to make sure they stay the same.
- Fix broken handling of primitive associated items
- Remove fragment hack

  Fixes 83083

- more logging
- Update CrateNum hacks

  The CrateNum has no relation to where in the dependency tree the crate
  is, only when it's loaded. Explicitly special-case core instead of
  assuming it will be the first DefId.

- Update and add tests
- Cache calculation of primitive locations

  This could possibly be avoided by passing a Cache into
  collect_intra_doc_links; but that's a much larger change, and doesn't
  seem valuable other than for this.
Most of these are because alloc uses `#[lang_item]` to define methods,
but core documents primitives before those methods are available.

- Fix rustdoc-js-std test

  For some reason this change made CStr not show up in the results for
  `str,u8`. Since it still shows up for str, and since it wasn't a great
  match for that query anyway, I think this is ok to let slide.

- Add test that all primitives can be linked to
- Enable `doc(primitive)` in `core` as well
- Add linkcheck exception specifically for Windows

  Ideally this would be done automatically by the linkchecker by
  replacing `\\` with forward slashes, but this PR is already a ton of
  work ...

- Don't forcibly fail linkchecking if there's a broken intra-doc link on Windows

  Previously, it would exit with a hard error if a missing file had `::`
  in it. This changes it to report a missing file instead, which allows
  adding an exception.
This prevents the following (very strange) errors:

```
error: linking with `link.exe` failed: exit code: 1120
  |
  = note: "C:\\Program Files (x86)\\Microsoft Visual Studio\\2019\\Enterprise\\VC\\Tools\\MSVC\\14.29.30133\\bin\\HostX64\\x86\\link.exe" "/DEF:C:\\Users\\runneradmin\\AppData\\Local\\Temp\\rustcJih4fa\\lib.def" "/NOLOGO" "/LARGEADDRESSAWARE" "/SAFESEH" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\test\\rustdoc\\issue-15318-2\\auxiliary\\issue-15318.issue_15318.0a2a8554-cgu.0.rcgu.o" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\test\\rustdoc\\issue-15318-2\\auxiliary\\issue-15318.1na9aylmt25n6w3f.rcgu.o" "/LIBPATH:D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\native\\rust-test-helpers" "/LIBPATH:D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\test\\rustdoc\\issue-15318-2\\auxiliary" "/LIBPATH:D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage2\\lib\\rustlib\\i686-pc-windows-msvc\\lib" "vcruntime.lib" "ucrt.lib" "/WHOLEARCHIVE:D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage2\\lib\\rustlib\\i686-pc-windows-msvc\\lib\\librustc_std_workspace_core-78744e1360284b1e.rlib" "/WHOLEARCHIVE:D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage2\\lib\\rustlib\\i686-pc-windows-msvc\\lib\\libcore-a900fa3d16956226.rlib" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage2\\lib\\rustlib\\i686-pc-windows-msvc\\lib\\libcompiler_builtins-eb97e6b4dfd2f421.rlib" "/NXCOMPAT" "/LIBPATH:D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage2\\lib\\rustlib\\i686-pc-windows-msvc\\lib" "/OUT:D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\test\\rustdoc\\issue-15318-2\\auxiliary\\issue_15318.dll" "/OPT:REF,ICF" "/DLL" "/IMPLIB:D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\test\\rustdoc\\issue-15318-2\\auxiliary\\issue_15318.dll.lib" "/DEBUG" "/NATVIS:D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage2\\lib\\rustlib\\etc\\intrinsic.natvis" "/NATVIS:D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage2\\lib\\rustlib\\etc\\liballoc.natvis" "/NATVIS:D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage2\\lib\\rustlib\\etc\\libcore.natvis" "/NATVIS:D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage2\\lib\\rustlib\\etc\\libstd.natvis"
  = note: LINK : warning LNK4216: Exported entry point __DllMainCRTStartup@12
             Creating library D:\a\rust\rust\build\i686-pc-windows-msvc\test\rustdoc\issue-15318-2\auxiliary\issue_15318.dll.lib and object D:\a\rust\rust\build\i686-pc-windows-msvc\test\rustdoc\issue-15318-2\auxiliary\issue_15318.dll.exp
          libcore-a900fa3d16956226.rlib(core-a900fa3d16956226.core.95dedc69-cgu.0.rcgu.o) : error LNK2019: unresolved external symbol __aulldiv referenced in function __ZN4core3num7dec2flt7decimal7Decimal10left_shift17hfb9b6c23d6ff0383E
          libcompiler_builtins-eb97e6b4dfd2f421.rlib(compiler_builtins-eb97e6b4dfd2f421.compiler_builtins.a5ef280a-cgu.51.rcgu.o) : error LNK2001: unresolved external symbol __aulldiv
          libcore-a900fa3d16956226.rlib(core-a900fa3d16956226.core.95dedc69-cgu.0.rcgu.o) : error LNK2019: unresolved external symbol __aullrem referenced in function __ZN4core3fmt3num14parse_u64_into17h90eb20517ec3bd86E
          D:\a\rust\rust\build\i686-pc-windows-msvc\test\rustdoc\issue-15318-2\auxiliary\issue_15318.dll : fatal error LNK1120: 2 unresolved externals

```
@jyn514
Copy link
Member Author

jyn514 commented Sep 12, 2021

@bors r=GuillaumeGomez,jyn514

@bors
Copy link
Contributor

bors commented Sep 12, 2021

📌 Commit 86fd250 has been approved by GuillaumeGomez,jyn514

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 12, 2021
@bors
Copy link
Contributor

bors commented Sep 12, 2021

⌛ Testing commit 86fd250 with merge 0273e3b...

@bors
Copy link
Contributor

bors commented Sep 12, 2021

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez,jyn514
Pushing 0273e3b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 12, 2021
@bors bors merged commit 0273e3b into rust-lang:master Sep 12, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 12, 2021
@jyn514 jyn514 deleted the primitive-docs branch September 12, 2021 05:39
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (0273e3b): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet