Chalkify: Add builtin Copy/Clone #60183
Conversation
This was left over from when closure copy and clone were gated behind feature flags.
rust-highfive
added
the
S-waiting-on-review
|
cc @scalexm, feel free to self-assign if you'd like to review |
|
r? @scalexm |
| use rustc::hir::def_id::DefId; | ||
| use crate::lowering::Lower; | ||
| use crate::generic_types; | ||
|
|
||
| fn builtin_impl_clause( |
There was a problem hiding this comment.
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
| // int vars and float vars are always `Copy`. | ||
| // Other vars will trigger an ambiguity. | ||
| ty::Infer(..) => { | ||
| push_builtin_impl(tcx.types.i32, &[]); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
tmandry
force-pushed
the
chalk-builtin-copy-clone
branch
from
c55dfe2 to
56ab3e7
Compare
|
@bors r+ |
|
📌 Commit 56ab3e7 has been approved by |
bors
added
S-waiting-on-bors
…=scalexm Chalkify: Add builtin Copy/Clone r? @nikomatsakis
…=scalexm Chalkify: Add builtin Copy/Clone r? @nikomatsakis
…=scalexm Chalkify: Add builtin Copy/Clone r? @nikomatsakis
Rollup of 12 pull requests Successful merges: - #59734 (Prevent failure in case no space left on device in rustdoc) - #59940 (Set cfg(test) when rustdoc is running with --test option) - #60134 (Fix index-page generation) - #60165 (Add Pin::{into_inner,into_inner_unchecked}) - #60183 (Chalkify: Add builtin Copy/Clone) - #60225 (Introduce hir::ExprKind::Use and employ in for loop desugaring.) - #60247 (Implement Debug for Place using Place::iterate) - #60259 (Derive Default instead of new in applicable lint) - #60267 (Add feature-gate for f16c target feature) - #60284 (Do not allow const generics to depend on type parameters) - #60285 (Do not ICE when checking types against foreign fn) - #60289 (Make `-Z allow-features` work for stdlib features) Failed merges: r? @ghost
r? @nikomatsakis