Clippy breaks when we change anything about the std macros

[m-ou-se]

Clippy recognizes macros like panic!() and format_args!() by its exact expansion, such as here:

rust-clippy/clippy_utils/src/higher.rs

Lines 719 to 736 in 389a74b

	pub fn parse(expr: &'tcx Expr<'tcx>) -> Option<Self> {
	if_chain! {
	if let ExprKind::Block(block, _) = expr.kind;
	if let Some(init) = block.expr;
	if let ExprKind::Call(_, [format_args]) = init.kind;
	let expn_data = expr.span.ctxt().outer_expn_data();
	if let ExprKind::AddrOf(_, _, format_args) = format_args.kind;
	if let Some(format_args) = FormatArgsExpn::parse(format_args);
	then {
	Some(PanicExpn {
	call_site: expn_data.call_site,
	format_args,
	})
	} else {
	None
	}
	}
	}

And here:

rust-clippy/clippy_utils/src/higher.rs

Lines 505 to 675 in 389a74b

	/// A parsed `format_args!` expansion
	pub struct FormatArgsExpn<'tcx> {
	/// Span of the first argument, the format string
	pub format_string_span: Span,
	/// Values passed after the format string
	pub value_args: Vec<&'tcx Expr<'tcx>>,
	/// String literal expressions which represent the format string split by "{}"
	pub format_string_parts: &'tcx [Expr<'tcx>],
	/// Symbols corresponding to [`Self::format_string_parts`]
	pub format_string_symbols: Vec<Symbol>,
	/// Expressions like `ArgumentV1::new(arg0, Debug::fmt)`
	pub args: &'tcx [Expr<'tcx>],
	/// The final argument passed to `Arguments::new_v1_formatted`, if applicable
	pub fmt_expr: Option<&'tcx Expr<'tcx>>,
	}
	impl FormatArgsExpn<'tcx> {
	/// Parses an expanded `format_args!` or `format_args_nl!` invocation
	pub fn parse(expr: &'tcx Expr<'tcx>) -> Option<Self> {
	if_chain! {
	if let ExpnKind::Macro(_, name) = expr.span.ctxt().outer_expn_data().kind;
	let name = name.as_str();
	if name.ends_with("format_args") || name.ends_with("format_args_nl");
	if let ExprKind::Call(_, args) = expr.kind;
	if let Some((strs_ref, args, fmt_expr)) = match args {
	// Arguments::new_v1
	[strs_ref, args] => Some((strs_ref, args, None)),
	// Arguments::new_v1_formatted
	[strs_ref, args, fmt_expr, _unsafe_arg] => Some((strs_ref, args, Some(fmt_expr))),
	_ => None,
	};
	if let ExprKind::AddrOf(BorrowKind::Ref, _, strs_arr) = strs_ref.kind;
	if let ExprKind::Array(format_string_parts) = strs_arr.kind;
	if let Some(format_string_symbols) = format_string_parts
	.iter()
	.map(|e| {
	if let ExprKind::Lit(lit) = &e.kind {
	if let LitKind::Str(symbol, _style) = lit.node {
	return Some(symbol);
	}
	}
	None
	})
	.collect();
	if let ExprKind::AddrOf(BorrowKind::Ref, _, args) = args.kind;
	if let ExprKind::Match(args, [arm], _) = args.kind;
	if let ExprKind::Tup(value_args) = args.kind;
	if let Some(value_args) = value_args
	.iter()
	.map(|e| match e.kind {
	ExprKind::AddrOf(_, _, e) => Some(e),
	_ => None,
	})
	.collect();
	if let ExprKind::Array(args) = arm.body.kind;
	then {
	Some(FormatArgsExpn {
	format_string_span: strs_ref.span,
	value_args,
	format_string_parts,
	format_string_symbols,
	args,
	fmt_expr,
	})
	} else {
	None
	}
	}
	}
	/// Returns a vector of `FormatArgsArg`.
	pub fn args(&self) -> Option<Vec<FormatArgsArg<'tcx>>> {
	if let Some(expr) = self.fmt_expr {
	if_chain! {
	if let ExprKind::AddrOf(BorrowKind::Ref, _, expr) = expr.kind;
	if let ExprKind::Array(exprs) = expr.kind;
	then {
	exprs.iter().map(|fmt| {
	if_chain! {
	// 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 i = usize::try_from(position).unwrap();
	Some(FormatArgsArg { value: self.value_args[i], arg: &self.args[i], fmt: Some(fmt) })
	} else {
	None
	}
	}
	}).collect()
	} else {
	None
	}
	}
	} else {
	Some(
	self.value_args
	.iter()
	.zip(self.args.iter())
	.map(|(value, arg)| FormatArgsArg { value, arg, fmt: None })
	.collect(),
	)
	}
	}
	}
	/// Type representing a `FormatArgsExpn`'s format arguments
	pub struct FormatArgsArg<'tcx> {
	/// An element of `value_args` according to `position`
	pub value: &'tcx Expr<'tcx>,
	/// An element of `args` according to `position`
	pub arg: &'tcx Expr<'tcx>,
	/// An element of `fmt_expn`
	pub fmt: Option<&'tcx Expr<'tcx>>,
	}
	impl<'tcx> FormatArgsArg<'tcx> {
	/// Returns true if any formatting parameters are used that would have an effect on strings,
	/// like `{:+2}` instead of just `{}`.
	pub fn has_string_formatting(&self) -> bool {
	self.fmt.map_or(false, |fmt| {
	// `!` because these conditions check that `self` is unformatted.
	!if_chain! {
	// struct `core::fmt::rt::v1::Argument`
	if let ExprKind::Struct(_, fields, _) = fmt.kind;
	if let Some(format_field) = fields.iter().find(|f| f.ident.name == sym::format);
	// struct `core::fmt::rt::v1::FormatSpec`
	if let ExprKind::Struct(_, subfields, _) = format_field.expr.kind;
	let mut precision_found = false;
	let mut width_found = false;
	if subfields.iter().all(|field| {
	match field.ident.name {
	sym::precision => {
	precision_found = true;
	if let ExprKind::Path(ref precision_path) = field.expr.kind {
	last_path_segment(precision_path).ident.name == sym::Implied
	} else {
	false
	}
	}
	sym::width => {
	width_found = true;
	if let ExprKind::Path(ref width_qpath) = field.expr.kind {
	last_path_segment(width_qpath).ident.name == sym::Implied
	} else {
	false
	}
	}
	_ => true,
	}
	});
	if precision_found && width_found;
	then { true } else { false }
	}
	})
	}
	/// Returns true if the argument is formatted using `Display::fmt`.
	pub fn is_display(&self) -> bool {
	if_chain! {
	if let ExprKind::Call(_, [_, format_field]) = self.arg.kind;
	if let ExprKind::Path(QPath::Resolved(_, path)) = format_field.kind;
	if let [.., t, _] = path.segments;
	if t.ident.name == sym::Display;
	then { true } else { false }
	}
	}
	}

This is very fragile, as it needs to be changed whenever we improve/change their implementations in std/core. For example: #7723 removes a &, after which clippy doesn't Recognize the arguments to panic anymore.

Worse, since Clippy is now not a submodule but a subtree in rust-lang/rust, it means that we can no longer make any changes at all to those macros without it getting blocked in CI on clippy. Clippy's dependence on all these implementations details effectively blocks development on panic, assert*, and format_args/fmt::Arguments in rust-lang/rust.

So as a maintainer of std, I'd like to ask again: please do not depend on these implementation details. We change this stuff. I'm expecting some big changes to fmt::Arguments and the assert macros in the future, and the panic macros we've also repeatedly changed tiny details of over the past year, and and they will change again.

See also:

• Deduplicate panic_fmt rust#88860
• if_then_panic doesn't fire when edition is 2021 #7723
• Qualify panic! as core::panic! in non-built-in core macros rust#78343 (comment)
• Fix handling of panic calls rust#79145
• Implement Rust 2021 panic rust#80851 (comment)
• Improve assert_eq! and assert_ne! rust#79100
• Improve the rustc-side clippy development experience rust#76495

---

I don't know what the best solution is, but making Clippy depend heavily on every detail of our macros is just going to cause a big headache down the road for someone. We should probably work on some way in std or rustc to make it easy to for clippy to have the information without having to do these hacks to revert a macro expansion.

Maybe we can have an empty and hidden #[inline(always)] fn macro_hint<T>(hint: &str, _: T) {} somewhere that we can call in our macros (e.g. macro_hint("hey clippy, this is an assert_eq", (left, right))). Then Clippy could recognize that instead.

[Manishearth]

cc @rust-lang/clippy

Yeah I've wanted a way to for clippy to be able to query the original expressions for a while; and ideally be able to retrieve typed info on the expressions.

The really tricky thing is that some clippy lints need typed expressions from the macro invocation. The (left, right) solution kinda fixes it (needs to be a reference) but not if we actually want to see what is in the left/right without potentially duplicating function calls. I suspect the "proper" way to do this is to tag each format_args argument as a format_args argument at the place it shows up in the expansion, or turn it into an ast node (which i know folks are thinking about anyway). Something like let left = #[tagged_macro_argument] left_expr; could potentially work.

@estebank might have ideas

[Manishearth]

@m-ou-se suggested let left = macro_arg("left", left); which works nicely

[camsteffen]

I think this can largely be fixed in Clippy by using a Visitor to scan the expanded HIR of a macro and collect all expressions that are not from expansion - these are the inputs. The format string details are more tricky, but we could also use a Visitor to skip to the interesting parts and not rely on the exact expansion. Also we have access to the DefId of the macro that was called so we should be using diagnostic items to detect when entering a given macro.

[Manishearth]

Hmmm, that sounds interesting. Still might depend on some internals, but definitely not as many