rustdoc: render more cross-crate HRTBs properly #102707

fmease · 2022-10-05T13:07:17Z

Follow-up to #102439.
Render the for<> parameter lists of cross-crate higher-rank trait bounds (in where-clauses and in impl Trait).

I've added a new field bound_params to clean::WherePredicate::EqPredicate (mirroring its sibling variant BoundPredicate). However, I had to box the existing fields since EqPredicate used to be the largest variant (128 bytes on 64-bit systems) and it would only have gotten bigger).
Not sure if you like that approach. As an alternative, I could pass the uncleaned ty::Predicate alongside the cleaned WherePredicate to the various re-sugaring methods (similar to what clean::AutoTraitFinder::param_env_to_generics does).

I haven't yet added the HTML & JSON rendering code for the newly added bound_params field since I am waiting for your opinion. Those two rendering code paths should actually be unreachable in practice given the fact(?) that we re-sugar all equality predicates to associated type bindings (and arbitrary equality predicates are not part of the Rust surface language at the time of this writing).

If you agree with storing bound_params in EqPredicate, I think I can use it to greatly simplify the clean::auto_trait module (by also using simplify::merge_bounds). Maybe I can do that in any case though.

@rustbot label T-rustdoc A-cross-crate-reexports
r? @GuillaumeGomez

rustbot · 2022-10-05T13:07:20Z

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

cc @camelid

GuillaumeGomez · 2022-10-05T13:56:17Z

Thanks for the PR! The code changes look good to me, but two things before merging:

We need a perf check because of the changes in the impl_trait variable in clean.
I'd like to have a second opinion about this to be sure I didn't miss anything (so cc @notriddle).

@bors try @rust-timer queue

notriddle · 2022-10-05T14:11:30Z

@bors try @rust-timer queue

GuillaumeGomez · 2022-10-05T14:12:27Z

Perf check is down maybe?

fmease · 2022-10-05T14:20:41Z

https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/bors.20stuck.3F/near/302463516

bors · 2022-10-05T14:33:10Z

⌛ Trying commit 90b96f9206ffcf886e07d3a4b3dc244c508d8560 with merge d6466a39d3becc3d18a8e4a3b82e96a9cc12fa5f...

rust-timer · 2022-10-05T14:34:11Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-10-05T14:34:16Z

⌛ Trying commit 90b96f9206ffcf886e07d3a4b3dc244c508d8560 with merge c31aae1b8bc03d9e144dfa396ebbc27300921cb0...

rust-timer · 2022-10-05T14:34:18Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-10-05T14:50:14Z

💔 Test failed - checks-actions

fmease · 2022-10-05T14:53:46Z

spurious failure (network timeouts, 504)

GuillaumeGomez · 2022-10-05T15:10:40Z

Ah seems like the bot is back. Let's retry and if there is another spurious failure, we'll just wait a bit before trying again.

@bors try @rust-timer queue

rust-timer · 2022-10-05T15:43:05Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-10-05T15:43:12Z

⌛ Trying commit 90b96f9206ffcf886e07d3a4b3dc244c508d8560 with merge ba060cbeed267dd7d824a442e8e60be20a433e0b...

bors · 2022-10-05T17:10:06Z

☀️ Try build successful - checks-actions
Build commit: ba060cbeed267dd7d824a442e8e60be20a433e0b (ba060cbeed267dd7d824a442e8e60be20a433e0b)

rust-timer · 2022-10-05T17:10:08Z

Queued ba060cbeed267dd7d824a442e8e60be20a433e0b with parent 8c71b67, future comparison URL.

notriddle · 2022-10-05T22:09:23Z

@bors try @rust-timer queue

rust-timer · 2022-10-05T22:09:25Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-10-05T22:09:32Z

⌛ Trying commit 73c239e with merge 0fa4b176a28776a3c7df79c783f6f6642cf928ed...

bors · 2022-10-05T23:59:58Z

☀️ Try build successful - checks-actions
Build commit: 0fa4b176a28776a3c7df79c783f6f6642cf928ed (0fa4b176a28776a3c7df79c783f6f6642cf928ed)

rust-timer · 2022-10-06T00:00:01Z

Queued 0fa4b176a28776a3c7df79c783f6f6642cf928ed with parent 75ada3a, future comparison URL.

rust-timer · 2022-10-06T03:21:27Z

Finished benchmarking commit (0fa4b176a28776a3c7df79c783f6f6642cf928ed): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.2%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.1%	[1.3%, 3.0%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.5%	[-2.5%, -2.5%]	1
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

GuillaumeGomez · 2022-10-06T08:39:00Z

Thanks!

@bors r+

bors · 2022-10-06T08:39:01Z

📌 Commit 73c239e has been approved by GuillaumeGomez

It is now in the queue for this repository.

bors · 2022-10-06T08:58:05Z

⌛ Testing commit 73c239e with merge 6b6610b...

bors · 2022-10-06T12:14:14Z

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez
Pushing 6b6610b to master...

rust-timer · 2022-10-06T14:13:07Z

Finished benchmarking commit (6b6610b): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	1.4%	[1.4%, 1.4%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.4%	[1.4%, 1.4%]	1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.9%	[1.1%, 4.2%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.7%	[-2.3%, -1.0%]	2
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

fmease · 2022-10-06T14:22:57Z

Hmm, since the same commit results in two contradicting perf results, I'd say it might just be noise.

GuillaumeGomez · 2022-10-06T14:38:34Z

I think so too.

…te-hrtbs-properly, r=GuillaumeGomez rustdoc: render more cross-crate HRTBs properly Follow-up to rust-lang#102439. Render the `for<>` parameter lists of cross-crate higher-rank trait bounds (in where-clauses and in `impl Trait`). I've added a new field `bound_params` to `clean::WherePredicate::EqPredicate` (mirroring its sibling variant `BoundPredicate`). However, I had to box the existing fields since `EqPredicate` used to be the largest variant (128 bytes on 64-bit systems) and it would only have gotten bigger). Not sure if you like that approach. As an alternative, I could pass the uncleaned `ty::Predicate` alongside the cleaned `WherePredicate` to the various re-sugaring methods (similar to what `clean::AutoTraitFinder::param_env_to_generics` does). I haven't yet added the HTML & JSON rendering code for the newly added `bound_params` field since I am waiting for your opinion. Those two rendering code paths should actually be unreachable in practice given we re-sugar all(?) equality predicates to associated type bindings (and arbitrary equality predicates are not part of the Rust surface language at the time of this writing). If you agree with storing `bound_params` in `EqPredicate`, I think I can use it to greatly simplify the `clean::auto_trait` module (by also using `simplify::merge_bounds`). Maybe I can do that in any case though. `@rustbot` label T-rustdoc A-cross-crate-reexports r? `@GuillaumeGomez`

…ate-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.

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.

rust-highfive assigned GuillaumeGomez Oct 5, 2022

rustbot added A-rustdoc-json Area: Rustdoc JSON backend T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Oct 5, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 5, 2022

rustbot added the A-cross-crate-reexports Area: Documentation that has been re-exported from a different crate label Oct 5, 2022

fmease force-pushed the rustdoc-render-more-cross-crate-hrtbs-properly branch from 1052aa4 to 90b96f9 Compare October 5, 2022 13:13

GuillaumeGomez approved these changes Oct 5, 2022

View reviewed changes

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 5, 2022

This comment has been minimized.

Sign in to view

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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 5, 2022

rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 5, 2022

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 5, 2022

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 6, 2022

notriddle approved these changes Oct 6, 2022

View reviewed changes

GuillaumeGomez approved these changes Oct 6, 2022

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 6, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 6, 2022

bors merged commit 6b6610b into rust-lang:master Oct 6, 2022

rustbot added this to the 1.66.0 milestone Oct 6, 2022

fmease deleted the rustdoc-render-more-cross-crate-hrtbs-properly branch October 6, 2022 12:53

fmease mentioned this pull request Oct 3, 2023

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc: render more cross-crate HRTBs properly #102707

rustdoc: render more cross-crate HRTBs properly #102707

fmease commented Oct 5, 2022 •

edited

Loading

rustbot commented Oct 5, 2022

GuillaumeGomez commented Oct 5, 2022

notriddle commented Oct 5, 2022

GuillaumeGomez commented Oct 5, 2022

fmease commented Oct 5, 2022

bors commented Oct 5, 2022

rust-timer commented Oct 5, 2022

bors commented Oct 5, 2022

rust-timer commented Oct 5, 2022

This comment has been minimized.

bors commented Oct 5, 2022

fmease commented Oct 5, 2022

GuillaumeGomez commented Oct 5, 2022

rust-timer commented Oct 5, 2022

bors commented Oct 5, 2022

bors commented Oct 5, 2022

rust-timer commented Oct 5, 2022

notriddle commented Oct 5, 2022

rust-timer commented Oct 5, 2022

bors commented Oct 5, 2022

bors commented Oct 5, 2022

rust-timer commented Oct 6, 2022

rust-timer commented Oct 6, 2022

GuillaumeGomez commented Oct 6, 2022

bors commented Oct 6, 2022

bors commented Oct 6, 2022

bors commented Oct 6, 2022

rust-timer commented Oct 6, 2022

fmease commented Oct 6, 2022

GuillaumeGomez commented Oct 6, 2022

rustdoc: render more cross-crate HRTBs properly #102707

rustdoc: render more cross-crate HRTBs properly #102707

Conversation

fmease commented Oct 5, 2022 • edited Loading

rustbot commented Oct 5, 2022

GuillaumeGomez commented Oct 5, 2022

notriddle commented Oct 5, 2022

GuillaumeGomez commented Oct 5, 2022

fmease commented Oct 5, 2022

bors commented Oct 5, 2022

rust-timer commented Oct 5, 2022

bors commented Oct 5, 2022

rust-timer commented Oct 5, 2022

This comment has been minimized.

bors commented Oct 5, 2022

fmease commented Oct 5, 2022

GuillaumeGomez commented Oct 5, 2022

rust-timer commented Oct 5, 2022

bors commented Oct 5, 2022

bors commented Oct 5, 2022

rust-timer commented Oct 5, 2022

notriddle commented Oct 5, 2022

rust-timer commented Oct 5, 2022

bors commented Oct 5, 2022

bors commented Oct 5, 2022

rust-timer commented Oct 6, 2022

rust-timer commented Oct 6, 2022

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

GuillaumeGomez commented Oct 6, 2022

bors commented Oct 6, 2022

bors commented Oct 6, 2022

bors commented Oct 6, 2022

rust-timer commented Oct 6, 2022

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

fmease commented Oct 6, 2022

GuillaumeGomez commented Oct 6, 2022

fmease commented Oct 5, 2022 •

edited

Loading