mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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

Browse Source 
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`
This commit is contained in:
bors 2023-03-10 09:38:18 +00:00
commit 3c06e0b1ce
10 changed files with 221 additions and 90 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
clippy_lints/src/lib.rs
View File

@ -537,6 +537,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();

23
clippy_lints/src/utils/format_args_collector.rs Normal file
View File

@ -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);
}
}
}

1
clippy_lints/src/utils/mod.rs
View File

@ -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;

148
clippy_lints/src/write.rs
View File

@ -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 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
_ => return,
}
let Some(format_args) = FormatArgsExpn::find_nested(cx, expr, macro_call.expn) else { 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;
}
// ignore `writeln!(w)` and `write!(v, some_macro!())`
if format_args.format_string.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,16 +342,18 @@ 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()
};
@ -352,10 +361,9 @@ fn check_newline(cx: &LateContext<'_>, format_args: &FormatArgsExpn<'_>, macro_c
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,
);
}
},
);
}
}
}

75
clippy_utils/src/macros.rs
View File

@ -6,6 +6,8 @@
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_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,
@ -361,6 +365,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 {

9
tests/ui/crashes/ice-10148.rs Normal file
View File

@ -0,0 +1,9 @@
// aux-build:../../auxiliary/proc_macro_with_span.rs
extern crate proc_macro_with_span;
use proc_macro_with_span::with_span;
fn main() {
println!(with_span!(""something ""));
}

12
tests/ui/crashes/ice-10148.stderr Normal file
View File

@ -0,0 +1,12 @@
error: empty string literal in `println!`
--> $DIR/ice-10148.rs:8:5
|
LL | println!(with_span!(""something ""));
| ^^^^^^^^^^^^^^^^^^^^-----------^^^^^
| |
| help: remove the empty string
|
= note: `-D clippy::println-empty-string` implied by `-D warnings`
error: aborting due to previous error

6
tests/ui/format.fixed
View File

@ -1,5 +1,4 @@
// run-rustfix
// aux-build: proc_macro_with_span.rs
#![warn(clippy::useless_format)]
#![allow(
unused_tuple_struct_fields,
@ -10,8 +9,6 @@
clippy::uninlined_format_args
)]
extern crate proc_macro_with_span;
struct Foo(pub String);
macro_rules! foo {
@ -90,7 +87,4 @@ fn main() {
let _ = abc.to_string();
let xx = "xx";
let _ = xx.to_string();
// Issue #10148
println!(proc_macro_with_span::with_span!(""something ""));
}

6
tests/ui/format.rs
View File

@ -1,5 +1,4 @@
// run-rustfix
// aux-build: proc_macro_with_span.rs
#![warn(clippy::useless_format)]
#![allow(
unused_tuple_struct_fields,
@ -10,8 +9,6 @@
clippy::uninlined_format_args
)]
extern crate proc_macro_with_span;
struct Foo(pub String);
macro_rules! foo {
@ -92,7 +89,4 @@ fn main() {
let _ = format!("{abc}");
let xx = "xx";
let _ = format!("{xx}");
// Issue #10148
println!(proc_macro_with_span::with_span!(""something ""));
}

30
tests/ui/format.stderr
View File

@ -1,5 +1,5 @@
error: useless use of `format!`
--> $DIR/format.rs:22:5
--> $DIR/format.rs:19:5
|
LL | format!("foo");
| ^^^^^^^^^^^^^^ help: consider using `.to_string()`: `"foo".to_string()`
@ -7,19 +7,19 @@ LL | format!("foo");
= note: `-D clippy::useless-format` implied by `-D warnings`
error: useless use of `format!`
--> $DIR/format.rs:23:5
--> $DIR/format.rs:20:5
|
LL | format!("{{}}");
| ^^^^^^^^^^^^^^^ help: consider using `.to_string()`: `"{}".to_string()`
error: useless use of `format!`
--> $DIR/format.rs:24:5
--> $DIR/format.rs:21:5
|
LL | format!("{{}} abc {{}}");
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `.to_string()`: `"{} abc {}".to_string()`
error: useless use of `format!`
--> $DIR/format.rs:25:5
--> $DIR/format.rs:22:5
|
LL | / format!(
LL | | r##"foo {{}}
@ -34,67 +34,67 @@ LL ~ " bar"##.to_string();
|
error: useless use of `format!`
--> $DIR/format.rs:30:13
--> $DIR/format.rs:27:13
|
LL | let _ = format!("");
| ^^^^^^^^^^^ help: consider using `String::new()`: `String::new()`
error: useless use of `format!`
--> $DIR/format.rs:32:5
--> $DIR/format.rs:29:5
|
LL | format!("{}", "foo");
| ^^^^^^^^^^^^^^^^^^^^ help: consider using `.to_string()`: `"foo".to_string()`
error: useless use of `format!`
--> $DIR/format.rs:40:5
--> $DIR/format.rs:37:5
|
LL | format!("{}", arg);
| ^^^^^^^^^^^^^^^^^^ help: consider using `.to_string()`: `arg.to_string()`
error: useless use of `format!`
--> $DIR/format.rs:70:5
--> $DIR/format.rs:67:5
|
LL | format!("{}", 42.to_string());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `.to_string()`: `42.to_string()`
error: useless use of `format!`
--> $DIR/format.rs:72:5
--> $DIR/format.rs:69:5
|
LL | format!("{}", x.display().to_string());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `.to_string()`: `x.display().to_string()`
error: useless use of `format!`
--> $DIR/format.rs:76:18
--> $DIR/format.rs:73:18
|
LL | let _ = Some(format!("{}", a + "bar"));
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `.to_string()`: `a + "bar"`
error: useless use of `format!`
--> $DIR/format.rs:80:22
--> $DIR/format.rs:77:22
|
LL | let _s: String = format!("{}", &*v.join("/n"));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `.to_string()`: `(&*v.join("/n")).to_string()`
error: useless use of `format!`
--> $DIR/format.rs:86:13
--> $DIR/format.rs:83:13
|
LL | let _ = format!("{x}");
| ^^^^^^^^^^^^^^ help: consider using `.to_string()`: `x.to_string()`
error: useless use of `format!`
--> $DIR/format.rs:88:13
--> $DIR/format.rs:85:13
|
LL | let _ = format!("{y}", y = x);
| ^^^^^^^^^^^^^^^^^^^^^ help: consider using `.to_string()`: `x.to_string()`
error: useless use of `format!`
--> $DIR/format.rs:92:13
--> $DIR/format.rs:89:13
|
LL | let _ = format!("{abc}");
| ^^^^^^^^^^^^^^^^ help: consider using `.to_string()`: `abc.to_string()`
error: useless use of `format!`
--> $DIR/format.rs:94:13
--> $DIR/format.rs:91:13
|
LL | let _ = format!("{xx}");
| ^^^^^^^^^^^^^^^ help: consider using `.to_string()`: `xx.to_string()`