-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
move leak-check to during coherence, candidate eval #72493
Commits on Jun 22, 2020
-
modify leak-check to track only outgoing edges from placeholders
Also, update the affected tests. This seems strictly better but it is actually more permissive than I initially intended. In particular it accepts this ``` forall<'a, 'b> { exists<'intersection> { 'a: 'intersection, 'b: 'intersection, } } ``` and I'm not sure I want to accept that. It implies that we have a `'empty` in the new universe intoduced by the `forall`.
Configuration menu - View commit details
-
Copy full SHA for bcc0a9c - Browse repository at this point
Copy the full SHA bcc0a9cView commit details -
Revert "modify leak-check to track only outgoing edges from placehold…
…ers" This reverts commit 2e01db4b396a1e161f7a73933fff34bc9421dba0.
Configuration menu - View commit details
-
Copy full SHA for 4199b3a - Browse repository at this point
Copy the full SHA 4199b3aView commit details -
rewrite leak check to be based on universes
In the new leak check, instead of getting a list of placeholders to track, we look for any placeholder that is part of a universe which was created during the snapshot. We are looking for the following error patterns: * P1: P2, where P1 != P2 * P1: R, where R is in some universe that cannot name P1 This new leak check is more precise than before, in that it accepts this patterns: * R: P1, even if R cannot name P1, because R = 'static is a valid sol'n * R: P1, R: P2, as above Note that this leak check, when running during subtyping, is less efficient than before in some sense because it is going to check and re-check all the universes created since the snapshot. We're going to move when the leak check runs to try and correct that.
Configuration menu - View commit details
-
Copy full SHA for f2cf994 - Browse repository at this point
Copy the full SHA f2cf994View commit details -
move leak-check to during coherence, candidate eval
In particular, it no longer occurs during the subtyping check. This is important for enabling lazy normalization, because the subtyping check will be producing sub-obligations that could affect its results. Consider an example like for<'a> fn(<&'a as Mirror>::Item) = fn(&'b u8) where `<T as Mirror>::Item = T` for all `T`. We will wish to produce a new subobligation like <'!1 as Mirror>::Item = &'b u8 This will, after being solved, ultimately yield a constraint that `'!1 = 'b` which will fail. But with the leak-check being performed on subtyping, there is no opportunity to normalize `<'!1 as Mirror>::Item` (unless we invoke that normalization directly from within subtyping, and I would prefer that subtyping and unification are distinct operations rather than part of the trait solving stack). The reason to keep the leak check during coherence and trait evaluation is partly for backwards compatibility. The coherence change permits impls for `fn(T)` and `fn(&T)` to co-exist, and the trait evaluation change means that we can distinguish those two cases without ambiguity errors. It also avoids recreating rust-lang#57639, where we were incorrectly choosing a where clause that would have failed the leak check over the impl which succeeds. The other reason to keep the leak check in those places is that I think it is actually close to the model we want. To the point, I think the trait solver ought to have the job of "breaking down" higher-ranked region obligation like ``!1: '2` into into region obligations that operate on things in the root universe, at which point they should be handed off to polonius. The leak check isn't *really* doing that -- these obligations are still handed to the region solver to process -- but if/when we do adopt that model, the decision to pass/fail would be happening in roughly this part of the code. This change had somewhat more side-effects than I anticipated. It seems like there are cases where the leak-check was not being enforced during method proving and trait selection. I haven't quite tracked this down but I think it ought to be documented, so that we know what precisely we are committing to. One surprising test was `issue-30786.rs`. The behavior there seems a bit "fishy" to me, but the problem is not related to the leak check change as far as I can tell, but more to do with the closure signature inference code and perhaps the associated type projection, which together seem to be conspiring to produce an unexpected signature. Nonetheless, it is an example of where changing the leak-check can have some unexpected consequences: we're now failing to resolve a method earlier than we were, which suggests we might change some method resolutions that would have been ambiguous to be successful. TODO: * figure out remainig test failures * add new coherence tests for the patterns we ARE disallowing
Configuration menu - View commit details
-
Copy full SHA for 5a7a850 - Browse repository at this point
Copy the full SHA 5a7a850View commit details -
upcasting traits requires only that things become more general
Revert the code that states that upcasting traits requires full equality and change to require that the source type is a subtype of the target type, as one would expect. As the comment states, this was an old bug that we didn't want to fix yet as it interacted poorly with the old leak-check. This fixes the old-lub-glb-object test, which was previously reporting too many errors (i.e., in the previous commit).
Configuration menu - View commit details
-
Copy full SHA for 1e00e1b - Browse repository at this point
Copy the full SHA 1e00e1bView commit details -
remove snapshot calls from "match" operations during select
Motivation: - we want to use leak-check sparingly, first off - these calls were essentially the same as doing the check during subtyping
Configuration menu - View commit details
-
Copy full SHA for 70cf33f - Browse repository at this point
Copy the full SHA 70cf33fView commit details -
Configuration menu - View commit details
-
Copy full SHA for 6873a76 - Browse repository at this point
Copy the full SHA 6873a76View commit details -
Configuration menu - View commit details
-
Copy full SHA for 3a68d56 - Browse repository at this point
Copy the full SHA 3a68d56View commit details -
Configuration menu - View commit details
-
Copy full SHA for be0d10f - Browse repository at this point
Copy the full SHA be0d10fView commit details -
Configuration menu - View commit details
-
Copy full SHA for 93e2982 - Browse repository at this point
Copy the full SHA 93e2982View commit details -
Configuration menu - View commit details
-
Copy full SHA for c88a76e - Browse repository at this point
Copy the full SHA c88a76eView commit details -
fix subtle bug in NLL type checker
The bug was revealed by the behavior of the old-lub-glb-hr-noteq1.rs test. The old-lub-glb-hr-noteq2 test shows the current 'order dependent' behavior of coercions around higher-ranked functions, at least when running with `-Zborrowck=mir`. Also, run compare-mode=nll.
Configuration menu - View commit details
-
Copy full SHA for 6929013 - Browse repository at this point
Copy the full SHA 6929013View commit details -
Configuration menu - View commit details
-
Copy full SHA for d57689f - Browse repository at this point
Copy the full SHA d57689fView commit details