Skip to content

Rustdoc ty consistency fixes #93385

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.

Sign up for GitHub

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 5 commits into from
Mar 1, 2022
Merged

Rustdoc ty consistency fixes #93385

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2f7b7d5
Don't use is_local to determine function cleaning method call intent
CraftSpider Jan 27, 2022
1c82e5e
Convert empty tuple to DefaultReturn in ty path
CraftSpider Jan 27, 2022
eae2026
Set visibility to inherited in trait impls to match HIR
CraftSpider Jan 27, 2022
d90138b
Remove now-unnecessary blanket impl HIR check
CraftSpider Jan 27, 2022
2d2163b
Same code for ty and hir impl items
CraftSpider Jan 27, 2022
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
27 changes: 5 additions & 22 deletions src/librustdoc/clean/blanket_impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,27 +101,6 @@ impl<'a, 'tcx> BlanketImplFinder<'a, 'tcx> {

cx.generated_synthetics.insert((ty, trait_def_id));

let hir_imp = impl_def_id.as_local()
.map(|local| cx.tcx.hir().expect_item(local))
.and_then(|item| if let hir::ItemKind::Impl(i) = &item.kind {
Some(i)
} else {
None
});

let items = match hir_imp {
Some(imp) => imp
.items
.iter()
.map(|ii| cx.tcx.hir().impl_item(ii.id).clean(cx))
.collect::<Vec<_>>(),
None => cx.tcx
.associated_items(impl_def_id)
.in_definition_order()
.map(|x| x.clean(cx))
.collect::<Vec<_>>(),
};

impls.push(Item {
name: None,
attrs: Default::default(),
Expand All @@ -138,7 +117,11 @@ impl<'a, 'tcx> BlanketImplFinder<'a, 'tcx> {
// the post-inference `trait_ref`, as it's more accurate.
trait_: Some(trait_ref.clean(cx)),
for_: ty.clean(cx),
items,
items: cx.tcx
.associated_items(impl_def_id)
.in_definition_order()
.map(|x| x.clean(cx))
.collect::<Vec<_>>(),
polarity: ty::ImplPolarity::Positive,
kind: ImplKind::Blanket(box trait_ref.self_ty().clean(cx)),
}),
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/clean/inline.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ fn build_external_function(cx: &mut DocContext<'_>, did: DefId) -> clean::Functi
let (generics, decl) = clean::enter_impl_trait(cx, |cx| {
// NOTE: generics need to be cleaned before the decl!
let generics = clean_ty_generics(cx, cx.tcx.generics_of(did), predicates);
let decl = clean_fn_decl_from_did_and_sig(cx, did, sig);
let decl = clean_fn_decl_from_did_and_sig(cx, Some(did), sig);
(generics, decl)
});
clean::Function {
Expand Down
53 changes: 34 additions & 19 deletions src/librustdoc/clean/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -890,13 +890,20 @@ fn clean_fn_decl_with_args(

fn clean_fn_decl_from_did_and_sig(
cx: &mut DocContext<'_>,
did: DefId,
did: Option<DefId>,
sig: ty::PolyFnSig<'_>,
) -> FnDecl {
let mut names = if did.is_local() { &[] } else { cx.tcx.fn_arg_names(did) }.iter();
let mut names = did.map_or(&[] as &[_], |did| cx.tcx.fn_arg_names(did)).iter();
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, could this be a behavior change when this is called with an external function? I think if it is, it'd be a bugfix, but I wanted to check. In the inline code path (inline.rs), this is called with Some(did), where did should be non-local.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should only change code on local functions, external functions always took the else case and are thus equivalent to Some. Local function behavior should be affected as an intentional fix related to this PR.

Copy link
Member

Choose a reason for hiding this comment

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

You're right, I got confused. Thanks!


// We assume all empty tuples are default return type. This theoretically can discard `-> ()`,
// but shouldn't change any code meaning.
let output = match sig.skip_binder().output().clean(cx) {
Type::Tuple(inner) if inner.len() == 0 => DefaultReturn,
ty => Return(ty),
};

FnDecl {
output: Return(sig.skip_binder().output().clean(cx)),
output,
c_variadic: sig.skip_binder().c_variadic,
inputs: Arguments {
values: sig
Expand Down Expand Up @@ -1029,20 +1036,18 @@ impl Clean<Item> for hir::ImplItem<'_> {
}
};

let what_rustc_thinks =
let mut what_rustc_thinks =
Item::from_def_id_and_parts(local_did, Some(self.ident.name), inner, cx);
let parent_item = cx.tcx.hir().expect_item(cx.tcx.hir().get_parent_item(self.hir_id()));
if let hir::ItemKind::Impl(impl_) = &parent_item.kind {
if impl_.of_trait.is_some() {
// Trait impl items always inherit the impl's visibility --
// we don't want to show `pub`.
Item { visibility: Inherited, ..what_rustc_thinks }
} else {
what_rustc_thinks
}
} else {
panic!("found impl item with non-impl parent {:?}", parent_item);

let impl_ref = cx.tcx.parent(local_did).and_then(|did| cx.tcx.impl_trait_ref(did));

// Trait impl items always inherit the impl's visibility --
// we don't want to show `pub`.
if impl_ref.is_some() {
what_rustc_thinks.visibility = Inherited;
}

what_rustc_thinks
})
}
}
Expand All @@ -1067,7 +1072,7 @@ impl Clean<Item> for ty::AssocItem {
tcx.explicit_predicates_of(self.def_id),
);
let sig = tcx.fn_sig(self.def_id);
let mut decl = clean_fn_decl_from_did_and_sig(cx, self.def_id, sig);
let mut decl = clean_fn_decl_from_did_and_sig(cx, Some(self.def_id), sig);

if self.fn_has_self_parameter {
let self_ty = match self.container {
Expand Down Expand Up @@ -1197,7 +1202,18 @@ impl Clean<Item> for ty::AssocItem {
}
};

Item::from_def_id_and_parts(self.def_id, Some(self.name), kind, cx)
let mut what_rustc_thinks =
Item::from_def_id_and_parts(self.def_id, Some(self.name), kind, cx);

let impl_ref = tcx.parent(self.def_id).and_then(|did| tcx.impl_trait_ref(did));

// Trait impl items always inherit the impl's visibility --
// we don't want to show `pub`.
if impl_ref.is_some() {
what_rustc_thinks.visibility = Visibility::Inherited;
}

what_rustc_thinks
}
}

Expand Down Expand Up @@ -1466,8 +1482,7 @@ impl<'tcx> Clean<Type> for Ty<'tcx> {
ty::FnDef(..) | ty::FnPtr(_) => {
let ty = cx.tcx.lift(*self).expect("FnPtr lift failed");
let sig = ty.fn_sig(cx.tcx);
let def_id = DefId::local(CRATE_DEF_INDEX);
let decl = clean_fn_decl_from_did_and_sig(cx, def_id, sig);
let decl = clean_fn_decl_from_did_and_sig(cx, None, sig);
BareFunction(box BareFunctionDecl {
unsafety: sig.unsafety(),
generic_params: Vec::new(),
Expand Down