rustdoc: correctly deal with self ty params when eliding default object lifetimes #115276

fmease · 2023-08-27T15:28:05Z

Fixes #115179.

rustbot · 2023-08-27T15:28:11Z

r? @GuillaumeGomez

(rustbot has picked a reviewer for you, use r? to override)

src/librustdoc/clean/mod.rs

fmease · 2023-08-27T17:13:28Z

Ah, wait, the root issue is something else 😅

@rustbot author

Enet4

Notwithstanding any eventual things missing here, I'd just like to point out that I have just built the documentation of both the reproducible project and the upstream dicom-parser crate via this revision, and they both built successfully. 👍

Thank you very much for working on this!

fmease · 2023-08-29T13:58:00Z

It seemingly fixes the problem but it's not the correct fix. The actual issue is an off-by-one error: we should be looking at the type parameter W of trait EncodeTo but right now we're looking at Self which is incorrect. The current version of my PR prevents the crash by handling Self properly (nothing wrong with that) but that masks the actual bug.

It's a bit awkward to fix the off-by-one error but seeing that the priority of the issue has been ranked critical recently, I'm going to prioritize this PR now.

fmease · 2023-08-30T12:57:34Z

Since I've increased the size of a widely used function argument of a multi-parameter function, this might have a perf impact

@bors try @rust-timer queue

bors · 2023-08-30T12:57:44Z

⌛ Trying commit 4e71139142fb41322249a2f01c639aff6dae8136 with merge f79a93c17c4c06926817c89541e82597491cd6cf...

bors · 2023-08-30T14:08:57Z

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

fmease

I'd like to wait for the perf result before merging this PR.
This is ready for review, you can mostly ignore the FIXMEs (I'm gonna remove them later).
@rustbot ready

fmease · 2023-08-30T14:53:45Z

src/librustdoc/clean/mod.rs

+                let offset =
+                    if !has_self && generics.parent.is_none() && generics.has_self { 1 } else { 0 };
+                let param = generics.params[index + offset].def_id;


This here is the most important part of the PR. It fixes the off-by-one error that leads to the ICE, see also #115276 (comment). Basically, if the container type is a trait object type (e.g. in dyn Container<dyn Content>), the DefId refers to the trait Container whose generics contain the type parameter Self but the args never contain a substitution for the self type since it's dynamic for trait object types. In such case, we offset by one.

If the DefKind of the container is Trait, we don't necessarily have a trait object type, we might also have a trait bound like impl Container<dyn Content> whose args do contain the concrete self type.

Please add this explanation as a comment.

fmease · 2023-08-30T14:56:46Z

src/librustdoc/clean/mod.rs

    PathSegment {
        name: item.name,
        args: GenericArgs::AngleBracketed {
            args: ty_args_to_args(
                cx,
                ty.map_bound(|ty| &ty.args[generics.parent_count..]),
                false,
-                None,
+                // FIXME: Revert to `None`, CC #115379
+                Some(def_id),


This is not relevant for fixing the ICE. I'm now just properly passing along the associated type as a container type. However, I've found out (#115379) that rustc itself doesn't handle associated type containers correctly (unless I'm mistaken), so theoretically, I can go back to None without any repercussion.

Better to keep the correct value, so Some in this case.

fmease · 2023-08-30T14:59:33Z

src/librustdoc/clean/mod.rs

                let default = tcx.object_lifetime_default(param);

                match default {
                    rbv::ObjectLifetimeDefault::Param(lifetime) => {
                        let index = generics.param_def_id_to_index[&lifetime];
-                        let arg = args.skip_binder()[index as usize].expect_region();
+                        let index = index as usize - generics.parent_count;


This is only relevant for associated type containers. The indices in param_def_id_to_index are relative to the parent generics, thus we need to translate the index.

Again, please add this as a comment. ;)

fmease · 2023-08-30T15:04:58Z

https://rust-lang.zulipchat.com/#narrow/stream/245100-t-compiler.2Fwg-prioritization.2Falerts/topic/.23115179.20ICE.20when.20building.20documentation.3A.20DefId.2820.3A797.20.2E.2E.2E.29.20.E2.80.A6/near/387874152

@rustbot label beta-nominated

GuillaumeGomez · 2023-08-30T18:34:29Z

src/librustdoc/clean/mod.rs

@@ -469,6 +469,7 @@ fn clean_projection<'tcx>(

    let trait_ =
        clean_trait_ref_with_bindings(cx, ty.map_bound(|ty| ty.trait_ref(cx.tcx)), ThinVec::new());
+    // FIXME: Maybe add comment here why `None` is fine.


Well, please do. :D

For posterity's sake, if/once we respect object lifetime defaults of GATs, None would be fine here since Self doesn't have a meaningful object lifetime default according to

rust/compiler/rustc_hir_analysis/src/collect/resolve_bound_vars.rs

Lines 1563 to 1564 in b30b535

// We may also get a `Trait` or `TraitAlias` because of how generics `Self` parameter

// works. Ignore it because it can't have a meaningful lifetime default.

If #115379 (the 1000th time I'm mentioning it lol) is indeed a bug, then I will come back to this an add a comment :)

GuillaumeGomez · 2023-08-30T18:37:13Z

Code looks good to me. We'll see what perf checks say. Also please add the nice review comments you wrote into the code, will be useful for everyone. :)

GuillaumeGomez · 2023-09-01T21:02:14Z

Thanks!

@bors r+

bors · 2023-09-01T21:02:16Z

📌 Commit f5a68f6 has been approved by GuillaumeGomez

It is now in the queue for this repository.

bors · 2023-09-01T22:11:26Z

⌛ Testing commit f5a68f6 with merge 35e4163...

bors · 2023-09-01T23:58:30Z

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

rust-timer · 2023-09-02T17:05:49Z

Finished benchmarking commit (35e4163): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -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)	-1.4%	[-2.1%, -0.8%]	4
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.4%	[-2.1%, -0.8%]	4

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: 630.43s -> 630.838s (0.06%)
Artifact size: 316.63 MiB -> 316.58 MiB (-0.02%)

…k-Simulacrum [beta] backport This PR backports: - rust-lang#115559: implied bounds: do not ICE on unconstrained region vars - rust-lang#115446: fix version for abi_thiscall to 1.73.0, which was forgotten to change when stabilized and (later) moved to beta - rust-lang#115276: rustdoc: correctly deal with self ty params when eliding default object lifetimes r? `@Mark-Simulacrum`

…Simulacrum [stable] 1.72.1 release This backports: * Remove assert that checks type equality rust-lang#115215 * implied bounds: do not ICE on unconstrained region vars rust-lang#115559 * rustdoc: correctly deal with self ty params when eliding default object lifetimes rust-lang#115276 * Stop emitting non-power-of-two vectors in (non-portable-SIMD) codegen rust-lang#115236 * Normalize before checking if local is freeze in deduced_param_attrs rust-lang#114948 Some cherry-picks required merge conflict resolution, we'll see if I got it right based on CI (rustdoc fix and LLVM codegen test). r? `@Mark-Simulacrum`

> 1.72.1 resolves a few regressions introduced in 1.72.0: > - [Partially revert codegen change, improving codegen](rust-lang/rust#115236) > - [rustdoc: Fix self ty params in objects with lifetimes](rust-lang/rust#115276) > - [Fix regression in compile times](rust-lang/rust#114948) > - Resolve some ICEs in the compiler: > - [#115215](rust-lang/rust#115215) > - [#115559](rust-lang/rust#115559)

rustbot assigned GuillaumeGomez Aug 27, 2023

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 Aug 27, 2023

fmease commented Aug 27, 2023

View reviewed changes

src/librustdoc/clean/mod.rs Outdated Show resolved Hide resolved

fmease force-pushed the rustdoc-obj-lt-defs-handle-self-ty-params branch 2 times, most recently from f56bd1e to 2fbc6a2 Compare August 27, 2023 15:40

fmease marked this pull request as draft August 27, 2023 17:12

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 27, 2023

Enet4 approved these changes Aug 29, 2023

View reviewed changes

fmease force-pushed the rustdoc-obj-lt-defs-handle-self-ty-params branch from 2fbc6a2 to 4e71139 Compare August 30, 2023 12:49

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 Aug 30, 2023

This comment has been minimized.

Sign in to view

fmease commented Aug 30, 2023

View reviewed changes

fmease marked this pull request as ready for review August 30, 2023 15:03

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 30, 2023

rustbot added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 30, 2023

This comment has been minimized.

Sign in to view

GuillaumeGomez reviewed Aug 30, 2023

View reviewed changes

GuillaumeGomez approved these changes Sep 1, 2023

View reviewed changes

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 Sep 1, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 1, 2023

bors merged commit 35e4163 into rust-lang:master Sep 1, 2023
11 checks passed

rustbot added this to the 1.74.0 milestone Sep 1, 2023

fmease deleted the rustdoc-obj-lt-defs-handle-self-ty-params branch September 2, 2023 17:59

Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 9, 2023

Mark-Simulacrum modified the milestones: 1.74.0, 1.73.0 Sep 9, 2023

Mark-Simulacrum mentioned this pull request Sep 9, 2023

[beta] backport #115722

Merged

Mark-Simulacrum modified the milestones: 1.73.0, 1.72.1 Sep 12, 2023

Mark-Simulacrum removed the stable-nominated Nominated for backporting to the compiler in the stable channel. label Sep 12, 2023

Mark-Simulacrum mentioned this pull request Sep 12, 2023

[stable] 1.72.1 release #115787

Merged

mmastrac mentioned this pull request Sep 19, 2023

chore: bump Rust to 1.72.1 and cargo update denoland/deno#20572

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc: correctly deal with self ty params when eliding default object lifetimes #115276

rustdoc: correctly deal with self ty params when eliding default object lifetimes #115276

fmease commented Aug 27, 2023

rustbot commented Aug 27, 2023

fmease commented Aug 27, 2023 •

edited

Loading

Enet4 left a comment •

edited

Loading

fmease commented Aug 29, 2023 •

edited

Loading

fmease commented Aug 30, 2023 •

edited

Loading

This comment has been minimized.

bors commented Aug 30, 2023

bors commented Aug 30, 2023

This comment has been minimized.

fmease left a comment

fmease Aug 30, 2023 •

edited

Loading

GuillaumeGomez Aug 30, 2023

fmease Aug 30, 2023 •

edited

Loading

GuillaumeGomez Aug 30, 2023

fmease Aug 30, 2023

GuillaumeGomez Sep 1, 2023

fmease commented Aug 30, 2023

This comment has been minimized.

GuillaumeGomez Aug 30, 2023

fmease Aug 31, 2023

GuillaumeGomez commented Aug 30, 2023

GuillaumeGomez commented Sep 1, 2023

bors commented Sep 1, 2023

bors commented Sep 1, 2023

bors commented Sep 1, 2023

rust-timer commented Sep 2, 2023

	// We may also get a `Trait` or `TraitAlias` because of how generics `Self` parameter
	// works. Ignore it because it can't have a meaningful lifetime default.

rustdoc: correctly deal with self ty params when eliding default object lifetimes #115276

rustdoc: correctly deal with self ty params when eliding default object lifetimes #115276

Conversation

fmease commented Aug 27, 2023

rustbot commented Aug 27, 2023

fmease commented Aug 27, 2023 • edited Loading

Enet4 left a comment • edited Loading

Choose a reason for hiding this comment

fmease commented Aug 29, 2023 • edited Loading

fmease commented Aug 30, 2023 • edited Loading

This comment has been minimized.

bors commented Aug 30, 2023

bors commented Aug 30, 2023

This comment has been minimized.

fmease left a comment

Choose a reason for hiding this comment

fmease Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmease Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmease commented Aug 30, 2023

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Aug 30, 2023

GuillaumeGomez commented Sep 1, 2023

bors commented Sep 1, 2023

bors commented Sep 1, 2023

bors commented Sep 1, 2023

rust-timer commented Sep 2, 2023

Overall result: no relevant changes - no action needed

fmease commented Aug 27, 2023 •

edited

Loading

Enet4 left a comment •

edited

Loading

fmease commented Aug 29, 2023 •

edited

Loading

fmease commented Aug 30, 2023 •

edited

Loading

fmease Aug 30, 2023 •

edited

Loading

fmease Aug 30, 2023 •

edited

Loading