refactor autoderef to avoid prematurely registering obligations #33852

arielb1 · May 24, 2016

Refactor FnCtxt::autoderef to use an external iterator and to not
register any obligation from the main autoderef loop, but rather to
register them after (and if) the loop successfully completes.

Fixes #24819
Fixes #25801
Fixes #27631
Fixes #31258
Fixes #31964
Fixes #32320
Fixes #33515
Fixes #33755

r? @eddyb

eddyb · May 24, 2016

eddyb reviewed May 24, 2016

View changes

eddyb · May 24, 2016

src/librustc_typeck/check/callee.rs

+        let mut autoderef = self.autoderef(callee_expr.span, original_callee_ty);
+        let result = autoderef.by_ref().flat_map(|(adj_ty, idx)| {
+            self.try_overloaded_call_step(call_expr, callee_expr, adj_ty, idx)
+        }).next();


Very nice pattern, I like it!

eddyb · May 24, 2016

eddyb reviewed May 24, 2016

View changes

eddyb · May 24, 2016

src/librustc_typeck/check/method/suggest.rs

-                None
-            }
-        }).2.is_some()
+        self.autoderef(span, rcvr_ty).any(|(ty, _)| is_local(ty))


Another great cleanup 😄.

eddyb · May 24, 2016

eddyb reviewed May 24, 2016

View changes

eddyb · May 24, 2016

src/test/compile-fail/borrowck/borrowck-borrow-overloaded-auto-deref-mut.rs

@@ -99,7 +99,7 @@ fn assign_field1<'a>(x: Own<Point>) {
 }

 fn assign_field2<'a>(x: &'a Own<Point>) {
-    x.y = 3; //~ ERROR cannot assign
+    x.y = 3; //~ ERROR cannot borrow


Is this change a bug-fix?
It seems to typeck as (*DerefMut::deref_mut(&mut *x)).y = 3; where previously it was (*Deref::deref(&*x)).y = 3;.

Both cases are fine. The special casing that caused Deref::deref to be chosen is just plain unnecessary.

eddyb · May 24, 2016

eddyb reviewed May 24, 2016

View changes

eddyb · May 24, 2016

src/librustc_typeck/check/coercion.rs

            None => {
                let err = first_error.expect("coerce_borrowed_pointer had no error");
                debug!("coerce_borrowed_pointer: failed with err = {:?}", err);
                return Err(err);
            }
        };

+        autoderef.finalize(LvaluePreference::from_mutbl(mt_b.mutbl), exprs());


Could use a comment here explaining that this is only correct because nothing can later fail, i.e. we're guaranteed a successful coercion at this point.

eddyb · May 24, 2016

eddyb reviewed May 24, 2016

View changes

eddyb · May 24, 2016

src/librustc_typeck/check/autoderef.rs

+        }
+
+        for obligation in self.obligations {
+            self.fcx.register_predicate(obligation);


Would it be possible to make rolling back a snapshot in which obligations have been registered, into an ICE?
To prevent future misuse, I mean, you seem to have covered the existing problems.

eddyb · May 24, 2016

#27631 is already closed btw, should it be in the description?

I've noticed an observed change in behavior (picking mutable over immutable on field assignment, even through an immutable reference), and there's also the question of potentially hardening the system we have so obscure index out of bounds errors do not happen in the future.

But other than that, it looks like a very nice refactoring, r=me with nits/questions addressed.

arielb1 · May 25, 2016

updated

eddyb · May 25, 2016

@bors r+

bors · May 25, 2016

📌 Commit 040fc94 has been approved by eddyb

nikomatsakis · May 26, 2016

cc me

nikomatsakis · May 26, 2016

nikomatsakis reviewed May 26, 2016

View changes

nikomatsakis · May 26, 2016

src/librustc_typeck/check/method/confirm.rs

@@ -463,24 +448,10 @@ impl<'a, 'gcx, 'tcx> ConfirmContext<'a, 'gcx, 'tcx> {
    ///////////////////////////////////////////////////////////////////////////
    // RECONCILIATION

-    /// When we select a method with an `&mut self` receiver, we have to go convert any
+    /// When we select a method with a mutable autoref, we have to go convert any


Nit: I think this comment is not as good as the one before, even if it is potentially more precise (at least in the future...). I always like to see concrete Rust code examples, particularly as our terminology tends to drift over time. :) "a mutable autoref (e.g., &mut self)" would be better, for example.

Edit: tweaked wording.

nikomatsakis · May 26, 2016

nikomatsakis reviewed May 26, 2016

View changes

nikomatsakis · May 26, 2016

src/librustc_typeck/check/method/confirm.rs

-        // e.g. `Deref` to `DerefMut` in overloaded derefs and so on).
-        self.fixup_derefs_on_method_receiver_if_necessary(&callee);
+
+        if let Some(hir::MutMutable) = pick.autoref {


Nit: Sad to lose the comment here. I realize it overlaps with convert_lvalue_derefs_to_mutable, but still seems like it will it easier for readers to follow.

nikomatsakis · May 26, 2016

nikomatsakis reviewed May 26, 2016

View changes

nikomatsakis · May 26, 2016

src/librustc_typeck/check/method/confirm.rs

-        match result {
-            Some(r) => r,
-            None => {
+        // FIXME: this feels, like, super dubious


Nit: This comment is not particularly specific... it'd be good to elaborate what you think is dubious about it :)

nikomatsakis · May 26, 2016

(The nits I noted aren't worth stopping bors, btw.)

nikomatsakis · May 26, 2016

So, just to be sure I'm following along: the ICEs here were caused by obligations that were being registered by the autoderef code, which were then in the fulfillcx but referencing type variables added in the snapshot -- thus when we rolled back, the variables leaked out, right? Hence this fixes it by just not registering any obligations during the snapshot.

This seems good -- and in general I am trying to move us more towards returning obligations back up the stack vs registering them eagerly -- though I do wonder if we will encounter problems at some point where e.g. the type that we Deref to is an unresolved type variable, and we would have to process some obligations to figure out what it is, but because those obligations are not in the fufilllment context, we fail to make progress.

arielb1 · May 28, 2016

Beta-nominating for being a very annoying ICE.

nikomatsakis · Jun 2, 2016

In @rust-lang/compiler meeting, decided not to backport:

non-trivial refactoring
long-standing problem, not a regression afaik
most if not all of the cases where this occur would have been errors anyway

rust-highfive assigned eddyb May 24, 2016

arielb1 changed the title ~~refactor autoderef to avoid registering obligations~~ refactor autoderef to avoid prematurely registering obligations May 24, 2016

catch attempts to leak obligations out of snapshots

Loading status checks…

040fc94

eddyb referenced this pull request May 27, 2016
Closed
Type Checker getting confused with constraints + ICE #33904

Manishearth referenced this pull request May 28, 2016
Merged
Rollup of 15 pull requests #33927

bors merged commit 040fc94 into rust-lang:master May 28, 2016
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

arielb1 added the beta-nominated label May 28, 2016

nikomatsakis removed the beta-nominated label Jun 2, 2016

TimNN referenced this pull request Jun 30, 2016
Closed
thread 'rustc' panicked at 'index out of bounds: the len is 176 but the index is 178', src/libcollections/vec.rs:1166 #34568

TimNN referenced this pull request Jul 14, 2016
Closed
Unexpected compiler panic with a static Vec #34823

TimNN referenced this pull request Jul 25, 2016
Closed
Panic on a simple type error #35024

eddyb referenced this pull request Apr 15, 2017
Merged
Ban registering obligations during InferCtxt snapshots. #41325

rust-lang/rust

Join GitHub today

refactor autoderef to avoid prematurely registering obligations #33852

Conversation

arielb1 commented May 24, 2016

rust-highfive assigned eddyb May 24, 2016

arielb1 changed the title refactor autoderef to avoid registering obligations refactor autoderef to avoid prematurely registering obligations May 24, 2016

eddyb reviewed May 24, 2016 View changes

This comment has been minimized.

eddyb reviewed May 24, 2016 View changes

This comment has been minimized.

eddyb reviewed May 24, 2016 View changes

This comment has been minimized.

This comment has been minimized.

eddyb reviewed May 24, 2016 View changes

This comment has been minimized.

eddyb reviewed May 24, 2016 View changes

This comment has been minimized.

This comment has been minimized.

eddyb commented May 24, 2016

This comment has been minimized.

arielb1 commented May 25, 2016

This comment has been minimized.

eddyb commented May 25, 2016

This comment has been minimized.

bors commented May 25, 2016

This comment has been minimized.

nikomatsakis commented May 26, 2016

nikomatsakis reviewed May 26, 2016 View changes

This comment has been minimized.

nikomatsakis May 26, 2016 • edited

nikomatsakis reviewed May 26, 2016 View changes

This comment has been minimized.

nikomatsakis reviewed May 26, 2016 View changes

This comment has been minimized.

This comment has been minimized.

nikomatsakis commented May 26, 2016

This comment has been minimized.

nikomatsakis commented May 26, 2016

eddyb referenced this pull request May 27, 2016

Manishearth added a commit to Manishearth/rust that referenced this pull request May 28, 2016

Manishearth referenced this pull request May 28, 2016

bors added a commit that referenced this pull request May 28, 2016

Manishearth added a commit to Manishearth/rust that referenced this pull request May 28, 2016

bors added a commit that referenced this pull request May 28, 2016

Hide details View details bors merged commit 040fc94 into rust-lang:master May 28, 2016 1 check passed

1 check passed

arielb1 added the beta-nominated label May 28, 2016

This comment has been minimized.

arielb1 commented May 28, 2016

nikomatsakis removed the beta-nominated label Jun 2, 2016

This comment has been minimized.

nikomatsakis commented Jun 2, 2016

TimNN referenced this pull request Jun 30, 2016

TimNN referenced this pull request Jul 14, 2016

TimNN referenced this pull request Jul 25, 2016

eddyb referenced this pull request Apr 15, 2017

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 18, 2017

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 18, 2017

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 18, 2017

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 18, 2017

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 19, 2017

bors added a commit that referenced this pull request Apr 19, 2017

eddyb reviewed May 24, 2016

View changes

eddyb reviewed May 24, 2016

View changes

eddyb reviewed May 24, 2016

View changes

eddyb reviewed May 24, 2016

View changes

eddyb reviewed May 24, 2016

View changes

nikomatsakis reviewed May 26, 2016

View changes

nikomatsakis May 26, 2016 •

edited

nikomatsakis reviewed May 26, 2016

View changes

nikomatsakis reviewed May 26, 2016

View changes

bors merged commit `040fc94` into rust-lang:master May 28, 2016
1 check passed