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

Chalkify: Add builtin Copy/Clone #60183

Merged
merged 3 commits into from
Apr 26, 2019
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
14 changes: 4 additions & 10 deletions src/librustc/traits/select.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2505,16 +2505,10 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
}

ty::Closure(def_id, substs) => {
let trait_id = obligation.predicate.def_id();
let is_copy_trait = Some(trait_id) == self.tcx().lang_items().copy_trait();
let is_clone_trait = Some(trait_id) == self.tcx().lang_items().clone_trait();
if is_copy_trait || is_clone_trait {
Where(ty::Binder::bind(
substs.upvar_tys(def_id, self.tcx()).collect(),
))
} else {
None
}
// (*) binder moved here
Where(ty::Binder::bind(
substs.upvar_tys(def_id, self.tcx()).collect(),
))
}

ty::Adt(..) | ty::Projection(..) | ty::Param(..) | ty::Opaque(..) => {
Expand Down
168 changes: 141 additions & 27 deletions src/librustc_traits/chalk_context/program_clauses/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,42 @@ use rustc::traits::{
};
use rustc::ty;
use rustc::ty::subst::{InternalSubsts, Subst};
use rustc::hir;
use rustc::hir::def_id::DefId;
use crate::lowering::Lower;
use crate::generic_types;

/// Returns a predicate of the form
/// `Implemented(ty: Trait) :- Implemented(nested: Trait)...`
/// where `Trait` is specified by `trait_def_id`.
fn builtin_impl_clause(
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just add a small comment like:

/// Return a predicate of the form:
/// `Implemented(ty: Trait) :- Implemented(nested: Trait)... `
/// where `Trait == Trait(trait_def_id)`

or whatever

tcx: ty::TyCtxt<'_, '_, 'tcx>,
ty: ty::Ty<'tcx>,
nested: &[ty::Ty<'tcx>],
trait_def_id: DefId
) -> ProgramClause<'tcx> {
ProgramClause {
goal: ty::TraitPredicate {
trait_ref: ty::TraitRef {
def_id: trait_def_id,
substs: tcx.mk_substs_trait(ty, &[]),
},
}.lower(),
hypotheses: tcx.mk_goals(
nested.iter()
.cloned()
.map(|nested_ty| ty::TraitRef {
def_id: trait_def_id,
substs: tcx.mk_substs_trait(nested_ty, &[]),
})
.map(|trait_ref| ty::TraitPredicate { trait_ref })
.map(|pred| GoalKind::DomainGoal(pred.lower()))
.map(|goal_kind| tcx.mk_goal(goal_kind))
),
category: ProgramClauseCategory::Other,
}
}

crate fn assemble_builtin_unsize_impls<'tcx>(
tcx: ty::TyCtxt<'_, '_, 'tcx>,
unsize_def_id: DefId,
Expand Down Expand Up @@ -93,26 +125,7 @@ crate fn assemble_builtin_sized_impls<'tcx>(
clauses: &mut Vec<Clause<'tcx>>
) {
let mut push_builtin_impl = |ty: ty::Ty<'tcx>, nested: &[ty::Ty<'tcx>]| {
let clause = ProgramClause {
goal: ty::TraitPredicate {
trait_ref: ty::TraitRef {
def_id: sized_def_id,
substs: tcx.mk_substs_trait(ty, &[]),
},
}.lower(),
hypotheses: tcx.mk_goals(
nested.iter()
.cloned()
.map(|nested_ty| ty::TraitRef {
def_id: sized_def_id,
substs: tcx.mk_substs_trait(nested_ty, &[]),
})
.map(|trait_ref| ty::TraitPredicate { trait_ref })
.map(|pred| GoalKind::DomainGoal(pred.lower()))
.map(|goal_kind| tcx.mk_goal(goal_kind))
),
category: ProgramClauseCategory::Other,
};
let clause = builtin_impl_clause(tcx, ty, nested, sized_def_id);
// Bind innermost bound vars that may exist in `ty` and `nested`.
clauses.push(Clause::ForAll(ty::Binder::bind(clause)));
};
Expand All @@ -124,6 +137,8 @@ crate fn assemble_builtin_sized_impls<'tcx>(
ty::Int(..) |
ty::Uint(..) |
ty::Float(..) |
ty::Infer(ty::IntVar(_)) |
ty::Infer(ty::FloatVar(_)) |
ty::Error |
ty::Never => push_builtin_impl(ty, &[]),

Expand Down Expand Up @@ -175,14 +190,11 @@ crate fn assemble_builtin_sized_impls<'tcx>(
push_builtin_impl(adt, &sized_constraint);
}

// Artificially trigger an ambiguity.
ty::Infer(..) => {
// Everybody can find at least two types to unify against:
// general ty vars, int vars and float vars.
// Artificially trigger an ambiguity by adding two possible types to
// unify against.
ty::Infer(ty::TyVar(_)) => {
push_builtin_impl(tcx.types.i32, &[]);
push_builtin_impl(tcx.types.u32, &[]);
push_builtin_impl(tcx.types.f32, &[]);
push_builtin_impl(tcx.types.f64, &[]);
}

ty::Projection(_projection_ty) => {
Expand All @@ -203,6 +215,108 @@ crate fn assemble_builtin_sized_impls<'tcx>(
ty::Opaque(..) => (),

ty::Bound(..) |
ty::GeneratorWitness(..) => bug!("unexpected type {:?}", ty),
ty::GeneratorWitness(..) |
ty::Infer(ty::FreshTy(_)) |
ty::Infer(ty::FreshIntTy(_)) |
ty::Infer(ty::FreshFloatTy(_)) => bug!("unexpected type {:?}", ty),
}
}

crate fn assemble_builtin_copy_clone_impls<'tcx>(
tcx: ty::TyCtxt<'_, '_, 'tcx>,
trait_def_id: DefId,
ty: ty::Ty<'tcx>,
clauses: &mut Vec<Clause<'tcx>>
) {
let mut push_builtin_impl = |ty: ty::Ty<'tcx>, nested: &[ty::Ty<'tcx>]| {
let clause = builtin_impl_clause(tcx, ty, nested, trait_def_id);
// Bind innermost bound vars that may exist in `ty` and `nested`.
clauses.push(Clause::ForAll(ty::Binder::bind(clause)));
};

match &ty.sty {
// Implementations provided in libcore.
ty::Bool |
ty::Char |
ty::Int(..) |
ty::Uint(..) |
ty::Float(..) |
ty::RawPtr(..) |
ty::Never |
ty::Ref(_, _, hir::MutImmutable) => (),

// Non parametric primitive types.
ty::Infer(ty::IntVar(_)) |
ty::Infer(ty::FloatVar(_)) |
ty::Error => push_builtin_impl(ty, &[]),

// These implement `Copy`/`Clone` if their element types do.
&ty::Array(_, length) => {
let element_ty = generic_types::bound(tcx, 0);
push_builtin_impl(tcx.mk_ty(ty::Array(element_ty, length)), &[element_ty]);
}
&ty::Tuple(type_list) => {
let type_list = generic_types::type_list(tcx, type_list.len());
push_builtin_impl(tcx.mk_ty(ty::Tuple(type_list)), &**type_list);
}
&ty::Closure(def_id, ..) => {
let closure_ty = generic_types::closure(tcx, def_id);
let upvar_tys: Vec<_> = match &closure_ty.sty {
ty::Closure(_, substs) => substs.upvar_tys(def_id, tcx).collect(),
_ => bug!(),
};
push_builtin_impl(closure_ty, &upvar_tys);
}

// These ones are always `Clone`.
ty::FnPtr(fn_ptr) => {
let fn_ptr = fn_ptr.skip_binder();
let fn_ptr = generic_types::fn_ptr(
tcx,
fn_ptr.inputs_and_output.len(),
fn_ptr.c_variadic,
fn_ptr.unsafety,
fn_ptr.abi
);
push_builtin_impl(fn_ptr, &[]);
}
&ty::FnDef(def_id, ..) => {
push_builtin_impl(generic_types::fn_def(tcx, def_id), &[]);
}

// These depend on whatever user-defined impls might exist.
ty::Adt(_, _) => (),

// Artificially trigger an ambiguity by adding two possible types to
// unify against.
ty::Infer(ty::TyVar(_)) => {
push_builtin_impl(tcx.types.i32, &[]);
Copy link
Member

Choose a reason for hiding this comment

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

These four solutions are only there to artificially trigger an ambiguity if we encounter an inference variable whatever its type, because we don't want to actually enumerate all solutions.

If we want int and float vars to always be Copy / Clone without having to resolve them, we should add a match arm:

ty::Infer(Infer::Float(..)) | ty::Infer(Infer::Int(..)) => push_builtin_impl(ty, &[]),

And probably do that for Sized as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding the match arm makes sense, but is this more of an optimization or a behavior change? (Leaving aside the fact that not all integer/float types were included.) Was there any reason that you included these four types in particular?

Copy link
Member

Choose a reason for hiding this comment

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

This would be a change of behavior, but probably would only affect error reporting in the end?

We’re talking about goals like ?T: Clone, eventually either ?T is resolved and the goal will be re-evaluated, or ?T remains unresolved and we’ll have a type inference error so I guess it does not matter...

Anyway I think that the code in rustc::traits::select does say that unresolved int / float inference variables always implement Copy / Clone (and probably does the same for Sized), so let’s do that.

I included these four types so that each kind of inference variable has at least two solutions to unify against, in order to trigger ambiguity. For example, if I only pushed i32 and f32 as solutions, then an ?IntVar inference variable would only unify against i32 and the solver would think that the only solution to my goal is i32: Clone and would arbitrarily think that ?IntVar == i32, while I wanted an ambiguous answer.

However if we only cared about triggering ambiguity for type variables (because we would now eagerly answer Implemented(?IntVar: Clone)), then pushing only two arbitrary solutions (e.g. f32 and i32) would suffice since type variables will unify with whatever they want.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, done.

push_builtin_impl(tcx.types.f32, &[]);
}

ty::Projection(_projection_ty) => {
// FIXME: add builtin impls from the associated type values found in
// trait impls of `projection_ty.trait_ref(tcx)`.
}

// The `Copy`/`Clone` bound can only come from the environment.
ty::Param(..) |
ty::Placeholder(..) |
ty::UnnormalizedProjection(..) |
ty::Opaque(..) => (),

// Definitely not `Copy`/`Clone`.
ty::Dynamic(..) |
ty::Foreign(..) |
ty::Generator(..) |
ty::Str |
ty::Slice(..) |
ty::Ref(_, _, hir::MutMutable) => (),

ty::Bound(..) |
ty::GeneratorWitness(..) |
ty::Infer(ty::FreshTy(_)) |
ty::Infer(ty::FreshIntTy(_)) |
ty::Infer(ty::FreshFloatTy(_)) => bug!("unexpected type {:?}", ty),
}
}
21 changes: 20 additions & 1 deletion src/librustc_traits/chalk_context/program_clauses/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,27 @@ impl ChalkInferenceContext<'cx, 'gcx, 'tcx> {
);
}

if Some(trait_predicate.def_id()) == self.infcx.tcx.lang_items().copy_trait() {
assemble_builtin_copy_clone_impls(
self.infcx.tcx,
trait_predicate.def_id(),
trait_predicate.self_ty(),
&mut clauses
);
}

if Some(trait_predicate.def_id()) == self.infcx.tcx.lang_items().clone_trait() {
// For all builtin impls, the conditions for `Copy` and
// `Clone` are the same.
assemble_builtin_copy_clone_impls(
self.infcx.tcx,
trait_predicate.def_id(),
trait_predicate.self_ty(),
&mut clauses
);
}

// FIXME: we need to add special rules for other builtin impls:
// * `Copy` / `Clone`
// * `Generator`
// * `FnOnce` / `FnMut` / `Fn`
// * trait objects
Expand Down
43 changes: 43 additions & 0 deletions src/test/run-pass/chalkify/builtin-copy-clone.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// compile-flags: -Z chalk

// Test that `Clone` is correctly implemented for builtin types.

#[derive(Copy, Clone)]
struct S(i32);

fn test_clone<T: Clone>(arg: T) {
let _ = arg.clone();
}

fn test_copy<T: Copy>(arg: T) {
let _ = arg;
let _ = arg;
}

fn test_copy_clone<T: Copy + Clone>(arg: T) {
test_copy(arg);
test_clone(arg);
}

fn foo() { }

fn main() {
test_copy_clone(foo);
let f: fn() = foo;
test_copy_clone(f);
// FIXME: add closures when they're considered WF
test_copy_clone([1; 56]);
test_copy_clone((1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1));
test_copy_clone((1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, true, 'a', 1.1));
test_copy_clone(());
test_copy_clone(((1, 1), (1, 1, 1), (1.1, 1, 1, 'a'), ()));

let a = (
(S(1), S(0)),
(
(S(0), S(0), S(1)),
S(0)
)
);
test_copy_clone(a);
}