-
Notifications
You must be signed in to change notification settings - Fork 13k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Infinite recursion error instead of trait bound error #40827
Comments
This is a simplified example reproducing the issue (playground). use std::rc::Rc;
use std::sync::Arc;
// Rc<_>: !Send
// => Bar: !Send
// => Arc<Bar>: !Send
// => Foo: !Send
//
// Shouldn't the compiler come to the same conclusion instead of entering this
// infinite recursion?
struct Foo(Arc<Bar>);
enum Bar {
A(Rc<Foo>),
B(Option<Foo>),
}
fn f<T: Send>(_: T) {}
fn main() {
f(Foo(Arc::new(Bar::B(None))));
}
|
Note that if |
This is the case I was referring to @aturon :) |
@dnorman Thanks! Fascinating bug... |
I think I have hit the same bug with use std::rc::Rc;
use std::sync::Arc;
struct List {
value: Rc<i32>,
next: Option<Arc<List>>
}
fn is_send() {
let _: Box<Send> = Box::new(List { value: Rc::new(4), next: None });
}
I did some digging around in the trait resolver and I believe what causes it is pointed by @nikomatsakis in #30533 (comment):
If this is truly the cause of the bug I will give it a go and try to fix it, but I would really appreciate some mentoring instructions. @nikomatsakis can you give me a few pointers? |
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.
Observed behavior:
Receiving the following error when attempting to compile the sample program.
Expected behavior:
Indicate that there is a trait bound error.
Minimal reproduction case:
https://is.gd/KgD5Tk
Many thanks to the tireless efforts of @stephaneyfx in creating the repro above!
Bonus round
I haven't yet figured out a minimal repro for this second part, but if I'm interpreting correctly, it's possible that a variant of this endless recursion error does not require a trait bound error to be triggered. (noob warning: it's possible that the omission of +Send+Sync in 4ab0fdf is actually a trait bounds error, but I don't presently understand why)
Non-minimal repro:
https://travis-ci.org/dnorman/unbase/jobs/209643435#L240
Minimal change that fixed it:
dnorman/unbase@47c68bf
https://travis-ci.org/dnorman/unbase/jobs/209649602#L228
The text was updated successfully, but these errors were encountered: