-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Improve non_fmt_panics suggestion based on trait impls. #88083
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
let fmt_applicability = if suggest_panic_any { | ||
// If we can use panic_any, use that as the MachineApplicable suggestion. | ||
Applicability::MaybeIncorrect | ||
} else { | ||
// If we don't suggest panic_any, using a format string is our best bet. | ||
Applicability::MachineApplicable | ||
}; |
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.
Isn't this logic reversed?
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.
No. It marks the format-string suggestion (adding "{}"
or "{:?}"
) as machine applicable only when the panic_any
suggestion isn't, as there shouldn't be more than one alternative marked as machine applicable.
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.
LGTM, except for the concern around applicability.
@bors r+ |
📌 Commit ab8cbc3 has been approved by |
Raising priority so it can be part of the edition migration crater run. @bors p=1 |
☀️ Test successful - checks-actions |
This improves the non_fmt_panics lint suggestions by checking first which trait (Display or Debug) are actually implemented on the type.
Fixes #87313
Fixes #87999
Before:
After:
r? @estebank