Skip to content

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

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

Description

@m-ou-se
m-ou-se
opened on Oct 19, 2021

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:

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.

Activity

  1. Manishearth commented on Oct 19, 2021

    @Manishearth
    Manishearth
    on Oct 19, 2021
    Member

    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

  2. Manishearth commented on Oct 19, 2021

    @Manishearth
    Manishearth
    on Oct 19, 2021
    Member

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

  3. camsteffen commented on Oct 20, 2021

    @camsteffen
    camsteffen
    on Oct 20, 2021
    Contributor

    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.

  4. Manishearth commented on Oct 20, 2021

    @Manishearth
    Manishearth
    on Oct 20, 2021
    Member

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

  5. flip1995
    added a commit that references this issue on Nov 18, 2021

    Auto merge of #90485 - camsteffen:fmt-args-less-bind, r=m-ou-se

    6536c58
  6. matthiaskrgr
    added a commit that references this issue on Nov 28, 2021

    Rollup merge of rust-lang#90131 - camsteffen:fmt-args-span-fix, r=cjg…

    9715724
  7. bors
    added 2 commits that reference this issue on Jan 4, 2022

    New macro utils

    786f874

    Auto merge of #8219 - camsteffen:macro-decoupling, r=llogiq

    ba03dc7
  8. bors
    closed this as completedin 786f874on Jan 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

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