Stream: wg-traits

Topic: Logic panics


matklad (Sep 27 2019 at 11:01, on Zulip):

Hey, we've seen this assertion trigger in rust-analyzer a number of times: https://github.com/rust-lang/chalk/blob/df09cc603c0cff9ed95e5554055d483fdd756fbc/chalk-engine/src/logic.rs#L106, the most recent issue is https://github.com/rust-analyzer/rust-analyzer/issues/1927.

The failure seems somewhat non-deterministic (it only triggers if invoke compiletion in a particular way). Does this assert means some internal problem in chalk, or are we holding it wrong?

nikomatsakis (Sep 27 2019 at 15:54, on Zulip):

hmm

nikomatsakis (Sep 27 2019 at 15:54, on Zulip):

mmmmmmmmmm I wonder

nikomatsakis (Sep 27 2019 at 15:55, on Zulip):

it could be perhaps that salsa is panicking

nikomatsakis (Sep 27 2019 at 15:55, on Zulip):

and that chalk is not panic-safe

nikomatsakis (Sep 27 2019 at 15:55, on Zulip):

(I bet that's correct)

nikomatsakis (Sep 27 2019 at 15:55, on Zulip):

e.g., if there were a panic in one of the callbacks that occurred when solving someting

nikomatsakis (Sep 27 2019 at 15:55, on Zulip):

we'd probably leave things in self.stack

nikomatsakis (Sep 27 2019 at 15:56, on Zulip):

and then if you call back in, you'll see that error @matklad

nikomatsakis (Sep 27 2019 at 15:56, on Zulip):

I can open an issue with some notes on what it would take to fix, probably do-able

matklad (Sep 27 2019 at 16:04, on Zulip):

sounds plausible. When we panic due to cancellation, we advance the revision, which will cause chalk's engine to be re-created (b/c its a volatile query), however there's still a window of opportunity to observe chalk in this revision

matklad (Sep 27 2019 at 16:09, on Zulip):

I've just tried to reproduce this without cancellation, and I see 30 seconds requestsion, but now panics. So, the plausible narrative is that, in this case, chalk takes too long to infer something (which is an unrelated problem). However, because it's slow, it's likely to observe cancellation, which additionally triggers a panic. I think we can easily do a work-around on our side, by using a mutex with poisoning around the chalk_solve::Solver. We currently use parking_lot::Mutex

matklad (Sep 27 2019 at 16:15, on Zulip):

Heh, I am looking into why UnwindSafe didn't caught this issue, and I think that we didn't fix that cyclic bound problem in salsa, and instread I've just put AssertUnwindSafe around db

matklad (Sep 27 2019 at 16:18, on Zulip):

Hello overflow evaluating the requirement db::RootDatabase: ra_hir::db::HirDatabase, my old friend...

matklad (Oct 08 2019 at 09:28, on Zulip):

filed https://github.com/rust-lang/chalk/issues/260

Last update: Nov 18 2019 at 00:55UTC