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 RFC 2056 trivial constraints in where clauses #48557

Merged
merged 6 commits into from
May 16, 2018
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
8 changes: 8 additions & 0 deletions src/librustc/traits/error_reporting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1475,6 +1475,14 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
}
ObligationCauseCode::ReturnType(_) |
ObligationCauseCode::BlockTailExpression(_) => (),
ObligationCauseCode::TrivialBound => {
err.help("see issue #48214");
if tcx.sess.opts.unstable_features.is_nightly_build() {
err.help("add #![feature(trivial_bounds)] to the \
crate attributes to enable",
);
}
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/librustc/traits/fulfill.rs
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@ fn process_predicate<'a, 'gcx, 'tcx>(
ty::Predicate::Trait(ref data) => {
let trait_obligation = obligation.with(data.clone());

if data.is_global() {
if data.is_global() && !data.has_late_bound_regions() {
// no type variables present, can use evaluation for better caching.
// FIXME: consider caching errors too.
if selcx.infcx().predicate_must_hold(&obligation) {
Expand Down
12 changes: 3 additions & 9 deletions src/librustc/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,9 @@ pub enum ObligationCauseCode<'tcx> {

/// Block implicit return
BlockTailExpression(ast::NodeId),

/// #[feature(trivial_bounds)] is not enabled
TrivialBound,
}

#[derive(Clone, Debug, PartialEq, Eq, Hash)]
Expand Down Expand Up @@ -641,17 +644,8 @@ pub fn normalize_param_env_or_error<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,

let predicates: Vec<_> =
util::elaborate_predicates(tcx, unnormalized_env.caller_bounds.to_vec())
.filter(|p| !p.is_global()) // (*)
.collect();
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, can you clarify how you avoid the soundness problems discussed here =)

Is that related to the global check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't create any soundness problems after removing this, so I can't really be sure that I have avoided it. See the tests for an idea of what I attempted.

It looks like the current caching is (at the time of making this PR) very conservative, which probably explains this though.

fn can_use_global_caches(&self, param_env: ty::ParamEnv<'tcx>) -> bool {
// If there are any where-clauses in scope, then we always use
// a cache local to this particular scope. Otherwise, we
// switch to a global cache. We used to try and draw
// finer-grained distinctions, but that led to a serious of
// annoying and weird bugs like #22019 and #18290. This simple
// rule seems to be pretty clearly safe and also still retains
// a very high hit rate (~95% when compiling rustc).
if !param_env.caller_bounds.is_empty() {
return false;
}


// (*) Any predicate like `i32: Trait<u32>` or whatever doesn't
// need to be in the *environment* to be proven, so screen those
// out. This is important for the soundness of inter-fn
// caching. Note though that we should probably check that these
// predicates hold at the point where the environment is
// constructed, but I am not currently doing so out of laziness.
// -nmatsakis

debug!("normalize_param_env_or_error: elaborated-predicates={:?}",
predicates);

Expand Down
65 changes: 28 additions & 37 deletions src/librustc/traits/select.rs
Original file line number Diff line number Diff line change
Expand Up @@ -305,9 +305,6 @@ enum BuiltinImplConditions<'tcx> {
/// There is no built-in impl. There may be some other
/// candidate (a where-clause or user-defined impl).
None,
/// There is *no* impl for this, builtin or not. Ignore
/// all where-clauses.
Never,
/// It is unknown whether there is an impl.
Ambiguous
}
Expand Down Expand Up @@ -781,13 +778,13 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
mut obligation: TraitObligation<'tcx>)
-> Result<EvaluationResult, OverflowError>
{
debug!("evaluate_trait_predicate_recursively({:?})",
obligation);
debug!("evaluate_trait_predicate_recursively({:?})", obligation);

if !self.intercrate.is_some() && obligation.is_global() {
// If a param env is consistent, global obligations do not depend on its particular
// value in order to work, so we can clear out the param env and get better
// caching. (If the current param env is inconsistent, we don't care what happens).
if self.intercrate.is_none() && obligation.is_global()
&& obligation.param_env.caller_bounds.iter().all(|bound| bound.needs_subst()) {
// If a param env has no global bounds, global obligations do not
// depend on its particular value in order to work, so we can clear
// out the param env and get better caching.
debug!("evaluate_trait_predicate_recursively({:?}) - in global", obligation);
obligation.param_env = obligation.param_env.without_caller_bounds();
}
Expand Down Expand Up @@ -1451,22 +1448,22 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
let sized_conditions = self.sized_conditions(obligation);
self.assemble_builtin_bound_candidates(sized_conditions,
&mut candidates)?;
} else if lang_items.unsize_trait() == Some(def_id) {
self.assemble_candidates_for_unsizing(obligation, &mut candidates);
} else {
if lang_items.clone_trait() == Some(def_id) {
// Same builtin conditions as `Copy`, i.e. every type which has builtin support
// for `Copy` also has builtin support for `Clone`, + tuples and arrays of `Clone`
// types have builtin support for `Clone`.
let clone_conditions = self.copy_clone_conditions(obligation);
self.assemble_builtin_bound_candidates(clone_conditions, &mut candidates)?;
}

self.assemble_generator_candidates(obligation, &mut candidates)?;
self.assemble_closure_candidates(obligation, &mut candidates)?;
self.assemble_fn_pointer_candidates(obligation, &mut candidates)?;
self.assemble_candidates_from_impls(obligation, &mut candidates)?;
self.assemble_candidates_from_object_ty(obligation, &mut candidates);
} else if lang_items.unsize_trait() == Some(def_id) {
self.assemble_candidates_for_unsizing(obligation, &mut candidates);
} else {
if lang_items.clone_trait() == Some(def_id) {
// Same builtin conditions as `Copy`, i.e. every type which has builtin support
// for `Copy` also has builtin support for `Clone`, + tuples and arrays of `Clone`
// types have builtin support for `Clone`.
let clone_conditions = self.copy_clone_conditions(obligation);
self.assemble_builtin_bound_candidates(clone_conditions, &mut candidates)?;
}

self.assemble_generator_candidates(obligation, &mut candidates)?;
self.assemble_closure_candidates(obligation, &mut candidates)?;
self.assemble_fn_pointer_candidates(obligation, &mut candidates)?;
self.assemble_candidates_from_impls(obligation, &mut candidates)?;
self.assemble_candidates_from_object_ty(obligation, &mut candidates);
}

self.assemble_candidates_from_projected_tys(obligation, &mut candidates);
Expand Down Expand Up @@ -2081,13 +2078,8 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
// BUILTIN BOUNDS
//
// These cover the traits that are built-in to the language
// itself. This includes `Copy` and `Sized` for sure. For the
// moment, it also includes `Send` / `Sync` and a few others, but
// those will hopefully change to library-defined traits in the
// future.
// itself: `Copy`, `Clone` and `Sized`.

// HACK: if this returns an error, selection exits without considering
// other impls.
fn assemble_builtin_bound_candidates<'o>(&mut self,
conditions: BuiltinImplConditions<'tcx>,
candidates: &mut SelectionCandidateSet<'tcx>)
Expand All @@ -2106,14 +2098,13 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
debug!("assemble_builtin_bound_candidates: ambiguous builtin");
Ok(candidates.ambiguous = true)
}
BuiltinImplConditions::Never => { Err(Unimplemented) }
}
}

fn sized_conditions(&mut self, obligation: &TraitObligation<'tcx>)
-> BuiltinImplConditions<'tcx>
{
use self::BuiltinImplConditions::{Ambiguous, None, Never, Where};
use self::BuiltinImplConditions::{Ambiguous, None, Where};

// NOTE: binder moved to (*)
let self_ty = self.infcx.shallow_resolve(
Expand All @@ -2130,7 +2121,7 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
Where(ty::Binder::dummy(Vec::new()))
}

ty::TyStr | ty::TySlice(_) | ty::TyDynamic(..) | ty::TyForeign(..) => Never,
ty::TyStr | ty::TySlice(_) | ty::TyDynamic(..) | ty::TyForeign(..) => None,

ty::TyTuple(tys) => {
Where(ty::Binder::bind(tys.last().into_iter().cloned().collect()))
Expand Down Expand Up @@ -2164,7 +2155,7 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
let self_ty = self.infcx.shallow_resolve(
obligation.predicate.skip_binder().self_ty());

use self::BuiltinImplConditions::{Ambiguous, None, Never, Where};
use self::BuiltinImplConditions::{Ambiguous, None, Where};

match self_ty.sty {
ty::TyInfer(ty::IntVar(_)) | ty::TyInfer(ty::FloatVar(_)) |
Expand All @@ -2182,7 +2173,7 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
ty::TyDynamic(..) | ty::TyStr | ty::TySlice(..) |
ty::TyGenerator(..) | ty::TyGeneratorWitness(..) | ty::TyForeign(..) |
ty::TyRef(_, _, hir::MutMutable) => {
Never
None
}

ty::TyArray(element_ty, _) => {
Expand All @@ -2202,7 +2193,7 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
if is_copy_trait || is_clone_trait {
Where(ty::Binder::bind(substs.upvar_tys(def_id, self.tcx()).collect()))
} else {
Never
None
}
}

Expand Down
1 change: 1 addition & 0 deletions src/librustc/traits/structural_impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@ impl<'a, 'tcx> Lift<'tcx> for traits::ObligationCauseCode<'a> {
super::IntrinsicType => Some(super::IntrinsicType),
super::MethodReceiver => Some(super::MethodReceiver),
super::BlockTailExpression(id) => Some(super::BlockTailExpression(id)),
super::TrivialBound => Some(super::TrivialBound),
}
}
}
Expand Down
8 changes: 4 additions & 4 deletions src/librustc/ty/flags.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ impl FlagComputation {
}

&ty::TyParam(ref p) => {
self.add_flags(TypeFlags::HAS_LOCAL_NAMES);
self.add_flags(TypeFlags::HAS_FREE_LOCAL_NAMES);
if p.is_self() {
self.add_flags(TypeFlags::HAS_SELF);
} else {
Expand All @@ -89,7 +89,7 @@ impl FlagComputation {

&ty::TyGenerator(_, ref substs, _) => {
self.add_flags(TypeFlags::HAS_TY_CLOSURE);
self.add_flags(TypeFlags::HAS_LOCAL_NAMES);
self.add_flags(TypeFlags::HAS_FREE_LOCAL_NAMES);
self.add_substs(&substs.substs);
}

Expand All @@ -101,12 +101,12 @@ impl FlagComputation {

&ty::TyClosure(_, ref substs) => {
self.add_flags(TypeFlags::HAS_TY_CLOSURE);
self.add_flags(TypeFlags::HAS_LOCAL_NAMES);
self.add_flags(TypeFlags::HAS_FREE_LOCAL_NAMES);
self.add_substs(&substs.substs);
}

&ty::TyInfer(infer) => {
self.add_flags(TypeFlags::HAS_LOCAL_NAMES); // it might, right?
self.add_flags(TypeFlags::HAS_FREE_LOCAL_NAMES); // it might, right?
self.add_flags(TypeFlags::HAS_TY_INFER);
match infer {
ty::FreshTy(_) |
Expand Down
10 changes: 7 additions & 3 deletions src/librustc/ty/fold.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,10 +116,14 @@ pub trait TypeFoldable<'tcx>: fmt::Debug + Clone {

/// Indicates whether this value references only 'global'
/// types/lifetimes that are the same regardless of what fn we are
/// in. This is used for caching. Errs on the side of returning
/// false.
/// in. This is used for caching.
fn is_global(&self) -> bool {
!self.has_type_flags(TypeFlags::HAS_LOCAL_NAMES)
!self.has_type_flags(TypeFlags::HAS_FREE_LOCAL_NAMES)
}

/// True if there are any late-bound regions
fn has_late_bound_regions(&self) -> bool {
self.has_type_flags(TypeFlags::HAS_RE_LATE_BOUND)
}
}

Expand Down
11 changes: 8 additions & 3 deletions src/librustc/ty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,7 @@ bitflags! {

// true if there are "names" of types and regions and so forth
// that are local to a particular fn
const HAS_LOCAL_NAMES = 1 << 10;
const HAS_FREE_LOCAL_NAMES = 1 << 10;

// Present if the type belongs in a local type context.
// Only set for TyInfer other than Fresh.
Expand All @@ -455,6 +455,10 @@ bitflags! {
// ought to be true only for the results of canonicalization.
const HAS_CANONICAL_VARS = 1 << 13;

/// Does this have any `ReLateBound` regions? Used to check
/// if a global bound is safe to evaluate.
const HAS_RE_LATE_BOUND = 1 << 14;

const NEEDS_SUBST = TypeFlags::HAS_PARAMS.bits |
TypeFlags::HAS_SELF.bits |
TypeFlags::HAS_RE_EARLY_BOUND.bits;
Expand All @@ -472,9 +476,10 @@ bitflags! {
TypeFlags::HAS_TY_ERR.bits |
TypeFlags::HAS_PROJECTION.bits |
TypeFlags::HAS_TY_CLOSURE.bits |
TypeFlags::HAS_LOCAL_NAMES.bits |
TypeFlags::HAS_FREE_LOCAL_NAMES.bits |
TypeFlags::KEEP_IN_LOCAL_TCX.bits |
TypeFlags::HAS_CANONICAL_VARS.bits;
TypeFlags::HAS_CANONICAL_VARS.bits |
TypeFlags::HAS_RE_LATE_BOUND.bits;
}
}

Expand Down
8 changes: 5 additions & 3 deletions src/librustc/ty/sty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1268,7 +1268,9 @@ impl RegionKind {
flags = flags | TypeFlags::HAS_FREE_REGIONS;
flags = flags | TypeFlags::HAS_RE_SKOL;
}
ty::ReLateBound(..) => { }
ty::ReLateBound(..) => {
flags = flags | TypeFlags::HAS_RE_LATE_BOUND;
}
ty::ReEarlyBound(..) => {
flags = flags | TypeFlags::HAS_FREE_REGIONS;
flags = flags | TypeFlags::HAS_RE_EARLY_BOUND;
Expand All @@ -1291,8 +1293,8 @@ impl RegionKind {
}

match *self {
ty::ReStatic | ty::ReEmpty | ty::ReErased => (),
_ => flags = flags | TypeFlags::HAS_LOCAL_NAMES,
ty::ReStatic | ty::ReEmpty | ty::ReErased | ty::ReLateBound(..) => (),
_ => flags = flags | TypeFlags::HAS_FREE_LOCAL_NAMES,
}

debug!("type_flags({:?}) = {:?}", self, flags);
Expand Down
58 changes: 58 additions & 0 deletions src/librustc_lint/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1591,3 +1591,61 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ExternCrate {
self.0 += 1;
}
}

/// Lint for trait and lifetime bounds that don't depend on type parameters
/// which either do nothing, or stop the item from being used.
pub struct TrivialConstraints;

declare_lint! {
TRIVIAL_BOUNDS,
Warn,
"these bounds don't depend on an type parameters"
}

impl LintPass for TrivialConstraints {
fn get_lints(&self) -> LintArray {
lint_array!(TRIVIAL_BOUNDS)
}
}

impl<'a, 'tcx> LateLintPass<'a, 'tcx> for TrivialConstraints {
fn check_item(
&mut self,
cx: &LateContext<'a, 'tcx>,
item: &'tcx hir::Item,
) {
use rustc::ty::fold::TypeFoldable;
use rustc::ty::Predicate::*;


if cx.tcx.features().trivial_bounds {
let def_id = cx.tcx.hir.local_def_id(item.id);
let predicates = cx.tcx.predicates_of(def_id);
for predicate in &predicates.predicates {
let predicate_kind_name = match *predicate {
Trait(..) => "Trait",
TypeOutlives(..) |
RegionOutlives(..) => "Lifetime",

// Ignore projections, as they can only be global
// if the trait bound is global
Projection(..) |
// Ignore bounds that a user can't type
WellFormed(..) |
ObjectSafe(..) |
ClosureKind(..) |
Subtype(..) |
ConstEvaluatable(..) => continue,
};
if predicate.is_global() {
cx.span_lint(
TRIVIAL_BOUNDS,
item.span,
&format!("{} bound {} does not depend on any type \
or lifetime parameters", predicate_kind_name, predicate),
);
}
}
}
}
}
1 change: 1 addition & 0 deletions src/librustc_lint/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) {
UnreachablePub,
TypeAliasBounds,
UnusedBrokenConst,
TrivialConstraints,
);

add_builtin_with_new!(sess,
Expand Down
Loading