Ban registering obligations during InferCtxt snapshots. #41325

eddyb · 2017-04-15T21:14:22Z

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

arielb1 · 2017-04-15T21:50:47Z

src/librustc/infer/mod.rs

    //
-    // HOWEVER, in some cases the flag is wrong. In particular, we


Maybe it's better to have a within_snapshot boolean on FulfillmentCx that disables the check?

Yeah it's not super clear what the best option is. It's probably a good idea to tie fulfillment contexts to inference contexts (through lifetimes?) and maybe have a snapshot depth... or maybe an "epoch"?

Longer term, I have been hoping to just get rid of the notion of snapshots except for very short-term (e.g., to make a unification operation atomic) and then create nested fulfillment contexts for "probing sessions".

Well, maybe that's not a complete answer. Still, I guess I hope that we can move things into nested contexts and generally remove the need for snapshots.

eddyb · 2017-04-16T01:58:09Z

Crater run has found one root regression.

EDIT: reproduced it (looks like obligations need to be selected eagerly for binops - Travis failure is similar):

fn main() {
    let mut s = String::new();
    s += {&String::new()};
//        ^^^^^^^^^^^^^^ expected str, found struct `std::string::String`
// note: expected type `&str`
//          found type `&std::string::String`
}

EDIT2: pushed a fix.

nikomatsakis

Looks good to me, do we want to do a crater run or anything like that? I could imagine some subtle effects (as evidenced by the last commit).

nikomatsakis · 2017-04-17T22:43:23Z

Ah, I see @eddyb is ahead of me here. Very good.

nikomatsakis · 2017-04-17T22:43:51Z

@bors r+

bors · 2017-04-17T22:43:52Z

📌 Commit cd64ff9 has been approved by nikomatsakis

nikomatsakis · 2017-04-17T22:44:13Z

@bors r-

nikomatsakis · 2017-04-17T22:44:31Z

Actually, @arielb1, were you satisfied? (If so, feel free to r=me, or you :)

arielb1 · 2017-04-18T21:23:32Z

@bors r+

bors · 2017-04-18T21:23:43Z

📌 Commit cd64ff9 has been approved by 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

…=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

bors · 2017-04-19T08:40:33Z

⌛ Testing commit cd64ff9 with merge 467aaab...

@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

bors · 2017-04-19T11:07:04Z

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing 467aaab to master...

eddyb added 5 commits April 15, 2017 23:51

rustc: ban registering obligations during InferCtxt snapshots.

99f5c85

rustc_typeck: keep register_infer_ok_obligations calls out of snapshots.

e56b119

rustc_typeck: Autoderef::finalize is always called with one &hir::Expr.

2ad1964

rustc_typeck: move some obligation methods to Inherited.

516570f

rustc_typeck: return InferOk from lookup_method_in_trait_adjusted.

3725cab

eddyb requested review from nikomatsakis and arielb1 April 15, 2017 21:14

rust-highfive assigned nikomatsakis Apr 15, 2017

eddyb added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 15, 2017

arielb1 reviewed Apr 15, 2017

View reviewed changes

GuillaumeGomez mentioned this pull request Apr 15, 2017

Ref suggestion #37658

Merged

rustc_typeck: fix binops needing more type informations to coerce.

cd64ff9

nikomatsakis approved these changes Apr 17, 2017

View reviewed changes

eddyb mentioned this pull request Apr 18, 2017

Fix ICE building gluon_vm #41377

Merged

frewsxcv mentioned this pull request Apr 18, 2017

Rollup of 5 pull requests #41379

Closed

frewsxcv mentioned this pull request Apr 18, 2017

Rollup of 7 pull requests #41381

Closed

frewsxcv mentioned this pull request Apr 18, 2017

Rollup of 6 pull requests #41382

Closed

frewsxcv mentioned this pull request Apr 18, 2017

Rollup of 8 pull requests #41383

Closed

frewsxcv mentioned this pull request Apr 19, 2017

Rollup of 8 pull requests #41385

Closed

bors merged commit cd64ff9 into rust-lang:master Apr 19, 2017

eddyb deleted the isolate-snapshots-for-good branch April 19, 2017 11:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ban registering obligations during InferCtxt snapshots. #41325

Ban registering obligations during InferCtxt snapshots. #41325

eddyb commented Apr 15, 2017

arielb1 Apr 15, 2017 •

edited

Loading

eddyb Apr 15, 2017

nikomatsakis Apr 17, 2017

nikomatsakis Apr 17, 2017

eddyb commented Apr 16, 2017 •

edited

Loading

nikomatsakis left a comment

nikomatsakis commented Apr 17, 2017

nikomatsakis commented Apr 17, 2017

bors commented Apr 17, 2017

nikomatsakis commented Apr 17, 2017

nikomatsakis commented Apr 17, 2017

arielb1 commented Apr 18, 2017

bors commented Apr 18, 2017

bors commented Apr 19, 2017

bors commented Apr 19, 2017

		//
		// HOWEVER, in some cases the flag is wrong. In particular, we

Ban registering obligations during InferCtxt snapshots. #41325

Ban registering obligations during InferCtxt snapshots. #41325

Conversation

eddyb commented Apr 15, 2017

arielb1 Apr 15, 2017 • edited Loading

Choose a reason for hiding this comment

eddyb Apr 15, 2017

Choose a reason for hiding this comment

nikomatsakis Apr 17, 2017

Choose a reason for hiding this comment

nikomatsakis Apr 17, 2017

Choose a reason for hiding this comment

eddyb commented Apr 16, 2017 • edited Loading

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis commented Apr 17, 2017

nikomatsakis commented Apr 17, 2017

bors commented Apr 17, 2017

nikomatsakis commented Apr 17, 2017

nikomatsakis commented Apr 17, 2017

arielb1 commented Apr 18, 2017

bors commented Apr 18, 2017

bors commented Apr 19, 2017

bors commented Apr 19, 2017

arielb1 Apr 15, 2017 •

edited

Loading

eddyb commented Apr 16, 2017 •

edited

Loading