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: correctly deal with self ty params when eliding default object lifetimes #115276

Merged

Conversation

fmease
Copy link
Member

@fmease fmease commented Aug 27, 2023

Fixes #115179.

@rustbot
Copy link
Collaborator

rustbot commented Aug 27, 2023

r? @GuillaumeGomez

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

@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 Aug 27, 2023
src/librustdoc/clean/mod.rs Outdated Show resolved Hide resolved
@fmease 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 fmease marked this pull request as draft August 27, 2023 17:12
@fmease
Copy link
Member Author

fmease commented Aug 27, 2023

Ah, wait, the root issue is something else 😅

@rustbot author

@rustbot 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
Copy link
Contributor

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member Author

fmease commented Aug 29, 2023

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 fmease force-pushed the rustdoc-obj-lt-defs-handle-self-ty-params branch from 2fbc6a2 to 4e71139 Compare August 30, 2023 12:49
@fmease
Copy link
Member Author

fmease commented Aug 30, 2023

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

@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 Aug 30, 2023
@bors
Copy link
Contributor

bors commented Aug 30, 2023

⌛ Trying commit 4e71139142fb41322249a2f01c639aff6dae8136 with merge f79a93c17c4c06926817c89541e82597491cd6cf...

@bors
Copy link
Contributor

bors commented Aug 30, 2023

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

@rust-timer

This comment has been minimized.

Copy link
Member Author

@fmease fmease left a comment

Choose a reason for hiding this comment

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

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

Comment on lines +1990 to +1993
let offset =
if !has_self && generics.parent.is_none() && generics.has_self { 1 } else { 0 };
let param = generics.params[index + offset].def_id;
Copy link
Member Author

@fmease fmease Aug 30, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Please add this explanation as a comment.

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),
Copy link
Member Author

@fmease fmease Aug 30, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

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;
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 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.

Copy link
Member

Choose a reason for hiding this comment

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

Again, please add this as a comment. ;)

@fmease fmease marked this pull request as ready for review August 30, 2023 15:03
@rustbot 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 rustbot added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 30, 2023
@rust-timer

This comment has been minimized.

@@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

Well, please do. :D

Copy link
Member Author

Choose a reason for hiding this comment

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

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

// 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
Copy link
Member

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
Copy link
Member

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Sep 1, 2023

📌 Commit f5a68f6 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 Sep 1, 2023
@bors
Copy link
Contributor

bors commented Sep 1, 2023

⌛ Testing commit f5a68f6 with merge 35e4163...

@bors
Copy link
Contributor

bors commented Sep 1, 2023

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 1, 2023
@bors bors merged commit 35e4163 into rust-lang:master Sep 1, 2023
11 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Sep 1, 2023
@rust-timer
Copy link
Collaborator

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%)

@fmease fmease deleted the rustdoc-obj-lt-defs-handle-self-ty-params branch September 2, 2023 17:59
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 9, 2023
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.74.0, 1.73.0 Sep 9, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 9, 2023
…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`
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.73.0, 1.72.1 Sep 12, 2023
@Mark-Simulacrum Mark-Simulacrum removed the stable-nominated Nominated for backporting to the compiler in the stable channel. label Sep 12, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 12, 2023
…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`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 12, 2023
…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`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 13, 2023
…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`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 13, 2023
…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`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 13, 2023
…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`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 13, 2023
…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`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 13, 2023
…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`
mmastrac added a commit to denoland/deno that referenced this pull request Sep 20, 2023
> 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. stable-accepted Accepted for backporting to the compiler in the stable channel. 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.

ICE when building documentation: DefId(20:797 ...) does not have a "object_lifetime_default"
9 participants