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.

Sign up for GitHub

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

Jump to bottom

Refactor format_args! expansion parsing #7442

Merged
merged 5 commits into from
Jul 13, 2021
Merged

Refactor format_args! expansion parsing #7442

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3fc8dae
Allow needless_bool in macro
camsteffen Jun 9, 2021
20dbb27
Fix useless_format false positive
camsteffen Jul 4, 2021
e6ab222
Refactor format macro parsing
camsteffen Jul 6, 2021
73ffae6
Reduce redundant code
camsteffen Jul 12, 2021
306f9e8
Split a lint message into help
camsteffen Jul 12, 2021
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
117 changes: 36 additions & 81 deletions clippy_lints/src/explicit_write.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
use clippy_utils::diagnostics::{span_lint, span_lint_and_sugg};
use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg};
use clippy_utils::higher::FormatArgsExpn;
use clippy_utils::{is_expn_of, match_function_call, paths};
use if_chain::if_chain;
use rustc_ast::ast::LitKind;
use rustc_errors::Applicability;
use rustc_hir::{BorrowKind, Expr, ExprKind};
use rustc_hir::{Expr, ExprKind};
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::sym;
Expand Down Expand Up @@ -34,29 +34,26 @@ impl<'tcx> LateLintPass<'tcx> for ExplicitWrite {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
if_chain! {
// match call to unwrap
if let ExprKind::MethodCall(unwrap_fun, _, unwrap_args, _) = expr.kind;
if let ExprKind::MethodCall(unwrap_fun, _, [write_call], _) = expr.kind;
if unwrap_fun.ident.name == sym::unwrap;
// match call to write_fmt
if !unwrap_args.is_empty();
if let ExprKind::MethodCall(write_fun, _, write_args, _) =
unwrap_args[0].kind;
if let ExprKind::MethodCall(write_fun, _, [write_recv, write_arg], _) = write_call.kind;
if write_fun.ident.name == sym!(write_fmt);
// match calls to std::io::stdout() / std::io::stderr ()
if !write_args.is_empty();
if let Some(dest_name) = if match_function_call(cx, &write_args[0], &paths::STDOUT).is_some() {
if let Some(dest_name) = if match_function_call(cx, write_recv, &paths::STDOUT).is_some() {
Some("stdout")
} else if match_function_call(cx, &write_args[0], &paths::STDERR).is_some() {
} else if match_function_call(cx, write_recv, &paths::STDERR).is_some() {
Some("stderr")
} else {
None
};
if let Some(format_args) = FormatArgsExpn::parse(write_arg);
then {
let write_span = unwrap_args[0].span;
let calling_macro =
// ordering is important here, since `writeln!` uses `write!` internally
if is_expn_of(write_span, "writeln").is_some() {
if is_expn_of(write_call.span, "writeln").is_some() {
Some("writeln")
} else if is_expn_of(write_span, "write").is_some() {
} else if is_expn_of(write_call.span, "write").is_some() {
Some("write")
} else {
None
Expand All @@ -70,82 +67,40 @@ impl<'tcx> LateLintPass<'tcx> for ExplicitWrite {
// We need to remove the last trailing newline from the string because the
// underlying `fmt::write` function doesn't know whether `println!` or `print!` was
// used.
if let Some(mut write_output) = write_output_string(write_args) {
let (used, sugg_mac) = if let Some(macro_name) = calling_macro {
(
format!("{}!({}(), ...)", macro_name, dest_name),
macro_name.replace("write", "print"),
)
} else {
(
format!("{}().write_fmt(...)", dest_name),
"print".into(),
)
};
let msg = format!("use of `{}.unwrap()`", used);
if let [write_output] = *format_args.format_string_symbols {
let mut write_output = write_output.to_string();
if write_output.ends_with('\n') {
write_output.pop();
}

if let Some(macro_name) = calling_macro {
span_lint_and_sugg(
cx,
EXPLICIT_WRITE,
expr.span,
&format!(
"use of `{}!({}(), ...).unwrap()`",
macro_name,
dest_name
),
"try this",
format!("{}{}!(\"{}\")", prefix, macro_name.replace("write", "print"), write_output.escape_default()),
Applicability::MachineApplicable
);
} else {
span_lint_and_sugg(
cx,
EXPLICIT_WRITE,
expr.span,
&format!("use of `{}().write_fmt(...).unwrap()`", dest_name),
"try this",
format!("{}print!(\"{}\")", prefix, write_output.escape_default()),
Applicability::MachineApplicable
);
}
let sugg = format!("{}{}!(\"{}\")", prefix, sugg_mac, write_output.escape_default());
span_lint_and_sugg(
cx,
EXPLICIT_WRITE,
expr.span,
&msg,
"try this",
sugg,
Applicability::MachineApplicable
);
} else {
// We don't have a proper suggestion
if let Some(macro_name) = calling_macro {
span_lint(
cx,
EXPLICIT_WRITE,
expr.span,
&format!(
"use of `{}!({}(), ...).unwrap()`. Consider using `{}{}!` instead",
macro_name,
dest_name,
prefix,
macro_name.replace("write", "print")
)
);
} else {
span_lint(
cx,
EXPLICIT_WRITE,
expr.span,
&format!("use of `{}().write_fmt(...).unwrap()`. Consider using `{}print!` instead", dest_name, prefix),
);
}
let help = format!("consider using `{}{}!` instead", prefix, sugg_mac);
span_lint_and_help(cx, EXPLICIT_WRITE, expr.span, &msg, None, &help);
}

}
}
}
}

// Extract the output string from the given `write_args`.
fn write_output_string(write_args: &[Expr<'_>]) -> Option<String> {
if_chain! {
// Obtain the string that should be printed
if write_args.len() > 1;
if let ExprKind::Call(_, output_args) = write_args[1].kind;
if !output_args.is_empty();
if let ExprKind::AddrOf(BorrowKind::Ref, _, output_string_expr) = output_args[0].kind;
if let ExprKind::Array(string_exprs) = output_string_expr.kind;
// we only want to provide an automatic suggestion for simple (non-format) strings
if string_exprs.len() == 1;
if let ExprKind::Lit(ref lit) = string_exprs[0].kind;
if let LitKind::Str(ref write_output, _) = lit.node;
then {
return Some(write_output.to_string())
}
}
None
}
192 changes: 69 additions & 123 deletions clippy_lints/src/format.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,16 @@
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::paths;
use clippy_utils::source::{snippet, snippet_opt};
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::higher::FormatExpn;
use clippy_utils::last_path_segment;
use clippy_utils::source::{snippet_opt, snippet_with_applicability};
use clippy_utils::sugg::Sugg;
use clippy_utils::ty::is_type_diagnostic_item;
use clippy_utils::{is_expn_of, last_path_segment, match_def_path, match_function_call};
use if_chain::if_chain;
use rustc_ast::ast::LitKind;
use rustc_errors::Applicability;
use rustc_hir::{Arm, BorrowKind, Expr, ExprKind, MatchSource, PatKind};
use rustc_lint::{LateContext, LateLintPass, LintContext};
use rustc_hir::{BorrowKind, Expr, ExprKind, QPath};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty;
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::source_map::Span;
use rustc_span::sym;
use rustc_span::symbol::kw;
use rustc_span::{sym, Span};

declare_clippy_lint! {
/// **What it does:** Checks for the use of `format!("string literal with no
Expand Down Expand Up @@ -44,130 +43,78 @@ declare_lint_pass!(UselessFormat => [USELESS_FORMAT]);

impl<'tcx> LateLintPass<'tcx> for UselessFormat {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
let span = match is_expn_of(expr.span, "format") {
Some(s) if !s.from_expansion() => s,
let FormatExpn { call_site, format_args } = match FormatExpn::parse(expr) {
Some(e) if !e.call_site.from_expansion() => e,
_ => return,
};

// Operate on the only argument of `alloc::fmt::format`.
if let Some(sugg) = on_new_v1(cx, expr) {
span_useless_format(cx, span, "consider using `.to_string()`", sugg);
} else if let Some(sugg) = on_new_v1_fmt(cx, expr) {
span_useless_format(cx, span, "consider using `.to_string()`", sugg);
}
}
}

fn span_useless_format<T: LintContext>(cx: &T, span: Span, help: &str, mut sugg: String) {
let to_replace = span.source_callsite();

// The callsite span contains the statement semicolon for some reason.
let snippet = snippet(cx, to_replace, "..");
if snippet.ends_with(';') {
sugg.push(';');
}

span_lint_and_then(cx, USELESS_FORMAT, span, "useless use of `format!`", |diag| {
diag.span_suggestion(
to_replace,
help,
sugg,
Applicability::MachineApplicable, // snippet
);
});
}

fn on_argumentv1_new<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, arms: &'tcx [Arm<'_>]) -> Option<String> {
if_chain! {
if let ExprKind::AddrOf(BorrowKind::Ref, _, format_args) = expr.kind;
if let ExprKind::Array(elems) = arms[0].body.kind;
if elems.len() == 1;
if let Some(args) = match_function_call(cx, &elems[0], &paths::FMT_ARGUMENTV1_NEW);
// matches `core::fmt::Display::fmt`
if args.len() == 2;
if let ExprKind::Path(ref qpath) = args[1].kind;
if let Some(did) = cx.qpath_res(qpath, args[1].hir_id).opt_def_id();
if match_def_path(cx, did, &paths::DISPLAY_FMT_METHOD);
// check `(arg0,)` in match block
if let PatKind::Tuple(pats, None) = arms[0].pat.kind;
if pats.len() == 1;
then {
let ty = cx.typeck_results().pat_ty(pats[0]).peel_refs();
if *ty.kind() != rustc_middle::ty::Str && !is_type_diagnostic_item(cx, ty, sym::string_type) {
return None;
}
if let ExprKind::Lit(ref lit) = format_args.kind {
if let LitKind::Str(ref s, _) = lit.node {
return Some(format!("{:?}.to_string()", s.as_str()));
let mut applicability = Applicability::MachineApplicable;
if format_args.value_args.is_empty() {
if_chain! {
if let [e] = &*format_args.format_string_parts;
if let ExprKind::Lit(lit) = &e.kind;
if let Some(s_src) = snippet_opt(cx, lit.span);
then {
// Simulate macro expansion, converting {{ and }} to { and }.
let s_expand = s_src.replace("{{", "{").replace("}}", "}");
let sugg = format!("{}.to_string()", s_expand);
span_useless_format(cx, call_site, sugg, applicability);
}
} else {
let sugg = Sugg::hir(cx, format_args, "<arg>");
if let ExprKind::MethodCall(path, _, _, _) = format_args.kind {
if path.ident.name == sym!(to_string) {
return Some(format!("{}", sugg));
}
} else if let ExprKind::Binary(..) = format_args.kind {
return Some(format!("{}", sugg));
}
} else if let [value] = *format_args.value_args {
if_chain! {
if format_args.format_string_symbols == [kw::Empty];
if match cx.typeck_results().expr_ty(value).peel_refs().kind() {
ty::Adt(adt, _) => cx.tcx.is_diagnostic_item(sym::string_type, adt.did),
ty::Str => true,
_ => false,
};
if format_args.args.iter().all(|e| is_display_arg(e));
if format_args.fmt_expr.map_or(true, |e| check_unformatted(e));
then {
let is_new_string = match value.kind {
ExprKind::Binary(..) => true,
ExprKind::MethodCall(path, ..) => path.ident.name.as_str() == "to_string",
_ => false,
};
let sugg = if is_new_string {
snippet_with_applicability(cx, value.span, "..", &mut applicability).into_owned()
} else {
let sugg = Sugg::hir_with_applicability(cx, value, "<arg>", &mut applicability);
format!("{}.to_string()", sugg.maybe_par())
};
span_useless_format(cx, call_site, sugg, applicability);
}
return Some(format!("{}.to_string()", sugg.maybe_par()));
}
}
};
}
None
}

fn on_new_v1<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<String> {
if_chain! {
if let Some(args) = match_function_call(cx, expr, &paths::FMT_ARGUMENTS_NEW_V1);
if args.len() == 2;
// Argument 1 in `new_v1()`
if let ExprKind::AddrOf(BorrowKind::Ref, _, arr) = args[0].kind;
if let ExprKind::Array(pieces) = arr.kind;
if pieces.len() == 1;
if let ExprKind::Lit(ref lit) = pieces[0].kind;
if let LitKind::Str(ref s, _) = lit.node;
// Argument 2 in `new_v1()`
if let ExprKind::AddrOf(BorrowKind::Ref, _, arg1) = args[1].kind;
if let ExprKind::Match(matchee, arms, MatchSource::Normal) = arg1.kind;
if arms.len() == 1;
if let ExprKind::Tup(tup) = matchee.kind;
then {
// `format!("foo")` expansion contains `match () { () => [], }`
if tup.is_empty() {
if let Some(s_src) = snippet_opt(cx, lit.span) {
// Simulate macro expansion, converting {{ and }} to { and }.
let s_expand = s_src.replace("{{", "{").replace("}}", "}");
return Some(format!("{}.to_string()", s_expand));
}
} else if s.as_str().is_empty() {
return on_argumentv1_new(cx, &tup[0], arms);
}
}
fn span_useless_format(cx: &LateContext<'_>, span: Span, mut sugg: String, mut applicability: Applicability) {
// The callsite span contains the statement semicolon for some reason.
if snippet_with_applicability(cx, span, "..", &mut applicability).ends_with(';') {
sugg.push(';');
}
None

span_lint_and_sugg(
cx,
USELESS_FORMAT,
span,
"useless use of `format!`",
"consider using `.to_string()`",
sugg,
applicability,
);
}

fn on_new_v1_fmt<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<String> {
fn is_display_arg(expr: &Expr<'_>) -> bool {
if_chain! {
if let Some(args) = match_function_call(cx, expr, &paths::FMT_ARGUMENTS_NEW_V1_FORMATTED);
if args.len() == 3;
if check_unformatted(&args[2]);
// Argument 1 in `new_v1_formatted()`
if let ExprKind::AddrOf(BorrowKind::Ref, _, arr) = args[0].kind;
if let ExprKind::Array(pieces) = arr.kind;
if pieces.len() == 1;
if let ExprKind::Lit(ref lit) = pieces[0].kind;
if let LitKind::Str(..) = lit.node;
// Argument 2 in `new_v1_formatted()`
if let ExprKind::AddrOf(BorrowKind::Ref, _, arg1) = args[1].kind;
if let ExprKind::Match(matchee, arms, MatchSource::Normal) = arg1.kind;
if arms.len() == 1;
if let ExprKind::Tup(tup) = matchee.kind;
then {
return on_argumentv1_new(cx, &tup[0], arms);
}
if let ExprKind::Call(_, [_, fmt]) = expr.kind;
if let ExprKind::Path(QPath::Resolved(_, path)) = fmt.kind;
if let [.., t, _] = path.segments;
if t.ident.name.as_str() == "Display";
then { true } else { false }
}
None
}

/// Checks if the expression matches
Expand All @@ -184,10 +131,9 @@ fn on_new_v1_fmt<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<S
fn check_unformatted(expr: &Expr<'_>) -> bool {
if_chain! {
if let ExprKind::AddrOf(BorrowKind::Ref, _, expr) = expr.kind;
if let ExprKind::Array(exprs) = expr.kind;
if exprs.len() == 1;
if let ExprKind::Array([expr]) = expr.kind;
// struct `core::fmt::rt::v1::Argument`
if let ExprKind::Struct(_, fields, _) = exprs[0].kind;
if let ExprKind::Struct(_, fields, _) = expr.kind;
if let Some(format_field) = fields.iter().find(|f| f.ident.name == sym::format);
// struct `core::fmt::rt::v1::FormatSpec`
if let ExprKind::Struct(_, fields, _) = format_field.expr.kind;
Expand Down
Loading