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

[red-knot] Print non-string panic payloads and (sometimes) backtraces #15363

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

dcreager
Copy link
Member

@dcreager dcreager commented Jan 8, 2025

More refinements to the panic messages for failing mdtests to mimic the output of the default panic hook more closely:

  • We now print out Box<dyn Any> if the panic payload is not a string (which is typically the case for salsa panics).
  • We now include the panic's backtrace if you set the RUST_BACKTRACE environment variable.

Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Great, thank you!

Comment on lines -6 to +8
pub info: String,
pub location: Option<String>,
pub payload: Option<String>,
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that we can't put the actual payload here, because what we get back is a reference to a dyn Any, and that does not guarantee that the payload is cloneable.

Comment on lines +151 to +153
// Mimic the default panic hook's rendering of the panic payload if it's
// not a string.
None => messages.push("Box<dyn Any>".to_string()),
Copy link
Member Author

Choose a reason for hiding this comment

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

So instead we just mimic the output of the default panic hook here if the payload is not a string. (Note that neither us nor std::panic can print out anything more meaningful, since you can only downcast Any to specific known types, not to traits like Display or Debug)

Copy link
Contributor

github-actions bot commented Jan 8, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@dcreager dcreager merged commit 5f5eb7c into main Jan 8, 2025
21 checks passed
@dcreager dcreager deleted the dcreager/panic-payload branch January 8, 2025 23:12
@MichaReiser MichaReiser added the red-knot Multi-file analysis & type inference label Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants