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

Implement a feature for a sound specialization subset #68970

Merged
merged 6 commits into from
Mar 17, 2020
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
1 change: 1 addition & 0 deletions src/libcore/marker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ impl<T: ?Sized> !Send for *mut T {}
ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>"
)]
#[fundamental] // for Default, for example, which requires that `[T]: !Default` be evaluatable
#[cfg_attr(not(bootstrap), rustc_specialization_trait)]
pub trait Sized {
// Empty.
}
Expand Down
3 changes: 2 additions & 1 deletion src/libproc_macro/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@
#![feature(in_band_lifetimes)]
#![feature(optin_builtin_traits)]
#![feature(rustc_attrs)]
#![feature(specialization)]
#![cfg_attr(bootstrap, feature(specialization))]
#![cfg_attr(not(bootstrap), feature(min_specialization))]
#![recursion_limit = "256"]

#[unstable(feature = "proc_macro_internals", issue = "27812")]
Expand Down
29 changes: 19 additions & 10 deletions src/librustc/traits/specialization_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use crate::ty::{self, TyCtxt};
use rustc_ast::ast::Ident;
use rustc_data_structures::fx::FxHashMap;
use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
use rustc_errors::ErrorReported;
use rustc_hir::def_id::{DefId, DefIdMap};

/// A per-trait graph of impls in specialization order. At the moment, this
Expand All @@ -23,17 +24,20 @@ use rustc_hir::def_id::{DefId, DefIdMap};
/// has at most one parent.
#[derive(RustcEncodable, RustcDecodable, HashStable)]
pub struct Graph {
// All impls have a parent; the "root" impls have as their parent the `def_id`
// of the trait.
/// All impls have a parent; the "root" impls have as their parent the `def_id`
/// of the trait.
pub parent: DefIdMap<DefId>,

// The "root" impls are found by looking up the trait's def_id.
/// The "root" impls are found by looking up the trait's def_id.
pub children: DefIdMap<Children>,

/// Whether an error was emitted while constructing the graph.
pub has_errored: bool,
}

impl Graph {
pub fn new() -> Graph {
Graph { parent: Default::default(), children: Default::default() }
Graph { parent: Default::default(), children: Default::default(), has_errored: false }
}

/// The parent of a given impl, which is the `DefId` of the trait when the
Expand Down Expand Up @@ -179,17 +183,22 @@ impl<'tcx> Ancestors<'tcx> {
}

/// Walk up the specialization ancestors of a given impl, starting with that
/// impl itself.
/// impl itself. Returns `None` if an error was reported while building the
/// specialization graph.
pub fn ancestors(
tcx: TyCtxt<'tcx>,
trait_def_id: DefId,
start_from_impl: DefId,
) -> Ancestors<'tcx> {
) -> Result<Ancestors<'tcx>, ErrorReported> {
let specialization_graph = tcx.specialization_graph_of(trait_def_id);
Ancestors {
trait_def_id,
specialization_graph,
current_source: Some(Node::Impl(start_from_impl)),
if specialization_graph.has_errored {
Err(ErrorReported)
} else {
Ok(Ancestors {
trait_def_id,
specialization_graph,
current_source: Some(Node::Impl(start_from_impl)),
})
}
}

Expand Down
36 changes: 34 additions & 2 deletions src/librustc/ty/trait_def.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use rustc_hir::def_id::DefId;

use rustc_data_structures::fx::FxHashMap;
use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
use rustc_errors::ErrorReported;
use rustc_macros::HashStable;

/// A trait's definition with type information.
Expand All @@ -33,11 +34,33 @@ pub struct TraitDef {
/// and thus `impl`s of it are allowed to overlap.
pub is_marker: bool,

/// Used to determine whether the standard library is allowed to specialize
/// on this trait.
pub specialization_kind: TraitSpecializationKind,

/// The ICH of this trait's DefPath, cached here so it doesn't have to be
/// recomputed all the time.
pub def_path_hash: DefPathHash,
}

/// Whether this trait is treated specially by the standard library
/// specialization lint.
#[derive(HashStable, PartialEq, Clone, Copy, RustcEncodable, RustcDecodable)]
pub enum TraitSpecializationKind {
/// The default. Specializing on this trait is not allowed.
None,
/// Specializing on this trait is allowed because it doesn't have any
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this term ("Specializing on this trait") a bit ambiguous. I think it means "Specialized impls of this trait are allowed"? Maybe we can update the comment to be more precise.

You mentioned that this is not sound in general. I'm not sure if that's true, actually -- for marker traits, I don't think specialization causes any problems. Maybe I don't know what you mean by that, though.

/// methods. For example `Sized` or `FusedIterator`.
/// Applies to traits with the `rustc_unsafe_specialization_marker`
/// attribute.
Marker,
/// Specializing on this trait is allowed because all of the impls of this
nikomatsakis marked this conversation as resolved.
Show resolved Hide resolved
/// trait are "always applicable". Always applicable means that if
/// `X<'x>: T<'y>` for any lifetimes, then `for<'a, 'b> X<'a>: T<'b>`.
/// Applies to traits with the `rustc_specialization_trait` attribute.
AlwaysApplicable,
}

#[derive(Default)]
pub struct TraitImpls {
blanket_impls: Vec<DefId>,
Expand All @@ -52,16 +75,25 @@ impl<'tcx> TraitDef {
paren_sugar: bool,
has_auto_impl: bool,
is_marker: bool,
specialization_kind: TraitSpecializationKind,
def_path_hash: DefPathHash,
) -> TraitDef {
TraitDef { def_id, unsafety, paren_sugar, has_auto_impl, is_marker, def_path_hash }
TraitDef {
def_id,
unsafety,
paren_sugar,
has_auto_impl,
is_marker,
specialization_kind,
def_path_hash,
}
}

pub fn ancestors(
&self,
tcx: TyCtxt<'tcx>,
of_impl: DefId,
) -> specialization_graph::Ancestors<'tcx> {
) -> Result<specialization_graph::Ancestors<'tcx>, ErrorReported> {
specialization_graph::ancestors(tcx, self.def_id, of_impl)
}
}
Expand Down
20 changes: 14 additions & 6 deletions src/librustc_ast_passes/feature_gate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -542,15 +542,12 @@ impl<'a> Visitor<'a> for PostExpansionVisitor<'a> {
}

fn visit_assoc_item(&mut self, i: &'a ast::AssocItem, ctxt: AssocCtxt) {
if let ast::Defaultness::Default(_) = i.kind.defaultness() {
gate_feature_post!(&self, specialization, i.span, "specialization is unstable");
}

match i.kind {
let is_fn = match i.kind {
ast::AssocItemKind::Fn(_, ref sig, _, _) => {
if let (ast::Const::Yes(_), AssocCtxt::Trait) = (sig.header.constness, ctxt) {
gate_feature_post!(&self, const_fn, i.span, "const fn is unstable");
}
true
}
ast::AssocItemKind::TyAlias(_, ref generics, _, ref ty) => {
if let (Some(_), AssocCtxt::Trait) = (ty, ctxt) {
Expand All @@ -565,8 +562,19 @@ impl<'a> Visitor<'a> for PostExpansionVisitor<'a> {
self.check_impl_trait(ty);
}
self.check_gat(generics, i.span);
false
}
_ => {}
_ => false,
};
if let ast::Defaultness::Default(_) = i.kind.defaultness() {
// Limit `min_specialization` to only specializing functions.
gate_feature_fn!(
&self,
|x: &Features| x.specialization || (is_fn && x.min_specialization),
Comment on lines +570 to +573
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there soundness issues with specializing associated constants? If not, could they be permissible in min_specialization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason to only for functions being a simpler than constants and types is that we only ever work out which impl to select for a (non-const) function when generating code. For constants we can evaluate them during type checking and during codegen and I wanted to avoid potential bugs with any mismatches there.

i.span,
sym::specialization,
"specialization is unstable"
);
}
visit::walk_assoc_item(self, i, ctxt)
}
Expand Down
5 changes: 5 additions & 0 deletions src/librustc_feature/active.rs
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,11 @@ declare_features! (
/// Allows specialization of implementations (RFC 1210).
(active, specialization, "1.7.0", Some(31844), None),

/// A minimal, sound subset of specialization intended to be used by the
/// standard library until the soundness issues with specialization
/// are fixed.
(active, min_specialization, "1.7.0", Some(31844), None),

/// Allows using `#[naked]` on functions.
(active, naked_functions, "1.9.0", Some(32408), None),

Expand Down
8 changes: 8 additions & 0 deletions src/librustc_feature/builtin_attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -530,6 +530,14 @@ pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[
rustc_test_marker, Normal, template!(Word),
"the `#[rustc_test_marker]` attribute is used internally to track tests",
),
rustc_attr!(
rustc_unsafe_specialization_marker, Normal, template!(Word),
"the `#[rustc_unsafe_specialization_marker]` attribute is used to check specializations"
),
rustc_attr!(
rustc_specialization_trait, Normal, template!(Word),
"the `#[rustc_specialization_trait]` attribute is used to check specializations"
),

// ==========================================================================
// Internal attributes, Testing:
Expand Down
2 changes: 2 additions & 0 deletions src/librustc_metadata/rmeta/decoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -651,6 +651,7 @@ impl<'a, 'tcx> CrateMetadataRef<'a> {
data.paren_sugar,
data.has_auto_impl,
data.is_marker,
data.specialization_kind,
self.def_path_table.def_path_hash(item_id),
)
}
Expand All @@ -660,6 +661,7 @@ impl<'a, 'tcx> CrateMetadataRef<'a> {
false,
false,
false,
ty::trait_def::TraitSpecializationKind::None,
self.def_path_table.def_path_hash(item_id),
),
_ => bug!("def-index does not refer to trait or trait alias"),
Expand Down
14 changes: 8 additions & 6 deletions src/librustc_metadata/rmeta/encoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1077,12 +1077,13 @@ impl EncodeContext<'tcx> {
let polarity = self.tcx.impl_polarity(def_id);
let parent = if let Some(trait_ref) = trait_ref {
let trait_def = self.tcx.trait_def(trait_ref.def_id);
trait_def.ancestors(self.tcx, def_id).nth(1).and_then(|node| {
match node {
specialization_graph::Node::Impl(parent) => Some(parent),
_ => None,
}
})
trait_def.ancestors(self.tcx, def_id).ok()
.and_then(|mut an| an.nth(1).and_then(|node| {
match node {
specialization_graph::Node::Impl(parent) => Some(parent),
_ => None,
}
}))
} else {
None
};
Expand Down Expand Up @@ -1114,6 +1115,7 @@ impl EncodeContext<'tcx> {
paren_sugar: trait_def.paren_sugar,
has_auto_impl: self.tcx.trait_is_auto(def_id),
is_marker: trait_def.is_marker,
specialization_kind: trait_def.specialization_kind,
};

EntryKind::Trait(self.lazy(data))
Expand Down
1 change: 1 addition & 0 deletions src/librustc_metadata/rmeta/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,7 @@ struct TraitData {
paren_sugar: bool,
has_auto_impl: bool,
is_marker: bool,
specialization_kind: ty::trait_def::TraitSpecializationKind,
}

#[derive(RustcEncodable, RustcDecodable)]
Expand Down
3 changes: 3 additions & 0 deletions src/librustc_span/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,7 @@ symbols! {
min_align_of,
min_const_fn,
min_const_unsafe_fn,
min_specialization,
mips_target_feature,
mmx_target_feature,
module,
Expand Down Expand Up @@ -654,6 +655,8 @@ symbols! {
rustc_proc_macro_decls,
rustc_promotable,
rustc_regions,
rustc_unsafe_specialization_marker,
rustc_specialization_trait,
rustc_stable,
rustc_std_internal_symbol,
rustc_symbol_name,
Expand Down
24 changes: 14 additions & 10 deletions src/librustc_trait_selection/traits/project.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ use rustc::ty::fold::{TypeFoldable, TypeFolder};
use rustc::ty::subst::{InternalSubsts, Subst};
use rustc::ty::{self, ToPolyTraitRef, ToPredicate, Ty, TyCtxt, WithConstness};
use rustc_ast::ast::Ident;
use rustc_errors::ErrorReported;
use rustc_hir::def_id::DefId;
use rustc_span::symbol::sym;
use rustc_span::DUMMY_SP;
Expand Down Expand Up @@ -1010,7 +1011,8 @@ fn assemble_candidates_from_impls<'cx, 'tcx>(
// NOTE: This should be kept in sync with the similar code in
// `rustc::ty::instance::resolve_associated_item()`.
let node_item =
assoc_ty_def(selcx, impl_data.impl_def_id, obligation.predicate.item_def_id);
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`
Expand Down Expand Up @@ -1405,7 +1407,10 @@ fn confirm_impl_candidate<'cx, 'tcx>(
let trait_def_id = tcx.trait_id_of_impl(impl_def_id).unwrap();

let param_env = obligation.param_env;
let assoc_ty = assoc_ty_def(selcx, impl_def_id, assoc_item_id);
let assoc_ty = match assoc_ty_def(selcx, impl_def_id, assoc_item_id) {
Ok(assoc_ty) => assoc_ty,
Err(ErrorReported) => return Progress { ty: tcx.types.err, obligations: nested },
};

if !assoc_ty.item.defaultness.has_value() {
// This means that the impl is missing a definition for the
Expand Down Expand Up @@ -1444,14 +1449,14 @@ fn assoc_ty_def(
selcx: &SelectionContext<'_, '_>,
impl_def_id: DefId,
assoc_ty_def_id: DefId,
) -> specialization_graph::NodeItem<ty::AssocItem> {
) -> Result<specialization_graph::NodeItem<ty::AssocItem>, 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;
let trait_def = tcx.trait_def(trait_def_id);

// This function may be called while we are still building the
// specialization graph that is queried below (via TraidDef::ancestors()),
// specialization graph that is queried below (via TraitDef::ancestors()),
// so, in order to avoid unnecessary infinite recursion, we manually look
// for the associated item at the given impl.
// If there is no such item in that impl, this function will fail with a
Expand All @@ -1461,17 +1466,16 @@ 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 specialization_graph::NodeItem {
return Ok(specialization_graph::NodeItem {
node: specialization_graph::Node::Impl(impl_def_id),
item: *item,
};
});
}
}

if let Some(assoc_item) =
trait_def.ancestors(tcx, impl_def_id).leaf_def(tcx, assoc_ty_name, ty::AssocKind::Type)
{
assoc_item
let ancestors = trait_def.ancestors(tcx, impl_def_id)?;
if let Some(assoc_item) = ancestors.leaf_def(tcx, assoc_ty_name, ty::AssocKind::Type) {
Ok(assoc_item)
} else {
// This is saying that neither the trait nor
// the impl contain a definition for this
Expand Down
Loading