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.

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

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

Closed
wants to merge 1 commit into from

Conversation

Niki4tap
Copy link
Contributor

@Niki4tap Niki4tap commented Apr 22, 2023

This PR aims to migrate clippy::author lint to new FormatArgs AST, since it's been ICEing after the change.

repro.rs:

#![feature(stmt_expr_attributes)]

fn main() {
    #[clippy::author]
    {
        println!("Hello, {}!", "ferris");
    }
}
Current ICE
if let ExprKind::Block(block, None) = expr.kind
    && block.stmts.len() == 1
    && let StmtKind::Semi(e) = block.stmts[0].kind
    && let ExprKind::Block(block1, None) = e.kind
    && block1.stmts.len() == 1
    && let StmtKind::Semi(e1) = block1.stmts[0].kind
    && let ExprKind::Call(func, args) = e1.kind
    && let ExprKind::Path(ref qpath) = func.kind
    && match_qpath(qpath, &["$crate", "io", "_print"])
    && args.len() == 1
    && let ExprKind::Call(func1, args1) = args[0].kind
    && let ExprKind::Path(ref qpath1) = func1.kind
thread 'rustc' panicked at 'path_to_string: called for lang item qpath', clippy_lints/src/utils/author.rs:746:36
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust-clippy/issues/new

note: Clippy version: clippy 0.1.70 (928349795 2023-04-21)

query stack during panic:
#0 [analysis] running analysis passes on this crate
end of query stack
After this PR
if let ExprKind::Block(block, None) = expr.kind
    && block.stmts.len() == 1
    && let StmtKind::Semi(e) = block.stmts[0].kind
    && let Some(macro_call) = macros::root_macro_call_first_node(cx, e)
    && cx.tcx.is_diagnostic_item(sym::println_macro, macro_call.def_id)
    && let args = macros::collect_format_args(cx, |_fmt_args| {}, e, macro_call.expn)
    && args.len() == 1
    && let ExprKind::Lit(ref lit) = args[0].kind
    && let LitKind::Str(s, _) = lit.node
    && s.as_str() == "ferris"
    && block.expr.is_none()
{
    // report your lint here
}

r? @Alexendoo

I've seen you migrate other lints, so I think your feedback will be valuable here. Thanks in advance :)
(if anyone else has an opinion that might be useful here, you're free to comment too)

Also, I'm not exactly sure if a new utility function is needed here, or it would better to reuse existing ones like clippy_utils::macros::find_format_args.

note: I think author lint is mostly internal, and the change is too, but on the other hand it's a bugfix, so I'm not sure what to put in the changelog, let there be none for now :)

changelog: none

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 22, 2023
clippy_lints/src/utils/author.rs Outdated Show resolved Hide resolved
clippy_lints/src/utils/author.rs Outdated Show resolved Hide resolved
@Alexendoo
Copy link
Member

Thanks for this, didn't know clippy::author had support for that

Also, I'm not exactly sure if a new utility function is needed here, or it would better to reuse existing ones like clippy_utils::macros::find_format_args.

A new utility function could be nice, I'm not that happy with the current API. However we can't match on the HIR of format_args as collect_format_args does because that was holding back progress in rustc, it ought to be based on find_format_args/find_format_arg_expr

@Niki4tap
Copy link
Contributor Author

A new utility function could be nice, I'm not that happy with the current API. However we can't match on the HIR of format_args as collect_format_args does because that was holding back progress in rustc, it ought to be based on find_format_args/find_format_arg_expr

Ah, yeah that makes sense. I've changed collect_format_args implementation to use these instead of HIR matching, should be fine now ^^

Comment on lines +440 to +443
callback: impl FnOnce(&FormatArgs),
start: &'hir Expr<'hir>,
expn_id: ExpnId,
) -> Vec<&'hir Expr<'hir>> {
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

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

@bors
Copy link
Collaborator

bors commented Apr 29, 2023

☔ The latest upstream changes (presumably #10647) made this pull request unmergeable. Please resolve the merge conflicts.

@Alexendoo
Copy link
Member

Since #11444 was merged find_format_args now returns a value instead of taking a callback making this easier if you wanted to pick it up again, otherwise I'll add it to my todo list

@xFrednet
Copy link
Member

Hey @Alexendoo, this is a ping from triage. It seems like @Niki4tap hasn't responded to your last message where you suggested that you'll pick this PR up. Do we want to close this one due to inactivity?

@Alexendoo
Copy link
Member

Yeah I'll take a look at it after #12567 which changes how they work (again 😛)

@Alexendoo Alexendoo closed this Mar 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants