Skip to content
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

Guard against infinite loop in recursive solver #569

Merged
merged 2 commits into from Aug 28, 2020

Conversation

flodiebold
Copy link
Member

This happened in some tests in rust-analyzer. I think it can only happen with overlapping impls, so I don't know how to actually test it. (In rust-analyzer, it happened because of built-in impls provided both by RA and now Chalk.)

tests/test/misc.rs Outdated Show resolved Hide resolved
chalk-recursive/src/fulfill.rs Show resolved Hide resolved
Copy link
Contributor

@jackh726 jackh726 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with this with a comment explaining the condition. I think having an option to disable coherence isn't bad, but can definitely be left up to a followup issue. (I imagine we might just want to remove checked_program and be explicit.)

@nikomatsakis
Copy link
Contributor

@flodiebold want to add a comment here?

@flodiebold
Copy link
Member Author

Yeah, I actually wanted to look into making it testable, but didn't get to it yet.

This happened in some tests in rust-analyzer. I think it can only happen with
overlapping impls, so I don't know how to actually test it. (In rust-analyzer,
it happened because of built-in impls provided both by RA and now Chalk.)
... to make the infinite loop regression test work.
@flodiebold
Copy link
Member Author

@jackh726 @nikomatsakis I've added a comment and a way to disable coherence in tests.

@jackh726
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 28, 2020

📌 Commit 3b13808 has been approved by jackh726

@bors
Copy link
Collaborator

bors commented Aug 28, 2020

⌛ Testing commit 3b13808 with merge d852017...

@bors
Copy link
Collaborator

bors commented Aug 28, 2020

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing d852017 to master...

@bors bors merged commit d852017 into rust-lang:master Aug 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants