Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upImplement the new-style trait solver #56384
+1,311
−293
Conversation
rust-highfive
assigned
nikomatsakis
Nov 30, 2018
rust-highfive
added
the
S-waiting-on-review
label
Nov 30, 2018
This comment was marked as resolved.
This comment was marked as resolved.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
scalexm
force-pushed the
scalexm:chalk
branch
from
1cbf37f
to
eb4fa48
Nov 30, 2018
This comment has been minimized.
This comment has been minimized.
|
|
scalexm
force-pushed the
scalexm:chalk
branch
from
eb4fa48
to
ecc8604
Nov 30, 2018
This comment has been minimized.
This comment has been minimized.
|
The title of this PR is so epic |
This comment has been minimized.
This comment has been minimized.
|
|
scalexm
force-pushed the
scalexm:chalk
branch
from
ecc8604
to
f1b0eb4
Dec 8, 2018
nikomatsakis
requested changes
Dec 17, 2018
Sorry for being so slow @scalexm -- this looks great, but I do have a few concerns:
- We should decide how to handle subtyping before landing; a FIXME may suffice here
- I'm a bit concerned about adding the def-id to param-env, it seems like this could affect queries in unintended ways, leading to less re-use than we would otherwise get
(Maybe we can only set the def-id if -Zchalk is used for now?)
| ty::Infer(..) => { | ||
| // Everybody can find at least two types to unify against: | ||
| // general ty vars, int vars and float vars. | ||
| push_builtin_impl(tcx.types.i32, &[]); |
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Dec 17, 2018
Contributor
Hmm, this worries me. I feel like there could be cases where chalk is able to try out all 4 of those possibilities and conclude that something is true since because it doesn't know of the other Sized impls. But I see the challenge here, of course, in that to generate the "full set of sized impls" would in some sense require iterating over all the types in the system.
(This problem, though, doesn't seem entirely unique to sized)
I wonder if we should think about tweaking how the engine walks around this case. It's probably ok to land the code as is for now, but we likely want to add some sort of FIXME issue here to revisit this question specifically.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
scalexm
Dec 17, 2018
•
Member
Also I believe this problem is indeed not unique to Sized, but at least it would only show for traits that have built-in impls generated by the compiler. Other traits would be fine since we would be able to enumerate all the (user-written) impls.
| } | ||
|
|
||
| ty::Projection(_projection_ty) => { | ||
| // FIXME: add builtin impls from the associated type values found in |
This comment has been minimized.
This comment has been minimized.
| var_values: cs.subst, | ||
| region_constraints: Vec::new(), | ||
|
|
||
| // FIXME: restore this later once we get better at handling regions |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
scalexm
Dec 17, 2018
Member
There are some lost in the chalk-integration Zulip channel :)
I’ll elaborate on that soon.
| @@ -458,7 +459,8 @@ impl context::UnificationOps<ChalkArenas<'gcx>, ChalkArenas<'tcx>> | |||
| b: &Kind<'tcx>, | |||
| ) -> Fallible<UnificationResult<'tcx>> { | |||
| self.infcx.commit_if_ok(|_| { | |||
| unify(self.infcx, *environment, a, b).map_err(|_| chalk_engine::fallible::NoSolution) | |||
| unify(self.infcx, *environment, ty::Variance::Covariant, a, b) | |||
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Dec 17, 2018
Contributor
Wait, why do we want subtyping here? I think this should be equality (unification)
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Dec 17, 2018
Contributor
(I'm not sure if we can land this like this, or at least it requires a FIXME for sure)
This comment has been minimized.
This comment has been minimized.
scalexm
Dec 17, 2018
Member
Yes it’s a hack, I was planning to remove it from the final commit history.
| @@ -683,6 +685,13 @@ crate fn evaluate_goal<'a, 'tcx>( | |||
| GoalKind::DomainGoal(DomainGoal::WellFormed(WellFormed::Ty(ty))) | |||
| ), | |||
|
|
|||
| ty::Predicate::Subtype(predicate) => tcx.mk_goal( | |||
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Dec 17, 2018
Contributor
Why is this predicate arising exactly? Ah, hmm, I guess the librustc_typeck code requires it. Hmm. I feel like we may want to handle this by extending HhGoal to include a Subtype variant, alongside Unify, though I had always imagined though lowering the subtying rules to logical inference statements.
e.g.,
'a: 'b
T <: U
------
&'a T <: &'a U
One tricky bit though will be the handling of higher-ranked subtying like for<'a> fn(&T) <: ?U -- we presently infer ?U to fn(&T) in such a case, even though e.g. fn(&'static T) would be another option. I guess the "inference suggestions" feature could maybe handle this.
I admit I had kind of hoped to just sidestep this problem Subtype goals into the chalk system for now as a horrible hack or something.
This comment has been minimized.
This comment has been minimized.
|
|
||
| fn gimme<F: Foo>() { } | ||
|
|
||
| // Note: this also tests that `std::process::Termination` is implemented for `()`. |
This comment has been minimized.
This comment has been minimized.
| // The only type which implements `Foo` is `i32`, so the chalk trait solver | ||
| // is expecting a variable of type `i32`. This behavior differs from the | ||
| // old-style trait solver. I guess this will change, that's why I'm | ||
| // adding that test. |
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Dec 17, 2018
Contributor
Yeah, one of the interesting questions we do have to answer is how aggressively we want to be guessing types like this. Right now we are kind of "sometimes aggressive" -- never with Self, but yes with the other parameters. This is not exactly by design though... it just kind of came out that way due to various other constraints.
| // `Set<T>` is an input type of `take_a_set`, hence we know that | ||
| // `T` must implement `Hash`, and we know in turn that `T` must | ||
| // implement `Eq`. | ||
| only_eq::<T>() |
This comment has been minimized.
This comment has been minimized.
nikomatsakis
added
S-waiting-on-author
and removed
S-waiting-on-review
labels
Dec 19, 2018
This comment has been minimized.
This comment has been minimized.
|
Current status to my knowledge: Waiting on @scalexm to rebase over the new version of chalk, at least, and perhaps to answer a few more of the questions above. |
scalexm
added some commits
Nov 17, 2018
scalexm
force-pushed the
scalexm:chalk
branch
from
f1b0eb4
to
3926994
Dec 20, 2018
This comment was marked as resolved.
This comment was marked as resolved.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
scalexm
force-pushed the
scalexm:chalk
branch
from
3926994
to
9dab10d
Dec 20, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
scalexm commentedNov 30, 2018
•
edited
Final PR of what I believe to be a minimally working implementation of the new-style trait solver.
The new trait solver can be used by providing the
-Z chalkcommand line flag. It is currently used everywhere inrustc_typeck, and for everything relying onrustc::infer::canonical::query_response::enter_canonical_trait_query.The trait solver is invoked in rustc by using the
evaluate_goalcanonical query. This is not optimal because each call toevaluate_goalcreates a newchalk_engine::Forest, hence rustc cannot use answers to intermediate goals produced by the root goal. We'll need to change that but I guess that's ok for now.Some next steps, I think, are:
chalk_engine(as a side effect, types or trait references with outlive requirements cannot be proved well-formed)-Z chalkin order to leverage the lazy normalization strategy of the new-style trait solverSizedis supported currently)I added a few very simple tests to check that the new solver has the right behavior, they won't be needed anymore once it is mature enough. Additionally it shows off that we get implied bounds for free.
r? @nikomatsakis