refactor autoderef to avoid prematurely registering obligations #33852

arielb1 · 2016-05-24T21:07:15Z

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

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 rust-lang#24819 Fixes rust-lang#25801 Fixes rust-lang#27631 Fixes rust-lang#31258 Fixes rust-lang#31964 Fixes rust-lang#32320 Fixes rust-lang#33515 Fixes rust-lang#33755

eddyb · 2016-05-24T21:30:10Z

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 · 2016-05-24T22:03:53Z

#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 · 2016-05-25T18:13:12Z

updated

eddyb · 2016-05-25T23:05:19Z

@bors r+

bors · 2016-05-25T23:05:20Z

📌 Commit 040fc94 has been approved by eddyb

nikomatsakis · 2016-05-26T12:30:04Z

cc me

nikomatsakis · 2016-05-26T14:00:37Z

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 · 2016-05-26T14:46:08Z

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

nikomatsakis · 2016-05-26T14:50:28Z

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.

@eddyb

refactor autoderef to avoid prematurely registering obligations 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 rust-lang#24819 Fixes rust-lang#25801 Fixes rust-lang#27631 Fixes rust-lang#31258 Fixes rust-lang#31964 Fixes rust-lang#32320 Fixes rust-lang#33515 Fixes rust-lang#33755 r? @eddyb

Rollup of 15 pull requests - Successful merges: #33820, #33821, #33822, #33824, #33825, #33831, #33832, #33848, #33849, #33852, #33854, #33856, #33859, #33860, #33861 - Failed merges:

arielb1 · 2016-05-28T17:41:15Z

Beta-nominating for being a very annoying ICE.

nikomatsakis · 2016-06-02T20:20:06Z

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

@nikomatsakis

…=arielb1 Ban registering obligations during InferCtxt snapshots. Back in rust-lang#33852, a flag was added to `InferCtxt` to prevent rolling back a snapshot if obligations were added to some `FulfillmentContext` during the snapshot, to prevent leaking fresh inference variables (created during that snapshot, so their indices would get reused) in obligations, which could ICE or worse. But that isn't enough in the long run, as type-checking ends up relying on success implying that eager side-effects are fine, and while stray obligations *do* get caught nowadays, those errors prevent, e.g. the speculative coercions from rust-lang#37658, which *have to* be rolled back *even* if they succeed. We can't just allow those obligations to stay around though, because we end up, again, in ICEs or worse. Instead, this PR modifies `lookup_method_in_trait_adjusted` to return `InferOk` containing the obligations that `Autoderef::finalize_as_infer_ok` can propagate to deref coercions. As there shouldn't be *anything* left that registers obligations during snapshots, it's completely banned. r? @nikomatsakis @arielb1

@nikomatsakis

…=arielb1 Ban registering obligations during InferCtxt snapshots. Back in rust-lang#33852, a flag was added to `InferCtxt` to prevent rolling back a snapshot if obligations were added to some `FulfillmentContext` during the snapshot, to prevent leaking fresh inference variables (created during that snapshot, so their indices would get reused) in obligations, which could ICE or worse. But that isn't enough in the long run, as type-checking ends up relying on success implying that eager side-effects are fine, and while stray obligations *do* get caught nowadays, those errors prevent, e.g. the speculative coercions from rust-lang#37658, which *have to* be rolled back *even* if they succeed. We can't just allow those obligations to stay around though, because we end up, again, in ICEs or worse. Instead, this PR modifies `lookup_method_in_trait_adjusted` to return `InferOk` containing the obligations that `Autoderef::finalize_as_infer_ok` can propagate to deref coercions. As there shouldn't be *anything* left that registers obligations during snapshots, it's completely banned. r? @nikomatsakis @arielb1

@nikomatsakis

…=arielb1 Ban registering obligations during InferCtxt snapshots. Back in rust-lang#33852, a flag was added to `InferCtxt` to prevent rolling back a snapshot if obligations were added to some `FulfillmentContext` during the snapshot, to prevent leaking fresh inference variables (created during that snapshot, so their indices would get reused) in obligations, which could ICE or worse. But that isn't enough in the long run, as type-checking ends up relying on success implying that eager side-effects are fine, and while stray obligations *do* get caught nowadays, those errors prevent, e.g. the speculative coercions from rust-lang#37658, which *have to* be rolled back *even* if they succeed. We can't just allow those obligations to stay around though, because we end up, again, in ICEs or worse. Instead, this PR modifies `lookup_method_in_trait_adjusted` to return `InferOk` containing the obligations that `Autoderef::finalize_as_infer_ok` can propagate to deref coercions. As there shouldn't be *anything* left that registers obligations during snapshots, it's completely banned. r? @nikomatsakis @arielb1

@nikomatsakis

…=arielb1 Ban registering obligations during InferCtxt snapshots. Back in rust-lang#33852, a flag was added to `InferCtxt` to prevent rolling back a snapshot if obligations were added to some `FulfillmentContext` during the snapshot, to prevent leaking fresh inference variables (created during that snapshot, so their indices would get reused) in obligations, which could ICE or worse. But that isn't enough in the long run, as type-checking ends up relying on success implying that eager side-effects are fine, and while stray obligations *do* get caught nowadays, those errors prevent, e.g. the speculative coercions from rust-lang#37658, which *have to* be rolled back *even* if they succeed. We can't just allow those obligations to stay around though, because we end up, again, in ICEs or worse. Instead, this PR modifies `lookup_method_in_trait_adjusted` to return `InferOk` containing the obligations that `Autoderef::finalize_as_infer_ok` can propagate to deref coercions. As there shouldn't be *anything* left that registers obligations during snapshots, it's completely banned. r? @nikomatsakis @arielb1

@nikomatsakis

Ban registering obligations during InferCtxt snapshots. Back in #33852, a flag was added to `InferCtxt` to prevent rolling back a snapshot if obligations were added to some `FulfillmentContext` during the snapshot, to prevent leaking fresh inference variables (created during that snapshot, so their indices would get reused) in obligations, which could ICE or worse. But that isn't enough in the long run, as type-checking ends up relying on success implying that eager side-effects are fine, and while stray obligations *do* get caught nowadays, those errors prevent, e.g. the speculative coercions from #37658, which *have to* be rolled back *even* if they succeed. We can't just allow those obligations to stay around though, because we end up, again, in ICEs or worse. Instead, this PR modifies `lookup_method_in_trait_adjusted` to return `InferOk` containing the obligations that `Autoderef::finalize_as_infer_ok` can propagate to deref coercions. As there shouldn't be *anything* left that registers obligations during snapshots, it's completely banned. r? @nikomatsakis @arielb1

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 reviewed changes

catch attempts to leak obligations out of snapshots

040fc94

nikomatsakis reviewed May 26, 2016
View reviewed changes

eddyb mentioned this pull request May 27, 2016

Type Checker getting confused with constraints + ICE #33904

Closed

Manishearth mentioned this pull request May 28, 2016

Rollup of 15 pull requests #33927

Merged

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

arielb1 added the beta-nominated Nominated for backporting to the compiler in the beta channel. label May 28, 2016

nikomatsakis removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 2, 2016

TimNN mentioned this pull request Jun 30, 2016

thread 'rustc' panicked at 'index out of bounds: the len is 176 but the index is 178', src/libcollections/vec.rs:1166 #34568

Closed

TimNN mentioned this pull request Jul 14, 2016

Unexpected compiler panic with a static Vec #34823

Closed

TimNN mentioned this pull request Jul 25, 2016

Panic on a simple type error #35024

Closed

eddyb mentioned this pull request Apr 15, 2017

Ban registering obligations during InferCtxt snapshots. #41325

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor autoderef to avoid prematurely registering obligations #33852

refactor autoderef to avoid prematurely registering obligations #33852

arielb1 commented May 24, 2016

eddyb May 24, 2016

eddyb commented May 24, 2016

arielb1 commented May 25, 2016

eddyb commented May 25, 2016

bors commented May 25, 2016

nikomatsakis commented May 26, 2016

nikomatsakis May 26, 2016 •

edited

Loading

nikomatsakis commented May 26, 2016

nikomatsakis commented May 26, 2016

arielb1 commented May 28, 2016

nikomatsakis commented Jun 2, 2016

refactor autoderef to avoid prematurely registering obligations #33852

refactor autoderef to avoid prematurely registering obligations #33852

Conversation

arielb1 commented May 24, 2016

eddyb May 24, 2016

Choose a reason for hiding this comment

eddyb commented May 24, 2016

arielb1 commented May 25, 2016

eddyb commented May 25, 2016

bors commented May 25, 2016

nikomatsakis commented May 26, 2016

nikomatsakis May 26, 2016 • edited Loading

Choose a reason for hiding this comment

nikomatsakis commented May 26, 2016

nikomatsakis commented May 26, 2016

arielb1 commented May 28, 2016

nikomatsakis commented Jun 2, 2016

nikomatsakis May 26, 2016 •

edited

Loading