8000 Chalkify: Add builtin Copy/Clone by tmandry · Pull Request #60183 · rust-lang/rust · GitHub
[go: up one dir, main page]
Skip to content

Chalkify: Add builtin Copy/Clone #60183

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Apr 26, 2019
Merged

Chalkify: Add builtin Copy/Clone #60183

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b2c0fd0
Remove redundant code in copy_clone_conditions
tmandry Apr 21, 2019
c75e089
chalkify: Add Copy/Clone builtins
tmandry Apr 22, 2019
56ab3e7
Add builtin impls for int and float inference vars in chalk
tmandry Apr 24, 2019
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
Prev Previous commit
Add builtin impls for int and float inference vars in chalk
  • Loading branch information
@tmandry
tmandry committed Apr 24, 2019
commit 56ab3e70e7eeeaa9801c1e32c2a459df8dfc4ab8
33 changes: 19 additions & 14 deletions src/librustc_traits/chalk_context/program_clauses/builtin.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,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 @@ -188,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 @@ -216,7 +215,10 @@ 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),
}
}

Expand All @@ -243,7 +245,9 @@ crate fn assemble_builtin_copy_clone_impls<'tcx>(
ty::Never |
ty::Ref(_, _, hir::MutImmutable) => (),

// Non parametric primitive type.
// 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.
Expand Down Expand Up @@ -283,13 +287,11 @@ crate fn assemble_builtin_copy_clone_impls<'tcx>(
// These depend on whatever user-defined impls might exist.
ty::Adt(_, _) => (),

// int vars and float vars are always `Copy`.
// Other vars will trigger an ambiguity.
ty::Infer(..) => {
// 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.u32, &[]);
push_builtin_impl(tcx.types.f32, &[]);
push_builtin_impl(tcx.types.f64, &[]);
}

ty::Projection(_projection_ty) => {
Expand All @@ -312,6 +314,9 511C @@ crate fn assemble_builtin_copy_clone_impls<'tcx>(
ty::Ref(_, _, hir::MutMutable) => (),

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),
}
}
0