Add assert(true) and assert(false) lints by Arkweid · Pull Request #3582 · rust-lang/rust-clippy

Arkweid · 2018-12-25T23:13:23Z

This PR add two lints on assert!(true) and assert!(false).
#3575

matthiaskrgr

Generally looks good to me, but others might want to weigh in as well.

Arkweid · 2018-12-27T13:17:05Z

Error message changed to:

assert!(false) should probably be replaced by a panic!() or unreachable!()

ghost · 2018-12-27T14:44:28Z

I would combine these lints into one. Maybe call it assertions_on_constants... 🤔

flip1995

Thanks for your contribution and welcome to Clippy! 🎉

This LGTM as a first implementation of this lint. Some easy and some advanced enhancement could be added to the lint:

suggest ""/"panic!()" (easy)
also lint on constants (advanced)
if it is a assert!(false, "_") call, suggest panic!("_") instead of just panic!() (advanced)

The easy one should be done in this PR, the advanced ones are just nice to have enhancement, that could be added later.

Please also add tests for assert!(true/false, "some message")

flip1995 · 2018-12-27T15:10:44Z

+            then {
+                match inner.node {
+                    LitKind::Bool(true) => {
+                        span_lint(cx, EXPLICIT_TRUE, e.span,


You can add a pretty simple suggestion to this lints:

true => sugg: ""

false => sugg: "panic!()"

You can use

rust-clippy/clippy_lints/src/utils/mod.rs

Lines 667 to 675 in 44ffda7

pub fn span_lint_and_sugg<'a, 'tcx: 'a, T: LintContext<'tcx>>(

cx: &'a T,

lint: &'static Lint,

sp: Span,

msg: &str,

help: &str,

sugg: String,

applicability: Applicability,

) {

for this.

Arkweid · 2018-12-27T15:36:21Z

Thanks for your review @flip1995 👍
I will make changes in this PR soon.

bors · 2018-12-29T17:53:07Z

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

flip1995 · 2019-01-04T09:41:55Z

ping from triage @Arkweid: Any updates on this?

Arkweid · 2019-01-04T14:05:16Z

@flip1995 Ye, I am here. Just holidays in Russia until 9 January :)

oli-obk · 2019-01-09T11:26:04Z

+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+


Suggested change

// run-rustfix

ordovicia · 2019-01-09T11:46:02Z

+                            e.span,
+                            "assert!(false) should probably be replaced",
+                            "try",
+                            "panic!()".to_string(),


panic!() or unreachable!()

Work in progress. I pushed the current state of PR to test span_lint_and_sugg. Seems it not work as expected for assert!() macro. Maybe I should replace it with span_lint_and_help function?

Arkweid · 2019-01-09T18:45:11Z

@flip1995 changes pushed
Also, I faced with an issue where suggestion from span_lint_and_sugg was suppressed somewhere in rustc internals(probably). I have a conversation with @oli-obk about it in discord clippy channel. So I replace it with span_help_and_lint function. Because of this problem, I skip that feature assert!(false, "_") call, suggest panic!("_") instead of just panic!() (advanced)

ghost · 2019-01-10T04:19:39Z

                match inner.node {
                    LitKind::Bool(true) => {
-                        span_lint(cx, EXPLICIT_TRUE, e.span,
+                        span_lint(cx, ASSERTIONS_ON_CONSTANTS, e.span,


The span_lint_and_sugg should work. The problem you had was in the assert(true) branch but you were still using span_lint there.

@mikerite I check it here:
https://github.com/rust-lang/rust-clippy/pull/3582/files/7075015f311617710dcebfe6530bdca732287994#diff-fc99a0c4e187ac372040420fe4dcb24cR60
Test case:
https://github.com/rust-lang/rust-clippy/pull/3582/files/7075015f311617710dcebfe6530bdca732287994#diff-514c75729f8046fc08cc8bb536b2fd0aR13
Failed CI on this test:
https://travis-ci.com/rust-lang/rust-clippy/jobs/169168148#L1259

flip1995 · 2019-01-10T18:06:15Z

Thanks for the update! I'll take a closer look at it ASAP.

flip1995 · 2019-01-16T19:11:14Z

I didn't forget about this. I have a lot on my schedule right now. I'll try to get to this, this weekend.

flip1995 · 2019-01-22T12:45:25Z

Sorry this took so long.

This LGTM now. It's weird that the suggestion somehow didn't work, but this can be addressed in a later PR.

@bors r+ Thanks!

bors · 2019-01-22T12:45:26Z

📌 Commit 58abdb5 has been approved by flip1995

bors · 2019-01-22T12:45:35Z

⌛ Testing commit 58abdb5 with merge 9cee601...

Add assert(true) and assert(false) lints This PR add two lints on assert!(true) and assert!(false). #3575

bors · 2019-01-22T12:51:23Z

💔 Test failed - status-appveyor

flip1995 · 2019-01-22T15:42:48Z

@bors rollup

…lip1995 Add assert(true) and assert(false) lints This PR add two lints on assert!(true) and assert!(false). rust-lang#3575

@ghost

Rollup of 4 pull requests Successful merges: - #3582 (Add assert(true) and assert(false) lints) - #3679 (Fix automatic suggestion on `use_self`.) - #3684 ("make_return" and "blockify" convenience methods, fixes #3683) - #3685 (Rustup) Failed merges: r? @ghost

Arkweid · 2019-01-22T20:11:16Z

@flip1995 looks like I need to rebase it on fresh master, because of this:
https://github.com/rust-lang/rust-clippy/blob/master/tests/ui/collapsible_if.rs#L143
Or just to add #![allow(clippy::assertions_on_constants::assertions_on_constants)] in collapsible_if.rs
Previously it not trigger an error :(

phansch · 2019-01-23T06:44:33Z

@Arkweid Adding #![allow(clippy::assertions_on_constants)] to that file should be enough 👍

* master: (58 commits) Rustfmt all the things Don't make decisions on values that don't represent the decision Improving comments. Rustup Added rustfix to the test. Improve span shortening. Added "make_return" and "blockify" convenience methods in Sugg and used them in "needless_bool". Actually check for constants. Fixed potential mistakes with nesting. Added tests. formatting fix Update clippy_lints/src/needless_bool.rs formatting fix Fixing typo in CONTRIBUTING.md Fix breakage due to rust-lang/rust#57651 needless bool lint suggestion is wrapped in brackets if it is an "else" clause of an "if-else" statement Fix automatic suggestion on `use_self`. Remove negative integer literal checks. Fix `implicit_return` false positives. Run rustfmt Fixed breakage due to rust-lang/rust#57489 ...

Arkweid · 2019-01-23T10:22:46Z

@flip1995 @phansch
Compatibility work with current master completed :D

flip1995 · 2019-01-23T13:06:00Z

Nice! Could you squash the last few commits 2068463..e86b6ce?

Arkweid · 2019-01-23T15:23:28Z

@flip1995 4 last commits squashed in one.

flip1995 · 2019-01-23T17:42:27Z

@bors r+ Thanks!

bors · 2019-01-23T17:42:28Z

📌 Commit c771f33 has been approved by flip1995

bors · 2019-01-23T17:42:36Z

⌛ Testing commit c771f33 with merge 99c5388...

Add assert(true) and assert(false) lints This PR add two lints on assert!(true) and assert!(false). #3575

bors · 2019-01-23T18:03:31Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing 99c5388 to master...

flip1995 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 26, 2018

matthiaskrgr reviewed Dec 27, 2018

View reviewed changes

Comment thread clippy_lints/src/assert_checks.rs Outdated

flip1995 requested changes Dec 27, 2018

View reviewed changes

flip1995 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 Dec 27, 2018

A.A.Abroskin added 4 commits January 7, 2019 13:30

Add assert(true) and assert(false) lints

98c5f37

Add unreachable!() as option

3d9535a

run ./util/dev update_lints

9605861

change assert_checks to assertions_on_constants

906b516

Arkweid force-pushed the add-lints-aseert-checks branch from 6a100eb to 906b516 Compare January 9, 2019 10:40

Merge branch 'master' into add-lints-aseert-checks

7075015

oli-obk reviewed Jan 9, 2019

View reviewed changes

ordovicia reviewed Jan 9, 2019

View reviewed changes

A.A.Abroskin added 2 commits January 9, 2019 21:30

add assert(true/false, some message) tests

a9f8d3c

run ./util/dev update_lints

58abdb5

phansch added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jan 9, 2019

ghost reviewed Jan 10, 2019

View reviewed changes

flip1995 approved these changes Jan 22, 2019

View reviewed changes

bors added a commit that referenced this pull request Jan 22, 2019

Auto merge of #3582 - Arkweid:add-lints-aseert-checks, r=flip1995

9cee601

Add assert(true) and assert(false) lints This PR add two lints on assert!(true) and assert!(false). #3575

flip1995 mentioned this pull request Jan 22, 2019

lint assert!(true / false) #3575

Closed

flip1995 mentioned this pull request Jan 22, 2019

Rollup of 4 pull requests #3686

Closed

flip1995 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 Jan 22, 2019

allow assertions_on_constants for collapsible_if and missing_test_files

c771f33

Arkweid force-pushed the add-lints-aseert-checks branch from e86b6ce to c771f33 Compare January 23, 2019 15:20

bors added a commit that referenced this pull request Jan 23, 2019

Auto merge of #3582 - Arkweid:add-lints-aseert-checks, r=flip1995

99c5388

Add assert(true) and assert(false) lints This PR add two lints on assert!(true) and assert!(false). #3575

bors merged commit c771f33 into rust-lang:master Jan 23, 2019

bors mentioned this pull request Jan 23, 2019

Add initial version of const_fn lint #3648

Merged

2 tasks

flip1995 mentioned this pull request Apr 10, 2019

Tracking Issue: Bugs introduced by Rustup #3905 #3913

Closed

3 tasks

This was referenced May 20, 2019

Improve non ascii literal #4119

Merged

Sometimes DiagnosticBuilder::span_suggestion doesn't print the suggestion rust-lang/rust#60979

Closed

matthiaskrgr mentioned this pull request Jun 17, 2020

Warn on assert!(always_false) rust-lang/rust#73435

Open

	pub fn span_lint_and_sugg<'a, 'tcx: 'a, T: LintContext<'tcx>>(
	cx: &'a T,
	lint: &'static Lint,
	sp: Span,
	msg: &str,
	help: &str,
	sugg: String,
	applicability: Applicability,
	) {

Conversation

Arkweid commented Dec 25, 2018

Uh oh!

matthiaskrgr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Arkweid commented Dec 27, 2018

Uh oh!

ghost commented Dec 27, 2018

Uh oh!

flip1995 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

flip1995 Dec 27, 2018

Choose a reason for hiding this comment

Uh oh!

Arkweid commented Dec 27, 2018

Uh oh!

bors commented Dec 29, 2018

Uh oh!

flip1995 commented Jan 4, 2019

Uh oh!

Arkweid commented Jan 4, 2019

Uh oh!

oli-obk Jan 9, 2019

Choose a reason for hiding this comment

Uh oh!

ordovicia Jan 9, 2019

Choose a reason for hiding this comment

Uh oh!

Arkweid Jan 9, 2019

Choose a reason for hiding this comment

Uh oh!

Arkweid commented Jan 9, 2019

Uh oh!

ghost Jan 10, 2019

Choose a reason for hiding this comment

Uh oh!

Arkweid Jan 10, 2019

Choose a reason for hiding this comment

Uh oh!

flip1995 commented Jan 10, 2019

Uh oh!

flip1995 commented Jan 16, 2019

Uh oh!

flip1995 commented Jan 22, 2019

Uh oh!

bors commented Jan 22, 2019

Uh oh!

bors commented Jan 22, 2019

Uh oh!

bors commented Jan 22, 2019

Uh oh!

flip1995 commented Jan 22, 2019

Uh oh!

Arkweid commented Jan 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phansch commented Jan 23, 2019

Uh oh!

Arkweid commented Jan 23, 2019

Uh oh!

flip1995 commented Jan 23, 2019

Uh oh!

Arkweid commented Jan 23, 2019

Uh oh!

flip1995 commented Jan 23, 2019

Uh oh!

bors commented Jan 23, 2019

Uh oh!

bors commented Jan 23, 2019

Uh oh!

bors commented Jan 23, 2019

Uh oh!

Reviewers

flip1995 left a comment •

edited

Loading

Arkweid commented Jan 22, 2019 •

edited

Loading