Skip to content

Commit

Permalink
Address review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@smoelius
smoelius committed Oct 8, 2021
1 parent 6045478 commit 9f2bd79
Show file tree
Hide file tree
Showing 9 changed files with 379 additions and 362 deletions.
7 changes: 4 additions & 3 deletions clippy_lints/src/format.rs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::higher::FormatExpn;
use clippy_utils::higher::{FormatArgsArg, FormatExpn};
use clippy_utils::source::{snippet_opt, snippet_with_applicability};
use clippy_utils::sugg::Sugg;
use if_chain::if_chain;
Expand Down Expand Up @@ -68,8 +68,9 @@ impl<'tcx> LateLintPass<'tcx> for UselessFormat {
ty::Str => true,
_ => false,
};
if format_args.args().all(|arg| arg.is_display());
if !format_args.has_string_formatting();
if let Some(args) = format_args.args();
if args.iter().all(FormatArgsArg::is_display);
if !args.iter().any(FormatArgsArg::has_string_formatting);
then {
let is_new_string = match value.kind {
ExprKind::Binary(..) => true,
Expand Down
294 changes: 105 additions & 189 deletions clippy_lints/src/format_args.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,15 @@ use clippy_utils::source::snippet_opt;
use clippy_utils::ty::implements_trait;
use clippy_utils::{get_trait_def_id, match_def_path, paths};
use if_chain::if_chain;
use rustc_ast::ast::LitKind;
use rustc_errors::Applicability;
use rustc_hir::def_id::DefId;
use rustc_hir::{Expr, ExprKind};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty::subst::GenericArg;
use rustc_middle::ty::Ty;
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::{BytePos, ExpnKind, Span, Symbol};
use rustc_span::{sym, BytePos, ExpnKind, Span, Symbol};

declare_clippy_lint! {
/// ### What it does
Expand All @@ -34,14 +38,6 @@ declare_clippy_lint! {
"`format!` used in a macro that does formatting"
}

declare_lint_pass!(FormatInFormatArgs => [FORMAT_IN_FORMAT_ARGS]);

impl<'tcx> LateLintPass<'tcx> for FormatInFormatArgs {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
check_expr(cx, expr, format_in_format_args);
}
}

declare_clippy_lint! {
/// ### What it does
/// Checks for [`ToString::to_string`](https://doc.rust-lang.org/std/string/trait.ToString.html#tymethod.to_string)
Expand All @@ -67,216 +63,115 @@ declare_clippy_lint! {
"`to_string` applied to a type that implements `Display` in format args"
}

declare_lint_pass!(ToStringInFormatArgs => [TO_STRING_IN_FORMAT_ARGS]);

impl<'tcx> LateLintPass<'tcx> for ToStringInFormatArgs {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
check_expr(cx, expr, to_string_in_format_args);
}
}

fn check_expr<'tcx, F>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, check_value: F)
where
F: Fn(&LateContext<'_>, &FormatArgsExpn<'_>, Span, Symbol, usize, &FormatArgsArg<'_>) -> bool,
{
if_chain! {
if let Some(format_args) = FormatArgsExpn::parse(expr);
let call_site = expr.span.ctxt().outer_expn_data().call_site;
if call_site.from_expansion();
let expn_data = call_site.ctxt().outer_expn_data();
if let ExpnKind::Macro(_, name) = expn_data.kind;
if !format_args.has_string_formatting();
then {
for (i, arg) in format_args.args().enumerate() {
if !arg.is_display() {
continue;
declare_lint_pass!(FormatArgs => [FORMAT_IN_FORMAT_ARGS, TO_STRING_IN_FORMAT_ARGS]);

impl<'tcx> LateLintPass<'tcx> for FormatArgs {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
if_chain! {
if let Some(format_args) = FormatArgsExpn::parse(expr);
let expn_data = {
let expn_data = expr.span.ctxt().outer_expn_data();
if expn_data.call_site.from_expansion() {
expn_data.call_site.ctxt().outer_expn_data()
} else {
expn_data
}
if check_value(cx, &format_args, expn_data.call_site, name, i, &arg) {
break;
};
if let ExpnKind::Macro(_, name) = expn_data.kind;
if let Some(args) = format_args.args();
if !args.iter().any(FormatArgsArg::has_string_formatting);
then {
for (i, arg) in args.iter().enumerate() {
if !arg.is_display() {
continue;
}
if is_aliased(&args, i) {
continue;
}
check_format_in_format_args(cx, expn_data.call_site, name, arg);
check_to_string_in_format_args(cx, expn_data.call_site, name, arg);
}
}
}
}
}

fn format_in_format_args(
cx: &LateContext<'_>,
format_args: &FormatArgsExpn<'_>,
call_site: Span,
name: Symbol,
i: usize,
arg: &FormatArgsArg<'_>,
) -> bool {
if let Some(FormatExpn {
format_args: inner_format_args,
..
}) = FormatExpn::parse(arg.value())
{
if let Some((sugg, applicability)) = format_in_format_args_sugg(cx, name, format_args, i, &inner_format_args) {
span_lint_and_sugg(
cx,
FORMAT_IN_FORMAT_ARGS,
trim_semicolon(cx, call_site),
&format!("`format!` in `{}!` args", name),
"inline the `format!` call",
sugg,
applicability,
);
} else {
span_lint_and_help(
cx,
FORMAT_IN_FORMAT_ARGS,
trim_semicolon(cx, call_site),
&format!("`format!` in `{}!` args", name),
None,
"inline the `format!` call",
);
}
// Report only the first instance.
return true;
fn check_format_in_format_args(cx: &LateContext<'_>, call_site: Span, name: Symbol, arg: &FormatArgsArg<'_, '_>) {
if FormatExpn::parse(arg.value).is_some() {
span_lint_and_help(
cx,
FORMAT_IN_FORMAT_ARGS,
trim_semicolon(cx, call_site),
&format!("`format!` in `{}!` args", name),
None,
"inline the `format!` call",
);
}
false
}

fn to_string_in_format_args(
cx: &LateContext<'_>,
_format_args: &FormatArgsExpn<'_>,
fn check_to_string_in_format_args<'tcx>(
cx: &LateContext<'tcx>,
_call_site: Span,
name: Symbol,
_i: usize,
arg: &FormatArgsArg<'_>,
) -> bool {
let value = arg.value();
arg: &FormatArgsArg<'_, 'tcx>,
) {
let value = arg.value;
if_chain! {
if let ExprKind::MethodCall(_, _, args, _) = value.kind;
if let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(value.hir_id);
if match_def_path(cx, method_def_id, &paths::TO_STRING_METHOD);
if let Some(receiver) = args.get(0);
let ty = cx.typeck_results().expr_ty(receiver);
if let Some(display_trait_id) = get_trait_def_id(cx, &paths::DISPLAY_TRAIT);
if implements_trait(cx, ty, display_trait_id, &[]);
if let Some(snippet) = snippet_opt(cx, value.span);
if let Some(dot) = snippet.rfind('.');
then {
let span = value.span.with_lo(
value.span.lo() + BytePos(u32::try_from(dot).unwrap())
);
span_lint_and_sugg(
cx,
TO_STRING_IN_FORMAT_ARGS,
span,
&format!("`to_string` applied to a type that implements `Display` in `{}!` args", name),
"remove this",
String::new(),
Applicability::MachineApplicable,
);
}
}
false
}

fn format_in_format_args_sugg(
cx: &LateContext<'_>,
name: Symbol,
format_args: &FormatArgsExpn<'_>,
i: usize,
inner_format_args: &FormatArgsExpn<'_>,
) -> Option<(String, Applicability)> {
let format_string = snippet_opt(cx, format_args.format_string_span)?;
if is_positional(&format_string) {
return None;
}
let (left, right) = split_format_string(&format_string, i);
let inner_format_string = snippet_opt(cx, inner_format_args.format_string_span)?;
if is_positional(&inner_format_string) {
return None;
}
// If the inner format string is raw, the user is on their own.
let (new_format_string, applicability) = if inner_format_string.starts_with('r') {
(left + ".." + &right, Applicability::HasPlaceholders)
} else {
(
left + &trim_quotes(&inner_format_string) + &right,
Applicability::MachineApplicable,
)
};
let values = format_args
.value_args
.iter()
.map(|value| snippet_opt(cx, value.span))
.collect::<Option<Vec<_>>>()?;
let inner_values = inner_format_args
.value_args
.iter()
.map(|value| snippet_opt(cx, value.span))
.collect::<Option<Vec<_>>>()?;
let new_values = itertools::join(
values
.iter()
.take(i)
.chain(inner_values.iter())
.chain(values.iter().skip(i + 1)),
", ",
);
Some((
format!(r#"{}!({}, {})"#, name, new_format_string, new_values),
applicability,
))
}

// Checking the position fields of the `core::fmt::rt::v1::Argument` would not be sufficient
// because, for example, "{}{}" and "{}{1:}" could not be distinguished. Note that the `{}` could be
// replaced in the former, but not the latter.
fn is_positional(format_string: &str) -> bool {
let mut iter = format_string.chars();
while let Some(first_char) = iter.next() {
if first_char != '{' {
continue;
}
let second_char = iter.next().unwrap();
if second_char.is_digit(10) {
return true;
if implements_trait(cx, ty, display_trait_id, &[]) {
span_lint_and_sugg(
cx,
TO_STRING_IN_FORMAT_ARGS,
span,
&format!("`to_string` applied to a type that implements `Display` in `{}!` args", name),
"remove this",
String::new(),
Applicability::MachineApplicable,
);
} else if implements_trait_via_deref(cx, ty, display_trait_id, &[]) {
span_lint_and_sugg(
cx,
TO_STRING_IN_FORMAT_ARGS,
span,
&format!("`to_string` applied to a type that implements `Display` in `{}!` args", name),
"use this",
String::from(".deref()"),
Applicability::MachineApplicable,
);
}
}
}
false
}

fn split_format_string(format_string: &str, i: usize) -> (String, String) {
let mut iter = format_string.chars();
for j in 0..=i {
assert!(advance(&mut iter) == '}' || j < i);
}

let right = iter.collect::<String>();

let size = format_string.len();
let right_size = right.len();
let left_size = size - right_size;
assert!(left_size >= 2);
let left = std::str::from_utf8(&format_string.as_bytes()[..left_size - 2])
.unwrap()
.to_owned();
(left, right)
}

fn advance(iter: &mut std::str::Chars<'_>) -> char {
loop {
let first_char = iter.next().unwrap();
if first_char != '{' {
continue;
}
let second_char = iter.next().unwrap();
if second_char == '{' {
continue;
// Returns true if `args[i]` "refers to" or "is referred to by" another argument.
fn is_aliased(args: &[FormatArgsArg<'_, '_>], i: usize) -> bool {
args.iter().enumerate().any(|(j, arg)| {
if_chain! {
if let Some(fmt) = arg.fmt;
// struct `core::fmt::rt::v1::Argument`
if let ExprKind::Struct(_, fields, _) = fmt.kind;
if let Some(position_field) = fields.iter().find(|f| f.ident.name == sym::position);
if let ExprKind::Lit(lit) = &position_field.expr.kind;
if let LitKind::Int(position, _) = lit.node;
then {
let position = usize::try_from(position).unwrap();
(j == i && position != i) || (j != i && position == i)
} else {
false
}
}
return second_char;
}
}

fn trim_quotes(string_literal: &str) -> String {
let len = string_literal.chars().count();
assert!(len >= 2);
string_literal.chars().skip(1).take(len - 2).collect()
})
}

fn trim_semicolon(cx: &LateContext<'_>, span: Span) -> Span {
Expand All @@ -285,3 +180,24 @@ fn trim_semicolon(cx: &LateContext<'_>, span: Span) -> Span {
span.with_hi(span.lo() + BytePos(u32::try_from(snippet.len()).unwrap()))
})
}

fn implements_trait_via_deref<'tcx>(
cx: &LateContext<'tcx>,
ty: Ty<'tcx>,
trait_id: DefId,
ty_params: &[GenericArg<'tcx>],
) -> bool {
if_chain! {
if let Some(deref_trait_id) = cx.tcx.lang_items().deref_trait();
if implements_trait(cx, ty, deref_trait_id, &[]);
if let Some(deref_target_id) = cx.tcx.lang_items().deref_target();
then {
let substs = cx.tcx.mk_substs_trait(ty, &[]);
let projection_ty = cx.tcx.mk_projection(deref_target_id, substs);
let target_ty = cx.tcx.normalize_erasing_regions(cx.param_env, projection_ty);
implements_trait(cx, target_ty, trait_id, ty_params)
} else {
false
}
}
}
3 changes: 1 addition & 2 deletions clippy_lints/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -772,8 +772,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
let enable_raw_pointer_heuristic_for_send = conf.enable_raw_pointer_heuristic_for_send;
store.register_late_pass(move || Box::new(non_send_fields_in_send_ty::NonSendFieldInSendTy::new(enable_raw_pointer_heuristic_for_send)));
store.register_late_pass(move || Box::new(undocumented_unsafe_blocks::UndocumentedUnsafeBlocks::default()));
store.register_late_pass(move || Box::new(format_args::FormatInFormatArgs));
store.register_late_pass(move || Box::new(format_args::ToStringInFormatArgs));
store.register_late_pass(move || Box::new(format_args::FormatArgs));
}

#[rustfmt::skip]
Expand Down
Loading

0 comments on commit 9f2bd79

Please sign in to comment.