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 054f2c8

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

[samueltardieu]

Indentation level is incorrect

View changes since the review

[samueltardieu]

Also, it looks like assert!() with a message will loose the message when split. See lintcheck output for examples. Please include tests where there is an extra message.

View changes since this review

[jcarey9149]

Also, it looks like assert!() with a message will loose the message when split. See lintcheck output for examples. Please include tests where there is an extra message.

Thanks for pointing this out. I'll look into it.

[ada4a]

Sorry for all the waiting Jim, I seem to not have the energy to bring this to completion. Let me reroll..

@rustbot reroll

[llogiq]

I left a few comments.

View changes since this review

[jcarey9149]

@rustbot ready

[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.

[rustbot]

⚠️ Warning ⚠️

• The following commits have merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

  • 2258d0f
  • b84816b

  You can start a rebase with the following commands:

  $ # rebase
  $ git pull --rebase https://github.com/rust-lang/rust-clippy.git master
  $ git push --force-with-lease