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

Improve non_fmt_panics suggestion based on trait impls. #88083

Merged
merged 5 commits into from
Aug 17, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -3932,6 +3932,7 @@ dependencies = [
"rustc_feature",
"rustc_hir",
"rustc_index",
"rustc_infer",
"rustc_middle",
"rustc_parse_format",
"rustc_serialize",
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_lint/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,4 @@ rustc_session = { path = "../rustc_session" }
rustc_serialize = { path = "../rustc_serialize" }
rustc_trait_selection = { path = "../rustc_trait_selection" }
rustc_parse_format = { path = "../rustc_parse_format" }
rustc_infer = { path = "../rustc_infer" }
66 changes: 53 additions & 13 deletions compiler/rustc_lint/src/non_fmt_panic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,15 @@ use crate::{LateContext, LateLintPass, LintContext};
use rustc_ast as ast;
use rustc_errors::{pluralize, Applicability};
use rustc_hir as hir;
use rustc_infer::infer::TyCtxtInferExt;
use rustc_middle::lint::in_external_macro;
use rustc_middle::ty;
use rustc_middle::ty::subst::InternalSubsts;
use rustc_parse_format::{ParseMode, Parser, Piece};
use rustc_session::lint::FutureIncompatibilityReason;
use rustc_span::edition::Edition;
use rustc_span::{hygiene, sym, symbol::kw, symbol::SymbolStr, InnerSpan, Span, Symbol};
use rustc_trait_selection::infer::InferCtxtExt;

declare_lint! {
/// The `non_fmt_panics` lint detects `panic!(..)` invocations where the first
Expand Down Expand Up @@ -99,7 +102,7 @@ fn check_panic<'tcx>(cx: &LateContext<'tcx>, f: &'tcx hir::Expr<'tcx>, arg: &'tc

cx.struct_span_lint(NON_FMT_PANICS, arg_span, |lint| {
let mut l = lint.build("panic message is not a string literal");
l.note("this usage of panic!() is deprecated; it will be a hard error in Rust 2021");
l.note(&format!("this usage of {}!() is deprecated; it will be a hard error in Rust 2021", symbol_str));
l.note("for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2021/panic-macro-consistency.html>");
if !span.contains(arg_span) {
// No clue where this argument is coming from.
Expand Down Expand Up @@ -129,20 +132,57 @@ fn check_panic<'tcx>(cx: &LateContext<'tcx>, f: &'tcx hir::Expr<'tcx>, arg: &'tc
ty.ty_adt_def(),
Some(ty_def) if cx.tcx.is_diagnostic_item(sym::string_type, ty_def.did),
);
l.span_suggestion_verbose(
arg_span.shrink_to_lo(),
"add a \"{}\" format string to Display the message",
"\"{}\", ".into(),
if is_str {
Applicability::MachineApplicable
} else {
Applicability::MaybeIncorrect
},
);
if !is_str && panic == sym::std_panic_macro {

let (suggest_display, suggest_debug) = cx.tcx.infer_ctxt().enter(|infcx| {
let display = is_str || cx.tcx.get_diagnostic_item(sym::display_trait).map(|t| {
infcx.type_implements_trait(t, ty, InternalSubsts::empty(), cx.param_env).may_apply()
}) == Some(true);
let debug = !display && cx.tcx.get_diagnostic_item(sym::debug_trait).map(|t| {
infcx.type_implements_trait(t, ty, InternalSubsts::empty(), cx.param_env).may_apply()
}) == Some(true);
(display, debug)
});

let suggest_panic_any = !is_str && panic == sym::std_panic_macro;

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
};
Comment on lines +148 to +154
Copy link
Contributor

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?

Copy link
Member Author

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.


if suggest_display {
l.span_suggestion_verbose(
arg_span.shrink_to_lo(),
"add a \"{}\" format string to Display the message",
"\"{}\", ".into(),
fmt_applicability,
);
} else if suggest_debug {
l.span_suggestion_verbose(
arg_span.shrink_to_lo(),
&format!(
"add a \"{{:?}}\" format string to use the Debug implementation of `{}`",
ty,
),
"\"{:?}\", ".into(),
fmt_applicability,
);
}

if suggest_panic_any {
if let Some((open, close, del)) = find_delimiters(cx, span) {
l.multipart_suggestion(
"or use std::panic::panic_any instead",
&format!(
"{}use std::panic::panic_any instead",
if suggest_display || suggest_debug {
"or "
} else {
""
},
),
if del == '(' {
vec![(span.until(open), "std::panic::panic_any".into())]
} else {
Expand Down
30 changes: 30 additions & 0 deletions src/test/ui/non-fmt-panic.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,16 @@ fn main() {
//~^ WARN panic message contains unused formatting placeholders
assert!(false, "{}", S);
//~^ WARN panic message is not a string literal
assert!(false, "{}", 123);
//~^ WARN panic message is not a string literal
assert!(false, "{:?}", Some(123));
//~^ WARN panic message is not a string literal
debug_assert!(false, "{}", "{{}} bla"); //~ WARN panic message contains braces
panic!("{}", C); //~ WARN panic message is not a string literal
panic!("{}", S); //~ WARN panic message is not a string literal
std::panic::panic_any(123); //~ WARN panic message is not a string literal
core::panic!("{}", &*"abc"); //~ WARN panic message is not a string literal
std::panic::panic_any(Some(123)); //~ WARN panic message is not a string literal
panic!("{}", concat!("{", "}")); //~ WARN panic message contains an unused formatting placeholder
panic!("{}", concat!("{", "{")); //~ WARN panic message contains braces

Expand Down Expand Up @@ -51,4 +56,29 @@ fn main() {
}
panic!("{}"); // OK
panic!(S); // OK

a(1);
b(1);
c(1);
d(1);
}

fn a<T: Send + 'static>(v: T) {
std::panic::panic_any(v); //~ WARN panic message is not a string literal
assert!(false, v); //~ WARN panic message is not a string literal
}

fn b<T: std::fmt::Debug + Send + 'static>(v: T) {
std::panic::panic_any(v); //~ WARN panic message is not a string literal
assert!(false, "{:?}", v); //~ WARN panic message is not a string literal
}

fn c<T: std::fmt::Display + Send + 'static>(v: T) {
std::panic::panic_any(v); //~ WARN panic message is not a string literal
assert!(false, "{}", v); //~ WARN panic message is not a string literal
}

fn d<T: std::fmt::Display + std::fmt::Debug + Send + 'static>(v: T) {
std::panic::panic_any(v); //~ WARN panic message is not a string literal
assert!(false, "{}", v); //~ WARN panic message is not a string literal
}
30 changes: 30 additions & 0 deletions src/test/ui/non-fmt-panic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,16 @@ fn main() {
//~^ WARN panic message contains unused formatting placeholders
assert!(false, S);
//~^ WARN panic message is not a string literal
assert!(false, 123);
//~^ WARN panic message is not a string literal
assert!(false, Some(123));
//~^ WARN panic message is not a string literal
debug_assert!(false, "{{}} bla"); //~ WARN panic message contains braces
panic!(C); //~ WARN panic message is not a string literal
panic!(S); //~ WARN panic message is not a string literal
std::panic!(123); //~ WARN panic message is not a string literal
core::panic!(&*"abc"); //~ WARN panic message is not a string literal
panic!(Some(123)); //~ WARN panic message is not a string literal
panic!(concat!("{", "}")); //~ WARN panic message contains an unused formatting placeholder
panic!(concat!("{", "{")); //~ WARN panic message contains braces

Expand Down Expand Up @@ -51,4 +56,29 @@ fn main() {
}
panic!("{}"); // OK
panic!(S); // OK

a(1);
b(1);
c(1);
d(1);
}

fn a<T: Send + 'static>(v: T) {
panic!(v); //~ WARN panic message is not a string literal
assert!(false, v); //~ WARN panic message is not a string literal
}

fn b<T: std::fmt::Debug + Send + 'static>(v: T) {
panic!(v); //~ WARN panic message is not a string literal
assert!(false, v); //~ WARN panic message is not a string literal
}

fn c<T: std::fmt::Display + Send + 'static>(v: T) {
panic!(v); //~ WARN panic message is not a string literal
assert!(false, v); //~ WARN panic message is not a string literal
}

fn d<T: std::fmt::Display + std::fmt::Debug + Send + 'static>(v: T) {
panic!(v); //~ WARN panic message is not a string literal
assert!(false, v); //~ WARN panic message is not a string literal
}
Loading