Replace the obligation forest with a graph #33491
Conversation
arielb1
force-pushed
the
obligation-jungle
branch
from
7eb8baa to
bd1fa2f
Compare
|
☔ The latest upstream changes (presumably #33425) made this pull request unmergeable. Please resolve the merge conflicts. |
arielb1
force-pushed
the
obligation-jungle
branch
from
bd1fa2f to
40f7d85
Compare
| .or_insert(index); | ||
| } | ||
| self.populated = true; | ||
| fn process_backedge<'c, I>(&mut self, cycle: I) |
There was a problem hiding this comment.
also 'c is unused here
|
@arielb1 r=me but travis says: /home/travis/build/rust-lang/rust/src/librustc/traits/fulfill.rs:541: line longer than 100 chars |
arielb1
force-pushed
the
obligation-jungle
branch
from
9d29e17 to
fea78bf
Compare
|
@bors r=nikomatsakis |
|
📌 Commit fea78bf has been approved by |
|
⌛ Testing commit fea78bf with merge 5e3f3f0... |
|
💔 Test failed - auto-linux-64-opt-rustbuild |
arielb1
force-pushed
the
obligation-jungle
branch
from
fea78bf to
cbcabe4
Compare
|
@bors r- |
arielb1
force-pushed
the
obligation-jungle
branch
from
7ee2d2d to
5458d8b
Compare
|
@nikomatsakis - awaiting review. |
| 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| { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
I'm not sure how the earlier approach worked for that. EDIT: that never worked.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
That's just my logic being stupid. I have a fix to that.
|
@bors r+ |
|
📌 Commit 65ad935 has been approved by |
|
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 |
|
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... |
|
I think the primary motivator was that it was causing ICEs due to getting (I'm still interesting in finding some mechanism that takes into account On Mon, May 16, 2016 at 6:25 PM, Guillaume Gomez notifications@github.com
|
|
Let's take a simple example: let v = vec!();
v[2i32];It's incorrect, but Index trait is implemented on |
|
@GuillaumeGomez right, so is this not the logic covered by |
|
@arielb1 was the fuzzy logic in particular causing any issues, or you were simply concerned that it was overkill for the problem at hand? |
No, this PR's algorithm is way too simple. We're going back on this feature. |
|
I still think that this is the code change that made my code crash but whatever... |
|
@GuillaumeGomez so, I thought the conclusion from IRC was:
|
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
| @@ -27,12 +27,14 @@ fn f<T>(val: T) { | |||
| 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 | |||
There was a problem hiding this comment.
FYI it can be nice to use the "arrow form" here for successive lines, ie //~| ... instead of //~^^ ...
| @@ -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" | |||
There was a problem hiding this comment.
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")"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
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