-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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 chalk unification routines #56214
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything seems fine except for the question of unifying two existential inference variables...
//! - This code uses "universes" to handle higher-ranked regions and | ||
//! not the leak-check. This is "more correct" than what rustc does | ||
//! and we are generally migrating in this direction, but NLL had to | ||
//! get there first. | ||
//! | ||
//! Also, this code assumes that there are no bound type vars at all, not even | ||
//! free ones. This is ok because: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure what this means. For example, we might have for<'a> fn(&'a u32)
, right? That's a bound variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no "bound type vars", so e.g. no for<T> ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I replaced with "no bound types at all" (I dropped "vars")
@@ -340,7 +453,8 @@ where | |||
a, b, self.ambient_variance | |||
); | |||
|
|||
relate::super_relate_tys(self, a, b) | |||
// Will also handle unification of `IntVar` and `FloatVar`. | |||
self.infcx.super_combine_tys(self, a, b) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting. I guess that's ok. feels creepy to go into the "other" relate code but...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking the same, but basically super_combine_tys
just unifies int/float vars and calls super_relate_tys
so...
src/librustc/infer/nll_relate/mod.rs
Outdated
self.universe, | ||
universe | ||
); | ||
return Err(TypeError::Mismatch); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This surprises me. I don't think it is correct. In particular, unifying two unbound existential variables is infallible: it results in a unified variable whose universe is the minimum of the two universes, no? Am I misreading this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes that's definitely a mistake
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
`make_solution` does not return any guidance for now
@bors r+ |
📌 Commit 1fce415 has been approved by |
@bors p=1 |
⌛ Testing commit 1fce415 with merge 3dbe6bae99ecd6a2e6dd7a84ac603332d8ed678b... |
💔 Test failed - status-travis |
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 |
@bors retry |
⌛ Testing commit 1fce415 with merge 814805b8941fbf8ea2258a6b1c28c57a87fa00e9... |
💔 Test failed - status-travis |
@bors retry |
⌛ Testing commit 1fce415 with merge af2a8b786c99b3ec5e117c751ee95209c1334ef7... |
💔 Test failed - status-appveyor |
@bors retry
It seems like this PR will never land! |
⌛ Testing commit 1fce415 with merge 58d5ad432943e55fd60c1c548be00a5c625e70f9... |
⌛ Testing commit 1fce415 with merge 8f1e94578a06f2c896a13d760c04904f70f881e3... |
@bors p=0 |
💔 Test failed - status-appveyor |
Same failure as in |
@bors retry |
Implement chalk unification routines `ResolventOps` and `AggregateOps` are mostly straightforwardly translated from chalk. I had caught a few bugs already in my `chalk` branch and backported fixes to this branch, but there may be other ones left. EDIT: I hope there are none left now :) Fixes rust-lang#54935.
Rollup of 19 pull requests Successful merges: - #55011 (Add libstd Cargo feature "panic_immediate_abort") - #55821 (Use sort_by_cached_key when the key function is not trivial/free) - #56014 (add test for issue #21335) - #56131 (Assorted tweaks) - #56214 (Implement chalk unification routines) - #56216 (Add TryFrom<&[T]> for [T; $N] where T: Copy) - #56268 (Reuse the `P` in `InvocationCollector::fold_{,opt_}expr`.) - #56324 (Use raw_entry for more efficient interning) - #56336 (Clean up and streamline the pretty-printer) - #56337 (Fix const_fn ICE with non-const function pointer) - #56339 (Remove not used option) - #56341 (Rename conversion util; remove duplicate util in librustc_codegen_llvm.) - #56349 (rustc 1.30.0's linker flavor inference is a non-backwards compat change to -Clinker) - #56355 (Add inline attributes and add unit to CommonTypes) - #56360 (Optimize local linkchecker program) - #56364 (Fix panic with outlives in existential type) - #56365 (Stabilize self_struct_ctor feature.) - #56367 (Moved some feature gate tests to correct location) - #56373 (Update books)
ResolventOps
andAggregateOps
are mostly straightforwardly translated from chalk. I had caught a few bugs already in mychalk
branch and backported fixes to this branch, but there may be other ones left. EDIT: I hope there are none left now :)Fixes #54935.
r? @nikomatsakis