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

Some of rustdoc's output is not stable #119597

Closed
fmease opened this issue Jan 5, 2024 · 4 comments · Fixed by #123340
Closed

Some of rustdoc's output is not stable #119597

fmease opened this issue Jan 5, 2024 · 4 comments · Fixed by #123340
Assignees
Labels
A-metadata Area: Crate metadata C-bug Category: This is a bug. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@fmease
Copy link
Member

fmease commented Jan 5, 2024

Meta: I'm going to expand upon the description of this issue over time and hopefully compile a tasklist of concrete issues.

Seemingly innocuous and irrelevant modifications to a crate (like the reordering, addition or removal of unrelated items) may alter rustdoc's output compared to a previous build (which often manifests itself in a change in the order of “the things we are interested in”1).

I presume that this can only affect things for which rustdoc needs to look at the crate metadata (e.g., cross-crate re-exports and synthetic impls (blanket and auto-trait impls)).

I'm not in the know regarding the root cause since I don't know much about the internals of the crate metadata format (layout, serialization, deserialization). I suspect it might have something to do with the use of FxHash{Set,Map}s during or after deserialization but that could be flat out wrong.

Maybe it isn't even related to rmeta but only to FxHash{Set,Map}, I really don't know. I just know that these kinds of “fluctuations” are stable under recompilation (of the input crate(s) and of rustdoc itself) and under reruns (of rustdoc) and only unstable under what I have inferred to be “crate metadata changes”. So while rustdoc's output is apparently deterministic, it's not necessarily fully predictable.


Concrete examples and elaborations can be found below:

Footnotes

  1. Ideally, the order wouldn't change at all and take the order either from the source code or from some predictable or known ordering (alphabetical, etc.)

@fmease fmease added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-metadata Area: Crate metadata P-low Low priority C-bug Category: This is a bug. labels Jan 5, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jan 5, 2024
@fmease fmease removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jan 5, 2024
@fmease fmease self-assigned this Jan 5, 2024
@fmease
Copy link
Member Author

fmease commented Jan 5, 2024

Judging by #119609 (comment), these fluctuations can be unstable under architecture.
Here: x86_64-gnu-llvm-16 vs. aarch-64-gnu.

@fmease fmease removed the P-low Low priority label Jan 5, 2024
@fmease
Copy link
Member Author

fmease commented Jan 5, 2024

@fmease
Copy link
Member Author

fmease commented Jan 26, 2024

cc #120371

bors added a commit to rust-lang-ci/rust that referenced this issue Feb 23, 2024
…meta-changes, r=<try>

[try-only] [demo] rustdoc is unstable under rmeta changes

Part of rust-lang#119597.
@fmease
Copy link
Member Author

fmease commented Feb 23, 2024

I can no longer reproduce #119609 (comment), see #121486 but still clean/auto_traits.rs needs to be fixed. I don't want to have to swap the bounds around in no_redundancy.rs.

@fmease fmease changed the title Some of rustdoc's output is not stable under crate metadata changes Some of rustdoc's output is not stable Mar 24, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 24, 2024
…s, r=<try>

[perf-only] rustdoc: synthetic impls: auto traits: Fx{Hash↦Index}{Map,Set}

Part of rust-lang#119597 and rust-lang#113015.
r? ghost
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 1, 2024
…mpl-synth, r=<try>

rustdoc: heavily simplify the synthesis of auto trait impls

`gd --numstat HEAD~2 HEAD src/librustdoc/clean/auto_trait.rs`
**+315 -705** 🟩🟥🟥🟥⬛

---

As outlined in issue rust-lang#113015, there are currently 3[^1] large separate routines that “clean” `rustc_middle::ty` data types related to generics & predicates to rustdoc data types. Every single one has their own kinds of bugs. While I've patched a lot of bugs in each of the routines in the past, it's about time to unify them. This PR is only the first in a series. It completely *yanks* the custom “bounds cleaning” of mod `auto_trait` and reuses the routines found in mod `simplify`. As mentioned, `simplify` is also flawed but still it's more complete than `auto_trait`'s routines. See also my review comment over at `tests/rustdoc/synthetic_auto/bounds.rs`.

This is preparatory work for rewriting “bounds cleaning” in follow-up PRs in order to finally [fix] rust-lang#113015.

Apart from that, I've eliminated all potential sources of *unstable rendering output*.
See also rust-lang#119597. I'm pretty sure this fixes rust-lang#119597.

This PR does not attempt to fix [any other issues related to synthetic auto trait impls](https://github.com/rust-lang/rust/issues?q=is%3Aissue+is%3Aopen+label%3AA-synthetic-impls%20label%3AA-auto-traits).
However, it's definitely meant to be a *stepping stone* by making `auto_trait` more contributor friendly.

---

* Replace `FxHash{Map,Set}` with `FxIndex{Map,Set}` to guarantee a stable / more predictable iteration order
  * Or as a perf opt, `UnordSet` (a thin wrapper around `FxHashSet`) in cases where we never iterate over the set.
* Remove the types `RegionTarget` and `RegionDeps` from `librustdoc`. They were duplicates of the identical types found in `rustc`. Just import them from `rustc`. For some reason, they were duplicated when splitting `auto_trait` in two in rust-lang#49711.
* Get rid of the useless “type namespace” `AutoTraitFinder` in `librustdoc`
  * The struct only held a `DocContext`, it was over-engineered
  * Turn the associated functions into free ones
    * Eliminates rightward drift; increases legibility
  * `rustc` also contains a useless `AutoTraitFinder` struct but I plan on removing that in a follow-up PR
* Rename a bunch of methods to be way more descriptive
* Eliminate `use super::*;`
  * Lead to `clean/mod.rs` accumulating a lot of unnecessary imports and therefore
  * made `auto_traits` less modular

I plan on adding extensive documentation to `librustdoc`'s `auto_trait` in follow-up PRs.
I don't want to do that in this PR because further refactoring & bug fix PRs may alter the overall structure of `librustdoc`'s & `rustc`'s `auto_trait` modules to a great degree. I'm slowly digging into the dark details of `rustc`'s `auto_trait` module again and once I have the full picture I will be able to provide proper docs.

---

While this PR does indeed touch `rustc`'s `auto_trait` — mostly tiny refactorings — I argue this PR doesn't need any compiler reviewers next to rustdoc ones since that module falls under the purview of rustdoc — it used to be part of `librustdoc` after all (rust-lang#49711).

Sorry for not having split this into more commits. If you'd like me to I can try to retroactively separate it into more atomic commits. However, I don't know if that would actually make reviewing this PR easier.

r? `@GuillaumeGomez`

[^1]: Or even 4 depending on the way you're counting.
@bors bors closed this as completed in 5dbaafd Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-metadata Area: Crate metadata 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.

2 participants