Introduce "obligation forest" data structure into fulfillment to track backtraces #30533

nikomatsakis · 2015-12-23T00:59:56Z

This PR introduces an ObligationForest data structure that the fulfillment context can use to track what's going on, instead of the current flat vector. This enables a number of improvements:

transactional support, at least for pushing new obligations
remove the "errors will be reported" hack -- instead, we only add types to the global cache once their entire subtree has been proven safe. Before, we never knew when this point was reached because we didn't track the subtree.
- this in turn allows us to limit coinductive reasoning to structural traits, which sidesteps Cyclic traits allow arbitrary traits to be synthesized #29859
keeping the backtrace should allow for an improved error message, where we give the user full context
- we can also remove chained obligation causes

This PR is not 100% complete. In particular:

Currently, types that embed themselves like struct Foo { f: Foo } give an overflow when evaluating whether Foo: Sized. This is not a very user-friendly error message, and this is a common beginner error. I plan to special-case this scenario, I think.
I should do some perf. measurements. (Update: 2% regression.)
More tests targeting Cyclic traits allow arbitrary traits to be synthesized #29859
The transactional support is not fully integrated, though that should be easy enough.
The error messages are not taking advantage of the backtrace.

I'd certainly like to do 1 through 3 before landing, but 4 and 5 could come as separate PRs.

r? @aturon // good way to learn more about this part of the trait system
f? @arielb1 // already knows this part of the trait system :)

nikomatsakis · 2015-12-23T02:26:08Z

Cc @jroesch

arielb1 · 2015-12-23T23:06:57Z

This looks pretty nice from the big-picture view.

bors · 2015-12-28T02:29:42Z

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

jroesch · 2015-12-31T20:14:22Z

@nikomatsakis I think that this is mostly compatible with the branch I have laying around that moves more RefCells, and the fulfillment context into the inference context. I would like to integrate this into my branch and move forward if possible, let's talk when you are back from the holidays 😄 overall 👍

jroesch · 2015-12-31T20:17:36Z

Actually as I'm looking this over this is perfect. I just had some unfinished code around the SnapshotTree. I should be able to replace the SnapshotTree with this on my branch and end up with a fully transactional inference context.

aturon · 2016-01-08T18:43:08Z

OK, I've done a reviewing pass through this series of commits, and it all looks great to me. I left a number of small nits mostly about making a few things more clear in the new forest code.

It's fine by me to do 4 and 5 separately. I'd be willing to help with that work, as well.

nikomatsakis · 2016-01-11T22:04:02Z

@aturon OK, so, I have to go through your nits (and rebase), but the current HEAD of 3423caae956ce8d3525baf3bf5ec7e689339765d builds and passes make check. I am not fully satisfied with that code however, but I plan to land it as is and open a FIXME issue. In particular, I think there are a number of questions to address about how to process obligations most efficiently --

If you find that a new obligation is a duplicate of one already in the tree, the proper processing is:
- if that other location is your parent, you should abort with a cycle error (or accept it, if coinductive)
- if that other location is not an ancestor, you can safely ignore the new obligation
We currently check for cycles when processing obligations, but we should really just check when filing them (as suggested in point 1). After all, whether something is a cycle is not going to change.
When should we update the inference variables? This affects the desire to sometimes use sets and other things. Currently we do a lot of inference updates, more than seem necessary -- either we should try to find one good spot to do them or, if that is not possible because the code has multiple entry points (e.g., select vs fulfill etc), then we should probably add some kind of cheap check so that trying to refresh inference variables multiple times in a row is relatively cheap.

(These are in addition to the points I raised in the intro about better error messages, transactional support, etc.)

nikomatsakis · 2016-01-11T22:05:05Z

I wonder if I should measure the impact on crater. I found very little impact (none?) in libstd/rustc, but a few test cases were affected. We could probably retool the code to issue warnings rather than abort by having a cycle be detected and then issue a warning and drop the new obligation.

nikomatsakis · 2016-01-11T22:05:21Z

The other thing I have not done which may be worth doing is measuring the impact on performance.

nikomatsakis · 2016-01-13T00:35:06Z

@aturon ok I cleaned up the commit history some. I left in the WIP commits that are just tiny things I will rebase in. Otherwise, ready for r? on the remaining commits. Then I'll rebase, open the aforementioned FIXME issue, and land.

nikomatsakis · 2016-01-13T14:56:24Z

I am thinking now that, before landing, I ought to modify this code so that it issues warnings on cycles if they do not consist solely of co-inductive traits (but accepts them). Seems like a prudent step. It would also be nice to do a crater run to get an idea of what patterns might be affected in the wild.

nikomatsakis · 2016-01-13T21:07:22Z

I measured performance on libsyntax.

Before: 114.85user 1.23system 1:56.24elapsed
After: 116.62user 1.16system 1:57.92elapsed

1.4% performance difference. Pretty minor, I'm sure it can be improved over time as well.

stalled rather than keeping this annoying mark; I checked that the original compile-time regression that the mark was intended to fix (rust-lang#18208) was still reasonable, but I've not done exhaustive measurements to see how important this "optimization" really is anymore

this commit won't build because, as of this version, no coinductive reasoning at all is really supported

nikomatsakis · 2016-01-15T19:11:42Z

Good news. A crater report shows ZERO regressions.

aturon · 2016-01-15T23:01:36Z

Reviewed the recent string of commits. LGTM! Note tidy error.

nikomatsakis · 2016-01-16T10:23:48Z

So now the only question is: land this immediately, or wait for beta to be cut?

nikomatsakis · 2016-01-16T10:24:03Z

Given the successful crater run, I guess I might as well land it.

nikomatsakis · 2016-01-16T14:39:56Z

I found one test that still permits #29859 even under this branch, but I think the real error here is that the OIBIT orphan rules are not being correctly enforced. I'm actually in the process of writing up a more precise OIBIT specification that would make this clearer:

#![feature(optin_builtin_traits)]

trait Magic: Copy {}
impl Magic for .. {}
impl<T:Magic> Magic for T {}

fn copy<T: Magic>(x: T) -> (T, T) { (x, x) }

#[derive(Debug)]
struct NoClone;

fn main() {
    let (a, b) = copy(NoClone); //~ ERROR E0277
    println!("{:?} {:?}", a, b);
}

nikomatsakis · 2016-01-16T14:55:56Z

I take it back. I think that this example can be transformed into one indicates why co-inductive semantics are broken, even for OIBIT traits. However, it is still true that the document I've been working -- which tries to elaborate an alternative to those coinductive semantics -- ought to address this problem. In other words, I think this patch transforms the issue into a (smaller) issue with OIBIT trait impls. But it does not fix #29859

…not fully fixed.

nikomatsakis · 2016-01-16T14:58:57Z

@bors r=aturon

bors · 2016-01-16T14:58:59Z

📌 Commit 4fbb71f has been approved by aturon

bors · 2016-01-16T16:03:23Z

⌛ Testing commit 4fbb71f with merge c14b615...

@aturon

This PR introduces an `ObligationForest` data structure that the fulfillment context can use to track what's going on, instead of the current flat vector. This enables a number of improvements: 1. transactional support, at least for pushing new obligations 2. remove the "errors will be reported" hack -- instead, we only add types to the global cache once their entire subtree has been proven safe. Before, we never knew when this point was reached because we didn't track the subtree. - this in turn allows us to limit coinductive reasoning to structural traits, which sidesteps #29859 3. keeping the backtrace should allow for an improved error message, where we give the user full context - we can also remove chained obligation causes This PR is not 100% complete. In particular: - [x] Currently, types that embed themselves like `struct Foo { f: Foo }` give an overflow when evaluating whether `Foo: Sized`. This is not a very user-friendly error message, and this is a common beginner error. I plan to special-case this scenario, I think. - [x] I should do some perf. measurements. (Update: 2% regression.) - [x] More tests targeting #29859 - [ ] The transactional support is not fully integrated, though that should be easy enough. - [ ] The error messages are not taking advantage of the backtrace. I'd certainly like to do 1 through 3 before landing, but 4 and 5 could come as separate PRs. r? @aturon // good way to learn more about this part of the trait system f? @arielb1 // already knows this part of the trait system :)

bors · 2016-01-16T17:56:55Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

…, r=aturon" This reverts commit c14b615, reversing changes made to dda25f2. This caused a regression in compiler performance and backporting the fix to beta was deemed too risky and too challenging, since it is non-trivial and builds on prior commits in various ways. Conflicts: src/librustc/middle/traits/fulfill.rs src/librustc_data_structures/lib.rs

@nikomatsakis

Add a per-tree error cache to the obligation forest This implements part of what @nikomatsakis mentioned in #30533 (comment): > 1. If you find that a new obligation is a duplicate of one already in the tree, the proper processing is: > * if that other location is your parent, you should abort with a cycle error (or accept it, if coinductive) > * if that other location is not an ancestor, you can safely ignore the new obligation In particular it implements the "if that other location is your parent accept it, if coinductive" part. This fixes #40827. I have to say that I'm not 100% confident that this is rock solid. This is my first pull request 🎉, and I didn't know anything about the trait resolver before this. In particular I'm not totally sure that comparing predicates is enough (for instance, do we need to compare `param_env` as well?). Also, I'm not sure what @nikomatsakis mentions [here](#30977 (comment)), but it might be something that affects this PR: > In particular, I am wary of getting things wrong around inference variables! We can always add things to the set in their current state, and if unifications occur then the obligation is just kind of out-of-date, but I want to be sure we don't accidentally fail to notice that something is our ancestor. I decided this was subtle enough to merit its own PR. Anyway, go ahead and review 🙂. Ref #30977. # Performance We are now copying vectors around, so I decided to do some benchmarking. A simple benchmark shows that this does not seem to affect performance in a measurable way: I ran `cargo clean && cargo build` 20 times on actix-web (84b27db) and these are the results: ```text rustc master: Mean Std.Dev. Min Median Max real 66.637 2.996 57.220 67.714 69.314 user 307.293 14.741 258.093 312.209 320.702 sys 12.524 0.653 10.499 12.726 13.193 rustc fix-bug-overflow-send: Mean Std.Dev. Min Median Max real 66.297 4.310 53.532 67.516 70.348 user 306.812 22.371 236.917 314.748 326.229 sys 12.757 0.952 9.671 13.125 13.544 ``` I will do a more comprehensive benchmark (compiling rustc stage1) and post the results. r? @nikomatsakis, @nnethercote PS: It is better to review this commit-by-commit.

rust-highfive assigned aturon Dec 23, 2015

nikomatsakis force-pushed the fulfillment-tree branch from 6f9dd59 to bdc1c61 Compare December 23, 2015 01:00

nikomatsakis mentioned this pull request Jan 4, 2016

error: overflow evaluating the requirement <_ as core::iter::Iterator>::Item #23707

Open

jroesch mentioned this pull request Jan 6, 2016

Internal Fulfillment Context assertion tripped, needs more investigation #26721

Closed

nikomatsakis force-pushed the fulfillment-tree branch 2 times, most recently from fd9bf8f to cede41a Compare January 7, 2016 11:40

nikomatsakis mentioned this pull request Jan 8, 2016

Make coherence more tolerant of error types. #30676

Merged

nikomatsakis mentioned this pull request Jan 8, 2016

Associated types are not correctly computed (at least not at the right time) #28828

Closed

nikomatsakis changed the title ~~[WIP] Introduce "obligation forest" data structure into fulfillment to track backtraces~~ Introduce "obligation forest" data structure into fulfillment to track backtraces Jan 12, 2016

nikomatsakis force-pushed the fulfillment-tree branch 2 times, most recently from db09897 to bb0fdf8 Compare January 13, 2016 00:33

nikomatsakis force-pushed the fulfillment-tree branch from bb0fdf8 to bc309c4 Compare January 13, 2016 15:12

nikomatsakis added 3 commits January 14, 2016 12:59

implement the obligation forest data structure and add some unit tests

0defb15

switch the fulfillment context to use the new obligation forest;

4375693

this commit won't build because, as of this version, no coinductive reasoning at all is really supported

nikomatsakis force-pushed the fulfillment-tree branch from bc309c4 to 60c302f Compare January 14, 2016 18:01

nikomatsakis mentioned this pull request Jan 16, 2016

Cyclic traits allow arbitrary traits to be synthesized #29859

Closed

Add regression tests for rust-lang#29859. However rust-lang#29859 is …

4fbb71f

…not fully fixed.

nikomatsakis force-pushed the fulfillment-tree branch from 527cdc3 to 4fbb71f Compare January 16, 2016 14:58

bors merged commit 4fbb71f into rust-lang:master Jan 16, 2016

This was referenced Jan 17, 2016

Error reporting for trait cycles could be improved #30974

Closed

Error reporting for trait failures should give backtrace #30976

Closed

Fulfillment context should support DAGs better, integrate with caching better #30977

Open

dirk mentioned this pull request Jan 24, 2016

Massive compiler perf regression #31157

Closed

strega-nil mentioned this pull request Feb 12, 2016

Associated types aren't normalized with impl cycles #31593

Closed

nikomatsakis deleted the fulfillment-tree branch March 30, 2016 16:13

orium mentioned this pull request Jul 23, 2018

Infinite recursion error instead of trait bound error #40827

Closed

orium mentioned this pull request Aug 10, 2018

Add a per-tree error cache to the obligation forest #53255

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce "obligation forest" data structure into fulfillment to track backtraces #30533

Introduce "obligation forest" data structure into fulfillment to track backtraces #30533

nikomatsakis commented Dec 23, 2015

nikomatsakis commented Dec 23, 2015

arielb1 commented Dec 23, 2015

bors commented Dec 28, 2015

jroesch commented Dec 31, 2015

jroesch commented Dec 31, 2015

aturon commented Jan 8, 2016

nikomatsakis commented Jan 11, 2016

nikomatsakis commented Jan 11, 2016

nikomatsakis commented Jan 11, 2016

nikomatsakis commented Jan 13, 2016

nikomatsakis commented Jan 13, 2016

nikomatsakis commented Jan 13, 2016

nikomatsakis commented Jan 15, 2016

aturon commented Jan 15, 2016

nikomatsakis commented Jan 16, 2016

nikomatsakis commented Jan 16, 2016

nikomatsakis commented Jan 16, 2016

nikomatsakis commented Jan 16, 2016

nikomatsakis commented Jan 16, 2016

bors commented Jan 16, 2016

bors commented Jan 16, 2016

bors commented Jan 16, 2016

Introduce "obligation forest" data structure into fulfillment to track backtraces #30533

Introduce "obligation forest" data structure into fulfillment to track backtraces #30533

Conversation

nikomatsakis commented Dec 23, 2015

nikomatsakis commented Dec 23, 2015

arielb1 commented Dec 23, 2015

bors commented Dec 28, 2015

jroesch commented Dec 31, 2015

jroesch commented Dec 31, 2015

aturon commented Jan 8, 2016

nikomatsakis commented Jan 11, 2016

nikomatsakis commented Jan 11, 2016

nikomatsakis commented Jan 11, 2016

nikomatsakis commented Jan 13, 2016

nikomatsakis commented Jan 13, 2016

nikomatsakis commented Jan 13, 2016

nikomatsakis commented Jan 15, 2016

aturon commented Jan 15, 2016

nikomatsakis commented Jan 16, 2016

nikomatsakis commented Jan 16, 2016

nikomatsakis commented Jan 16, 2016

nikomatsakis commented Jan 16, 2016

nikomatsakis commented Jan 16, 2016

bors commented Jan 16, 2016

bors commented Jan 16, 2016

bors commented Jan 16, 2016