panic with string message again for cycle panics#898
Conversation
✅ Deploy Preview for salsa-rs canceled.
|
CodSpeed Performance ReportMerging #898 will improve performances by 5.52%Comparing Summary
Benchmarks breakdown
|
| panic!( | ||
| "dependency graph cycle when querying {database_key_index:#?}, \ | ||
| set cycle_fn/cycle_initial to fixpoint iterate.\n\ | ||
| Query stack:\n{stack:#?}", |
There was a problem hiding this comment.
Nit: I'm a bit undecided if we should print the query stack here. I think it's a useful default behavior, but it can be redundant when using a custom panic hook handler.
There was a problem hiding this comment.
I had the same question, and I know it ends up being redundant output for us in ty... but I think for Salsa we should optimize for the default experience, not the assumption that someone has installed a custom panic hook. And this is clearly valuable debugging information.
Someone with a custom panic hook can also eliminate this from the printed message, if they care enough to do so.
|
I do think we want to roll back #883 then as well fwiw. The user can capture the normal backtrace themselves then if desired |
Agreed, but doesn't this PR already do that by removing the |
|
Oh ye it does sorry :) Mixed this up with |
3 participants
Switch back to regular panic with string message, instead of
resume_unwindwith custom payload, for cycle panics.This avoids having the default behavior be silent panic with no information, and allows the use of a panic hook to collect additional information, if needed.
It's not clear what the use case is for catching cycle panics specifically and treating them differently from other panics; they are not more recoverable.
Fixes #891