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

Migrate clippy::author to rustc_ast::FormatArgs #10698

Closed
wants to merge 1 commit into from
Closed

Migrate clippy::author to rustc_ast::FormatArgs #10698

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
45 changes: 32 additions & 13 deletions clippy_lints/src/utils/author.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
//! A group of attributes that can be attached to Rust code in order
//! to generate a clippy lint detecting said code automatically.

use clippy_utils::macros::{collect_format_args, is_format_macro, root_macro_call_first_node};
use clippy_utils::{get_attr, higher};
use rustc_ast::ast::{LitFloatType, LitKind};
use rustc_ast::LitIntType;
Expand Down Expand Up @@ -239,14 +240,14 @@ impl<'a, 'tcx> PrintVisitor<'a, 'tcx> {
}
}

fn slice<T>(&self, slice: &Binding<&[T]>, f: impl Fn(&Binding<&T>)) {
fn slice<T>(&self, slice: &Binding<&[T]>, f: impl Fn(Binding<&T>)) {
if slice.value.is_empty() {
chain!(self, "{slice}.is_empty()");
} else {
chain!(self, "{slice}.len() == {}", slice.value.len());
for (i, value) in slice.value.iter().enumerate() {
let name = format!("{slice}[{i}]");
f(&Binding { name, value });
f(Binding { name, value });
}
}
}
Expand Down Expand Up @@ -333,6 +334,24 @@ impl<'a, 'tcx> PrintVisitor<'a, 'tcx> {

#[allow(clippy::too_many_lines)]
fn expr(&self, expr: &Binding<&hir::Expr<'_>>) {
if let Some(macro_call) = root_macro_call_first_node(self.cx, expr.value)
&& is_format_macro(self.cx, macro_call.def_id)
{
let diag = self.cx.tcx.get_diagnostic_name(macro_call.def_id).unwrap();
bind!(self, macro_call);
chain!(self, "let Some({macro_call}) = macros::root_macro_call_first_node(cx, {expr})");
chain!(self, "cx.tcx.is_diagnostic_item(sym::{diag}, {macro_call}.def_id)");
let exprs = collect_format_args(self.cx, |_fmt_args| {}, expr.value, macro_call.value.expn);
let args = exprs.as_slice();
bind!(self, args);
chain!(self, "let {args} = macros::collect_format_args(cx, |_fmt_args| {{}}, {expr}, {macro_call}.expn)");
self.slice(args, |e| {
let expr = Binding { name: e.name, value: *e.value };
self.expr(&expr);
});
return;
}

if let Some(higher::While { condition, body }) = higher::While::hir(expr.value) {
bind!(self, condition, body);
chain!(
Expand Down Expand Up @@ -398,25 +417,25 @@ impl<'a, 'tcx> PrintVisitor<'a, 'tcx> {
ExprKind::Array(elements) => {
bind!(self, elements);
kind!("Array({elements})");
self.slice(elements, |e| self.expr(e));
self.slice(elements, |e| self.expr(&e));
},
ExprKind::Call(func, args) => {
bind!(self, func, args);
kind!("Call({func}, {args})");
self.expr(func);
self.slice(args, |e| self.expr(e));
self.slice(args, |e| self.expr(&e));
},
ExprKind::MethodCall(method_name, receiver, args, _) => {
bind!(self, method_name, receiver, args);
kind!("MethodCall({method_name}, {receiver}, {args}, _)");
self.ident(field!(method_name.ident));
self.expr(receiver);
self.slice(args, |e| self.expr(e));
self.slice(args, |e| self.expr(&e));
},
ExprKind::Tup(elements) => {
bind!(self, elements);
kind!("Tup({elements})");
self.slice(elements, |e| self.expr(e));
self.slice(elements, |e| self.expr(&e));
},
ExprKind::Binary(op, left, right) => {
bind!(self, op, left, right);
Expand Down Expand Up @@ -469,7 +488,7 @@ impl<'a, 'tcx> PrintVisitor<'a, 'tcx> {
bind!(self, scrutinee, arms);
kind!("Match({scrutinee}, {arms}, MatchSource::{des:?})");
self.expr(scrutinee);
self.slice(arms, |arm| self.arm(arm));
self.slice(arms, |arm| self.arm(&arm));
},
ExprKind::Closure(&Closure {
capture_clause,
Expand Down Expand Up @@ -593,7 +612,7 @@ impl<'a, 'tcx> PrintVisitor<'a, 'tcx> {
}

fn block(&self, block: &Binding<&hir::Block<'_>>) {
self.slice(field!(block.stmts), |stmt| self.stmt(stmt));
self.slice(field!(block.stmts), |stmt| self.stmt(&stmt));
self.option(field!(block.expr), "trailing_expr", |expr| {
self.expr(expr);
});
Expand Down Expand Up @@ -639,13 +658,13 @@ impl<'a, 'tcx> PrintVisitor<'a, 'tcx> {
PatKind::Or(fields) => {
bind!(self, fields);
kind!("Or({fields})");
self.slice(fields, |pat| self.pat(pat));
self.slice(fields, |pat| self.pat(&pat));
},
PatKind::TupleStruct(ref qpath, fields, skip_pos) => {
bind!(self, qpath, fields);
kind!("TupleStruct(ref {qpath}, {fields}, {skip_pos:?})");
self.qpath(qpath);
self.slice(fields, |pat| self.pat(pat));
self.slice(fields, |pat| self.pat(&pat));
},
PatKind::Path(ref qpath) => {
bind!(self, qpath);
Expand All @@ -655,7 +674,7 @@ impl<'a, 'tcx> PrintVisitor<'a, 'tcx> {
PatKind::Tuple(fields, skip_pos) => {
bind!(self, fields);
kind!("Tuple({fields}, {skip_pos:?})");
self.slice(fields, |field| self.pat(field));
self.slice(fields, |field| self.pat(&field));
},
PatKind::Box(pat) => {
bind!(self, pat);
Expand Down Expand Up @@ -683,8 +702,8 @@ impl<'a, 'tcx> PrintVisitor<'a, 'tcx> {
opt_bind!(self, middle);
kind!("Slice({start}, {middle}, {end})");
middle.if_some(|p| self.pat(p));
self.slice(start, |pat| self.pat(pat));
self.slice(end, |pat| self.pat(pat));
self.slice(start, |pat| self.pat(&pat));
self.slice(end, |pat| self.pat(&pat));
},
}
}
Expand Down
34 changes: 33 additions & 1 deletion clippy_utils/src/macros.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
use crate::visitors::{for_each_expr, Descend};

use arrayvec::ArrayVec;
use rustc_ast::{FormatArgs, FormatArgument, FormatPlaceholder};
use rustc_ast::{FormatArgs, FormatArgsPiece, FormatArgument, FormatPlaceholder};
use rustc_data_structures::fx::FxHashMap;
use rustc_hir::{self as hir, Expr, ExprKind, HirId, Node, QPath};
use rustc_lint::LateContext;
Expand Down Expand Up @@ -425,6 +425,38 @@ pub fn find_format_arg_expr<'hir, 'ast>(
.ok_or(&target.expr)
}

/// Finds and extracts all format arguments from a format-like macro call.
///
/// `callback` can also be passed, to process the [`FormatArgs`] manually
/// if needed, without calling [`find_format_args`] separately.
///
/// ```ignore
/// // vvvvv any format-like macro
/// println!("Hello, {}!", "ferris")
/// // ^^^^^^^^ returns these expressions
/// ```
pub fn collect_format_args<'hir>(
cx: &LateContext<'_>,
callback: impl FnOnce(&FormatArgs),
start: &'hir Expr<'hir>,
expn_id: ExpnId,
) -> Vec<&'hir Expr<'hir>> {
Comment on lines +440 to +443
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd want to be callback: impl FnOnce(&FormatArgs, Vec<...>) rather than returning the Vec, lints generally want access to both at the same time

Copy link
Contributor Author

@Niki4tap Niki4tap Apr 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry for misunderstanding then. Although I'm not quite sure how to apply it in the author lint, if we don't return the args, it wouldn't quite fit into the if-chain author lint tries to build, i. e.

   && macros::collect_from_args(cx, |args, _fmt_args| { if /* another if-chain */ { /* report your lint here */ } }, e, macro_call.expn)

- this does not only introduce another level of indentation, but also requires some code refactoring to support, which is, in my opinion, quite a bit out of scope for the author lint.

If you have any ideas on how to mitigate this, I would be happy to hear, otherwise I can close the PR and submit an issue on this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For author hmm that's tricky, I guess you could always print a comment saying to take a look at the function

let mut args = vec![];
find_format_args(cx, start, expn_id, |format_args| {
Niki4tap marked this conversation as resolved.
Show resolved Hide resolved
for piece in &format_args.template {
if let FormatArgsPiece::Placeholder(placeholder) = piece
&& let Ok(index) = placeholder.argument.index
&& let Some(arg) = format_args.arguments.all_args().get(index)
&& let Ok(arg_expr) = find_format_arg_expr(start, arg)
{
args.push(arg_expr);
}
}
callback(format_args);
});
args
}

/// Span of the `:` and format specifiers
///
/// ```ignore
Expand Down