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

Macro check for assertion_on_constants lint #3740

Merged
merged 2 commits into from
Feb 10, 2019

Conversation

flip1995
Copy link
Member

@flip1995 flip1995 commented Feb 5, 2019

The assertion_on_constants lint currently has following output for this code Playground:

macro_rules! assert_const {
    ($len:expr) => {
        assert!($len > 0);
    }
}

fn main() {
    assert_const!(3);
    assert_const!(-1);
}
warning: assert!(const: true) will be optimized out by the compiler
 --> src/main.rs:3:9
  |
3 |         assert!($len > 0);
  |         ^^^^^^^^^^^^^^^^^^
...
8 |     assert_const!(3);
  |     ---------------- in this macro invocation
  |
  = note: #[warn(clippy::assertions_on_constants)] on by default
  = help: remove it
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#assertions_on_constants

warning: assert!(const: false) should probably be replaced
 --> src/main.rs:3:9
  |
3 |         assert!($len > 0);
  |         ^^^^^^^^^^^^^^^^^^
...
9 |     assert_const!(-1);
  |     ----------------- in this macro invocation
  |
  = help: use panic!() or unreachable!()
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#assertions_on_constants

This is contradictory. This lint should not trigger if the assert! is in a macro itself.

@flip1995 flip1995 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 5, 2019
@flip1995 flip1995 changed the title Macro check for assertion_on_constants lint WIP: Macro check for assertion_on_constants lint Feb 5, 2019
@flip1995 flip1995 changed the title WIP: Macro check for assertion_on_constants lint Macro check for assertion_on_constants lint Feb 5, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Feb 10, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Feb 10, 2019

📌 Commit cb2d987 has been approved by oli-obk

@bors
Copy link
Contributor

bors commented Feb 10, 2019

⌛ Testing commit cb2d987 with merge af43950...

bors added a commit that referenced this pull request Feb 10, 2019
Macro check for assertion_on_constants lint

The `assertion_on_constants` lint currently has following output for this code [Playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=6f2c9df6fc50baf847212d3b5136ee97):
```rust
macro_rules! assert_const {
    ($len:expr) => {
        assert!($len > 0);
    }
}

fn main() {
    assert_const!(3);
    assert_const!(-1);
}
```
```
warning: assert!(const: true) will be optimized out by the compiler
 --> src/main.rs:3:9
  |
3 |         assert!($len > 0);
  |         ^^^^^^^^^^^^^^^^^^
...
8 |     assert_const!(3);
  |     ---------------- in this macro invocation
  |
  = note: #[warn(clippy::assertions_on_constants)] on by default
  = help: remove it
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#assertions_on_constants

warning: assert!(const: false) should probably be replaced
 --> src/main.rs:3:9
  |
3 |         assert!($len > 0);
  |         ^^^^^^^^^^^^^^^^^^
...
9 |     assert_const!(-1);
  |     ----------------- in this macro invocation
  |
  = help: use panic!() or unreachable!()
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#assertions_on_constants
```

This is contradictory. This lint should not trigger if the `assert!` is in a macro itself.
@bors
Copy link
Contributor

bors commented Feb 10, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing af43950 to master...

@bors bors merged commit cb2d987 into rust-lang:master Feb 10, 2019
@flip1995 flip1995 deleted the const_assert_macro branch February 10, 2019 22:18
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.

3 participants