rustdoc: fix & clean up handling of cross-crate higher-ranked parameters #116388

fmease · 2023-10-03T16:31:56Z

Preparatory work for the refactoring planned in #113015 (for correctness & maintainability).

Render the higher-ranked parameters of cross-crate function pointer types (*).
Replace occurrences of collect_referenced_late_bound_regions() (CRLBR) with bound_vars().
The former is quite problematic and the use of the latter allows us to yank a lot of hacky code (†)
as you can tell from the diff! :)
Add support for cross-crate higher-ranked type parameters (#![feature(non_lifetime_binders)]).
We were previously ICE'ing on them (see inline_cross/non_lifetime_binders.rs).

(*): Extracted from test inline_cross/fn-type.rs:

- fn(_: &'z fn(_: &'b str), _: &'a ()) -> &'a ()
+ for<'z, 'a, '_unused> fn(_: &'z for<'b> fn(_: &'b str), _: &'a ()) -> &'a ()

(†): It returns an FxHashSet which isn't predictable or stable wrt. source code (.rmeta) changes. To elaborate, the ordering of late-bound regions doesn't necessarily reflect the ordering found in the source code. It does seem to be stable across compilations but modifying the source code of the to-be-documented crates (like adding or renaming items) may result in a different order:

Example

Let's assume that we're documenting the cross-crate re-export of produce from the code below. On master, rustdoc would render the list of binders as for<'x, 'y, 'z>. However, once you add back the functions a–l, it would be rendered as for<'z, 'y, 'x> (reverse order)! Results may vary. bound_vars() fixes this as it returns them in source order.

// pub fn a() {}
// pub fn b() {}
// pub fn c() {}
// pub fn d() {}
// pub fn e() {}
// pub fn f() {}
// pub fn g() {}
// pub fn h() {}
// pub fn i() {}
// pub fn j() {}
// pub fn k() {}
// pub fn l() {}

pub fn produce() -> impl for<'x, 'y, 'z> Trait<'z, 'y, 'x> {}

pub trait Trait<'a, 'b, 'c> {}

impl Trait<'_, '_, '_> for () {}

Further, as the name suggests, CRLBR only collects referenced regions and thus we drop unused binders. bound_vars() contains unused binders on the other hand. Let's stay closer to the source where possible and keep unused binders.

Lastly, using bound_vars() allows us to get rid of

the deduplication and alphabetical sorting hack in simplify.rs
the weird field bound_params on EqPredicate

both of which were introduced by me in #102707 back when I didn't know better.

To illustrate, let's look at the cross-crate bound T: for<'a, 'b> Trait<A<'a> = (), B<'b> = ()>.

With CRLBR + EqPredicate.bound_params, before bound simplification we would have the bounds T: Trait, for<'a> <T as Trait>::A<'a> == () and for<'b> <T as Trait>::B<'b> == () which required us to merge for<>, for<'a> and for<'b> into for<'a, 'b> in a ~~deterministic~~ predictable manner and without introducing duplicate binders.
With bound_vars(), we now have the bounds for<'a, b> T: Trait, <T as Trait>::A<'a> == () and <T as Trait>::B<'b> == () before bound simplification similar to rustc itself. This obviously no longer requires any funny merging of for<>s. On top of that for<'a, 'b> is guaranteed to be in source order.

rustbot · 2023-10-03T16:32:05Z

r? @notriddle

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-10-03T16:32:08Z

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

fmease · 2023-10-03T16:34:31Z

src/librustdoc/clean/mod.rs

+fn clean_bound_vars<'tcx>(
+    bound_vars: &'tcx ty::List<ty::BoundVariableKind>,
+) -> Vec<GenericParamDef> {


Theoretically, I could make this generic by letting it take a <T> bound_ty: ty::Binder<'tcx, T> instead but I've decided against it for simplicity and to avoid code size explosion due to excessive monomorphization. I could use a non-generic inner function though, if you'd like.

I'm happy with this function not being generic. It wouldn't save very much code at the call site if it was.

fmease · 2023-10-03T16:58:39Z

src/librustdoc/clean/mod.rs

            BareFunction(Box::new(BareFunctionDecl {
                unsafety: sig.unsafety(),
-                generic_params: Vec::new(),
+                generic_params,


This change allows us to render the for<> parameter list of cross-crate fn ptr tys.

notriddle · 2023-10-03T17:07:35Z

src/librustdoc/clean/mod.rs

-            ty::BrNamed(_, name) if br.is_named() => Some(GenericParamDef::lifetime(name)),
-            _ => None,
-        })
-        .collect();


If nothing else, I'm glad to see all this duplicate code factored out into a function. ❤️

notriddle · 2023-10-03T17:14:44Z

src/librustdoc/clean/mod.rs

+fn clean_bound_vars<'tcx>(
+    bound_vars: &'tcx ty::List<ty::BoundVariableKind>,
+) -> Vec<GenericParamDef> {


I'm happy with this function not being generic. It wouldn't save very much code at the call site if it was.

notriddle · 2023-10-03T19:20:02Z

src/librustdoc/clean/mod.rs

@@ -2109,9 +2071,11 @@ pub(crate) fn clean_middle_ty<'tcx>(
            // FIXME: should we merge the outer and inner binders somehow?


Does anybody know what kind of code would motivate fixing this? Is it actually possible to make rustdoc misrender something because the outer and inner binders aren't being merged?

I'm not super familiar with some of the details surrounding bound vars yet. I can't tell you all possible cases where the type and the signature have bound vars at the same time.

I can give you one example though which rustdoc renders just fine:

pub fn f(_: impl for<'a> Trait<for<'b> fn(&'a (), &'b ())>) { } pub trait Trait<T> {}

Here, bound_ty.bound_vars() is ['a] and sig.bound_vars() is ['b]. It would be incorrect to concatenate ['a] and ['b]. We don't want to render for<'a> … for<'a, 'b> fn(…).

In this case, the bound_ty.bound_vars() is ['a] because clean_poly_trait_ref_with_bindings passes this list of bound vars down to external_path to … to ty_args_to_args which calls clean_middle_ty. I'm not sure if that's the intended usage of map_bound but I guess it is since oli-obk implemented that as far as I know.
bound_ty.skip_binder() and just using sig.bound_vars() should therefore be fine.

In my limited understanding, I disagree with the FIXME, it would be super odd to merge different binder lists, rustc does it like one or two times in exceptional cases. There can be a semantic difference between for<'a> … for<'b> … and for<'a, 'b> … and rustc needs to shift the de Bruijn indices and the var indices for correctness after merging such lists.

A few months ago, I chatted a bit with compiler-errors about ty::Binder<_> and bound vars in rustdoc and from what I've learned rustdoc (still) doesn't properly track bound vars. This was relevant for #112463 where we don't have enough information to be able to deal with (escaping) late bound vars. I haven't understood everything yet and thus my explanation sucks.

I'm pretty sure from what I've learned so far, proper bound var handling would prevent ICEs in rustdoc like in PR #108503 (which you're acquainted with) and probably other ones like https://github.com/rust-lang/rust/issues?q=is%3Aissue+is%3Aopen+label%3AT-rustdoc%20label%3AI-ICE%20label%3AA-auto-traits%20label%3AA-synthetic-impls.

notriddle · 2023-10-04T01:18:00Z

That makes sense.

@bors r+ rollup

bors · 2023-10-04T01:18:02Z

📌 Commit ace85f0 has been approved by notriddle

It is now in the queue for this repository.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#115961 (Replace 'mutex' with 'lock' in RwLock documentation) - rust-lang#116146 (Clarify `arg` and `args` documentation) - rust-lang#116363 (Adapt `todo!` documentation to mention displaying custom values) - rust-lang#116365 (bootstrap: make copying linker binaries conditional) - rust-lang#116388 (rustdoc: fix & clean up handling of cross-crate higher-ranked parameters) - rust-lang#116393 (Emit feature gate *warning* for `auto` traits pre-expansion) - rust-lang#116395 (Mark myself as vacation or whatever) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#116388 - fmease:rustdoc-fix-n-clean-up-x-crate-higher-ranked-params, r=notriddle rustdoc: fix & clean up handling of cross-crate higher-ranked parameters Preparatory work for the refactoring planned in rust-lang#113015 (for correctness & maintainability). --- 1. Render the higher-ranked parameters of cross-crate function pointer types **(*)**. 2. Replace occurrences of `collect_referenced_late_bound_regions()` (CRLBR) with `bound_vars()`. The former is quite problematic and the use of the latter allows us to yank a lot of hacky code **(†)** as you can tell from the diff! :) 3. Add support for cross-crate higher-ranked types (`#![feature(non_lifetime_binders)]`). We were previously ICE'ing on them (see `inline_cross/non_lifetime_binders.rs`). --- **(*)**: Extracted from test `inline_cross/fn-type.rs`: ```diff - fn(_: &'z fn(_: &'b str), _: &'a ()) -> &'a () + for<'z, 'a, '_unused> fn(_: &'z for<'b> fn(_: &'b str), _: &'a ()) -> &'a () ``` **(†)**: It returns an `FxHashSet` which isn't *predictable* or *stable* wrt. source code (`.rmeta`) changes. To elaborate, the ordering of late-bound regions doesn't necessarily reflect the ordering found in the source code. It does seem to be stable across compilations but modifying the source code of the to-be-documented crates (like adding or renaming items) may result in a different order: <details><summary>Example</summary> Let's assume that we're documenting the cross-crate re-export of `produce` from the code below. On `master`, rustdoc would render the list of binders as `for<'x, 'y, 'z>`. However, once you add back the functions `a`–`l`, it would be rendered as `for<'z, 'y, 'x>` (reverse order)! Results may vary. `bound_vars()` fixes this as it returns them in source order. ```rs // pub fn a() {} // pub fn b() {} // pub fn c() {} // pub fn d() {} // pub fn e() {} // pub fn f() {} // pub fn g() {} // pub fn h() {} // pub fn i() {} // pub fn j() {} // pub fn k() {} // pub fn l() {} pub fn produce() -> impl for<'x, 'y, 'z> Trait<'z, 'y, 'x> {} pub trait Trait<'a, 'b, 'c> {} impl Trait<'_, '_, '_> for () {} ``` </details> Further, as the name suggests, CRLBR only collects *referenced* regions and thus we drop unused binders. `bound_vars()` contains unused binders on the other hand. Let's stay closer to the source where possible and keep unused binders. Lastly, using `bound_vars()` allows us to get rid of * the deduplication and alphabetical sorting hack in `simplify.rs` * the weird field `bound_params` on `EqPredicate` both of which were introduced by me in rust-lang#102707 back when I didn't know better. To illustrate, let's look at the cross-crate bound `T: for<'a, 'b> Trait<A<'a> = (), B<'b> = ()>`. * With CRLBR + `EqPredicate.bound_params`, *before* bounds simplification we would have the bounds `T: Trait`, `for<'a> <T as Trait>::A<'a> == ()` and `for<'b> <T as Trait>::B<'b> == ()` which required us to merge `for<>`, `for<'a>` and `for<'b>` into `for<'a, 'b>` in a deterministic manner and without introducing duplicate binders. * With `bound_vars()`, we now have the bounds `for<'a, b> T: Trait`, `<T as Trait>::A<'a> == ()` and `<T as Trait>::B<'b> == ()` before bound simplification similar to rustc itself. This obviously no longer requires any funny merging of `for<>`s. On top of that `for<'a, 'b>` is guaranteed to be in source order.

…mpl-synth, r=GuillaumeGomez 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 alluded to, `simplify` is also flawed but it's still more complete than `auto_trait`'s routines. [See also my review comment over at `tests/rustdoc/synthetic_auto/bounds.rs`](rust-lang#123340 (comment)). This is preparatory work for rewriting “bounds cleaning” from scratch in follow-up PRs in order to finally [fix] rust-lang#113015. Apart from that, I've eliminated all potential sources of *instability* in the rendered 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 iteration order * Or as a perf opt, `UnordSet` (a thin wrapper around `FxHashSet`) in cases where we never iterate over the set. * Yes, we do make use of `swap_remove` but that shouldn't matter since all the callers are deterministic. It does make the output less “predictable” but it's still better than before. Ofc, I rely on `rustc_infer` being deterministic. I hope that holds. * Utilizing `clean::simplify` over the custom “bounds cleaning” routines wipes out the last reference to `collect_referenced_late_bound_regions` in rustdoc (`simplify` uses `bound_vars`) which was a source of instability / unpredictability (cc rust-lang#116388) * 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 * Made `auto_traits` less modular * Eliminate a custom `TypeFolder`: We can just use the rustc helper `fold_regions` which does that for us 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 split it into more atomic commits retroactively. However, I don't know if that would actually make reviewing easier. I think the best way to review this might just be to place the master version of `auto_trait` on the left of your screen and the patched one on the right, not joking. r? `@GuillaumeGomez` [^1]: Or even 4 depending on the way you're counting.

fmease added 2 commits October 3, 2023 17:16

rustdoc: fix & clean up handling of cross-crate higher-ranked lifetimes

67de150

rustdoc: add support for cross-crate higher-ranked types

ace85f0

rustbot assigned notriddle Oct 3, 2023

rustbot added A-rustdoc-json Area: Rustdoc JSON backend S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Oct 3, 2023

fmease added A-cross-crate-reexports Area: Documentation that has been re-exported from a different crate and removed A-rustdoc-json Area: Rustdoc JSON backend labels Oct 3, 2023

fmease commented Oct 3, 2023

View reviewed changes

fmease added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Oct 3, 2023

fmease commented Oct 3, 2023

View reviewed changes

notriddle reviewed Oct 3, 2023

View reviewed changes

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 Oct 4, 2023

matthiaskrgr mentioned this pull request Oct 4, 2023

Rollup of 7 pull requests #116408

Merged

bors merged commit 3e29363 into rust-lang:master Oct 4, 2023
11 checks passed

rustbot added this to the 1.75.0 milestone Oct 4, 2023

fmease deleted the rustdoc-fix-n-clean-up-x-crate-higher-ranked-params branch October 4, 2023 07:27

pvdrz mentioned this pull request Oct 4, 2023

Fix divergence from upstream master ferrocene/ferrocene#23

Merged

fmease mentioned this pull request Jan 4, 2024

Use diagnostic namespace in stdlib #119216

Merged

fmease mentioned this pull request Jan 5, 2024

Some of rustdoc's output is not stable #119597

Closed

fmease mentioned this pull request Apr 2, 2024

rustdoc: heavily simplify the synthesis of auto trait impls #123340

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc: fix & clean up handling of cross-crate higher-ranked parameters #116388

rustdoc: fix & clean up handling of cross-crate higher-ranked parameters #116388

fmease commented Oct 3, 2023 •

edited

Loading

rustbot commented Oct 3, 2023

rustbot commented Oct 3, 2023

fmease Oct 3, 2023 •

edited

Loading

notriddle Oct 3, 2023

fmease Oct 3, 2023

notriddle Oct 3, 2023

notriddle Oct 3, 2023

notriddle Oct 3, 2023

fmease Oct 3, 2023 •

edited

Loading

fmease Oct 3, 2023 •

edited

Loading

notriddle commented Oct 4, 2023

bors commented Oct 4, 2023

		@@ -2109,9 +2071,11 @@ pub(crate) fn clean_middle_ty<'tcx>(
		// FIXME: should we merge the outer and inner binders somehow?

rustdoc: fix & clean up handling of cross-crate higher-ranked parameters #116388

rustdoc: fix & clean up handling of cross-crate higher-ranked parameters #116388

Conversation

fmease commented Oct 3, 2023 • edited Loading

rustbot commented Oct 3, 2023

rustbot commented Oct 3, 2023

fmease Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

notriddle Oct 3, 2023

Choose a reason for hiding this comment

fmease Oct 3, 2023

Choose a reason for hiding this comment

notriddle Oct 3, 2023

Choose a reason for hiding this comment

notriddle Oct 3, 2023

Choose a reason for hiding this comment

notriddle Oct 3, 2023

Choose a reason for hiding this comment

fmease Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

fmease Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

notriddle commented Oct 4, 2023

bors commented Oct 4, 2023

fmease commented Oct 3, 2023 •

edited

Loading

fmease Oct 3, 2023 •

edited

Loading

fmease Oct 3, 2023 •

edited

Loading

fmease Oct 3, 2023 •

edited

Loading