Auto merge of #10275 - Alexendoo:format-args-ast, r=flip1995

bors · bors · commit 717577f7a280 · 2023-03-06T20:43:50.000Z
Migrate `write.rs` to `rustc_ast::FormatArgs` changelog: none Part 1 of #10233 The additions to `clippy_utils` are the main novelty of this PR, there's no removals yet since other parts still rely on `FormatArgsExpn` The changes to `write.rs` itself are relatively straightforward this time around, as there's no lints in it that rely on type checking format params r? `@flip1995`
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -533,6 +533,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
                 .collect(),
         ))
     });
+    store.register_early_pass(|| Box::new(utils::format_args_collector::FormatArgsCollector));
     store.register_late_pass(|_| Box::new(utils::dump_hir::DumpHir));
     store.register_late_pass(|_| Box::new(utils::author::Author));
     let await_holding_invalid_types = conf.await_holding_invalid_types.clone();
diff --git a/clippy_lints/src/utils/format_args_collector.rs b/clippy_lints/src/utils/format_args_collector.rs
@@ -0,0 +1,23 @@
+use clippy_utils::macros::collect_ast_format_args;
+use rustc_ast::{Expr, ExprKind};
+use rustc_lint::{EarlyContext, EarlyLintPass};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Collects [`rustc_ast::FormatArgs`] so that future late passes can call
+    /// [`clippy_utils::macros::find_format_args`]
+    pub FORMAT_ARGS_COLLECTOR,
+    internal_warn,
+    "collects `format_args` AST nodes for use in later lints"
+}
+
+declare_lint_pass!(FormatArgsCollector => [FORMAT_ARGS_COLLECTOR]);
+
+impl EarlyLintPass for FormatArgsCollector {
+    fn check_expr(&mut self, _: &EarlyContext<'_>, expr: &Expr) {
+        if let ExprKind::FormatArgs(args) = &expr.kind {
+            collect_ast_format_args(expr.span, args);
+        }
+    }
+}
diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs
@@ -1,5 +1,6 @@
 pub mod author;
 pub mod conf;
 pub mod dump_hir;
+pub mod format_args_collector;
 #[cfg(feature = "internal")]
 pub mod internal_lints;
diff --git a/clippy_lints/src/write.rs b/clippy_lints/src/write.rs
@@ -1,10 +1,11 @@
 use clippy_utils::diagnostics::{span_lint, span_lint_and_then};
-use clippy_utils::macros::{root_macro_call_first_node, FormatArgsExpn, MacroCall};
+use clippy_utils::macros::{find_format_args, format_arg_removal_span, root_macro_call_first_node, MacroCall};
 use clippy_utils::source::{expand_past_previous_comma, snippet_opt};
 use clippy_utils::{is_in_cfg_test, is_in_test_function};
-use rustc_ast::LitKind;
+use rustc_ast::token::LitKind;
+use rustc_ast::{FormatArgPosition, FormatArgs, FormatArgsPiece, FormatOptions, FormatPlaceholder, FormatTrait};
 use rustc_errors::Applicability;
-use rustc_hir::{Expr, ExprKind, HirIdMap, Impl, Item, ItemKind};
+use rustc_hir::{Expr, Impl, Item, ItemKind};
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_session::{declare_tool_lint, impl_lint_pass};
 use rustc_span::{sym, BytePos};
@@ -297,34 +298,40 @@ impl<'tcx> LateLintPass<'tcx> for Write {
             _ => return,
         }
 
-        let Some(format_args) = FormatArgsExpn::find_nested(cx, expr, macro_call.expn) else { return };
-
-        // ignore `writeln!(w)` and `write!(v, some_macro!())`
-        if format_args.format_string.span.from_expansion() {
-            return;
-        }
+        find_format_args(cx, expr, macro_call.expn, |format_args| {
+            // ignore `writeln!(w)` and `write!(v, some_macro!())`
+            if format_args.span.from_expansion() {
+                return;
+            }
 
-        match diag_name {
-            sym::print_macro | sym::eprint_macro | sym::write_macro => {
-                check_newline(cx, &format_args, &macro_call, name);
-            },
-            sym::println_macro | sym::eprintln_macro | sym::writeln_macro => {
-                check_empty_string(cx, &format_args, &macro_call, name);
-            },
-            _ => {},
-        }
+            match diag_name {
+                sym::print_macro | sym::eprint_macro | sym::write_macro => {
+                    check_newline(cx, format_args, &macro_call, name);
+                },
+                sym::println_macro | sym::eprintln_macro | sym::writeln_macro => {
+                    check_empty_string(cx, format_args, &macro_call, name);
+                },
+                _ => {},
+            }
 
-        check_literal(cx, &format_args, name);
+            check_literal(cx, format_args, name);
 
-        if !self.in_debug_impl {
-            for arg in &format_args.args {
-                if arg.format.r#trait == sym::Debug {
-                    span_lint(cx, USE_DEBUG, arg.span, "use of `Debug`-based formatting");
+            if !self.in_debug_impl {
+                for piece in &format_args.template {
+                    if let &FormatArgsPiece::Placeholder(FormatPlaceholder {
+                        span: Some(span),
+                        format_trait: FormatTrait::Debug,
+                        ..
+                    }) = piece
+                    {
+                        span_lint(cx, USE_DEBUG, span, "use of `Debug`-based formatting");
+                    }
                 }
             }
-        }
+        });
     }
 }
+
 fn is_debug_impl(cx: &LateContext<'_>, item: &Item<'_>) -> bool {
     if let ItemKind::Impl(Impl { of_trait: Some(trait_ref), .. }) = &item.kind
         && let Some(trait_id) = trait_ref.trait_def_id()
@@ -335,27 +342,28 @@ fn is_debug_impl(cx: &LateContext<'_>, item: &Item<'_>) -> bool {
     }
 }
 
-fn check_newline(cx: &LateContext<'_>, format_args: &FormatArgsExpn<'_>, macro_call: &MacroCall, name: &str) {
-    let format_string_parts = &format_args.format_string.parts;
-    let mut format_string_span = format_args.format_string.span;
-
-    let Some(last) = format_string_parts.last() else { return };
+fn check_newline(cx: &LateContext<'_>, format_args: &FormatArgs, macro_call: &MacroCall, name: &str) {
+    let Some(FormatArgsPiece::Literal(last)) = format_args.template.last() else { return };
 
     let count_vertical_whitespace = || {
-        format_string_parts
+        format_args
+            .template
             .iter()
-            .flat_map(|part| part.as_str().chars())
+            .filter_map(|piece| match piece {
+                FormatArgsPiece::Literal(literal) => Some(literal),
+                FormatArgsPiece::Placeholder(_) => None,
+            })
+            .flat_map(|literal| literal.as_str().chars())
             .filter(|ch| matches!(ch, '\r' | '\n'))
             .count()
     };
 
     if last.as_str().ends_with('\n')
         // ignore format strings with other internal vertical whitespace
         && count_vertical_whitespace() == 1
-
-        // ignore trailing arguments: `print!("Issue\n{}", 1265);`
-        && format_string_parts.len() > format_args.args.len()
     {
+        let mut format_string_span = format_args.span;
+
         let lint = if name == "write" {
             format_string_span = expand_past_previous_comma(cx, format_string_span);
 
@@ -373,7 +381,7 @@ fn check_newline(cx: &LateContext<'_>, format_args: &FormatArgsExpn<'_>, macro_c
                 let name_span = cx.sess().source_map().span_until_char(macro_call.span, '!');
                 let Some(format_snippet) = snippet_opt(cx, format_string_span) else { return };
 
-                if format_string_parts.len() == 1 && last.as_str() == "\n" {
+                if format_args.template.len() == 1 && last.as_str() == "\n" {
                     // print!("\n"), write!(f, "\n")
 
                     diag.multipart_suggestion(
@@ -398,11 +406,12 @@ fn check_newline(cx: &LateContext<'_>, format_args: &FormatArgsExpn<'_>, macro_c
     }
 }
 
-fn check_empty_string(cx: &LateContext<'_>, format_args: &FormatArgsExpn<'_>, macro_call: &MacroCall, name: &str) {
-    if let [part] = &format_args.format_string.parts[..]
-        && let mut span = format_args.format_string.span
-        && part.as_str() == "\n"
+fn check_empty_string(cx: &LateContext<'_>, format_args: &FormatArgs, macro_call: &MacroCall, name: &str) {
+    if let [FormatArgsPiece::Literal(literal)] = &format_args.template[..]
+        && literal.as_str() == "\n"
     {
+        let mut span = format_args.span;
+
         let lint = if name == "writeln" {
             span = expand_past_previous_comma(cx, span);
 
@@ -428,33 +437,43 @@ fn check_empty_string(cx: &LateContext<'_>, format_args: &FormatArgsExpn<'_>, ma
     }
 }
 
-fn check_literal(cx: &LateContext<'_>, format_args: &FormatArgsExpn<'_>, name: &str) {
-    let mut counts = HirIdMap::<usize>::default();
-    for param in format_args.params() {
-        *counts.entry(param.value.hir_id).or_default() += 1;
+fn check_literal(cx: &LateContext<'_>, format_args: &FormatArgs, name: &str) {
+    let arg_index = |argument: &FormatArgPosition| argument.index.unwrap_or_else(|pos| pos);
+
+    let mut counts = vec![0u32; format_args.arguments.all_args().len()];
+    for piece in &format_args.template {
+        if let FormatArgsPiece::Placeholder(placeholder) = piece {
+            counts[arg_index(&placeholder.argument)] += 1;
+        }
     }
 
-    for arg in &format_args.args {
-        let value = arg.param.value;
-
-        if counts[&value.hir_id] == 1
-            && arg.format.is_default()
-            && let ExprKind::Lit(lit) = &value.kind
-            && !value.span.from_expansion()
-            && let Some(value_string) = snippet_opt(cx, value.span)
-        {
-            let (replacement, replace_raw) = match lit.node {
-                LitKind::Str(..) => extract_str_literal(&value_string),
-                LitKind::Char(ch) => (
-                    match ch {
-                        '"' => "\\\"",
-                        '\'' => "'",
+    for piece in &format_args.template {
+        if let FormatArgsPiece::Placeholder(FormatPlaceholder {
+            argument,
+            span: Some(placeholder_span),
+            format_trait: FormatTrait::Display,
+            format_options,
+        }) = piece
+            && *format_options == FormatOptions::default()
+            && let index = arg_index(argument)
+            && counts[index] == 1
+            && let Some(arg) = format_args.arguments.by_index(index)
+            && let rustc_ast::ExprKind::Lit(lit) = &arg.expr.kind
+            && !arg.expr.span.from_expansion()
+            && let Some(value_string) = snippet_opt(cx, arg.expr.span)
+    {
+            let (replacement, replace_raw) = match lit.kind {
+                LitKind::Str | LitKind::StrRaw(_)  => extract_str_literal(&value_string),
+                LitKind::Char => (
+                    match lit.symbol.as_str() {
+                        "\"" => "\\\"",
+                        "\\'" => "'",
                         _ => &value_string[1..value_string.len() - 1],
                     }
                     .to_string(),
                     false,
                 ),
-                LitKind::Bool(b) => (b.to_string(), false),
+                LitKind::Bool => (lit.symbol.to_string(), false),
                 _ => continue,
             };
 
@@ -464,7 +483,9 @@ fn check_literal(cx: &LateContext<'_>, format_args: &FormatArgsExpn<'_>, name: &
                 PRINT_LITERAL
             };
 
-            let format_string_is_raw = format_args.format_string.style.is_some();
+            let Some(format_string_snippet) = snippet_opt(cx, format_args.span) else { continue };
+            let format_string_is_raw = format_string_snippet.starts_with('r');
+
             let replacement = match (format_string_is_raw, replace_raw) {
                 (false, false) => Some(replacement),
                 (false, true) => Some(replacement.replace('"', "\\\"").replace('\\', "\\\\")),
@@ -485,23 +506,24 @@ fn check_literal(cx: &LateContext<'_>, format_args: &FormatArgsExpn<'_>, name: &
             span_lint_and_then(
                 cx,
                 lint,
-                value.span,
+                arg.expr.span,
                 "literal with an empty format string",
                 |diag| {
                     if let Some(replacement) = replacement
                         // `format!("{}", "a")`, `format!("{named}", named = "b")
                         //              ~~~~~                      ~~~~~~~~~~~~~
-                        && let Some(value_span) = format_args.value_with_prev_comma_span(value.hir_id)
+                        && let Some(removal_span) = format_arg_removal_span(format_args, index)
                     {
                         let replacement = replacement.replace('{', "{{").replace('}', "}}");
                         diag.multipart_suggestion(
                             "try this",
-                            vec![(arg.span, replacement), (value_span, String::new())],
+                            vec![(*placeholder_span, replacement), (removal_span, String::new())],
                             Applicability::MachineApplicable,
                         );
                     }
                 },
             );
+
         }
     }
 }
diff --git a/clippy_utils/src/macros.rs b/clippy_utils/src/macros.rs
@@ -6,6 +6,8 @@ use crate::visitors::{for_each_expr, Descend};
 use arrayvec::ArrayVec;
 use itertools::{izip, Either, Itertools};
 use rustc_ast::ast::LitKind;
+use rustc_ast::FormatArgs;
+use rustc_data_structures::fx::FxHashMap;
 use rustc_hir::intravisit::{walk_expr, Visitor};
 use rustc_hir::{self as hir, Expr, ExprField, ExprKind, HirId, LangItem, Node, QPath, TyKind};
 use rustc_lexer::unescape::unescape_literal;
@@ -15,8 +17,10 @@ use rustc_parse_format::{self as rpf, Alignment};
 use rustc_span::def_id::DefId;
 use rustc_span::hygiene::{self, MacroKind, SyntaxContext};
 use rustc_span::{sym, BytePos, ExpnData, ExpnId, ExpnKind, Pos, Span, SpanData, Symbol};
+use std::cell::RefCell;
 use std::iter::{once, zip};
 use std::ops::ControlFlow;
+use std::sync::atomic::{AtomicBool, Ordering};
 
 const FORMAT_MACRO_DIAG_ITEMS: &[Symbol] = &[
     sym::assert_eq_macro,
@@ -339,6 +343,77 @@ fn is_assert_arg(cx: &LateContext<'_>, expr: &Expr<'_>, assert_expn: ExpnId) ->
     }
 }
 
+thread_local! {
+    /// We preserve the [`FormatArgs`] structs from the early pass for use in the late pass to be
+    /// able to access the many features of a [`LateContext`].
+    ///
+    /// A thread local is used because [`FormatArgs`] is `!Send` and `!Sync`, we are making an
+    /// assumption that the early pass the populates the map and the later late passes will all be
+    /// running on the same thread.
+    static AST_FORMAT_ARGS: RefCell<FxHashMap<Span, FormatArgs>> = {
+        static CALLED: AtomicBool = AtomicBool::new(false);
+        debug_assert!(
+            !CALLED.swap(true, Ordering::SeqCst),
+            "incorrect assumption: `AST_FORMAT_ARGS` should only be accessed by a single thread",
+        );
+
+        RefCell::default()
+    };
+}
+
+/// Record [`rustc_ast::FormatArgs`] for use in late lint passes, this should only be called by
+/// `FormatArgsCollector`
+pub fn collect_ast_format_args(span: Span, format_args: &FormatArgs) {
+    AST_FORMAT_ARGS.with(|ast_format_args| {
+        ast_format_args.borrow_mut().insert(span, format_args.clone());
+    });
+}
+
+/// Calls `callback` with an AST [`FormatArgs`] node if one is found
+pub fn find_format_args(cx: &LateContext<'_>, start: &Expr<'_>, expn_id: ExpnId, callback: impl FnOnce(&FormatArgs)) {
+    let format_args_expr = for_each_expr(start, |expr| {
+        let ctxt = expr.span.ctxt();
+        if ctxt == start.span.ctxt() {
+            ControlFlow::Continue(Descend::Yes)
+        } else if ctxt.outer_expn().is_descendant_of(expn_id)
+            && macro_backtrace(expr.span)
+                .map(|macro_call| cx.tcx.item_name(macro_call.def_id))
+                .any(|name| matches!(name, sym::const_format_args | sym::format_args | sym::format_args_nl))
+        {
+            ControlFlow::Break(expr)
+        } else {
+            ControlFlow::Continue(Descend::No)
+        }
+    });
+
+    if let Some(format_args_expr) = format_args_expr {
+        AST_FORMAT_ARGS.with(|ast_format_args| {
+            ast_format_args.borrow().get(&format_args_expr.span).map(callback);
+        });
+    }
+}
+
+/// Returns the [`Span`] of the value at `index` extended to the previous comma, e.g. for the value
+/// `10`
+///
+/// ```ignore
+/// format("{}.{}", 10, 11)
+/// //            ^^^^
+/// ```
+pub fn format_arg_removal_span(format_args: &FormatArgs, index: usize) -> Option<Span> {
+    let ctxt = format_args.span.ctxt();
+
+    let current = hygiene::walk_chain(format_args.arguments.by_index(index)?.expr.span, ctxt);
+
+    let prev = if index == 0 {
+        format_args.span
+    } else {
+        hygiene::walk_chain(format_args.arguments.by_index(index - 1)?.expr.span, ctxt)
+    };
+
+    Some(current.with_lo(prev.hi()))
+}
+
 /// The format string doesn't exist in the HIR, so we reassemble it from source code
 #[derive(Debug)]
 pub struct FormatString {
