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

rustdoc: fix and refactor HTML rendering a bit #121160

Merged
merged 1 commit into from
Feb 18, 2024

Conversation

fmease
Copy link
Member

@fmease fmease commented Feb 15, 2024

  • refactoring: get rid of a bunch of manual f.alternate() branches
    • not sure why this wasn't done so already, is this perf-sensitive?
  • fix an ICE in debug builds of rustdoc
    • rustdoc used to crash on empty outlives-bounds: where 'a:
  • properly escape const generic defaults
  • actually print empty trait and outlives-bounds (doesn't work for cross-crate reexports yet, will fix that at some other point) since they can have semantic significance

@fmease fmease marked this pull request as ready for review February 15, 2024 19:43
@rustbot
Copy link
Collaborator

rustbot commented Feb 15, 2024

r? @GuillaumeGomez

rustbot has assigned @GuillaumeGomez.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added 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 Feb 15, 2024
if f.alternate() {
write!(f, " = {default:#}")?;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously this was formatting a String with {:#} but # doesn't have any effect on <String as Display>. Furthermore this didn't escape const generic defaults.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a nice picture cuz why not:

Screenshot 2024-02-15 at 05-23-51 S in unesc - Rust

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mouahaha. Nice one. :D

}
bounds_display.truncate(bounds_display.len() - " + ".len());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would overflow & wrap-around in release which didn't cause any issues but crash in debug builds of rustdoc. Just use the preexisting print_generic_bounds.

@GuillaumeGomez
Copy link
Member

Let's run a perf check to see if there is a change in perf:

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 15, 2024
where
T:,
'a:,
{}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gotta run now but I can add more tests demo'ing the importance of showing empty trait bounds, e.g., one involving generic_const_exprs & one affecting wf'cking.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have more regression tests, please add them. The more the better.

Copy link
Member Author

@fmease fmease Feb 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E.g.,

#![feature(generic_const_exprs)]

pub fn foo<const N: usize>() where [(); N + 1]: {
    let _: [(); N + 1];
}

cc #120994 (comment)

Copy link
Member Author

@fmease fmease Feb 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And re well-formedness checking (a bit artificial, I know ^^')

pub fn f() -> <[[[u8]]] as Discard>::Output where <[[[u8]]] as Discard>::Output: {}

pub trait Discard {
    type Output;
}

impl<T: ?Sized> Discard for T {
    type Output = ();
}

cc #100041

Copy link
Member Author

@fmease fmease Feb 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests/rustdoc/const-generics/generic_const_exprs.rs suffices imo, it demonstrates the use-case perfectly :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok!

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 15, 2024
…endering, r=<try>

rustdoc: fix and refactor HTML rendering a bit

* refactoring: get rid of a bunch of manual `f.alternative()` branches
  * not sure why this doesn't done yet, is this perf-sensitive?
* fix an ICE in debug builds of rustdoc
  * rustdoc used to crash on empty outlives-bounds: `where 'a:`
* properly escape const generic defaults
@bors
Copy link
Contributor

bors commented Feb 15, 2024

⌛ Trying commit 2f6a913 with merge 14c6553...

@rust-log-analyzer

This comment has been minimized.

@fmease
Copy link
Member Author

fmease commented Feb 15, 2024

Oops, yeah, was in a hurry to leave for some place and didn't run the test suite locally. Gonna fix the CI errors later.

@bors
Copy link
Contributor

bors commented Feb 15, 2024

☀️ Try build successful - checks-actions
Build commit: 14c6553 (14c6553fbbda663378f1e0346184328ce5707ac8)

1 similar comment
@bors
Copy link
Contributor

bors commented Feb 15, 2024

☀️ Try build successful - checks-actions
Build commit: 14c6553 (14c6553fbbda663378f1e0346184328ce5707ac8)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (14c6553): comparison URL.

Overall result: no relevant changes - 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-perf -perf-regression

Instruction count

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

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)
-7.3% [-7.3%, -7.3%] 1
All ❌✅ (primary) - - 0

Cycles

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

Binary size

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

Bootstrap: 636.031s -> 637.107s (0.17%)
Artifact size: 306.32 MiB -> 306.31 MiB (-0.00%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 16, 2024
@GuillaumeGomez
Copy link
Member

No perf change. So once tests are fixed, we can merge. Thanks a lot for this code improvement! :)

@fmease fmease force-pushed the rustdoc-fix-n-refactor-html-rendering branch from 2f6a913 to 5fbb1b2 Compare February 16, 2024 20:29
@GuillaumeGomez
Copy link
Member

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 17, 2024

📌 Commit 5fbb1b2 has been approved by GuillaumeGomez

It is now in the queue for this repository.

@bors 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 Feb 17, 2024
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Feb 17, 2024
…-rendering, r=GuillaumeGomez

rustdoc: fix and refactor HTML rendering a bit

* refactoring: get rid of a bunch of manual `f.alternate()` branches
  * not sure why this wasn't done so already, is this perf-sensitive?
* fix an ICE in debug builds of rustdoc
  * rustdoc used to crash on empty outlives-bounds: `where 'a:`
* properly escape const generic defaults
* actually print empty trait and outlives-bounds (doesn't work for cross-crate reexports yet, will fix that at some other point) since they can have semantic significance
  * outlives-bounds: forces lifetime params to be early-bound instead of late-bound which is technically speaking part of the public API
  * trait-bounds: can affect the well-formedness, consider
    * makeshift “const-evaluatable” bounds under `generic_const_exprs`
    * bounds to force wf-checking in light of rust-lang#100041 (quite artificial I know, I couldn't figure out something better), see rust-lang#121160 (comment)
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 17, 2024
…llaumeGomez

Rollup of 7 pull requests

Successful merges:

 - rust-lang#118264 (Optimize `VecDeque::drain` for (half-)open ranges)
 - rust-lang#121079 (distribute tool documentations and avoid file conflicts on `x install`)
 - rust-lang#121100 (Detect when method call on argument could be removed to fulfill failed trait bound)
 - rust-lang#121160 (rustdoc: fix and refactor HTML rendering a bit)
 - rust-lang#121198 (Add more checks for `unnamed_fields` during HIR analysis)
 - rust-lang#121221 (AstConv: Refactor lowering of associated item bindings a bit)
 - rust-lang#121237 (Use better heuristic for printing Cargo specific diagnostics)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 18, 2024
Rollup of 7 pull requests

Successful merges:

 - rust-lang#120526 (rustdoc: Correctly handle long crate names on mobile)
 - rust-lang#121100 (Detect when method call on argument could be removed to fulfill failed trait bound)
 - rust-lang#121160 (rustdoc: fix and refactor HTML rendering a bit)
 - rust-lang#121198 (Add more checks for `unnamed_fields` during HIR analysis)
 - rust-lang#121218 (Fix missing trait impls for type in rustc docs)
 - rust-lang#121221 (AstConv: Refactor lowering of associated item bindings a bit)
 - rust-lang#121237 (Use better heuristic for printing Cargo specific diagnostics)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 28c0fa8 into rust-lang:master Feb 18, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 18, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 18, 2024
Rollup merge of rust-lang#121160 - fmease:rustdoc-fix-n-refactor-html-rendering, r=GuillaumeGomez

rustdoc: fix and refactor HTML rendering a bit

* refactoring: get rid of a bunch of manual `f.alternate()` branches
  * not sure why this wasn't done so already, is this perf-sensitive?
* fix an ICE in debug builds of rustdoc
  * rustdoc used to crash on empty outlives-bounds: `where 'a:`
* properly escape const generic defaults
* actually print empty trait and outlives-bounds (doesn't work for cross-crate reexports yet, will fix that at some other point) since they can have semantic significance
  * outlives-bounds: forces lifetime params to be early-bound instead of late-bound which is technically speaking part of the public API
  * trait-bounds: can affect the well-formedness, consider
    * makeshift “const-evaluatable” bounds under `generic_const_exprs`
    * bounds to force wf-checking in light of rust-lang#100041 (quite artificial I know, I couldn't figure out something better), see rust-lang#121160 (comment)
@fmease fmease deleted the rustdoc-fix-n-refactor-html-rendering branch February 18, 2024 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants