-
Notifications
You must be signed in to change notification settings - Fork 13.3k
process cycles as soon as they are detected #32582
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
Conversation
// ~~~ (*) see above | ||
debug!("process_child_obligations: cycle index = {}", index); | ||
|
||
if coinductive_match(selcx, &obligation, &backtrace) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just realized that this is a bug -- we are overlooking the pending_obligation
, which is at the logical "top" of the backtrace here
☔ The latest upstream changes (presumably #32439) made this pull request unmergeable. Please resolve the merge conflicts. |
@nikomatsakis OK, I've reviewed this and it's a clear improvement. r=me after a rebase. |
We used to wait for the recursion limit, but that might well be too long!
When deciding on a coinductive match, we were examining the new obligation and the backtrace, but not the *current* obligation that goes in between the two. Refactoring the code to just have the cycle given as input also made things a lot simpler.
4f798f8
to
6a749c7
Compare
@bors r=aturon |
📌 Commit 6a749c7 has been approved by |
@bors r=aturon |
📌 Commit e733b86 has been approved by |
We used to wait for the recursion limit, but that might well be too
long!
Fixes #32326
r? @aturon