Repository navigation
[rustdoc] Fix how Deref items is handled. - #160915
Conversation
|
|
|
Oof, that's a big diff. It's mostly because of reindent, not much that we can do about. ^^' |
This comment has been minimized.
This comment has been minimized.
32c5654 to
e3fb18c
Compare
|
Fixed |
Fixed the typo. Correct sentence is:
|
| let for_ = clean_ty(impl_.self_ty, cx); | ||
|
|
There was a problem hiding this comment.
nit: Moving this let is now unnecessary. Better to move it back to where it's used.
| @@ -295,6 +295,8 @@ pub(crate) fn build_deref_target_impls( | |||
| inline::build_impls(cx, did, None, ret); | |||
| }); | |||
| } | |||
| } else { | |||
| break; | |||
There was a problem hiding this comment.
Why was this break added?
There was a problem hiding this comment.
Because it's actually looking for the Deref::Target item and doesn't do anything. So at that point, we can break as soon as we found it. Can remove it though, considering there are only 2 items, doesn't matter much.
There was a problem hiding this comment.
Also just realized that it shouldn't be in a else condition. Anyway, removing it.
| @@ -3150,3 +3174,30 @@ fn repr_attribute<'tcx>( | |||
|
|
|||
| (!result.is_empty()).then(|| format!("#[repr({})]", result.join(", ")).into()) | |||
| } | |||
|
|
|||
| pub(crate) fn compute_if_deref_target_implements_copy( | |||
| cx: &Context<'_>, | |||
There was a problem hiding this comment.
Let's have this take TyCtxt instead of Context and change the callers as well. It's better to take as little input as possible.
| AlsoCollectAssocFns { assoc_fns: &'r mut Vec<Link<'l>> }, | ||
| } | ||
|
|
||
| fn get_methods<'a>( | ||
| i: &'a clean::Impl, | ||
| mut mode: GetMethodsMode<'_, 'a>, | ||
| used_links: &mut FxHashSet<String>, | ||
| tcx: TyCtxt<'_>, | ||
| cx: &Context<'_>, |
There was a problem hiding this comment.
Ditto here.
ca399ba to
7a71fdf
Compare
This comment has been minimized.
This comment has been minimized.
|
Applied suggestions. @rustbot ready |
|
ping @camelid ;) |
| } else { | ||
| false | ||
| } | ||
| (deref_mut_ || !by_mut_ref) && !by_box && (!by_value || target_is_copy) |
There was a problem hiding this comment.
Any idea why Box is special-cased here? Seems weird to special-case at all, and also I would expect needing special cases for things like Rc or Pin as well if Box truly needs it.
There was a problem hiding this comment.
I think it's because Box is a known type implementing Deref for the end type, common enough to make sense as an optimization? And should likely add the other types you mentioned. However in another PR might make more sense.
There was a problem hiding this comment.
This causes a behavior difference though, right? It's not just a matter of optimization/performance.
There was a problem hiding this comment.
Could you see what behavior changes if you remove the !by_box part of this expression?
There was a problem hiding this comment.
Without by_box, when running the rustdoc-html tests:
failures:
---- [rustdoc-html] tests/rustdoc-html/deref-mut-35169-2.rs stdout ----
------python3 stdout------------------------------
------python3 stderr------------------------------
38: !has check failed
`XPATH PATTERN` unexpectedly matched
//@ !has - '//*[@id="method.by_explicit_box"]//h4[@class="code-header"]' 'fn by_explicit_box(self: Box<Foo>)'
39: !has check failed
`XPATH PATTERN` unexpectedly matched
//@ !has - '//*[@id="method.by_explicit_box"]' 'fn by_explicit_box(self: Box<Foo>)'
40: !has check failed
`XPATH PATTERN` unexpectedly matched
//@ !has - '//*[@id="method.by_explicit_self_box"]//h4[@class="code-header"]' 'fn by_explicit_self_box(self: Box<Self>)'
41: !has check failed
`XPATH PATTERN` unexpectedly matched
//@ !has - '//*[@id="method.by_explicit_self_box"]' 'fn by_explicit_self_box(self: Box<Self>)'
Encountered 4 errors
------------------------------------------
error: htmldocck failed!
status: exit status: 1
command: "/usr/bin/python3" "/home/imperio/rust/rust/src/etc/htmldocck.py" "/home/imperio/rust/rust/build/x86_64-unknown-linux-gnu/test/rustdoc-html/deref-mut-35169-2" "/home/imperio/rust/rust/tests/rustdoc-html/deref-mut-35169-2.rs"
stdout: none
--- stderr -------------------------------
38: !has check failed
`XPATH PATTERN` unexpectedly matched
//@ !has - '//*[@id="method.by_explicit_box"]//h4[@class="code-header"]' 'fn by_explicit_box(self: Box<Foo>)'
39: !has check failed
`XPATH PATTERN` unexpectedly matched
//@ !has - '//*[@id="method.by_explicit_box"]' 'fn by_explicit_box(self: Box<Foo>)'
40: !has check failed
`XPATH PATTERN` unexpectedly matched
//@ !has - '//*[@id="method.by_explicit_self_box"]//h4[@class="code-header"]' 'fn by_explicit_self_box(self: Box<Self>)'
41: !has check failed
`XPATH PATTERN` unexpectedly matched
//@ !has - '//*[@id="method.by_explicit_self_box"]' 'fn by_explicit_self_box(self: Box<Self>)'
Encountered 4 errors
------------------------------------------
---- [rustdoc-html] tests/rustdoc-html/deref-mut-35169-2.rs stdout end ----
---- [rustdoc-html] tests/rustdoc-html/deref-mut-35169.rs stdout ----
------python3 stdout------------------------------
------python3 stderr------------------------------
33: !has check failed
`XPATH PATTERN` unexpectedly matched
//@ !has - '//*[@id="method.by_explicit_box"]//h4[@class="code-header"]' 'fn by_explicit_box(self: Box<Foo>)'
34: !has check failed
`XPATH PATTERN` unexpectedly matched
//@ !has - '//*[@id="method.by_explicit_box"]' 'fn by_explicit_box(self: Box<Foo>)'
35: !has check failed
`XPATH PATTERN` unexpectedly matched
//@ !has - '//*[@id="method.by_explicit_self_box"]//h4[@class="code-header"]' 'fn by_explicit_self_box(self: Box<Self>)'
36: !has check failed
`XPATH PATTERN` unexpectedly matched
//@ !has - '//*[@id="method.by_explicit_self_box"]' 'fn by_explicit_self_box(self: Box<Self>)'
Encountered 4 errors
------------------------------------------
error: htmldocck failed!
status: exit status: 1
command: "/usr/bin/python3" "/home/imperio/rust/rust/src/etc/htmldocck.py" "/home/imperio/rust/rust/build/x86_64-unknown-linux-gnu/test/rustdoc-html/deref-mut-35169" "/home/imperio/rust/rust/tests/rustdoc-html/deref-mut-35169.rs"
stdout: none
--- stderr -------------------------------
33: !has check failed
`XPATH PATTERN` unexpectedly matched
//@ !has - '//*[@id="method.by_explicit_box"]//h4[@class="code-header"]' 'fn by_explicit_box(self: Box<Foo>)'
34: !has check failed
`XPATH PATTERN` unexpectedly matched
//@ !has - '//*[@id="method.by_explicit_box"]' 'fn by_explicit_box(self: Box<Foo>)'
35: !has check failed
`XPATH PATTERN` unexpectedly matched
//@ !has - '//*[@id="method.by_explicit_self_box"]//h4[@class="code-header"]' 'fn by_explicit_self_box(self: Box<Self>)'
36: !has check failed
`XPATH PATTERN` unexpectedly matched
//@ !has - '//*[@id="method.by_explicit_self_box"]' 'fn by_explicit_self_box(self: Box<Self>)'
Encountered 4 errors
------------------------------------------
---- [rustdoc-html] tests/rustdoc-html/deref-mut-35169.rs stdout end ----
failures:
[rustdoc-html] tests/rustdoc-html/deref-mut-35169-2.rs
[rustdoc-html] tests/rustdoc-html/deref-mut-35169.rs
I think we should leave it as is. ;)
There was a problem hiding this comment.
Hmm, I still think this special-casing is a little sketchy but it's pre-existing so we should deal with it in another PR.
7a71fdf to
6eaa385
Compare
This comment has been minimized.
This comment has been minimized.
…uwer Rollup of 12 pull requests Successful merges: - #163531 (rustc_ast_lowering: track implicit Self via explicit flag instead of name) - #160915 ([rustdoc] Fix how `Deref` items is handled.) - #163364 (make semicolon_in_expressions_from_non_local_macros not report-in-deps) - #163770 (document the rustc_comptime attribute) - #163780 (Skip optional asserts in SsaRangePropagation) - #163785 (Bump Windows CI LLVM to 22.1.8) - #163796 (continue crashes tests `-Znext-solver` work) - #163646 (Get `inputs_hir` directly from `decl`) - #163733 (Fix extra spaces in integer format_into docs) - #163773 (Update GitHub Actions to v26) - #163787 (moves rustc_legacy_const_generics checks into attribute parsing) - #163810 (Fix an issue for clippy's `search_is_some` with the next-solver)
|
💔 I suspect this PR failed tests as part of a rollup Failed during a local run I think you need to rebase |
|
This pull request was unapproved. This PR was contained in a rollup (#163827), which was unapproved. |
5463f66 to
32561a0
Compare
|
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. |
|
@JonathanBrouwer Yeah it needed a rebase. @bors r=camelid |
[rustdoc] Fix how `Deref` items is handled. Fixes rust-lang#160236. This PR fixes a few things around how we handle `Deref`: * Since nothing except for methods is callable through a `Deref`, only methods should be kept * If the `Deref::Target` is a type implementing `Copy`, then methods taking `self` work and should be displayed. <del>During the `clean` pass, I store the item `DefId` on which `Deref` is implemented in a `DefIdSet` if the `Deref::Target` implements `Copy`. Then during rendering, we check if the "parent item" is in in the `DefIdSet`, and if so, we keep methods with `self`.</del> <del>Now you might wonder why the item and not the derefed item. It's because the `DefId` we get from the computed type doesn't match the `DefId` of the actual type (that's where I spent most of my time, finding an ID (`DefId`/`ItemId`) I can use as key T_T).</del> It computes in `html/render` if the `Deref::Target` item is copy before rendering it. So to resume: * `&self` is always kept. * `&mut self` is kept if `DerefMut` is implemented (nothing changed there). * `self` is kept only if `Deref::Target` is `Copy`. * Everything else disappears as they're not callable through `Deref`. r? @camelid
…uwer Rollup of 4 pull requests Successful merges: - #160915 ([rustdoc] Fix how `Deref` items is handled.) - #163267 (fix `VisibleForLeakCheck` in `RegionOutlives` fast path) - #163673 (Move #[doc(inline)] and #[doc(no_inline)] validation into rustc_attr_parsing) - #163814 (Add some comments and docs related to `AllocatorNightly`)
…uwer Rollup of 5 pull requests Successful merges: - #160915 ([rustdoc] Fix how `Deref` items is handled.) - #163267 (fix `VisibleForLeakCheck` in `RegionOutlives` fast path) - #163673 (Move #[doc(inline)] and #[doc(no_inline)] validation into rustc_attr_parsing) - #163795 (When documenting without a specified `--edition`, emit a message) - #163814 (Add some comments and docs related to `AllocatorNightly`)
…uwer Rollup of 5 pull requests Successful merges: - #160915 ([rustdoc] Fix how `Deref` items is handled.) - #163267 (fix `VisibleForLeakCheck` in `RegionOutlives` fast path) - #163673 (Move #[doc(inline)] and #[doc(no_inline)] validation into rustc_attr_parsing) - #163795 (When documenting without a specified `--edition`, emit a message) - #163814 (Add some comments and docs related to `AllocatorNightly`)
…uwer Rollup of 5 pull requests Successful merges: - #160915 ([rustdoc] Fix how `Deref` items is handled.) - #163267 (fix `VisibleForLeakCheck` in `RegionOutlives` fast path) - #163673 (Move #[doc(inline)] and #[doc(no_inline)] validation into rustc_attr_parsing) - #163795 (When documenting without a specified `--edition`, emit a message) - #163814 (Add some comments and docs related to `AllocatorNightly`)
Rollup merge of #160915 - GuillaumeGomez:deref-items, r=camelid [rustdoc] Fix how `Deref` items is handled. Fixes #160236. This PR fixes a few things around how we handle `Deref`: * Since nothing except for methods is callable through a `Deref`, only methods should be kept * If the `Deref::Target` is a type implementing `Copy`, then methods taking `self` work and should be displayed. <del>During the `clean` pass, I store the item `DefId` on which `Deref` is implemented in a `DefIdSet` if the `Deref::Target` implements `Copy`. Then during rendering, we check if the "parent item" is in in the `DefIdSet`, and if so, we keep methods with `self`.</del> <del>Now you might wonder why the item and not the derefed item. It's because the `DefId` we get from the computed type doesn't match the `DefId` of the actual type (that's where I spent most of my time, finding an ID (`DefId`/`ItemId`) I can use as key T_T).</del> It computes in `html/render` if the `Deref::Target` item is copy before rendering it. So to resume: * `&self` is always kept. * `&mut self` is kept if `DerefMut` is implemented (nothing changed there). * `self` is kept only if `Deref::Target` is `Copy`. * Everything else disappears as they're not callable through `Deref`. r? @camelid
|
Note This PR was benchmarked as part of triage of its containing rollup: triage URL. Finished benchmarking commit (9d02933): comparison URL. Overall result: ❌ regressions - please read:Our benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)This perf run didn't have relevant results for this metric. CyclesResults (secondary 6.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Artifact size: 408.58 MiB -> 408.75 MiB (0.04%) |
View all comments
Fixes #160236.
This PR fixes a few things around how we handle
Deref:Deref, only methods should be keptDeref::Targetis a type implementingCopy, then methods takingselfwork and should be displayed.During thecleanpass, I store the itemDefIdon whichDerefis implemented in aDefIdSetif theDeref::TargetimplementsCopy. Then during rendering, we check if the "parent item" is in in theDefIdSet, and if so, we keep methods withself.Now you might wonder why the item and not the derefed item. It's because theDefIdwe get from the computed type doesn't match theDefIdof the actual type (that's where I spent most of my time, finding an ID (DefId/ItemId) I can use as key T_T).It computes in
html/renderif theDeref::Targetitem is copy before rendering it.So to resume:
&selfis always kept.&mut selfis kept ifDerefMutis implemented (nothing changed there).selfis kept only ifDeref::TargetisCopy.Deref.r? @camelid