Skip to content

rustdoc: handle cross-crate RPITITs correctly #113956

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

Merged
merged 1 commit into from
Jul 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -2081,9 +2081,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(
Copy link
Member Author

@fmease fmease Jul 22, 2023

Choose a reason for hiding this comment

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

I've renamed this since the previous name confused me quite a bit at first, I wasn't sure if the ICE happened due to the RPITIT-method or due to the synthetic RPITIT assoc type. It can handle every kind of associated item just fine (even generic consts I assume) and rustdoc does use the query for every kind already.

Copy link
Member

@compiler-errors compiler-errors Jul 22, 2023

Choose a reason for hiding this comment

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

Yeah, it used to be used during codegen to skip monomorphizing methods that have impossible to solve predicates, but now it's used for other things.

edit: nvm I was confusing it with another "is impossible" method. Yeah, idk why it was named like that.

tcx: TyCtxt<'_>,
(impl_def_id, trait_item_def_id): (DefId, DefId),
) -> bool {
Copy link
Member Author

Choose a reason for hiding this comment

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

Ofc, we could just defensively check at the start if the assoc item is_impl_trait_in_trait and if so return false. Wouldn't be reachable at this point in time but it might be in the future if this query gained more users.

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;