Replace the obligation forest with a graph#33491

arielb1 · 2016-05-08T00:17:41Z

In the presence of caching, arbitrary nodes in the obligation forest can be merged, which makes it a general graph. Handle it as such, using cycle-detection algorithms in the processing.

I should do performance measurements sometime.

This was pretty much written as a proof-of-concept. Please help me write this in a less-ugly way. I should also add comments explaining what is going on.

r? @nikomatsakis

bors · 2016-05-11T10:03:39Z

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

nikomatsakis · 2016-05-12T12:22:09Z

src/librustc/traits/fulfill.rs

this is nice

also 'c is unused here

nikomatsakis · 2016-05-13T00:29:30Z

@arielb1 r=me but travis says:

/home/travis/build/rust-lang/rust/src/librustc/traits/fulfill.rs:541: line longer than 100 chars

arielb1 · 2016-05-13T00:34:27Z

@bors r=nikomatsakis

bors · 2016-05-13T00:34:28Z

📌 Commit fea78bf has been approved by nikomatsakis

bors · 2016-05-13T12:30:52Z

⌛ Testing commit fea78bf with merge 5e3f3f0...

bors · 2016-05-13T13:23:52Z

💔 Test failed - auto-linux-64-opt-rustbuild

arielb1 · 2016-05-14T01:20:32Z

@bors r-

Fixes rust-lang#33344

arielb1 · 2016-05-14T06:03:08Z

@nikomatsakis - awaiting review.

nikomatsakis · 2016-05-15T01:17:15Z

src/librustc/traits/error_reporting.rs

+        let trait_self_ty = tcx.erase_late_bound_regions(&trait_ref).self_ty();
+
+        self.tcx.lookup_trait_def(trait_ref.def_id())
+            .for_each_relevant_impl(self.tcx, trait_self_ty, |def_id| {


so, while I do prefer this simplified approach, it seems like it would mean that vec[3_i32] will fail to get the "helpful message" we were shooting for? (I could be confused; I'll kick off a local build and check it out)

I'm not sure how the earlier approach worked for that. EDIT: that never worked.

both approaches do not report a helpful error message when a Vec is involved. Unless we are willing to add a "useless" impl Index<usize> for Vec, I don't see how this can be solved.

I guess I just mean slice, then. There is this test: check_on_unimplemented_on_slice.rs which fails.

The fact that this doesn't work for vec (if indeed true) suggests that we may want to tie the hint to the use of the [] syntax rather than having a more generic mechanism, honestly.

I tend to think that the "on unimplemented" mechanism may need some work in any case. For example, I think the hint would be a bit confusing when you have X: Index<u32> and you supply a slice. (I have often found the "on unimplemented" messages from other traits confusing for this reason.)

That's just my logic being stupid. I have a fix to that.

nikomatsakis · 2016-05-16T22:08:01Z

@bors r+

bors · 2016-05-16T22:08:02Z

📌 Commit 65ad935 has been approved by nikomatsakis

nikomatsakis · 2016-05-16T22:11:48Z

Ah, it seems only polite to cc @pnkfelix @GuillaumeGomez -- this PR encountered some ICEs centered on the "fuzzy matching" logic used to report errors for index operations, and replaces it with a simpler operation -- basically matching the self-type and looking for exactly one impl that has #[on_error] attached. This still achieves the original end. That said, I sort of think we should revisit the mechanism as a whole, and try to see if we can get something a bit more tailored (and more flexible, I think), so that one can give different messages based not only on the trait/types involved, but on the context (e.g., foo[3] is maybe different from T: Index<usize>, and the right message for the latter probably varies depending on which fn it is attached to).

GuillaumeGomez · 2016-05-16T22:24:30Z

But why removing all the code I just added? Did it bother in any way? I'm not sure to understand. :-/ It's far from perfect but very easy to make evolve. Such a disappointment...

nikomatsakis · 2016-05-16T22:41:51Z

I think the primary motivator was that it was causing ICEs due to getting
some details of higher-ranked regions incorrect, which led to arielb1 also
simplifying the overall heuristics. I guess one question is whether this
version actually triggers all the cases you were interested in?

(I'm still interesting in finding some mechanism that takes into account
more information overall.)

On Mon, May 16, 2016 at 6:25 PM, Guillaume Gomez notifications@github.com
wrote:

But why removing all the code I just added? Did it bother in any way? I'm
not sure to understand. :-/ It's far from perfect but very easy to make
evolve. Such a disappointment...

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#33491 (comment)

GuillaumeGomez · 2016-05-16T22:46:29Z

Let's take a simple example:

let v = vec!();

v[2i32];

It's incorrect, but Index trait is implemented on Vec three times: usize, RangeTo, RangeFull. In this case, it is easy to find out that usize is the closest one to i32. However, if we used a struct, RangeTo and RangeFull would have been returned and their "on_rustc_unimplemented" message would have displayed. This is not just to find out that i32 is closer to usize and that's where my incomprehension comes from.

nikomatsakis · 2016-05-16T22:48:50Z

@GuillaumeGomez right, so is this not the logic covered by check_on_unimplemented_on_slice.rs?

nikomatsakis · 2016-05-16T22:50:49Z

@arielb1 was the fuzzy logic in particular causing any issues, or you were simply concerned that it was overkill for the problem at hand?

GuillaumeGomez · 2016-05-16T23:07:50Z

@GuillaumeGomez right, so is this not the logic covered by check_on_unimplemented_on_slice.rs?

No, this PR's algorithm is way too simple. We're going back on this feature.

GuillaumeGomez · 2016-05-16T23:54:01Z

I still think that this is the code change that made my code crash but whatever...

nikomatsakis · 2016-05-17T00:08:09Z

@GuillaumeGomez so, I thought the conclusion from IRC was:

land this change (because the rest of this PR is really imp't and we don't want to block it);
@arielb1 agreed to add in the "fuzzy type" variant in a follow-up PR, where the extra arguments are converted into their "fuzzy" equivalents -- this should be roughly as expressive as your old logic

bors · 2016-05-17T01:40:00Z

⌛ Testing commit 65ad935 with merge 786b26d...

@nikomatsakis

Replace the obligation forest with a graph In the presence of caching, arbitrary nodes in the obligation forest can be merged, which makes it a general graph. Handle it as such, using cycle-detection algorithms in the processing. I should do performance measurements sometime. This was pretty much written as a proof-of-concept. Please help me write this in a less-ugly way. I should also add comments explaining what is going on. r? @nikomatsakis

pnkfelix · 2016-05-17T03:12:50Z

src/test/compile-fail/kindck-impl-type-params.rs

    let t: S<T> = S(marker::PhantomData);
    let a = &t as &Gettable<T>;
    //~^ ERROR : std::marker::Send` is not satisfied
+    //~^^ ERROR : std::marker::Copy` is not satisfied


FYI it can be nice to use the "arrow form" here for successive lines, ie //~| ... instead of //~^^ ...

bors · 2016-05-17T05:34:58Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-32cross-opt, auto-linux-64-cargotest, auto-linux-64-cross-armhf, auto-linux-64-cross-armsf, auto-linux-64-cross-freebsd, auto-linux-64-cross-netbsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-mir, auto-linux-64-opt-rustbuild, 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-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-cross-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-mir, auto-win-msvc-64-opt-rustbuild

bluss · 2016-05-17T12:34:10Z

src/librustc_metadata/common.rs

        "rustc {}",
-        option_env!("CFG_VERSION").unwrap_or("unknown version")
+//        option_env!("CFG_VERSION").unwrap_or("unknown version")
+        "nightly edition"


Is this an intentional change?

No.

On Tue, May 17, 2016 at 3:34 PM, bluss notifications@github.com wrote:

In src/librustc_metadata/common.rs
#33491 (comment):

@@ -247,7 +247,8 @@ pub const tag_rustc_version: usize = 0x10f;
pub fn rustc_version() -> String {
format!(
"rustc {}",

option_env!("CFG_VERSION").unwrap_or("unknown version")
+// option_env!("CFG_VERSION").unwrap_or("unknown version")

"nightly edition"

Is this an intentional change?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/rust-lang/rust/pull/33491/files/65ad935737138eb307fdd01279ba5553a047bb6c#r63513124

rust-highfive assigned nikomatsakis May 8, 2016

arielb1 force-pushed the obligation-jungle branch from 7eb8baa to bd1fa2f Compare May 10, 2016 06:18

arielb1 force-pushed the obligation-jungle branch from bd1fa2f to 40f7d85 Compare May 11, 2016 20:52

nikomatsakis reviewed May 12, 2016
View reviewed changes

arielb1 force-pushed the obligation-jungle branch from 9d29e17 to fea78bf Compare May 13, 2016 00:34

eddyb mentioned this pull request May 13, 2016

Rollup of 29 pull requests #33610

Closed

arielb1 force-pushed the obligation-jungle branch from fea78bf to cbcabe4 Compare May 13, 2016 16:48

Ariel Ben-Yehuda and others added 4 commits May 13, 2016 21:23

rewrite obligation forest. cycles still handled incorrectly.

957500b

add cycle-reporting logic

5c39a2a

Fixes rust-lang#33344

address review comments

f0f5ef5

rewrite fuzzy on_unimplemented matching to avoid ICEs

5458d8b

arielb1 force-pushed the obligation-jungle branch from 7ee2d2d to 5458d8b Compare May 14, 2016 06:02

nikomatsakis reviewed May 15, 2016
View reviewed changes

change on_unimplented logic

65ad935

pnkfelix reviewed May 17, 2016
View reviewed changes

pnkfelix mentioned this pull request May 17, 2016

provide on_unimplemented hint for ops::Index #31062

Closed

bors merged commit 65ad935 into rust-lang:master May 17, 2016

bluss reviewed May 17, 2016
View reviewed changes

pnkfelix mentioned this pull request May 17, 2016

Add rustc_on_unimplemented for Index implementation on slice #33401

Merged

xen0n mentioned this pull request May 21, 2016

[regression] rustc panic on nightly (0667ae9 2016-05-17): Encountered errors `[FulfillmentError(Obligation(predicate=Binder(TraitPredicate #33723

Closed

eddyb mentioned this pull request Jun 4, 2016

many major compile-time perf regressions in jld-day15-parser #34042

Closed

Uh oh!

Conversation

arielb1 commented May 8, 2016

Uh oh!

bors commented May 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented May 13, 2016

Uh oh!

arielb1 commented May 13, 2016

Uh oh!

bors commented May 13, 2016

Uh oh!

bors commented May 13, 2016

Uh oh!

bors commented May 13, 2016

Uh oh!

arielb1 commented May 14, 2016

Uh oh!

arielb1 commented May 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielb1 May 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented May 16, 2016

Uh oh!

bors commented May 16, 2016

Uh oh!

nikomatsakis commented May 16, 2016

Uh oh!

GuillaumeGomez commented May 16, 2016

Uh oh!

nikomatsakis commented May 16, 2016

Uh oh!

GuillaumeGomez commented May 16, 2016

Uh oh!

nikomatsakis commented May 16, 2016

Uh oh!

nikomatsakis commented May 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GuillaumeGomez commented May 16, 2016

Uh oh!

GuillaumeGomez commented May 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikomatsakis commented May 17, 2016

Uh oh!

bors commented May 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented May 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

arielb1 May 15, 2016 •

edited

Loading

nikomatsakis commented May 16, 2016 •

edited

Loading

GuillaumeGomez commented May 16, 2016 •

edited

Loading