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

[bool_assert_comparaison] improve suggestion #7612

Conversation

ABouttefeux
Copy link
Contributor

@ABouttefeux ABouttefeux commented Sep 1, 2021

close #7598

now code like

assert_eq!( a, true);

Will suggest to use assert(a) wherease previously it was assert(..).

Also what I would like to add is with

assert_eq!( a, true, "message {}", b);

is to suggest assert!( a, "message {}", b).
It is not the case at the moment.

changelog: [bool_assert_comparaison] improve suggestion

@rust-highfive
Copy link

r? @camsteffen

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 1, 2021
@ABouttefeux ABouttefeux force-pushed the bool_assert_comparison_improvment branch 3 times, most recently from bcfe69d to 522e3c7 Compare September 1, 2021 09:55
clippy_lints/src/bool_assert_comparison.rs Outdated Show resolved Hide resolved
clippy_lints/src/bool_assert_comparison.rs Show resolved Hide resolved
clippy_lints/src/bool_assert_comparison.rs Outdated Show resolved Hide resolved
clippy_lints/src/bool_assert_comparison.rs Outdated Show resolved Hide resolved
tests/ui/bool_assert_comparison.stderr Outdated Show resolved Hide resolved
for el in &args[2..] {
str = format!("{}, {}", str, source::snippet(cx, el.span, ""));
}
format!("{})", str)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the String API (push and push_str).

Or better yet, get just one snippet of all the format args with something like

match fmt_args {
    [] => None,
    [a] => Some(a.span),
    [a, .., b] => Some(a.span.to(b.span)),
}

@ABouttefeux
Copy link
Contributor Author

The problem I see is that higher::extract_assert_macro_args(expr) does not give me the formatting arguments. I do not know how to get them.

@camsteffen
Copy link
Contributor

The problem I see is that higher::extract_assert_macro_args(expr) does not give me the formatting arguments. I do not know how to get them.

You can open a Rust Playground with an assert_eq! and run "Tools > Expand macros" to see what the expansion looks like. You will see a match expression. Currently the macro parsing in extract_assert_macro_args basically stops there. You need to extend the parsing farther to the part where you see Option::Some(...). The contents of the Some is format_args!, so that expression can be parsed with FormatArgsExpn::parse and just add Option<FormatArgsExpn> to the output of extract_assert_macro_args. Let me know if that makes sense.

@ABouttefeux
Copy link
Contributor Author

Yes it does thank you.

@camsteffen
Copy link
Contributor

Hi @ABouttefeux, it looks like tests are not passing. Also please do a rebase instead of a merge since we follow rust's no-merge policy: https://rustc-dev-guide.rust-lang.org/git.html#no-merge-policy

@ABouttefeux ABouttefeux force-pushed the bool_assert_comparison_improvment branch 2 times, most recently from e44bb26 to f244ec7 Compare September 8, 2021 07:30
@ABouttefeux
Copy link
Contributor Author

I am not able yet to extract the fist formatting argument, I change it to ".." temporarily. I will work on it later. Do you have any idea how to extract it ?

@camsteffen
Copy link
Contributor

That would come from FormatArgsExpn::parse. See this.

@camsteffen camsteffen added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Sep 12, 2021
Copy link
Contributor

@camsteffen camsteffen left a comment

Choose a reason for hiding this comment

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

Nice work getting the macro stuff working! Just some feedback with how the utils are structured and we should enable rustfix for tests now that we have valid suggestions.

@@ -90,13 +101,46 @@ impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison {
}

let non_eq_mac = &mac[..mac.len() - 3];
let mut applicability = Applicability::MaybeIncorrect;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use MachineApplicable here. Also add // run-rustfix to the top of the test file.

| StmtKind::Semi(call_assert_failed) = stmts_if_block[1].kind;
if let ExprKind::Call(_, args_assert_failed) = call_assert_failed.kind;
if args_assert_failed.len() >= 4;
if let ExprKind::Call(_, args) = args_assert_failed[3].kind;
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer destructuring for stmts, args, etc.

Suggested change
if let ExprKind::Call(_, args) = args_assert_failed[3].kind;
if let ExprKind::Call(_, [arg]) = args_assert_failed[3].kind;

if let Some (mut format_arg_expn) = FormatArgsExpn::parse(&args[0]);
then {
vec_arg.push(format_arg_expn.format_string);
vec_arg.append(&mut format_arg_expn.value_args);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should change extract_assert_macro_args to be more consistent with other utils here. We could have AssertExpn::parse and AssertExpn is a struct that contains a FormatArgsExpn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks I have done something similar, but now I would like to test parsing assert(expr, "format {}", value), how can I test that ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Use Expand macros in the rust playground to see what you are trying to parse. For other tips, use dbg!() to set a checkpoint in your code and see if that line is being reached. Use dbg!(expr) to see the HIR. Use let _ = dbg!(); in if_chain!s.

clippy_utils/src/higher.rs Outdated Show resolved Hide resolved
@ABouttefeux ABouttefeux force-pushed the bool_assert_comparison_improvment branch from bc62c10 to 29fc388 Compare September 22, 2021 15:24
second_assert_argument: None,
format_arg: None,
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Try to avoid else in if_chain! here since it adds an else clause to every if in the expansion. You should only need to "fork" at a single point.

/// `Some(AssertExpn { first_assert_argument: a, second_assert_argument: Some(b),
/// format_arg:None })`
pub fn parse(e: &'tcx Expr<'tcx>) -> Option<Self> {
if let ExprKind::Block(block, _) = e.kind {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider doing something like this as a first step

if let ExpnKind::Macro(_, name) = expr.span.ctxt().outer_expn_data().kind;
match *name.as_str() {
    "assert" | "debug_assert" => ..,
    "assert_eq" | "debug_assert_eq" => ..,
}

@giraffate
Copy link
Contributor

ping from triage @ABouttefeux. It looks like there are a few things left to fix. Can you address these?

@bors
Copy link
Collaborator

bors commented Oct 12, 2021

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

@ABouttefeux
Copy link
Contributor Author

I have little time I will try to do it. Actually I am strugling a bit with assert!(exp, fmt, ...).

@ABouttefeux ABouttefeux force-pushed the bool_assert_comparison_improvment branch 2 times, most recently from d006faa to 5c0488f Compare October 15, 2021 10:21
@bors
Copy link
Collaborator

bors commented Oct 15, 2021

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

@ABouttefeux ABouttefeux force-pushed the bool_assert_comparison_improvment branch from 5c0488f to add7d76 Compare October 17, 2021 09:55
@bors
Copy link
Collaborator

bors commented Oct 19, 2021

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

@ABouttefeux ABouttefeux force-pushed the bool_assert_comparison_improvment branch from add7d76 to 929610a Compare October 20, 2021 10:27
@bors
Copy link
Collaborator

bors commented Oct 21, 2021

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

@ABouttefeux ABouttefeux force-pushed the bool_assert_comparison_improvment branch from 929610a to 3328501 Compare October 21, 2021 13:29
@camsteffen
Copy link
Contributor

Hey @ABouttefeux, let me know when you are ready for review.

Also I want to point out #7843 - we may need to take a different direction for parsing the assert macros that does not parse the exact structure of the expansion. Although this is somewhat pre-existing so maybe it's okay to continue with the current approach for this PR. (@rust-lang/clippy thoughts?) I think it is possible to take a different approach using a visitor and checking span expansion data, but this is quite tricky.

@ABouttefeux ABouttefeux force-pushed the bool_assert_comparison_improvment branch from 3328501 to 103ac52 Compare October 22, 2021 13:49
@ABouttefeux
Copy link
Contributor Author

Yes this is ready. I have no idea how do to in a way that would be more robust to change of the macro however.

@flip1995
Copy link
Member

I'm really unsure if we should take up more technical debt here, since we know that this is not sustainable long term. This is a nice improvement of the suggestion, so definitely a welcome change. But I'm not sure if it is worth to take up the technical debt that comes with it.

@bors
Copy link
Collaborator

bors commented Nov 2, 2021

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

@camsteffen
Copy link
Contributor

@ABouttefeux After #8219 is merged, have another look if you're still interested. There are new utils in place for assert macros.

@flip1995
Copy link
Member

ping from triage @ABouttefeux. #8219 was merged, and you should be able to use those new utils to implement this lint and get this PR merged.

@camsteffen
Copy link
Contributor

Closing due to inactivity. Feel free to reopen if you'd like to pick this up again.

@camsteffen camsteffen closed this Feb 16, 2022
@flip1995 flip1995 added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Feb 17, 2022
@jonboh
Copy link
Contributor

jonboh commented Oct 10, 2023

@rustbot label -S-inactive-closed

@rustbot rustbot removed the S-inactive-closed Status: Closed due to inactivity label Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bool_assert_comparison improve the suggestion to include the expression
8 participants