Skip to content
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

Fix Iter exhaustion in prove_predicates when debug is on #50076

Merged
merged 1 commit into from
Apr 21, 2018

Conversation

spastorino
Copy link
Member

@@ -1534,16 +1534,17 @@ impl<'a, 'gcx, 'tcx> TypeChecker<'a, 'gcx, 'tcx> {
predicates: impl IntoIterator<Item = ty::Predicate<'tcx>>,
location: Location,
) {
let mut predicates_iter = predicates.into_iter();
let predicates_vec = predicates.into_iter().by_ref().collect::<Vec<_>>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yikes, this seems excessive. why not just duplicate the iterator?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that was the first thing I've tried but I can't ...

error[E0599]: no method named `clone` found for type `impl IntoIterator<Item = ty::Predicate<'tcx>>` in the current scope

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were talking with @pnkfelix to maybe do that thing only in the debug case and avoid the extra cost when debug is off. He said that it may not worth, I'm ok with changing this to whatever is better :).

@tamird
Copy link
Contributor

tamird commented Apr 19, 2018

In fact, I don't see why making this function take an iterator is better - I think just reverting 0e2e179 is the right thing here 🤷‍♂️

@spastorino
Copy link
Member Author

@tamird yeah we were originally discussing this with @nikomatsakis and agreed to move that to an iterator. I agree with you now though that reverting would be better.

@spastorino spastorino force-pushed the fix_exhaust_iter_in_debug branch 2 times, most recently from 5c05ba9 to 8dfa4b5 Compare April 19, 2018 14:46
@spastorino
Copy link
Member Author

Ok, changed the commit in this PR to a revert + a fix to another call to prove_predicates that arrived in another commit.

@tamird
Copy link
Contributor

tamird commented Apr 19, 2018

I have a slightly nicer patch, but I won't lie, it took some battles with the compiler. The TL;DR is that you want IntoIterator::IntoIter to be Clone:

diff --git a/src/librustc_mir/borrow_check/nll/type_check/mod.rs b/src/librustc_mir/borrow_check/nll/type_check/mod.rs
index acd246b703..e9c86ea99b 100644
--- a/src/librustc_mir/borrow_check/nll/type_check/mod.rs
+++ b/src/librustc_mir/borrow_check/nll/type_check/mod.rs
@@ -275,7 +275,7 @@ impl<'a, 'b, 'gcx, 'tcx> TypeVerifier<'a, 'b, 'gcx, 'tcx> {
                         tcx.predicates_of(def_id).instantiate(tcx, substs);
                     let predicates =
                         type_checker.normalize(&instantiated_predicates.predicates, location);
-                    type_checker.prove_predicates(predicates.iter().cloned(), location);
+                    type_checker.prove_predicates(predicates, location);
                 }
 
                 value.ty
@@ -1516,34 +1516,33 @@ impl<'a, 'gcx, 'tcx> TypeChecker<'a, 'gcx, 'tcx> {
 
         let predicates = self.normalize(&instantiated_predicates.predicates, location);
         debug!("prove_aggregate_predicates: predicates={:?}", predicates);
-        self.prove_predicates(predicates.iter().cloned(), location);
+        self.prove_predicates(predicates, location);
     }
 
     fn prove_trait_ref(&mut self, trait_ref: ty::TraitRef<'tcx>, location: Location) {
         self.prove_predicates(
-            [ty::Predicate::Trait(
+            Some(ty::Predicate::Trait(
                 trait_ref.to_poly_trait_ref().to_poly_trait_predicate(),
-            )].iter()
-                .cloned(),
+            )),
             location,
         );
     }
 
-    fn prove_predicates(
-        &mut self,
-        predicates: impl IntoIterator<Item = ty::Predicate<'tcx>>,
-        location: Location,
-    ) {
-        let mut predicates_iter = predicates.into_iter();
+    fn prove_predicates<T>(&mut self, predicates: T, location: Location)
+        where T: IntoIterator<Item = ty::Predicate<'tcx>>,
+              T::IntoIter: Clone,
+    {
+        let predicates = predicates.into_iter();
 
         debug!(
             "prove_predicates(predicates={:?}, location={:?})",
-            predicates_iter.by_ref().collect::<Vec<_>>(),
-            location
+            predicates.clone().collect::<Vec<_>>(),
+            location,
         );
         self.fully_perform_op(location.at_self(), |this| {
             let cause = this.misc(this.last_span);
-            let obligations = predicates_iter
+            let obligations = predicates
+                .into_iter()
                 .map(|p| traits::Obligation::new(cause.clone(), this.param_env, p))
                 .collect();
             Ok(InferOk {

Feel free to use this, or lmk and I'll open a new PR.

@spastorino
Copy link
Member Author

Very nice trick @tamird

@spastorino spastorino force-pushed the fix_exhaust_iter_in_debug branch 2 times, most recently from 4283cb0 to 3271608 Compare April 19, 2018 15:59
@spastorino
Copy link
Member Author

pushed again, thanks @tamird

fn prove_predicates<T>(&mut self, predicates: T, location: Location)
where
T: IntoIterator<Item = ty::Predicate<'tcx>>,
T::IntoIter: Iterator + Clone,
Copy link
Contributor

@tamird tamird Apr 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I updated my diff after posting; this can just be Clone; Iterator is implied by IntoIterator

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@spastorino spastorino force-pushed the fix_exhaust_iter_in_debug branch from 3271608 to 55054ee Compare April 19, 2018 16:12
@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 19, 2018
@rust-lang rust-lang deleted a comment from rust-highfive Apr 19, 2018
}

fn prove_trait_ref(&mut self, trait_ref: ty::TraitRef<'tcx>, location: Location) {
self.prove_predicates(
[ty::Predicate::Trait(
Some(ty::Predicate::Trait(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI changing this to Option can be reverted when/if #49000 is merged.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@spastorino spastorino changed the title Fix Iter exhaustion in when debug! is called Fix Iter exhaustion in prove_predicates when debug is on Apr 19, 2018
@pnkfelix
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 20, 2018

📌 Commit 55054ee has been approved by pnkfelix

@bors 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 20, 2018
@bors
Copy link
Contributor

bors commented Apr 20, 2018

⌛ Testing commit 55054ee with merge b12e96f3cc3875b0ec5cc183b5fbd5fc9981330f...

@bors
Copy link
Contributor

bors commented Apr 20, 2018

💔 Test failed - status-appveyor

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

tamird commented Apr 20, 2018

Looks like a spurious linking failure.

@kennytm
Copy link
Member

kennytm commented Apr 21, 2018

@bors retry #50140

@bors 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 21, 2018
@bors
Copy link
Contributor

bors commented Apr 21, 2018

⌛ Testing commit 55054ee with merge e59f78f...

bors added a commit that referenced this pull request Apr 21, 2018
Fix Iter exhaustion in prove_predicates when debug is on

Fixes the issue noted in this comment https://github.com/rust-lang/rust/pull/49885/files#r182560268

r? @pnkfelix
/cc @nikomatsakis
@bors
Copy link
Contributor

bors commented Apr 21, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: pnkfelix
Pushing e59f78f to master...

@bors bors merged commit 55054ee into rust-lang:master Apr 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants