Skip to content

Commit

Permalink
Auto merge of #68970 - matthewjasper:min-spec, r=nikomatsakis
Browse files Browse the repository at this point in the history
Implement a feature for a sound specialization subset

This implements a new feature (`min_specialization`) that restricts specialization to a subset that is reasonable for the standard library to use.

The plan is to then:

* Update `libcore` and `liballoc` to compile with `min_specialization`.
* Add a lint to forbid use of `feature(specialization)` (and other unsound, type system extending features) in the standard library.
* Fix the soundness issues around `specialization`.
* Remove `min_specialization`

The rest of this is an overview from a comment in this PR

## Basic approach

To enforce this requirement on specializations we take the following approach:
1. Match up the substs for `impl2` so that the implemented trait and self-type match those for `impl1`.
2. Check for any direct use of `'static` in the substs of `impl2`.
3. Check that all of the generic parameters of `impl1` occur at most once in the *unconstrained* substs for `impl2`. A parameter is constrained if its value is completely determined by an associated type projection predicate.
4. Check that all predicates on `impl1` also exist on `impl2` (after matching substs).

## Example

Suppose we have the following always applicable impl:

```rust
impl<T> SpecExtend<T> for std::vec::IntoIter<T> { /* specialized impl */ }
impl<T, I: Iterator<Item=T>> SpecExtend<T> for I { /* default impl */ }
```

We get that the subst for `impl2` are `[T, std::vec::IntoIter<T>]`. `T` is constrained to be `<I as Iterator>::Item`, so we check only `std::vec::IntoIter<T>` for repeated parameters, which it doesn't have. The predicates of `impl1` are only `T: Sized`, which is also a predicate of impl2`. So this specialization is sound.

## Extensions

Unfortunately not all specializations in the standard library are allowed by this. So there are two extensions to these rules that allow specializing on some traits.

### rustc_specialization_trait

If a trait is always applicable, then it's sound to specialize on it. We check trait is always applicable in the same way as impls, except that step 4 is now "all predicates on `impl1` are always applicable". We require that `specialization` or `min_specialization` is enabled to implement these traits.

### rustc_specialization_marker

There are also some specialization on traits with no methods, including the `FusedIterator` trait which is advertised as allowing optimizations. We allow marking marker traits with an unstable attribute that means we ignore them in point 3 of the checks above. This is unsound but we allow it in the short term because it can't cause use after frees with purely safe code in the same way as specializing on traits methods can.

r? @nikomatsakis
cc #31844 #67194
  • Loading branch information
bors committed Mar 16, 2020
2 parents dd67187 + 39ee66a commit e24252a
Show file tree
Hide file tree
Showing 48 changed files with 1,095 additions and 73 deletions.
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 @@ -10,6 +10,7 @@ use rustc_hir::HirId;

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

Expand All @@ -35,11 +36,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
/// 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
/// 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 @@ -54,16 +77,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),
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 @@ -343,6 +343,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

0 comments on commit e24252a

Please sign in to comment.