rustdoc ICE fix: When collecting Deref impls with their targets, skip the negative ones#154444
rustdoc ICE fix: When collecting Deref impls with their targets, skip the negative ones#154444jakubadamw wants to merge 3 commits intorust-lang:mainfrom
Deref impls with their targets, skip the negative ones#154444Conversation
|
r? @fmease rustbot has assigned @fmease. Use Why was this reviewer chosen?The reviewer was selected based on:
|
There was a problem hiding this comment.
The two build_impl fns (one in clean/mod.rs, the other in clean/mod.rs) also invoke extra code when encountering Deref trait impls by calling build_deref_target_impls. Likewise, in render/sidebar.rs we have two places that perform extra work for deref impls.
I'm relatively sure that all four of these places still process negative deref trait impls, likely unnecessarily so. If it's not too much work for you, could you filter out negative impls in these places (unless it's impossible for negative deref impls to show up there). It probably doesn't save that much or any processing time but it might be more robust.
If you do that it might also be worth adding a HTML test in tests/rustdoc-html to ensure that we still actually render impl !Deref for … impls (in the sidebar and the main container).
There was a problem hiding this comment.
@fmease, thank you for reviewing the PR. 🙂
The two
build_implfns (one inclean/mod.rs, the other inclean/mod.rs)
I take it you mean clean/mod.rs and clean/inline.rs, as well as clean_impl and build_impl, respectively? I pushed a commit that addresses the suggestion for those four sites, and adds an HTML test. 🙂
Thanks again! Let me know if there’s anything else I’m missing here.
@rustbot ready
There was a problem hiding this comment.
@fmease, would you be able to take a look at this again? I made the requested change. Thanks! 🙂
There was a problem hiding this comment.
The two
build_implfns (one inclean/mod.rs, the other inclean/mod.rs)I take it you mean clean/mod.rs and clean/inline.rs, as well as
clean_implandbuild_impl, respectively?
Ah yes, haha. Silly typo.
Thanks for addressing my feedback! Sorry, the PR slipped through the cracks because apparently rustbot still doesn't honor @rustbot ready in review comments, so this PR didn't show up using my usual filter.
83091df to
d3cf791
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
d3cf791 to
845e2b7
Compare
This comment has been minimized.
This comment has been minimized.
…egative ones
rustdoc assumed every `Deref` impl has an associated `Target` type,
but negative impls (e.g. `impl !Deref for T {}`) have none.
Skip them in both the trait-impl collection pass and the
HTML render pass to avoid panicking on the missing Target.
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
rustdoc ICE fix: When collecting `Deref` impls with their targets, skip the negative ones
rustdoc assumed every `Deref` impl has an associated `Target` type, but negative impls (e.g. `impl !Deref for T {}`) have none.
Skip them in both the trait-impl collection pass and the HTML render pass to avoid panicking on the missing `Target`.
Closes rust-lang#128801.
…uwer Rollup of 4 pull requests Successful merges: - #154444 (rustdoc ICE fix: When collecting `Deref` impls with their targets, skip the negative ones) - #154590 (Make #[cfg] suggest any or all on #[cfg(a, b)]) - #154691 (core: Update the feature gate on `TryFrom<integer> for bool`) - #154697 (rustdoc: fix href of extern crates in search results) Failed merges: - #154722 (fix(lints): Improve `ill_formed_attribute_input` with better help message)
rustdoc ICE fix: When collecting `Deref` impls with their targets, skip the negative ones
rustdoc assumed every `Deref` impl has an associated `Target` type, but negative impls (e.g. `impl !Deref for T {}`) have none.
Skip them in both the trait-impl collection pass and the HTML render pass to avoid panicking on the missing `Target`.
Closes rust-lang#128801.
Rollup of 6 pull requests Successful merges: - #154444 (rustdoc ICE fix: When collecting `Deref` impls with their targets, skip the negative ones) - #154590 (Make #[cfg] suggest any or all on #[cfg(a, b)]) - #154691 (core: Update the feature gate on `TryFrom<integer> for bool`) - #154697 (rustdoc: fix href of extern crates in search results) - #154728 (rustdoc: Improve internal function name) - #154732 (fix(std): avoid AT_MINSIGSTKSZ on uclibc targets) Failed merges: - #154722 (fix(lints): Improve `ill_formed_attribute_input` with better help message)
rustdoc assumed every
Derefimpl has an associatedTargettype, but negative impls (e.g.impl !Deref for T {}) have none.Skip them in both the trait-impl collection pass and the HTML render pass to avoid panicking on the missing
Target.Closes #128801.