Skip to content

Commit

Permalink
Auto merge of rust-lang#113956 - fmease:rustdoc-fix-x-crate-rpitits, …
Browse files Browse the repository at this point in the history
…r=GuillaumeGomez,compiler-errors

rustdoc: handle cross-crate RPITITs correctly

Filter out the internal associated types synthesized during the desugaring of RPITITs, they really shouldn't show up in the docs.

This also fixes rust-lang#113929 since we're no longer invoking `is_impossible_associated_item` (renamed from `is_impossible_method`) which cannot handle them (leading to an ICE). I don't think it makes sense to try to make `is_impossible_associated_item` handle this exotic kind of associated type (CC original author `@compiler-errors).`

@ T-rustdoc reviewers, currently I'm throwing out ITIT assoc tys before cleaning assoc tys at each usage-site. I'm thinking about making `clean_middle_assoc_item` return an `Option<_>` instead and doing the check inside of it to prevent any call sites from forgetting the check for ITITs. Since I wasn't sure if you would like that approach, I didn't go through with it. Let me know what you think.

<details><summary>Explanation on why <code>is_impossible_associated_item(itit_assoc_ty)</code> leads to an ICE</summary>

Given the following code:

```rs
pub trait Trait { fn def<T>() -> impl Default {} }
impl Trait for () {}
```

The generated associated type looks something like (simplified):

```rs
type {opaque#0}<T>: Default = impl Default; // the name is actually `kw::Empty` but this is the `def_path_str` repr
```

The query `is_impossible_associated_item` goes through all predicates of the associated item – in this case `<T as Sized>` – to check if they contain any generic parameters from the (generic) associated type itself. For predicates that don't contain any *own* generics, it does further processing, part of which is instantiating the predicate with the generic arguments of the impl block (which is only correct if they truly don't contain any own generics since they wouldn't get instantiated this way leading to an ICE).

It checks if `parent_def_id(T) == assoc_ty_def_id` to get to know if `T` is owned by the assoc ty. Unfortunately this doesn't work for ITIT assoc tys. In this case, the parent of `T` is `Trait::def` (!) which is the associated function (I'm pretty sure this is very intentional) which is of course not equal to the assoc ty `Trait::{opaque#0}`.

</details>

`@rustbot` label A-cross-crate-reexports
  • Loading branch information
bors committed Jul 24, 2023
2 parents 48c0c25 + 5924043 commit cb6ab95
Show file tree
Hide file tree
Showing 7 changed files with 84 additions and 8 deletions.
4 changes: 2 additions & 2 deletions compiler/rustc_middle/src/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2064,9 +2064,9 @@ rustc_queries! {
}
}

query is_impossible_method(key: (DefId, DefId)) -> bool {
query is_impossible_associated_item(key: (DefId, DefId)) -> bool {
desc { |tcx|
"checking if `{}` is impossible to call within `{}`",
"checking if `{}` is impossible to reference within `{}`",
tcx.def_path_str(key.1),
tcx.def_path_str(key.0),
}
Expand Down
9 changes: 6 additions & 3 deletions compiler/rustc_trait_selection/src/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -474,11 +474,14 @@ fn subst_and_check_impossible_predicates<'tcx>(
result
}

/// Checks whether a trait's method is impossible to call on a given impl.
/// Checks whether a trait's associated item is impossible to reference on a given impl.
///
/// This only considers predicates that reference the impl's generics, and not
/// those that reference the method's generics.
fn is_impossible_method(tcx: TyCtxt<'_>, (impl_def_id, trait_item_def_id): (DefId, DefId)) -> bool {
fn is_impossible_associated_item(
tcx: TyCtxt<'_>,
(impl_def_id, trait_item_def_id): (DefId, DefId),
) -> bool {
struct ReferencesOnlyParentGenerics<'tcx> {
tcx: TyCtxt<'tcx>,
generics: &'tcx ty::Generics,
Expand Down Expand Up @@ -556,7 +559,7 @@ pub fn provide(providers: &mut Providers) {
specializes: specialize::specializes,
subst_and_check_impossible_predicates,
check_tys_might_be_eq: misc::check_tys_might_be_eq,
is_impossible_method,
is_impossible_associated_item,
..*providers
};
}
3 changes: 2 additions & 1 deletion src/librustdoc/clean/blanket_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,8 @@ impl<'a, 'tcx> BlanketImplFinder<'a, 'tcx> {
.tcx
.associated_items(impl_def_id)
.in_definition_order()
.map(|x| clean_middle_assoc_item(x, cx))
.filter(|item| !item.is_impl_trait_in_trait())
.map(|item| clean_middle_assoc_item(item, cx))
.collect::<Vec<_>>(),
polarity: ty::ImplPolarity::Positive,
kind: ImplKind::Blanket(Box::new(clean_middle_ty(
Expand Down
2 changes: 2 additions & 0 deletions src/librustdoc/clean/inline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,7 @@ pub(crate) fn build_external_trait(cx: &mut DocContext<'_>, did: DefId) -> clean
.tcx
.associated_items(did)
.in_definition_order()
.filter(|item| !item.is_impl_trait_in_trait())
.map(|item| clean_middle_assoc_item(item, cx))
.collect();

Expand Down Expand Up @@ -459,6 +460,7 @@ pub(crate) fn build_impl(
None => (
tcx.associated_items(did)
.in_definition_order()
.filter(|item| !item.is_impl_trait_in_trait())
.filter(|item| {
// If this is a trait impl, filter out associated items whose corresponding item
// in the associated trait is marked `doc(hidden)`.
Expand Down
4 changes: 2 additions & 2 deletions src/librustdoc/html/render/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1678,11 +1678,11 @@ fn render_impl(
rendering_params: ImplRenderingParameters,
) {
for trait_item in &t.items {
// Skip over any default trait items that are impossible to call
// Skip over any default trait items that are impossible to reference
// (e.g. if it has a `Self: Sized` bound on an unsized type).
if let Some(impl_def_id) = parent.item_id.as_def_id()
&& let Some(trait_item_def_id) = trait_item.item_id.as_def_id()
&& cx.tcx().is_impossible_method((impl_def_id, trait_item_def_id))
&& cx.tcx().is_impossible_associated_item((impl_def_id, trait_item_def_id))
{
continue;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
#![feature(return_position_impl_trait_in_trait)]

pub trait Trait {
fn create() -> impl Iterator<Item = u64> {
std::iter::empty()
}
}

pub struct Basic;
pub struct Intermediate;
pub struct Advanced;

impl Trait for Basic {
// method provided by the trait
}

impl Trait for Intermediate {
fn create() -> std::ops::Range<u64> { // concrete return type
0..1
}
}

impl Trait for Advanced {
fn create() -> impl Iterator<Item = u64> { // opaque return type
std::iter::repeat(0)
}
}

// Regression test for issue #113929:

pub trait Def {
fn def<T>() -> impl Default {}
}

impl Def for () {}
35 changes: 35 additions & 0 deletions tests/rustdoc/inline_cross/ret-pos-impl-trait-in-trait.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
#![crate_name = "user"]
// aux-crate:rpitit=ret-pos-impl-trait-in-trait.rs
// edition:2021

// Test that we can correctly render cross-crate RPITITs.
// In particular, check that we don't render the internal associated type generated by
// their desugaring. We count the number of associated items and ensure that it is exactly one.
// This is more robust than checking for the absence of the associated type.

// @has user/trait.Trait.html
// @has - '//*[@id="method.create"]' 'fn create() -> impl Iterator<Item = u64>'
// The class "method" is used for all three kinds of associated items at the time of writing.
// @count - '//*[@id="main-content"]//section[@class="method"]' 1
pub use rpitit::Trait;

// @has user/struct.Basic.html
// @has - '//*[@id="method.create"]' 'fn create() -> impl Iterator<Item = u64>'
// @count - '//*[@id="trait-implementations-list"]//*[@class="impl-items"]' 1
pub use rpitit::Basic;

// @has user/struct.Intermediate.html
// @has - '//*[@id="method.create"]' 'fn create() -> Range<u64>'
// @count - '//*[@id="trait-implementations-list"]//*[@class="impl-items"]' 1
pub use rpitit::Intermediate;

// @has user/struct.Advanced.html
// @has - '//*[@id="method.create"]' 'fn create() -> impl Iterator<Item = u64>'
// @count - '//*[@id="trait-implementations-list"]//*[@class="impl-items"]' 1
pub use rpitit::Advanced;

// Regression test for issue #113929:

// @has user/trait.Def.html
// @has - '//*[@id="method.def"]' 'fn def<T>() -> impl Default'
pub use rpitit::Def;

0 comments on commit cb6ab95

Please sign in to comment.