Create a canonical trait query for `evaluate_obligation` #48995

aravind-pg · 2018-03-13T21:44:45Z

This builds on the canonical query machinery introduced in #48411 to introduce a new canonical trait query for evaluate_obligation in the trait selector. Also ports most callers of the original evaluate_obligation to the new system (except in coherence, which requires support for intercrate mode). Closes #48537.

r? @nikomatsakis

rust-highfive · 2018-03-13T21:44:57Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

aravind-pg · 2018-03-13T21:47:57Z

src/librustc/traits/select.rs

@@ -617,6 +625,17 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
        })
    }

+    /// Evaluates whether the obligation `obligation` can be satisfied and returns
+    /// an `EvaluationResult`.
+    pub fn evaluate_obligation_recursively(&mut self,


Not sure if exposing this new API is a good thing to have done, but it was the cleanest way without also exposing TraitObligationStackList (which is even more of an implementation detail).

Seems OK -- I might prefer to remove evaluate_obligation_conservatively and evaluation_obligation in favor of just offering this, though. Or at least have them invoke it.

Good point. The only caller of the originals now is in coherence, so I'll just switch that to use this and remove both the originals. And rename this to just evaluate_obligation to boot.

nikomatsakis · 2018-03-14T18:10:08Z

Hmm, travis is unhappy

aravind-pg · 2018-03-15T01:45:40Z

The Travis error is unrelated, hopefully it'll go away on a retry:

[00:52:40]  Documenting core v0.0.0 (file:///checkout/src/libcore)
[00:52:40]    Compiling core v0.0.0 (file:///checkout/src/libcore)
[00:52:52] error: internal compiler error: librustc/ich/impls_ty.rs:907: ty::TypeVariants::hash_stable() - Unexpected variant TyInfer(?0).
[00:52:52] 
[00:52:52] thread 'rustc' panicked at 'Box<Any>', librustc_errors/lib.rs:538:9

Edit: was wrong, see below

bors · 2018-03-15T02:24:17Z

☔ The latest upstream changes (presumably #47630) made this pull request unmergeable. Please resolve the merge conflicts.

aravind-pg · 2018-03-15T04:21:29Z

Heads up: this breaks clippy, but the following diff should fix it once this lands (haven't tested though). cc @Manishearth, @llogiq

--- a/clippy_lints/src/utils/mod.rs
+++ b/clippy_lints/src/utils/mod.rs
@@ -306,11 +306,10 @@ pub fn implements_trait<'a, 'tcx>(
     ty_params: &[Ty<'tcx>],
 ) -> bool {
     let ty = cx.tcx.erase_regions(&ty);
-    let obligation =
-        cx.tcx
-            .predicate_for_trait_def(cx.param_env, traits::ObligationCause::dummy(), trait_id, 0, ty, ty_params);
+    let trait_ref = ty::TraitRef::new(trait_id,
+                                      cx.tcx.mk_substs_trait(ty, ty_params);
     cx.tcx.infer_ctxt().enter(|infcx| {
-        traits::SelectionContext::new(&infcx).evaluate_obligation_conservatively(&obligation)
+        infcx.predicate_must_hold(cx.param_env, trait_ref.to_predicate())
     })
 }

Manishearth · 2018-03-15T04:50:35Z

Thanks for providing the diff! I'm not going to be able to fix it yet but someone else will probably.

…

On Mar 15, 2018 12:21 AM, "Aravind Gollakota" ***@***.***> wrote: Heads up: this breaks clippy, but the following diff should fix it (haven't tested though). cc @Manishearth <https://github.com/manishearth>, @llogiq <https://github.com/llogiq> --- a/clippy_lints/src/utils/mod.rs+++ b/clippy_lints/src/utils/mod.rs@@ -306,11 +306,10 @@ pub fn implements_trait<'a, 'tcx>( ty_params: &[Ty<'tcx>], ) -> bool { let ty = cx.tcx.erase_regions(&ty);- let obligation =- cx.tcx- .predicate_for_trait_def(cx.param_env, traits::ObligationCause::dummy(), trait_id, 0, ty, ty_params);+ let trait_ref = ty::TraitRef::new(trait_id,+ cx.tcx.mk_substs_trait(ty, ty_params); cx.tcx.infer_ctxt().enter(|infcx| {- traits::SelectionContext::new(&infcx).evaluate_obligation_conservatively(&obligation)+ infcx.predicate_must_hold(cx.param_env, trait_ref.to_predicate()) }) } — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#48995 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABivSD428mWUo9c59ZZkJjTQvLp3I5Hvks5teexlgaJpZM4SpgHP> .

aravind-pg · 2018-03-15T05:53:15Z

Ah so the Travis error isn't spurious. It looks like somehow I've introduced a codepath that causes incr.comp. to attempt (and fail) to hash type inference variables, i.e. we're hitting

rust/src/librustc/ich/impls_ty.rs

Lines 892 to 894 in ae379bd

    
           TyInfer(..) => { 
        
               bug!("ty::TypeVariants::hash_stable() - Unexpected variant {:?}.", *self) 
        
           }

Unfortunately won't have time to debug this properly right now since I'm away the next 2ish days. @nikomatsakis, any pointers would be very helpful for when I get back!

llogiq · 2018-03-15T06:10:05Z

I may look into it in a few hours.

nikomatsakis · 2018-03-15T16:21:24Z

@aravind-pg very curious. Might be a bug in the canonicalizer.

bors · 2018-03-18T10:09:03Z

☔ The latest upstream changes (presumably #48985) made this pull request unmergeable. Please resolve the merge conflicts.

aravind-pg · 2018-03-19T09:53:06Z

Rebased to pick up #49091.

nikomatsakis · 2018-03-19T19:43:11Z

src/librustc/ty/maps/config.rs

@@ -73,6 +73,12 @@ impl<'tcx> QueryDescription<'tcx> for queries::normalize_ty_after_erasing_region
    }
 }

+impl<'tcx> QueryDescription<'tcx> for queries::evaluate_obligation<'tcx> {
+    fn describe(_tcx: TyCtxt, goal: CanonicalPredicateGoal<'tcx>) -> String {
+        format!("evaluating trait selection obligation `{:?}`", goal)


So the failing test impl-trait/auto-trait-leak.rs is now including this message in its output. In general, {:?} should be avoided here, as this a message that may be shown to the user. That said, nicely formatting this message could be kind of annoying. I guess something like this might be good enough for now:

format!("evaluating trait selection obligation `{}`", goal.value.value)

I'm trying this locally. This will just skip over the canonical binders and the param-env.

nikomatsakis · 2018-03-19T21:04:15Z

I pushed a commit that should fix the travis errors for now, but one of the fixes is not something I am yet comfortable landing (hence the update on the PR title to prevent accidental landing).

I just wanted to see if there are further errors, and measure perf impact.

nikomatsakis · 2018-03-20T13:57:11Z

@bors try

bors · 2018-03-20T13:57:21Z

⌛ Trying commit f37a1d2 with merge 0fdc38f...

@nikomatsakis

[WIP] Create a canonical trait query for `evaluate_obligation` This builds on the canonical query machinery introduced in #48411 to introduce a new canonical trait query for `evaluate_obligation` in the trait selector. Also ports most callers of the original `evaluate_obligation` to the new system (except in coherence, which requires support for intercrate mode). Closes #48537. r? @nikomatsakis

bors · 2018-03-20T14:12:09Z

💔 Test failed - status-travis

aravind-pg · 2018-04-23T01:07:52Z

I implemented the query mode idea and our error messages are back in good shape -- modulo this slightly unfortunate situation. Overall I think the trait selection error reporting setup has definitely gotten a little hairier, sadly, but it's the best we could think of. Later refactoring might be able to clean things up. Anyhow, ready for a re-review.

bors · 2018-04-24T10:51:18Z

☔ The latest upstream changes (presumably #49837) made this pull request unmergeable. Please resolve the merge conflicts.

nikomatsakis · 2018-04-26T21:11:28Z

r=me post rebase

@bors delegate=aravind-pg

bors · 2018-04-26T21:11:29Z

✌️ @aravind-pg can now approve this pull request

nikomatsakis · 2018-04-26T21:11:41Z

@sgrif didn't seem too terrified of the error regression. =) And it's hopefully a corner case.

sgrif · 2018-04-26T21:17:14Z

Oh I'm sure I have code that hits it

…stead of aborting eagerly We store the obligation that caused the overflow as part of the OverflowError, and report it at the public API endpoints (rather than in the implementation internals).

…rait query Except the one in coherence, which needs support for intercrate mode.

…policy in traits::select

We will shortly refactor things so that it is no longer needed

This is slightly hacky and hopefully only a somewhat temporary solution.

aravind-pg · 2018-04-27T01:31:21Z

@bors r=nikomatsakis

Let's land for now, and if any annoyances come up then we can gate this setup with -Zchalk later.

bors · 2018-04-27T01:31:21Z

📌 Commit e423dcc has been approved by nikomatsakis

bors · 2018-04-27T03:53:46Z

⌛ Testing commit e423dcc with merge 8a09bc6...

@nikomatsakis

Create a canonical trait query for `evaluate_obligation` This builds on the canonical query machinery introduced in #48411 to introduce a new canonical trait query for `evaluate_obligation` in the trait selector. Also ports most callers of the original `evaluate_obligation` to the new system (except in coherence, which requires support for intercrate mode). Closes #48537. r? @nikomatsakis

bors · 2018-04-27T06:30:19Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 8a09bc6 to master...

Due to rust-lang/rust#48995 and rust-lang/rust#49894

rust-highfive assigned nikomatsakis Mar 13, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 13, 2018

aravind-pg commented Mar 13, 2018

View reviewed changes

aravind-pg force-pushed the canonical-query branch from cbd3600 to 9b9c44e Compare March 15, 2018 01:47

aravind-pg force-pushed the canonical-query branch 3 times, most recently from 3da95d7 to 87ce396 Compare March 15, 2018 03:54

kennytm mentioned this pull request Mar 15, 2018

Compiler problem after upgrading the Nightly channel to 1.26.0-nightly (521d91c6b 2018-03-14), on x86_64-apple-darwin #49043

Closed

aravind-pg force-pushed the canonical-query branch from 87ce396 to 3939fba Compare March 19, 2018 09:51

nikomatsakis force-pushed the canonical-query branch from 3939fba to 2ee933d Compare March 19, 2018 12:47

nikomatsakis reviewed Mar 19, 2018

View reviewed changes

nikomatsakis changed the title ~~Create a canonical trait query for evaluate_obligation~~ [WIP] Create a canonical trait query for evaluate_obligation Mar 19, 2018

nikomatsakis force-pushed the canonical-query branch 2 times, most recently from 4f250a6 to f37a1d2 Compare March 19, 2018 22:19

aravind-pg changed the title ~~[WIP] Create a canonical trait query for evaluate_obligation~~ Create a canonical trait query for evaluate_obligation Apr 23, 2018

aravind-pg added 8 commits April 26, 2018 20:26

Refactor overflow handling in traits::select to propagate overflow in…

79f71f9

…stead of aborting eagerly We store the obligation that caused the overflow as part of the OverflowError, and report it at the public API endpoints (rather than in the implementation internals).

Create a canonical trait query for evaluate_obligation

3ab3a9f

Port existing callers of evaluate_obligation to the new canonical t…

bc16b8e

…rait query Except the one in coherence, which needs support for intercrate mode.

Simplify trait selector's evaluation API slightly

3dd26b8

Introduce trait query mode and use it to set overflow error handling …

e5535fc

…policy in traits::select

Remove the stored obligation in OverflowError to simplify things

5cb0372

We will shortly refactor things so that it is no longer needed

Retry canonical trait query in standard mode if overflow occurs

d5b2e90

This is slightly hacky and hopefully only a somewhat temporary solution.

Update a compile-fail test

e423dcc

aravind-pg force-pushed the canonical-query branch from 752e831 to e423dcc Compare April 27, 2018 01:28

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 27, 2018

bors merged commit e423dcc into rust-lang:master Apr 27, 2018

aravind-pg deleted the canonical-query branch April 27, 2018 06:52

phansch added a commit to phansch/rust-clippy that referenced this pull request Apr 28, 2018

Rustup to latest nightly

1712e18

Due to rust-lang/rust#48995 and rust-lang/rust#49894

phansch mentioned this pull request Apr 28, 2018

Rustup to latest nightly rust-lang/rust-clippy#2703

Merged

This was referenced Apr 16, 2019

cargo check fails only in incremental mode #58291

Closed

april 2018 trait resolution regression #60010

Closed

lqd mentioned this pull request Jul 17, 2019

ICE: Generic type alias to invalid type crashes during type check on latest stable #62742

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a canonical trait query for `evaluate_obligation` #48995

Create a canonical trait query for `evaluate_obligation` #48995

aravind-pg commented Mar 13, 2018

rust-highfive commented Mar 13, 2018

aravind-pg Mar 13, 2018

nikomatsakis Mar 14, 2018

aravind-pg Mar 15, 2018

nikomatsakis commented Mar 14, 2018

aravind-pg commented Mar 15, 2018 •

edited

Loading

bors commented Mar 15, 2018

aravind-pg commented Mar 15, 2018 •

edited

Loading

Manishearth commented Mar 15, 2018 via email

aravind-pg commented Mar 15, 2018

llogiq commented Mar 15, 2018

nikomatsakis commented Mar 15, 2018

bors commented Mar 18, 2018

aravind-pg commented Mar 19, 2018

nikomatsakis Mar 19, 2018

nikomatsakis commented Mar 19, 2018

nikomatsakis commented Mar 20, 2018

bors commented Mar 20, 2018

bors commented Mar 20, 2018

aravind-pg commented Apr 23, 2018

bors commented Apr 24, 2018

nikomatsakis commented Apr 26, 2018

bors commented Apr 26, 2018

nikomatsakis commented Apr 26, 2018

sgrif commented Apr 26, 2018

aravind-pg commented Apr 27, 2018

bors commented Apr 27, 2018

bors commented Apr 27, 2018

bors commented Apr 27, 2018

Create a canonical trait query for evaluate_obligation #48995

Create a canonical trait query for evaluate_obligation #48995

Conversation

aravind-pg commented Mar 13, 2018

rust-highfive commented Mar 13, 2018

aravind-pg Mar 13, 2018

Choose a reason for hiding this comment

nikomatsakis Mar 14, 2018

Choose a reason for hiding this comment

aravind-pg Mar 15, 2018

Choose a reason for hiding this comment

nikomatsakis commented Mar 14, 2018

aravind-pg commented Mar 15, 2018 • edited Loading

bors commented Mar 15, 2018

aravind-pg commented Mar 15, 2018 • edited Loading

Manishearth commented Mar 15, 2018 via email

aravind-pg commented Mar 15, 2018

llogiq commented Mar 15, 2018

nikomatsakis commented Mar 15, 2018

bors commented Mar 18, 2018

aravind-pg commented Mar 19, 2018

nikomatsakis Mar 19, 2018

Choose a reason for hiding this comment

nikomatsakis commented Mar 19, 2018

nikomatsakis commented Mar 20, 2018

bors commented Mar 20, 2018

bors commented Mar 20, 2018

aravind-pg commented Apr 23, 2018

bors commented Apr 24, 2018

nikomatsakis commented Apr 26, 2018

bors commented Apr 26, 2018

nikomatsakis commented Apr 26, 2018

sgrif commented Apr 26, 2018

aravind-pg commented Apr 27, 2018

bors commented Apr 27, 2018

bors commented Apr 27, 2018

bors commented Apr 27, 2018

Create a canonical trait query for `evaluate_obligation` #48995

Create a canonical trait query for `evaluate_obligation` #48995

aravind-pg commented Mar 15, 2018 •

edited

Loading

aravind-pg commented Mar 15, 2018 •

edited

Loading