Skip to content

Clippy breaks when we change anything about the std macros #7843

Closed

Description

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

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:

/// 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:


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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions