use ty::Binder in rustdoc instead of `skip_binder` #105612

oli-obk · 2022-12-12T14:45:22Z

this is a preliminary cleanup required to be able to normalize correctly/conveniently in rustdoc

…binding them with `Binder::dummy`)

rustbot · 2022-12-12T14:45:31Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @GuillaumeGomez (or someone else) soon.

Please see the contribution instructions for more information.

rustbot · 2022-12-12T14:45:34Z

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

cc @camelid

oli-obk · 2022-12-12T14:45:56Z

src/librustdoc/clean/mod.rs

-            if let Some(normalized_value) = normalize(cx, ty::Binder::dummy(ty)) {
-                return clean_middle_ty(normalized_value, cx, None);
+            // `hir_to_ty` can return projection types with escaping vars for GATs, e.g. `<() as Trait>::Gat<'_>`
+            if !ty.has_escaping_bound_vars() {
+                if let Some(normalized_value) = normalize(cx, ty::Binder::dummy(ty)) {
+                    return clean_middle_ty(normalized_value, cx, None);
+                }
            }


This is necessary due to the case that troubled us previously in #94683

GuillaumeGomez · 2022-12-12T15:28:19Z

Looks good to me, thanks! I don't think it'll have a perf impact but I'll let it up to you whether or not it should be in a rollup. r=me once CI pass.

oli-obk · 2022-12-12T15:32:55Z

@bors try @rust-timer queue

bors · 2022-12-12T15:33:06Z

⌛ Trying commit 3075451 with merge 6db79ca755889bcf58901a8690ebec8615550c83...

oli-obk · 2022-12-12T15:33:32Z

Even if it regresses perf for rustdoc, we should still land it as it's a correctness issue and will prevent ICEs down the road

bors · 2022-12-12T18:13:30Z

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

rust-timer · 2022-12-12T21:34:56Z

Finished benchmarking commit (6db79ca755889bcf58901a8690ebec8615550c83): comparison URL.

Overall result: ❌ regressions - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -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.4%	[0.2%, 0.7%]	5
Regressions ❌ (secondary)	0.5%	[0.3%, 0.7%]	12
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[0.2%, 0.7%]	5

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)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.0%	[-3.4%, -2.5%]	2
All ❌✅ (primary)	-	-	0

Cycles

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)	3.3%	[2.1%, 4.4%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

GuillaumeGomez · 2022-12-12T21:36:33Z

It's very acceptable. Let's go then!

@bors r+ rollup=never

bors · 2022-12-12T21:36:35Z

📌 Commit 3075451 has been approved by GuillaumeGomez

It is now in the queue for this repository.

aDotInTheVoid · 2022-12-13T00:58:24Z

we should still land it as it's a correctness issue

May I ask, if this is a correctness fix, is their anything in particular that this fixes, or is this just moving to a more principled way that's less brittle.

oli-obk · 2022-12-13T07:16:09Z

The latter. Right now all uses of normalization in rustdoc need to guard against broken types. This fixes that. I'll follow up with cleanups, but those may change rustdoc behaviour. The changes will be improvements to generics-heavy code

bors · 2022-12-13T13:09:44Z

⌛ Testing commit 3075451 with merge ed620cf...

bors · 2022-12-13T15:50:10Z

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

rust-timer · 2022-12-13T17:09:26Z

Finished benchmarking commit (ed620cf): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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.3%	[0.2%, 0.5%]	5
Regressions ❌ (secondary)	0.5%	[0.3%, 0.7%]	9
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.2%, 0.5%]	5

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)	5.8%	[2.4%, 7.7%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

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

oli-obk · 2022-12-13T18:37:54Z

The regressions are expected, solely in rustdoc and part of a bugfix

lqd · 2022-12-13T19:03:41Z

@rustbot label: +perf-regression-triaged

oli-obk added 4 commits December 12, 2022 12:12

Round 1: add some binders (fails due to losing bound vars and then re…

dbf4b8a

…binding them with `Binder::dummy`)

Round 2: make clean_middle_ty take a binder

8b09832

Round 3: require binders for substs

21917b0

Avoid trying to normalize unnormalizable types

3075451

rustbot assigned GuillaumeGomez Dec 12, 2022

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler 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. labels Dec 12, 2022

oli-obk commented Dec 12, 2022

View reviewed changes

GuillaumeGomez approved these changes Dec 12, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

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

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Dec 12, 2022

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 Dec 12, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 13, 2022

bors merged commit ed620cf into rust-lang:master Dec 13, 2022

rustbot added this to the 1.68.0 milestone Dec 13, 2022

oli-obk deleted the bind_rustdoc branch December 13, 2022 15:50

This was referenced Dec 13, 2022

reimplement ~const Trait bounds via a fourth kind of generic param #101900

Closed

Dedup bounds with parent impl block #105392

Open

rustbot added the perf-regression-triaged The performance regression has been triaged. label Dec 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use ty::Binder in rustdoc instead of `skip_binder` #105612

use ty::Binder in rustdoc instead of `skip_binder` #105612

oli-obk commented Dec 12, 2022

rustbot commented Dec 12, 2022

rustbot commented Dec 12, 2022

oli-obk Dec 12, 2022

GuillaumeGomez commented Dec 12, 2022

oli-obk commented Dec 12, 2022

This comment has been minimized.

bors commented Dec 12, 2022

oli-obk commented Dec 12, 2022

bors commented Dec 12, 2022

This comment has been minimized.

rust-timer commented Dec 12, 2022

GuillaumeGomez commented Dec 12, 2022

bors commented Dec 12, 2022

aDotInTheVoid commented Dec 13, 2022

oli-obk commented Dec 13, 2022

bors commented Dec 13, 2022

bors commented Dec 13, 2022

rust-timer commented Dec 13, 2022

oli-obk commented Dec 13, 2022

lqd commented Dec 13, 2022

use ty::Binder in rustdoc instead of skip_binder #105612

use ty::Binder in rustdoc instead of skip_binder #105612

Conversation

oli-obk commented Dec 12, 2022

rustbot commented Dec 12, 2022

rustbot commented Dec 12, 2022

oli-obk Dec 12, 2022

Choose a reason for hiding this comment

GuillaumeGomez commented Dec 12, 2022

oli-obk commented Dec 12, 2022

This comment has been minimized.

bors commented Dec 12, 2022

oli-obk commented Dec 12, 2022

bors commented Dec 12, 2022

This comment has been minimized.

rust-timer commented Dec 12, 2022

Overall result: ❌ regressions - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

GuillaumeGomez commented Dec 12, 2022

bors commented Dec 12, 2022

aDotInTheVoid commented Dec 13, 2022

oli-obk commented Dec 13, 2022

bors commented Dec 13, 2022

bors commented Dec 13, 2022

rust-timer commented Dec 13, 2022

Overall result: ❌ regressions - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

oli-obk commented Dec 13, 2022

lqd commented Dec 13, 2022

use ty::Binder in rustdoc instead of `skip_binder` #105612

use ty::Binder in rustdoc instead of `skip_binder` #105612