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

Track the finalizing node in the specialization graph #70535

Merged
merged 7 commits into from
Apr 1, 2020
Merged
Show file tree
Hide file tree
Changes from 6 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
70 changes: 59 additions & 11 deletions src/librustc_middle/traits/specialization_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -154,14 +154,44 @@ impl Iterator for Ancestors<'_> {
}
}

pub struct NodeItem<T> {
pub node: Node,
pub item: T,
/// Information about the most specialized definition of an associated item.
pub struct LeafDef {
/// The associated item described by this `LeafDef`.
pub item: ty::AssocItem,

/// The node in the specialization graph containing the definition of `item`.
pub defining_node: Node,

/// The "top-most" (ie. least specialized) specialization graph node that finalized the
/// definition of `item`.
///
/// Example:
///
/// ```
/// trait Tr {
/// fn assoc(&self);
/// }
///
/// impl<T> Tr for T {
/// default fn assoc(&self) {}
/// }
///
/// impl Tr for u8 {}
/// ```
///
/// If we start the leaf definition search at `impl Tr for u8`, that impl will be the
/// `finalizing_node`, while `defining_node` will be the generic impl.
///
/// If the leaf definition search is started at the generic impl, `finalizing_node` will be
/// `None`, since the most specialized impl we found still allows overriding the method
/// (doesn't finalize it).
pub finalizing_node: Option<Node>,
}

impl<T> NodeItem<T> {
pub fn map<U, F: FnOnce(T) -> U>(self, f: F) -> NodeItem<U> {
NodeItem { node: self.node, item: f(self.item) }
impl LeafDef {
/// Returns whether this definition is known to not be further specializable.
pub fn is_final(&self) -> bool {
self.finalizing_node.is_some()
}
}

Expand All @@ -173,18 +203,36 @@ impl<'tcx> Ancestors<'tcx> {
tcx: TyCtxt<'tcx>,
trait_item_name: Ident,
trait_item_kind: ty::AssocKind,
) -> Option<NodeItem<ty::AssocItem>> {
) -> Option<LeafDef> {
let trait_def_id = self.trait_def_id;
let mut finalizing_node = None;

self.find_map(|node| {
node.item(tcx, trait_item_name, trait_item_kind, trait_def_id)
.map(|item| NodeItem { node, item })
if let Some(item) = node.item(tcx, trait_item_name, trait_item_kind, trait_def_id) {
if finalizing_node.is_none() {
let is_specializable = item.defaultness.is_default()
|| tcx.impl_defaultness(node.def_id()).is_default();

if !is_specializable {
finalizing_node = Some(node);
}
}

Some(LeafDef { item, defining_node: node, finalizing_node })
} else {
// Item not mentioned. This "finalizes" any defaulted item provided by an ancestor.
finalizing_node = Some(node);
None
}
})
}
}

/// Walk up the specialization ancestors of a given impl, starting with that
/// impl itself. Returns `None` if an error was reported while building the
/// specialization graph.
/// impl itself.
///
/// Returns `Err` if an error was reported while building the specialization
/// graph.
pub fn ancestors(
tcx: TyCtxt<'tcx>,
trait_def_id: DefId,
Expand Down
4 changes: 1 addition & 3 deletions src/librustc_trait_selection/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ pub use self::project::{
};
pub use self::select::{EvaluationCache, SelectionCache, SelectionContext};
pub use self::select::{EvaluationResult, IntercrateAmbiguityCause, OverflowError};
pub use self::specialize::find_associated_item;
pub use self::specialize::specialization_graph::FutureCompatOverlapError;
pub use self::specialize::specialization_graph::FutureCompatOverlapErrorKind;
pub use self::specialize::{specialization_graph, translate_substs, OverlapError};
Expand All @@ -64,8 +63,7 @@ pub use self::structural_match::NonStructuralMatchTy;
pub use self::util::{elaborate_predicates, elaborate_trait_ref, elaborate_trait_refs};
pub use self::util::{expand_trait_aliases, TraitAliasExpander};
pub use self::util::{
get_vtable_index_of_object_method, impl_is_default, impl_item_is_final,
predicate_for_trait_def, upcast_choices,
get_vtable_index_of_object_method, impl_item_is_final, predicate_for_trait_def, upcast_choices,
};
pub use self::util::{
supertrait_def_ids, supertraits, transitive_bounds, SupertraitDefIds, Supertraits,
Expand Down
48 changes: 11 additions & 37 deletions src/librustc_trait_selection/traits/project.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1015,49 +1015,21 @@ fn assemble_candidates_from_impls<'cx, 'tcx>(
assoc_ty_def(selcx, impl_data.impl_def_id, obligation.predicate.item_def_id)
.map_err(|ErrorReported| ())?;

let is_default = if node_item.node.is_from_trait() {
// If true, the impl inherited a `type Foo = Bar`
// given in the trait, which is implicitly default.
// Otherwise, the impl did not specify `type` and
// neither did the trait:
//
// ```rust
// trait Foo { type T; }
// impl Foo for Bar { }
// ```
//
// This is an error, but it will be
// reported in `check_impl_items_against_trait`.
// We accept it here but will flag it as
// an error when we confirm the candidate
// (which will ultimately lead to `normalize_to_error`
// being invoked).
false
if node_item.is_final() {
// Non-specializable items are always projectable.
true
} else {
// If we're looking at a trait *impl*, the item is
// specializable if the impl or the item are marked
// `default`.
node_item.item.defaultness.is_default()
|| super::util::impl_is_default(selcx.tcx(), node_item.node.def_id())
};

match is_default {
// Non-specializable items are always projectable
false => true,

// Only reveal a specializable default if we're past type-checking
// and the obligation is monomorphic, otherwise passes such as
// transmute checking and polymorphic MIR optimizations could
// get a result which isn't correct for all monomorphizations.
true if obligation.param_env.reveal == Reveal::All => {
if obligation.param_env.reveal == Reveal::All {
// NOTE(eddyb) inference variables can resolve to parameters, so
// assume `poly_trait_ref` isn't monomorphic, if it contains any.
let poly_trait_ref =
selcx.infcx().resolve_vars_if_possible(&poly_trait_ref);
!poly_trait_ref.needs_infer() && !poly_trait_ref.needs_subst()
}

true => {
} else {
debug!(
"assemble_candidates_from_impls: not eligible due to default: \
assoc_ty={} predicate={}",
Expand Down Expand Up @@ -1422,7 +1394,8 @@ fn confirm_impl_candidate<'cx, 'tcx>(
return Progress { ty: tcx.types.err, obligations: nested };
}
let substs = obligation.predicate.substs.rebase_onto(tcx, trait_def_id, substs);
let substs = translate_substs(selcx.infcx(), param_env, impl_def_id, substs, assoc_ty.node);
let substs =
translate_substs(selcx.infcx(), param_env, impl_def_id, substs, assoc_ty.defining_node);
let ty = if let ty::AssocKind::OpaqueTy = assoc_ty.item.kind {
let item_substs = InternalSubsts::identity_for_item(tcx, assoc_ty.item.def_id);
tcx.mk_opaque(assoc_ty.item.def_id, item_substs)
Expand All @@ -1447,7 +1420,7 @@ fn assoc_ty_def(
selcx: &SelectionContext<'_, '_>,
impl_def_id: DefId,
assoc_ty_def_id: DefId,
) -> Result<specialization_graph::NodeItem<ty::AssocItem>, ErrorReported> {
) -> Result<specialization_graph::LeafDef, ErrorReported> {
let tcx = selcx.tcx();
let assoc_ty_name = tcx.associated_item(assoc_ty_def_id).ident;
let trait_def_id = tcx.impl_trait_ref(impl_def_id).unwrap().def_id;
Expand All @@ -1464,9 +1437,10 @@ fn assoc_ty_def(
if matches!(item.kind, ty::AssocKind::Type | ty::AssocKind::OpaqueTy)
&& tcx.hygienic_eq(item.ident, assoc_ty_name, trait_def_id)
{
return Ok(specialization_graph::NodeItem {
node: specialization_graph::Node::Impl(impl_def_id),
return Ok(specialization_graph::LeafDef {
item: *item,
defining_node: impl_node,
finalizing_node: if item.defaultness.is_default() { None } else { Some(impl_node) },
nikomatsakis marked this conversation as resolved.
Show resolved Hide resolved
});
}
}
Expand Down
44 changes: 1 addition & 43 deletions src/librustc_trait_selection/traits/specialize/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use rustc_errors::struct_span_err;
use rustc_hir::def_id::DefId;
use rustc_middle::lint::LintDiagnosticBuilder;
use rustc_middle::ty::subst::{InternalSubsts, Subst, SubstsRef};
use rustc_middle::ty::{self, TyCtxt, TypeFoldable};
use rustc_middle::ty::{self, TyCtxt};
use rustc_session::lint::builtin::COHERENCE_LEAK_CHECK;
use rustc_session::lint::builtin::ORDER_DEPENDENT_TRAIT_OBJECTS;
use rustc_span::DUMMY_SP;
Expand Down Expand Up @@ -112,48 +112,6 @@ pub fn translate_substs<'a, 'tcx>(
source_substs.rebase_onto(infcx.tcx, source_impl, target_substs)
}

/// Given a selected impl described by `impl_data`, returns the
/// definition and substitutions for the method with the name `name`
/// the kind `kind`, and trait method substitutions `substs`, in
/// that impl, a less specialized impl, or the trait default,
/// whichever applies.
pub fn find_associated_item<'tcx>(
tcx: TyCtxt<'tcx>,
param_env: ty::ParamEnv<'tcx>,
item: &ty::AssocItem,
substs: SubstsRef<'tcx>,
impl_data: &super::VtableImplData<'tcx, ()>,
) -> (DefId, SubstsRef<'tcx>) {
debug!("find_associated_item({:?}, {:?}, {:?}, {:?})", param_env, item, substs, impl_data);
assert!(!substs.needs_infer());

let trait_def_id = tcx.trait_id_of_impl(impl_data.impl_def_id).unwrap();
let trait_def = tcx.trait_def(trait_def_id);

if let Ok(ancestors) = trait_def.ancestors(tcx, impl_data.impl_def_id) {
match ancestors.leaf_def(tcx, item.ident, item.kind) {
Some(node_item) => {
let substs = tcx.infer_ctxt().enter(|infcx| {
let param_env = param_env.with_reveal_all();
let substs = substs.rebase_onto(tcx, trait_def_id, impl_data.substs);
let substs = translate_substs(
&infcx,
param_env,
impl_data.impl_def_id,
substs,
node_item.node,
);
infcx.tcx.erase_regions(&substs)
});
(node_item.item.def_id, substs)
}
None => bug!("{:?} not found in {:?}", item, impl_data.impl_def_id),
}
} else {
(item.def_id, substs)
}
}

/// Is `impl1` a specialization of `impl2`?
///
/// Specialization is determined by the sets of types to which the impls apply;
Expand Down
17 changes: 1 addition & 16 deletions src/librustc_trait_selection/traits/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ use smallvec::smallvec;
use smallvec::SmallVec;

use rustc_data_structures::fx::FxHashSet;
use rustc_hir as hir;
use rustc_hir::def_id::DefId;
use rustc_middle::ty::outlives::Component;
use rustc_middle::ty::subst::{GenericArg, Subst, SubstsRef};
Expand Down Expand Up @@ -651,22 +650,8 @@ pub fn generator_trait_ref_and_outputs(
ty::Binder::bind((trait_ref, sig.skip_binder().yield_ty, sig.skip_binder().return_ty))
}

pub fn impl_is_default(tcx: TyCtxt<'_>, node_item_def_id: DefId) -> bool {
match tcx.hir().as_local_hir_id(node_item_def_id) {
Some(hir_id) => {
let item = tcx.hir().expect_item(hir_id);
if let hir::ItemKind::Impl { defaultness, .. } = item.kind {
defaultness.is_default()
} else {
false
}
}
None => tcx.impl_defaultness(node_item_def_id).is_default(),
}
}

pub fn impl_item_is_final(tcx: TyCtxt<'_>, assoc_item: &ty::AssocItem) -> bool {
assoc_item.defaultness.is_final() && !impl_is_default(tcx, assoc_item.container.id())
assoc_item.defaultness.is_final() && tcx.impl_defaultness(assoc_item.container.id()).is_final()
}

pub enum TupleArgumentsFlag {
Expand Down
47 changes: 39 additions & 8 deletions src/librustc_ty/instance.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
use rustc_hir::def_id::DefId;
use rustc_middle::ty::subst::SubstsRef;
use rustc_middle::ty::{self, Instance, TyCtxt, TypeFoldable};
use rustc_infer::infer::TyCtxtInferExt;
use rustc_span::sym;
use rustc_target::spec::abi::Abi;
use rustc_trait_selection::traits;
use traits::{Reveal, translate_substs};

use log::debug;

Expand Down Expand Up @@ -82,21 +84,50 @@ fn resolve_associated_item<'tcx>(
// the actual function:
match vtbl {
traits::VtableImpl(impl_data) => {
let (def_id, substs) =
traits::find_associated_item(tcx, param_env, trait_item, rcvr_substs, &impl_data);

let resolved_item = tcx.associated_item(def_id);
debug!(
"resolving VtableImpl: {:?}, {:?}, {:?}, {:?}",
param_env, trait_item, rcvr_substs, impl_data
);
assert!(!rcvr_substs.needs_infer());
assert!(!trait_ref.needs_infer());

let trait_def_id = tcx.trait_id_of_impl(impl_data.impl_def_id).unwrap();
let trait_def = tcx.trait_def(trait_def_id);
let leaf_def = trait_def
.ancestors(tcx, impl_data.impl_def_id)
.ok()?
.leaf_def(tcx, trait_item.ident, trait_item.kind)
.unwrap_or_else(|| {
bug!("{:?} not found in {:?}", trait_item, impl_data.impl_def_id);
});
let def_id = leaf_def.item.def_id;

let substs = tcx.infer_ctxt().enter(|infcx| {
let param_env = param_env.with_reveal_all();
let substs = rcvr_substs.rebase_onto(tcx, trait_def_id, impl_data.substs);
let substs = translate_substs(
&infcx,
param_env,
impl_data.impl_def_id,
substs,
leaf_def.defining_node,
);
infcx.tcx.erase_regions(&substs)
});
Comment on lines +105 to +116
Copy link
Member

Choose a reason for hiding this comment

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

A bit concerning that an inference context is needed for this. Shouldn't building the specialization graph also compute the necessary information to do this "more directly"? Or is this because of type parameters that are constrained by associated items?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like it kinda does, yeah. fulfill_implication is what needs the inference context since it unifies the impls, and it returns the Substs to rebase onto in translate_substs. It's also called when building the specialization graph, but there we just throw away the result. I'll look into storing the result, but I'm not sure I can figure out how everything works exactly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, this does not look straightforward, and I think I also lack some understanding about the type checker to do this.

Copy link
Member

Choose a reason for hiding this comment

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

Seems fair, it's easier to infer everything through unification. I suppose the only thing that could be done ahead of time is to get the Substs for a less specialized impl, in terms of types from a more specialized impl.

That is, impl<T, U> Foo<U> for Vec<T> and impl<'a, X> Foo<() for Vec<&'a X> would give you Substs [&'a X, ()], and I guess if you had these on every edge in the specialization graph, you could walk "up" it (i.e. in the "less specialized" direction) by successive substitution.

Actually, is that what's necessary? Because it doesn't seem that hard to compute, you'd use translate_subst or w/e ahead of time and keep the result.
But it can be done in a separate PR.


// Since this is a trait item, we need to see if the item is either a trait default item
// or a specialization because we can't resolve those unless we can `Reveal::All`.
// NOTE: This should be kept in sync with the similar code in
// `rustc_middle::traits::project::assemble_candidates_from_impls()`.
let eligible = if !resolved_item.defaultness.is_default() {
let eligible = if leaf_def.is_final() {
// Non-specializable items are always projectable.
true
} else if param_env.reveal == traits::Reveal::All {
!trait_ref.needs_subst()
} else {
false
// Only reveal a specializable default if we're past type-checking
// and the obligation is monomorphic, otherwise passes such as
// transmute checking and polymorphic MIR optimizations could
// get a result which isn't correct for all monomorphizations.
if param_env.reveal == Reveal::All { !trait_ref.needs_subst() } else { false }
jonas-schievink marked this conversation as resolved.
Show resolved Hide resolved
};

if !eligible {
Expand Down
11 changes: 11 additions & 0 deletions src/librustc_ty/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,16 @@ fn associated_item(tcx: TyCtxt<'_>, def_id: DefId) -> ty::AssocItem {
)
}

fn impl_defaultness(tcx: TyCtxt<'_>, def_id: DefId) -> hir::Defaultness {
let hir_id = tcx.hir().as_local_hir_id(def_id).unwrap();
let item = tcx.hir().expect_item(hir_id);
if let hir::ItemKind::Impl { defaultness, .. } = item.kind {
defaultness
} else {
bug!("`impl_defaultness` called on {:?}", item);
}
}

/// Calculates the `Sized` constraint.
///
/// In fact, there are only a few options for the types in the constraint:
Expand Down Expand Up @@ -371,6 +381,7 @@ pub fn provide(providers: &mut ty::query::Providers<'_>) {
crate_hash,
instance_def_size_estimate,
issue33140_self_ty,
impl_defaultness,
..*providers
};
}
Loading