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

Refactor FormatArgsExpn #9349

Merged
merged 1 commit into from
Aug 19, 2022
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
24 changes: 11 additions & 13 deletions clippy_lints/src/format.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::macros::{root_macro_call_first_node, FormatArgsExpn};
use clippy_utils::source::{snippet_opt, snippet_with_applicability};
use clippy_utils::source::snippet_with_applicability;
use clippy_utils::sugg::Sugg;
use if_chain::if_chain;
use rustc_errors::Applicability;
Expand Down Expand Up @@ -56,29 +56,27 @@ impl<'tcx> LateLintPass<'tcx> for UselessFormat {
};

let mut applicability = Applicability::MachineApplicable;
if format_args.value_args.is_empty() {
match *format_args.format_string_parts {
if format_args.args.is_empty() {
match *format_args.format_string.parts {
[] => span_useless_format_empty(cx, call_site, "String::new()".to_owned(), applicability),
[_] => {
if let Some(s_src) = snippet_opt(cx, format_args.format_string_span) {
// 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);
}
// Simulate macro expansion, converting {{ and }} to { and }.
let s_expand = format_args.format_string.snippet.replace("{{", "{").replace("}}", "}");
let sugg = format!("{}.to_string()", s_expand);
span_useless_format(cx, call_site, sugg, applicability);
},
[..] => {},
}
} else if let [value] = *format_args.value_args {
} else if let [arg] = &*format_args.args {
let value = arg.param.value;
if_chain! {
if format_args.format_string_parts == [kw::Empty];
if format_args.format_string.parts == [kw::Empty];
if match cx.typeck_results().expr_ty(value).peel_refs().kind() {
ty::Adt(adt, _) => cx.tcx.is_diagnostic_item(sym::String, adt.did()),
ty::Str => true,
_ => false,
};
if let Some(args) = format_args.args();
if args.iter().all(|arg| arg.format_trait == sym::Display && !arg.has_string_formatting());
if !arg.format.has_string_formatting();
then {
let is_new_string = match value.kind {
ExprKind::Binary(..) => true,
Expand Down
31 changes: 14 additions & 17 deletions clippy_lints/src/format_args.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
use clippy_utils::is_diag_trait_item;
use clippy_utils::macros::{is_format_macro, FormatArgsArg, FormatArgsExpn};
use clippy_utils::macros::{is_format_macro, FormatArgsExpn};
use clippy_utils::source::snippet_opt;
use clippy_utils::ty::implements_trait;
use if_chain::if_chain;
use itertools::Itertools;
use rustc_errors::Applicability;
use rustc_hir::{Expr, ExprKind};
use rustc_hir::{Expr, ExprKind, HirId};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty::adjustment::{Adjust, Adjustment};
use rustc_middle::ty::Ty;
Expand Down Expand Up @@ -74,20 +75,16 @@ impl<'tcx> LateLintPass<'tcx> for FormatArgs {
if let Some(macro_def_id) = outermost_expn_data.macro_def_id;
if is_format_macro(cx, macro_def_id);
if let ExpnKind::Macro(_, name) = outermost_expn_data.kind;
if let Some(args) = format_args.args();
then {
for (i, arg) in args.iter().enumerate() {
if arg.format_trait != sym::Display {
for arg in &format_args.args {
if arg.format.has_string_formatting() {
continue;
}
if arg.has_string_formatting() {
if is_aliased(&format_args, arg.param.value.hir_id) {
continue;
}
if is_aliased(&args, i) {
continue;
}
check_format_in_format_args(cx, outermost_expn_data.call_site, name, arg.value);
check_to_string_in_format_args(cx, name, arg.value);
check_format_in_format_args(cx, outermost_expn_data.call_site, name, arg.param.value);
check_to_string_in_format_args(cx, name, arg.param.value);
}
}
}
Expand Down Expand Up @@ -167,12 +164,12 @@ fn check_to_string_in_format_args(cx: &LateContext<'_>, name: Symbol, value: &Ex
}
}

// Returns true if `args[i]` "refers to" or "is referred to by" another argument.
fn is_aliased(args: &[FormatArgsArg<'_>], i: usize) -> bool {
let value = args[i].value;
args.iter()
.enumerate()
.any(|(j, arg)| i != j && std::ptr::eq(value, arg.value))
// Returns true if `hir_id` is referred to by multiple format params
fn is_aliased(args: &FormatArgsExpn<'_>, hir_id: HirId) -> bool {
args.params()
.filter(|param| param.value.hir_id == hir_id)
.at_most_one()
.is_err()
flip1995 marked this conversation as resolved.
Show resolved Hide resolved
}

fn count_needed_derefs<'tcx, I>(mut ty: Ty<'tcx>, mut iter: I) -> (usize, Ty<'tcx>)
Expand Down
11 changes: 5 additions & 6 deletions clippy_lints/src/format_impl.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use clippy_utils::diagnostics::{span_lint, span_lint_and_sugg};
use clippy_utils::macros::{is_format_macro, root_macro_call_first_node, FormatArgsArg, FormatArgsExpn};
use clippy_utils::macros::{is_format_macro, root_macro_call_first_node, FormatArg, FormatArgsExpn};
use clippy_utils::{get_parent_as_impl, is_diag_trait_item, path_to_local, peel_ref_operators};
use if_chain::if_chain;
use rustc_errors::Applicability;
Expand Down Expand Up @@ -168,10 +168,9 @@ fn check_self_in_format_args<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>,
if let macro_def_id = outer_macro.def_id;
if let Some(format_args) = FormatArgsExpn::find_nested(cx, expr, outer_macro.expn);
if is_format_macro(cx, macro_def_id);
if let Some(args) = format_args.args();
then {
for arg in args {
if arg.format_trait != impl_trait.name {
for arg in format_args.args {
if arg.format.r#trait != impl_trait.name {
continue;
}
check_format_arg_self(cx, expr, &arg, impl_trait);
Expand All @@ -180,11 +179,11 @@ fn check_self_in_format_args<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>,
}
}

fn check_format_arg_self(cx: &LateContext<'_>, expr: &Expr<'_>, arg: &FormatArgsArg<'_>, impl_trait: FormatTrait) {
fn check_format_arg_self(cx: &LateContext<'_>, expr: &Expr<'_>, arg: &FormatArg<'_>, impl_trait: FormatTrait) {
// Handle multiple dereferencing of references e.g. &&self
// Handle dereference of &self -> self that is equivalent (i.e. via *self in fmt() impl)
// Since the argument to fmt is itself a reference: &self
let reference = peel_ref_operators(cx, arg.value);
let reference = peel_ref_operators(cx, arg.param.value);
let map = cx.tcx.hir();
// Is the reference self?
if path_to_local(reference).map(|x| map.name(x)) == Some(kw::SelfLower) {
Expand Down
4 changes: 2 additions & 2 deletions clippy_lints/src/methods/uninit_assumed_init.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use clippy_utils::diagnostics::span_lint;
use clippy_utils::{is_expr_diagnostic_item, ty::is_uninit_value_valid_for_ty};
use clippy_utils::{is_path_diagnostic_item, ty::is_uninit_value_valid_for_ty};
use if_chain::if_chain;
use rustc_hir as hir;
use rustc_lint::LateContext;
Expand All @@ -12,7 +12,7 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr
if_chain! {
if let hir::ExprKind::Call(callee, args) = recv.kind;
if args.is_empty();
if is_expr_diagnostic_item(cx, callee, sym::maybe_uninit_uninit);
if is_path_diagnostic_item(cx, callee, sym::maybe_uninit_uninit);
if !is_uninit_value_valid_for_ty(cx, cx.typeck_results().expr_ty_adjusted(expr));
then {
span_lint(
Expand Down
4 changes: 2 additions & 2 deletions clippy_lints/src/transmute/transmuting_null.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use clippy_utils::consts::{constant_context, Constant};
use clippy_utils::diagnostics::span_lint;
use clippy_utils::is_expr_diagnostic_item;
use clippy_utils::is_path_diagnostic_item;
use if_chain::if_chain;
use rustc_ast::LitKind;
use rustc_hir::{Expr, ExprKind};
Expand Down Expand Up @@ -45,7 +45,7 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, arg: &'t
// `std::mem::transmute(std::ptr::null::<i32>())`
if_chain! {
if let ExprKind::Call(func1, []) = arg.kind;
if is_expr_diagnostic_item(cx, func1, sym::ptr_null);
if is_path_diagnostic_item(cx, func1, sym::ptr_null);
then {
span_lint(cx, TRANSMUTING_NULL, expr.span, LINT_MSG);
return true;
Expand Down
1 change: 1 addition & 0 deletions clippy_utils/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ publish = false
[dependencies]
arrayvec = { version = "0.7", default-features = false }
if_chain = "1.0"
itertools = "0.10.1"
rustc-semver = "1.1"

[features]
Expand Down
15 changes: 10 additions & 5 deletions clippy_utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ extern crate rustc_infer;
extern crate rustc_lexer;
extern crate rustc_lint;
extern crate rustc_middle;
extern crate rustc_parse_format;
extern crate rustc_session;
extern crate rustc_span;
extern crate rustc_target;
Expand Down Expand Up @@ -371,15 +372,19 @@ pub fn match_qpath(path: &QPath<'_>, segments: &[&str]) -> bool {

/// If the expression is a path, resolves it to a `DefId` and checks if it matches the given path.
///
/// Please use `is_expr_diagnostic_item` if the target is a diagnostic item.
/// Please use `is_path_diagnostic_item` if the target is a diagnostic item.
pub fn is_expr_path_def_path(cx: &LateContext<'_>, expr: &Expr<'_>, segments: &[&str]) -> bool {
path_def_id(cx, expr).map_or(false, |id| match_def_path(cx, id, segments))
}

/// If the expression is a path, resolves it to a `DefId` and checks if it matches the given
/// diagnostic item.
pub fn is_expr_diagnostic_item(cx: &LateContext<'_>, expr: &Expr<'_>, diag_item: Symbol) -> bool {
path_def_id(cx, expr).map_or(false, |id| cx.tcx.is_diagnostic_item(diag_item, id))
/// If `maybe_path` is a path node which resolves to an item, resolves it to a `DefId` and checks if
/// it matches the given diagnostic item.
pub fn is_path_diagnostic_item<'tcx>(
flip1995 marked this conversation as resolved.
Show resolved Hide resolved
cx: &LateContext<'_>,
maybe_path: &impl MaybePath<'tcx>,
diag_item: Symbol,
) -> bool {
path_def_id(cx, maybe_path).map_or(false, |id| cx.tcx.is_diagnostic_item(diag_item, id))
}

/// THIS METHOD IS DEPRECATED and will eventually be removed since it does not match against the
Expand Down
Loading