rustdoc: Replace where-bounded Clean impl with simple function #90750

camelid · 2021-11-10T02:28:37Z

This is the first step in removing the Clean impls for tuples. Either way, this
significantly simplifies the code since it reduces the amount of "trait magic".

(To clarify, I'm referring to impls like impl Clean for (A, B), not Clean impls
that work on tuples in the user's program.)

cc @jyn514

This was the only Clean impl I found with `where` bounds. This impl was doubly-confusing: it was implemented on a tuple and it was polymorphic. Combined, this caused a "spooky action at a distance" effect to make the code very confusing.

rust-highfive · 2021-11-10T02:28:40Z

r? @CraftSpider

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

camelid · 2021-11-10T02:28:51Z

r? @GuillaumeGomez

camelid · 2021-11-10T02:29:18Z

Probably best to review commit-by-commit.

camelid · 2021-11-10T03:08:22Z

Hmm, I broke rustdoc somehow...

Looks like 41479e1d106460015f639e3b041fd5e6dde12e95 caused the failure.

camelid · 2021-11-10T04:01:26Z

Ok, I fixed the test failures. Long story short: generics have to be cleaned before arguments. You can read the full saga in the "Remove where bound from clean_fn_decl_with_args" commit message.

Basically, this entails moving the arguments cleaning to the call site. I extracted several local variables because: 1. It makes the code easier to read and understand. 2. If I hadn't, the extra `clean()` calls would have caused complicated tuples to be split across several lines. 3. I couldn't just extract local variables for `args` because then the arguments would be cleaned *before* the generics, while rustdoc expects them to be cleaned *after*. Only extracting `args` caused panics like this: thread 'rustc' panicked at 'assertion failed: cx.impl_trait_bounds.is_empty()', src/librustdoc/clean/utils.rs:462:5 Extracting variables makes the control flow -- and the required order of cleaning -- more explicit.

jyn514

Hmm, I'm kind of concerned this makes it easier to make mistakes when cleaning function declarations. As you noted, the order is important, and it would be really easy to miss that when adding a new call. I kind of liked the version in the first commit better, with the where clause - I think if you added a comment there about the ordering it would be better than requiring each caller to know about it.

Otherwise, rustdoc panics with messages like this: thread 'rustc' panicked at 'assertion failed: cx.impl_trait_bounds.is_empty()', src/librustdoc/clean/utils.rs:462:5 This ordering requirement is unrelated to the `clean_fn_decl_with_args` refactoring, but the requirement was uncovered as part of that change. I'm not sure if *all* of these places have the requirement, but I added comments to them just in case.

camelid · 2021-11-10T05:12:17Z

Hmm, I'm kind of concerned this makes it easier to make mistakes when cleaning function declarations. As you noted, the order is important, and it would be really easy to miss that when adding a new call. I kind of liked the version in the first commit better, with the where clause - I think if you added a comment there about the ordering it would be better than requiring each caller to know about it.

We discussed this and came to the conclusion that the "generics before args" ordering requirement was there all along; it is not affected by this PR. So, this change should be all good.

At Joshua's suggestion, I added comments to what I believe are the relevant places to reduce the risk of the order accidentally being changed in the future.

camelid · 2021-11-10T05:14:13Z

src/librustdoc/clean/inline.rs

@@ -229,6 +229,7 @@ fn build_external_function(cx: &mut DocContext<'_>, did: DefId) -> clean::Functi
    let asyncness = cx.tcx.asyncness(did);
    let predicates = cx.tcx.predicates_of(did);
    let (generics, decl) = clean::enter_impl_trait(cx, |cx| {
+        // NOTE: generics need to be cleaned before the decl!


As I mentioned in the commit message, some of these places may not have the ordering requirement, but just to be safe I added comments in almost all of them.

The only place I didn't add a comment is the following

clean/inline.rs 446: clean::enter_impl_trait(cx, |cx| (tcx.generics_of(did), predicates).clean(cx)),

since it seemed like a different scenario. Let me know if you think I should add a comment there though.

GuillaumeGomez · 2021-11-10T09:26:41Z

I agree with @jyn514: it's a bit worrying that the order matters. Would it be possible to create a function where you handle the clean calls in the right order and then call it from the other places? That would make it much less likely to call cleans in the wrong order.

camelid · 2021-11-10T19:01:16Z

I agree with @jyn514: it's a bit worrying that the order matters. Would it be possible to create a function where you handle the clean calls in the right order and then call it from the other places? That would make it much less likely to call cleans in the wrong order.

As I said above, Joshua and I came to the conclusion that the required order is not affected by this PR.

I agree that it'd be good to not have this ordering requirement spread across the code, but it should happen in a future PR since it is unrelated. The only reason I stumbled upon the ordering requirement in this PR is that I was trying to extract some local variables to make the code simpler, and I accidentally changed the order of cleaning. (The local variables are now in the correct order.) Like I said, the ordering is not affected by this PR.

GuillaumeGomez · 2021-11-10T19:11:08Z

Except that you do call them all "by hand". Even if the order isn't affected by this PR. It's not a blocker for me though, just a preference.

camelid · 2021-11-10T21:36:54Z

Followup: I opened #90778 to track encapsulating the ordering requirement in a helper function in a future PR.

jyn514 · 2021-11-18T14:52:44Z

@bors r+

bors · 2021-11-18T14:52:46Z

📌 Commit c20ee3e has been approved by jyn514

rustdoc: Replace where-bounded Clean impl with simple function This is the first step in removing the Clean impls for tuples. Either way, this significantly simplifies the code since it reduces the amount of "trait magic". (To clarify, I'm referring to impls like `impl Clean for (A, B)`, not Clean impls that work on tuples in the user's program.) cc `@jyn514`

Rollup of 8 pull requests Successful merges: - rust-lang#90386 (Add `-Zassert-incr-state` to assert state of incremental cache) - rust-lang#90438 (Clean up mess for --show-coverage documentation) - rust-lang#90480 (Mention `Vec::remove` in `Vec::swap_remove`'s docs) - rust-lang#90607 (Make slice->str conversion and related functions `const`) - rust-lang#90750 (rustdoc: Replace where-bounded Clean impl with simple function) - rust-lang#90895 (require full validity when determining the discriminant of a value) - rust-lang#90989 (Avoid suggesting literal formatting that turns into member access) - rust-lang#91002 (rustc: Remove `#[rustc_synthetic]`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

pnkfelix · 2021-11-23T17:51:22Z

This PR has been tagged as being the root cause of the performance regression flagged here.

It seems to me like the extra work injected by PR #90750 may be unavoidable; but was it expected to be significant?

pnkfelix · 2021-11-23T17:57:03Z

@rustbot label: +perf-regression

camelid · 2021-11-23T18:16:19Z

It seems to me like the extra work injected by PR #90750 may be unavoidable; but was it expected to be significant?

What extra work? This PR is just a refactoring that makes the work more explicit.

camelid · 2021-11-23T18:20:53Z

Sometimes changes to rustc impact only rustdoc, so one of the rustc changes could be at fault.

camelid · 2021-12-03T22:07:27Z

@pnkfelix Following up: as I said, this PR doesn't add any extra work, so it's unlikely that this caused the regression.

Replace where-bounded Clean impl with function

169b84f

This was the only Clean impl I found with `where` bounds. This impl was doubly-confusing: it was implemented on a tuple and it was polymorphic. Combined, this caused a "spooky action at a distance" effect to make the code very confusing.

camelid added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Nov 10, 2021

rust-highfive assigned CraftSpider Nov 10, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 10, 2021

rust-highfive assigned GuillaumeGomez and unassigned CraftSpider Nov 10, 2021

This comment has been minimized.

Sign in to view

camelid force-pushed the rm-tuple-impls-1 branch from 8b30d1b to 45f7220 Compare November 10, 2021 04:00

camelid force-pushed the rm-tuple-impls-1 branch from 45f7220 to 86b5975 Compare November 10, 2021 04:02

camelid added 2 commits November 9, 2021 20:05

Remove unnecessary reborrows

c615b11

camelid force-pushed the rm-tuple-impls-1 branch from 86b5975 to c615b11 Compare November 10, 2021 04:06

jyn514 reviewed Nov 10, 2021

View reviewed changes

jyn514 changed the title ~~Replace where-bounded Clean impl with simple function~~ rustdoc: Replace where-bounded Clean impl with simple function Nov 10, 2021

camelid force-pushed the rm-tuple-impls-1 branch from c5864e2 to c20ee3e Compare November 10, 2021 05:10

camelid commented Nov 10, 2021

View reviewed changes

camelid mentioned this pull request Nov 10, 2021

rustdoc: Encapsulate "clean generics before args" ordering requirement in a function #90778

Open

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 Nov 18, 2021

JohnTitor mentioned this pull request Nov 18, 2021

Rollup of 9 pull requests #91016

Closed

JohnTitor mentioned this pull request Nov 18, 2021

Rollup of 8 pull requests #91019

Merged

camelid assigned jyn514 and unassigned GuillaumeGomez Nov 18, 2021

bors merged commit 47c1bd1 into rust-lang:master Nov 18, 2021

rustbot added this to the 1.58.0 milestone Nov 18, 2021

camelid deleted the rm-tuple-impls-1 branch November 19, 2021 02:14

rustbot added the perf-regression Performance regression. label Nov 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc: Replace where-bounded Clean impl with simple function #90750

rustdoc: Replace where-bounded Clean impl with simple function #90750

camelid commented Nov 10, 2021 •

edited

Loading

rust-highfive commented Nov 10, 2021

camelid commented Nov 10, 2021

camelid commented Nov 10, 2021

This comment has been minimized.

camelid commented Nov 10, 2021 •

edited

Loading

camelid commented Nov 10, 2021 •

edited

Loading

jyn514 left a comment

camelid commented Nov 10, 2021

camelid Nov 10, 2021

GuillaumeGomez commented Nov 10, 2021

camelid commented Nov 10, 2021

GuillaumeGomez commented Nov 10, 2021

camelid commented Nov 10, 2021

jyn514 commented Nov 18, 2021

bors commented Nov 18, 2021

pnkfelix commented Nov 23, 2021 •

edited

Loading

pnkfelix commented Nov 23, 2021

camelid commented Nov 23, 2021

camelid commented Nov 23, 2021

camelid commented Dec 3, 2021

rustdoc: Replace where-bounded Clean impl with simple function #90750

rustdoc: Replace where-bounded Clean impl with simple function #90750

Conversation

camelid commented Nov 10, 2021 • edited Loading

rust-highfive commented Nov 10, 2021

camelid commented Nov 10, 2021

camelid commented Nov 10, 2021

This comment has been minimized.

camelid commented Nov 10, 2021 • edited Loading

camelid commented Nov 10, 2021 • edited Loading

jyn514 left a comment

Choose a reason for hiding this comment

camelid commented Nov 10, 2021

camelid Nov 10, 2021

Choose a reason for hiding this comment

GuillaumeGomez commented Nov 10, 2021

camelid commented Nov 10, 2021

GuillaumeGomez commented Nov 10, 2021

camelid commented Nov 10, 2021

jyn514 commented Nov 18, 2021

bors commented Nov 18, 2021

pnkfelix commented Nov 23, 2021 • edited Loading

pnkfelix commented Nov 23, 2021

camelid commented Nov 23, 2021

camelid commented Nov 23, 2021

camelid commented Dec 3, 2021

camelid commented Nov 10, 2021 •

edited

Loading

camelid commented Nov 10, 2021 •

edited

Loading

camelid commented Nov 10, 2021 •

edited

Loading

pnkfelix commented Nov 23, 2021 •

edited

Loading