Split assert

[jcarey9149]

View all comments

Closes #1810

changelog: [assert_multiple]: Add a lint for asserts with multiple boolean tests

[jcarey9149]

Sadly, I accidentally deleted the branch connected to my previous PR, causing it to close. @ada4a

[github-actions]

Lintcheck changes for 0ba3dd7

Lint	Added	Removed	Changed
clippy::assert_multiple	51	0	0

This comment will be updated if you push new changes

[ada4a]

Sadly, I accidentally deleted the branch connected to my previous PR, causing it to close.

Happens to the best of us, don't worry...

Looking at Lintcheck, there seem to be some classes of cases where we don't really want to lint:

• the assertion has a message – it usually presents information about everything that is asserted, so we shouldn't be splitting that up; and even if it didn't, we wouldn't know which of the newly assert we should move the message into
• assertions of the form n < x && x < m – splitting these would look unnatural imo, but I guess that's the whole premise of the lint... not sure, let me know what you think

[ada4a]

A bit more review.. I hope this will resolve the mystery of the missing suggestions

View changes since this review

[jcarey9149]

Sadly, I accidentally deleted the branch connected to my previous PR, causing it to close.

Happens to the best of us, don't worry...

Looking at Lintcheck, there seem to be some classes of cases where we don't really want to lint:

• the assertion has a message – it usually presents information about everything that is asserted, so we shouldn't be splitting that up; and even if it didn't, we wouldn't know which of the newly assert we should move the message into
• assertions of the form n < x && x < m – splitting these would look unnatural imo, but I guess that's the whole premise of the lint... not sure, let me know what you think

I'm not sure I understand the comment about lintcheck. can you clarify please?

This is based on issue 1810.

[ada4a]

Ah, sorry. Lintcheck is a thing where the PR is run on a bunch of popular crates, in order to see if the lint is doing the right thing, and catch false positives. You can see its results in #16677 (comment) by clicking on the clippy::assert_multiple thing

[ada4a]

in the changelog message, the lint name should be in backticks (`), not in quotes (')

[jcarey9149]

My latest update passes most tests, but the clippy test complains about collapsing my span_lint_and_then call into one (?) line. cargo dev fmt has a different idea.

How does this get resolved?

On the other front, it now supports debug_assert!() as well as assert!(). I also improved the handling of Or.

[ada4a]

the clippy test complains about collapsing my span_lint_and_then call into one (?) line

Ah no, it also suggests switching to span_lint_and_sugg (instead of span_lint_and_then). The reason it fires is because the then closure only contains one operation. This is an internal lint I personally dislike, but that's a topic for another time. For now, you can copy its suggestion:

span_lint_and_sugg(cx, ASSERT_MULTIPLE, e.span, "multiple asserts combined into one", "consider writing", am_visitor.suggests.join("\n"), Applicability::MaybeIncorrect)

[ada4a]

Also, I'm not sure how you ended up putting a bunch of other people's commits onto your branch?.. Try git pull --rebase && git rebase master or something

[jcarey9149]

@ada4a Any comments?

[ada4a]

Sorry, hadn't had the time to look at this yet, I'll probably do so tomorrow

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[rustbot]

r? @samueltardieu

rustbot has assigned @samueltardieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: 7 candidates
• 7 candidates expanded to 7 candidates
• Random selection from Jarcho, dswij, llogiq, samueltardieu

[ada4a]

I've stored the outer expression in the visitor, and changed all the .ctxt() calls to use it.

You can go one step further and store its .span.ctxt() directly, because calling Span::ctxt is somewhat expensive. You could then rename the field from outerexp to something like outer_ctxt.

This had an unfortunate side effect of changing the lhs/rhs names from macros like vec![] to $create::....
I've changed the lhs/rhs names call back to snippet, to restore the macro names for the ones that got rewritten as assert_eq/assert_neq.

Right. I'm pretty sure the reason for this is that your outerexp was e, i.e. the entire assert!(..);, while what you need is condition, i.e. the asserted expression -- this span adaptation business doesn't work if your outer expression is a macro call itself. With the patch below, everything works again:

diff --git i/clippy_lints/src/assert_multiple.rs w/clippy_lints/src/assert_multiple.rs
index dae19fc07..007d1d3e1 100644
--- i/clippy_lints/src/assert_multiple.rs
+++ w/clippy_lints/src/assert_multiple.rs
@@ -1,6 +1,6 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::macros::{find_assert_args, root_macro_call_first_node};
-use clippy_utils::source::{snippet, snippet_indent, snippet_with_context};
+use clippy_utils::source::{snippet_indent, snippet_with_context};
 use rustc_errors::Applicability;
 use rustc_hir::intravisit::Visitor;
 use rustc_hir::{BinOpKind, Expr, ExprKind, UnOp};
@@ -56,8 +56,8 @@ impl<'tcx> Visitor<'tcx> for AssertVisitor<'tcx, '_> {
         let mut app = Applicability::MaybeIncorrect;
         match e.kind {
             ExprKind::Binary(op, lhs, rhs) => {
-                let lhs_name = snippet(self.cx, lhs.span, "..");
-                let rhs_name = snippet(self.cx, rhs.span, "..");
+                let lhs_name = snippet_with_context(self.cx, lhs.span, self.outerexp.span.ctxt(), "..", &mut app).0;
+                let rhs_name = snippet_with_context(self.cx, rhs.span, self.outerexp.span.ctxt(), "..", &mut app).0;
 
                 match op.node {
                     BinOpKind::And => {
@@ -113,7 +113,7 @@ impl<'tcx> LateLintPass<'tcx> for AssertMultiple {
                 cx,
                 suggests: Vec::new(),
                 assert_string,
-                outerexp: e,
+                outerexp: condition,
             };
             rustc_hir::intravisit::walk_expr(&mut am_visitor, condition);

For what it's worth, (correct) macro handling is one of the most challenging parts of Clippy, so picking an assert!-related lint might have been an unlucky choice^^

FWIW, I also now get an extra pair of parentheses in the case of assert!(x || (y && z)) rewriting to assert!(x) and assert!((y & z)), which is technically right, but in this case the parens are unnecessary.

You could use clippy_utils::sugg::strip_enclosing_paren to, well, strip enclosing parentheses. The function is currently private -- don't hesitate to make it pub

[jcarey9149]

@rustbot ready

[ada4a]

You can go one step further and store its .span.ctxt() directly, because calling Span::ctxt is somewhat expensive. You could then rename the field from outerexp to something like outer_ctxt.

Could you please address this one as well?

Why do you write @rustbot ready without having addressed all the review comments... Please don't do that...

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[jcarey9149]

You can go one step further and store its .span.ctxt() directly, because calling Span::ctxt is somewhat expensive. You could then rename the field from outerexp to something like outer_ctxt.

Could you please address this one as well?

Why do you write @rustbot ready without having addressed all the review comments... Please don't do that...

Fixed. I think I lost that change in trying to rebase my branch in the previous changes, and failed to recheck that. More care this time.

BTW. you mentioned this might not have been the best first project to try--it was flagged as "good first issue".

[jcarey9149]

The pull request says there is a change requested by reviewers with write access. I'm having trouble locating that change request. The message has been around for quite a while.

Otherwise, I think I'm caught up.

[ada4a]

I think I lost that change in trying to rebase my branch in the previous changes, and failed to recheck that. More care this time.

Ah okay, thank you for clearing that up:)

BTW. you mentioned this might not have been the best first project to try--it was flagged as "good first issue".

That's.. unfortunate. Fwiw that's an older issue -- a lot of the processes surrounding contribution were in infancy at that time.

The pull request says there is a change requested by reviewers with write access. I'm having trouble locating that change request. The message has been around for quite a while.

It's just because I kind of gave up on Github's review UI, as we saw it was kind of flaky (with messages getting lost etc.). Feel free to ignore, I'll approve at the end.

[jcarey9149]

@rustbot ready