Skip to content

Remove unnecessary self_ty parameter to get_blanket_impls #81349

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 1 commit into from
Feb 28, 2021
Merged

Remove unnecessary self_ty parameter to get_blanket_impls #81349

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
53 changes: 23 additions & 30 deletions src/librustdoc/clean/auto_trait.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ impl<'a, 'tcx> AutoTraitFinder<'a, 'tcx> {
ty: Ty<'tcx>,
trait_def_id: DefId,
param_env: ty::ParamEnv<'tcx>,
Copy link
Member

Choose a reason for hiding this comment

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

Why does this still take a param_env given that the other places call tcx.param_env on the item_def_id?

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 was added after I opened the original PR, I'll fix it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, actually this is called in a loop, I don't know if it makes sense to change. It will have to lookup the type/param_env each time, even though it's memoized that still has overhead.

param_env_def_id: DefId,
item_def_id: DefId,
f: &auto_trait::AutoTraitFinder<'tcx>,
// If this is set, show only negative trait implementations, not positive ones.
discard_positive_impl: bool,
Expand All @@ -50,7 +50,7 @@ impl<'a, 'tcx> AutoTraitFinder<'a, 'tcx> {
let region_data = info.region_data;

let names_map = tcx
.generics_of(param_env_def_id)
.generics_of(item_def_id)
.params
.iter()
.filter_map(|param| match param.kind {
Expand All @@ -62,16 +62,16 @@ impl<'a, 'tcx> AutoTraitFinder<'a, 'tcx> {
let lifetime_predicates = Self::handle_lifetimes(&region_data, &names_map);
let new_generics = self.param_env_to_generics(
infcx.tcx,
param_env_def_id,
item_def_id,
info.full_user_env,
lifetime_predicates,
info.vid_to_region,
);

debug!(
"find_auto_trait_generics(param_env_def_id={:?}, trait_def_id={:?}): \
"find_auto_trait_generics(item_def_id={:?}, trait_def_id={:?}): \
finished with {:?}",
param_env_def_id, trait_def_id, new_generics
item_def_id, trait_def_id, new_generics
);

new_generics
Expand Down Expand Up @@ -101,7 +101,7 @@ impl<'a, 'tcx> AutoTraitFinder<'a, 'tcx> {
// Instead, we generate `impl !Send for Foo<T>`, which better
// expresses the fact that `Foo<T>` never implements `Send`,
// regardless of the choice of `T`.
let params = (tcx.generics_of(param_env_def_id), ty::GenericPredicates::default())
let params = (tcx.generics_of(item_def_id), ty::GenericPredicates::default())
.clean(self.cx)
.params;

Expand All @@ -115,7 +115,7 @@ impl<'a, 'tcx> AutoTraitFinder<'a, 'tcx> {
name: None,
attrs: Default::default(),
visibility: Inherited,
def_id: self.cx.next_def_id(param_env_def_id.krate),
def_id: self.cx.next_def_id(item_def_id.krate),
kind: box ImplItem(Impl {
unsafety: hir::Unsafety::Normal,
generics: new_generics,
Expand All @@ -130,26 +130,25 @@ impl<'a, 'tcx> AutoTraitFinder<'a, 'tcx> {
})
}

// FIXME(eddyb) figure out a better way to pass information about
// parametrization of `ty` than `param_env_def_id`.
crate fn get_auto_trait_impls(&mut self, ty: Ty<'tcx>, param_env_def_id: DefId) -> Vec<Item> {
crate fn get_auto_trait_impls(&mut self, item_def_id: DefId) -> Vec<Item> {
let tcx = self.cx.tcx;
let param_env = tcx.param_env(param_env_def_id);
let f = auto_trait::AutoTraitFinder::new(self.cx.tcx);
let param_env = tcx.param_env(item_def_id);
let ty = tcx.type_of(item_def_id);
let f = auto_trait::AutoTraitFinder::new(tcx);

debug!("get_auto_trait_impls({:?})", ty);
let auto_traits: Vec<_> = self.cx.auto_traits.iter().cloned().collect();
let mut auto_traits: Vec<Item> = auto_traits
.into_iter()
.filter_map(|trait_def_id| {
self.generate_for_trait(ty, trait_def_id, param_env, param_env_def_id, &f, false)
self.generate_for_trait(ty, trait_def_id, param_env, item_def_id, &f, false)
})
.collect();
// We are only interested in case the type *doesn't* implement the Sized trait.
if !ty.is_sized(self.cx.tcx.at(rustc_span::DUMMY_SP), param_env) {
if !ty.is_sized(tcx.at(rustc_span::DUMMY_SP), param_env) {
// In case `#![no_core]` is used, `sized_trait` returns nothing.
if let Some(item) = self.cx.tcx.lang_items().sized_trait().and_then(|sized_trait_did| {
self.generate_for_trait(ty, sized_trait_did, param_env, param_env_def_id, &f, true)
if let Some(item) = tcx.lang_items().sized_trait().and_then(|sized_trait_did| {
self.generate_for_trait(ty, sized_trait_did, param_env, item_def_id, &f, true)
}) {
auto_traits.push(item);
}
Expand Down Expand Up @@ -445,15 +444,15 @@ impl<'a, 'tcx> AutoTraitFinder<'a, 'tcx> {
fn param_env_to_generics(
&mut self,
tcx: TyCtxt<'tcx>,
param_env_def_id: DefId,
item_def_id: DefId,
Comment on lines -448 to +447
Copy link
Member

Choose a reason for hiding this comment

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

Why was this change made? Did the meaning change, or is item_def_id closer to the meaning than param_env_def_id? Do ParamEnvs even have DefIds?

Sorry for all the questions; I know almost nothing about ParamEnv :)

Copy link
Member Author

Choose a reason for hiding this comment

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

item_def_id is closer to the meaning. All items have a ParamEnv, but the same ParamEnv may be used for things other than items (e.g. each expression in the body of a function). Before, there was a semantic mismatch in the representation: it passed in a Ty, but not all types are items (for example &T). So to get the param env of the type it had to pass in the DefId too. Now it passes in the DefId only, which allows getting both the param env and the type without any additional info.

Copy link
Member

Choose a reason for hiding this comment

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

How will get_blanket_impls behave now for blanket impls like impl Foo for &Struct where the type is &Struct but the type_of the item_def_id is Struct? (Unless I'm misunderstanding what the item_def_id is referring to.)

Copy link
Member Author

Choose a reason for hiding this comment

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

The same as it did before, all this does is rename the parameter and move the type_of into the function. I don't know exactly how it worked before.

param_env: ty::ParamEnv<'tcx>,
Copy link
Member

@camelid camelid Feb 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

(Same here as #81349 (comment).)

mut existing_predicates: Vec<WherePredicate>,
vid_to_region: FxHashMap<ty::RegionVid, ty::Region<'tcx>>,
) -> Generics {
debug!(
"param_env_to_generics(param_env_def_id={:?}, param_env={:?}, \
"param_env_to_generics(item_def_id={:?}, param_env={:?}, \
existing_predicates={:?})",
param_env_def_id, param_env, existing_predicates
item_def_id, param_env, existing_predicates
);

// The `Sized` trait must be handled specially, since we only display it when
Expand All @@ -463,7 +462,7 @@ impl<'a, 'tcx> AutoTraitFinder<'a, 'tcx> {
let mut replacer = RegionReplacer { vid_to_region: &vid_to_region, tcx };

let orig_bounds: FxHashSet<_> =
self.cx.tcx.param_env(param_env_def_id).caller_bounds().iter().collect();
self.cx.tcx.param_env(item_def_id).caller_bounds().iter().collect();
let clean_where_predicates = param_env
.caller_bounds()
.iter()
Expand All @@ -477,14 +476,11 @@ impl<'a, 'tcx> AutoTraitFinder<'a, 'tcx> {
.map(|p| p.fold_with(&mut replacer));

let mut generic_params =
(tcx.generics_of(param_env_def_id), tcx.explicit_predicates_of(param_env_def_id))
(tcx.generics_of(item_def_id), tcx.explicit_predicates_of(item_def_id))
.clean(self.cx)
.params;

debug!(
"param_env_to_generics({:?}): generic_params={:?}",
param_env_def_id, generic_params
);
debug!("param_env_to_generics({:?}): generic_params={:?}", item_def_id, generic_params);

let mut has_sized = FxHashSet::default();
let mut ty_to_bounds: FxHashMap<_, FxHashSet<_>> = Default::default();
Expand Down Expand Up @@ -648,13 +644,10 @@ impl<'a, 'tcx> AutoTraitFinder<'a, 'tcx> {
.or_default()
.insert(*trait_.clone());
}
_ => panic!(
"Unexpected trait {:?} for {:?}",
trait_, param_env_def_id,
),
_ => panic!("Unexpected trait {:?} for {:?}", trait_, item_def_id),
}
}
_ => panic!("Unexpected LHS {:?} for {:?}", lhs, param_env_def_id),
_ => panic!("Unexpected LHS {:?} for {:?}", lhs, item_def_id),
}
}
};
Expand Down
9 changes: 4 additions & 5 deletions src/librustdoc/clean/blanket_impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,9 @@ crate struct BlanketImplFinder<'a, 'tcx> {
}

impl<'a, 'tcx> BlanketImplFinder<'a, 'tcx> {
// FIXME(eddyb) figure out a better way to pass information about
// parametrization of `ty` than `param_env_def_id`.
crate fn get_blanket_impls(&mut self, ty: Ty<'tcx>, param_env_def_id: DefId) -> Vec<Item> {
let param_env = self.cx.tcx.param_env(param_env_def_id);
crate fn get_blanket_impls(&mut self, item_def_id: DefId) -> Vec<Item> {
let param_env = self.cx.tcx.param_env(item_def_id);
let ty = self.cx.tcx.type_of(item_def_id);

debug!("get_blanket_impls({:?})", ty);
let mut impls = Vec::new();
Expand All @@ -39,7 +38,7 @@ impl<'a, 'tcx> BlanketImplFinder<'a, 'tcx> {
_ => return false,
}

let substs = infcx.fresh_substs_for_item(DUMMY_SP, param_env_def_id);
let substs = infcx.fresh_substs_for_item(DUMMY_SP, item_def_id);
let ty = ty.subst(infcx.tcx, substs);
let param_env = param_env.subst(infcx.tcx, substs);

Expand Down
9 changes: 4 additions & 5 deletions src/librustdoc/clean/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use rustc_hir::def::{DefKind, Res};
use rustc_hir::def_id::{DefId, LOCAL_CRATE};
use rustc_middle::mir::interpret::ConstValue;
use rustc_middle::ty::subst::{GenericArgKind, SubstsRef};
use rustc_middle::ty::{self, DefIdTree, Ty, TyCtxt};
use rustc_middle::ty::{self, DefIdTree, TyCtxt};
use rustc_span::symbol::{kw, sym, Symbol};
use std::mem;

Expand Down Expand Up @@ -426,19 +426,18 @@ crate fn resolve_type(cx: &mut DocContext<'_>, path: Path, id: hir::HirId) -> Ty

crate fn get_auto_trait_and_blanket_impls(
cx: &mut DocContext<'tcx>,
ty: Ty<'tcx>,
param_env_def_id: DefId,
item_def_id: DefId,
) -> impl Iterator<Item = Item> {
let auto_impls = cx
.sess()
.prof
.generic_activity("get_auto_trait_impls")
.run(|| AutoTraitFinder::new(cx).get_auto_trait_impls(ty, param_env_def_id));
.run(|| AutoTraitFinder::new(cx).get_auto_trait_impls(item_def_id));
let blanket_impls = cx
.sess()
.prof
.generic_activity("get_blanket_impls")
.run(|| BlanketImplFinder { cx }.get_blanket_impls(ty, param_env_def_id));
.run(|| BlanketImplFinder { cx }.get_blanket_impls(item_def_id));
auto_impls.into_iter().chain(blanket_impls)
}

Expand Down
10 changes: 2 additions & 8 deletions src/librustdoc/passes/collect_trait_impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,7 @@ crate fn collect_trait_impls(krate: Crate, cx: &mut DocContext<'_>) -> Crate {

// FIXME(eddyb) is this `doc(hidden)` check needed?
if !cx.tcx.get_attrs(def_id).lists(sym::doc).has_word(sym::hidden) {
let self_ty = cx.tcx.type_of(def_id);
let impls = get_auto_trait_and_blanket_impls(cx, self_ty, def_id);

let impls = get_auto_trait_and_blanket_impls(cx, def_id);
new_items.extend(impls.filter(|i| cx.renderinfo.inlined.insert(i.def_id)));
}
});
Expand Down Expand Up @@ -170,11 +168,7 @@ impl<'a, 'tcx> DocFolder for SyntheticImplCollector<'a, 'tcx> {
if i.is_struct() || i.is_enum() || i.is_union() {
// FIXME(eddyb) is this `doc(hidden)` check needed?
if !self.cx.tcx.get_attrs(i.def_id).lists(sym::doc).has_word(sym::hidden) {
self.impls.extend(get_auto_trait_and_blanket_impls(
self.cx,
self.cx.tcx.type_of(i.def_id),
i.def_id,
));
self.impls.extend(get_auto_trait_and_blanket_impls(self.cx, i.def_id));
}
}

Expand Down