Change naive_bytecount applicability to MaybeIncorrect
We can't use `MachineApplicable` here as applying the fix will cause
another error because `bytecount` would first have to be added to the
Cargo.toml.
Example:
```
error: You appear to be counting bytes the naive way
--> $DIR/bytecount.rs:5:13
|
LL | let _ = x.iter().filter(|&&a| a == 0).count(); // naive byte count
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Consider using the bytecount crate: `bytecount::count(x, 0)`
```
Just replacing it with the suggestion is not enough.
cc #3630
Change while_let_loop applicability to HasPlaceholders
The suggestion has been changed at some point to use `..` in the suggested code.
Due to that we can't make the lint MachineApplicable anymore.
cc #3630
This was causing two different ICEs in #3741.
The first was fixed in #3925.
The second one is fixed with this commit: We just don't `expect`
anymore. If the snippet doesn't contain an `else`, we stop emitting the
lint because it's not a suspiciously formatted else anyway.
Fix ICE in decimal_literal_representation lint
Handling the integer parsing properly instead of just unwrapping.
Note that the test is not catching the ICE because plain UI tests
[currently hide ICEs][compiletest_issue]. Once that issue is fixed, this
test would fail properly again.
Fixes#3891
[compiletest_issue]: https://github.com/laumann/compiletest-rs/issues/169
Handling the integer parsing properly instead of just unwrapping.
Note that the test is not catching the ICE because plain UI tests
[currently hide ICEs][compiletest_issue]. Once that issue is fixed, this
test would fail properly again.
[compiletest_issue]: https://github.com/laumann/compiletest-rs/issues/169
Fix `explicit_counter_loop` suggestion
#1670
This code seems to me to work, but I have two question.
* Because range expression desugared in hir, `Sugg::hir` doesn't add parenthesis to range expression. Which function is better to check range do you think, `check_for_loop_explicit_counter` or `hir_from_snippet`?
* Do you think we need to distinguish between range expression and struct expression that creates `std::ops::Range*`?
* Late Lint pass, catches:
* One liner: 0 -> null -> transmute
* One liner: std:null() -> transmute
* Const (which resolves to null) -> transmute
* UI Test case for Lint
* Updated test for issue 3849, because now the lint that code generated is in Clippy.
* Expanded `const.rs` miri-based Constant Folding code, to cover
raw pointers
Add `doc(include = ...)` detection to `missing_docs_in_private_items`
The whole `missing documentation in crate` part doesn't have any tests. If I should add test cases tell me.
Fix `boxed_local` suggestion
Don't warn about an argument that is moved into a closure.
ExprUseVisitor doesn't walk into nested bodies so use a new
visitor that collects the variables that are moved into closures.
Fixes#3739
Refactor: Cleanup one part of assign_ops lint
Removes a lot of indentation and separates lint emission from lint
logic. Only touches the `hir::ExprKind::AssignOp` part of the lint.
Refactor: Extract `trait_ref_of_method` function
This pattern was used in three places after #3844, so I think it's worth moving it into `utils/mod.rs` and documenting it.
* Ran automatic naming update
* Formalized rename of `cyclomatic_complexity` to `cognitive_complexity`
** Added the rename to `lib.rs`
** Added rename test
* Added warning for deprecated key `cyclomatic_complexity_threshold` and tests for it
* Added deprecation status for Clippy's builtin attribute
* Updated tests for new builtin attribute renaming
Fix `bool_comparison` with non-`bool` expressions
Fixes#3703.
It just moves around the type check that was already there for some comparison to all of them, because if one type isn't `bool`, none of those comparison can be simplified.
Fix ICE #3719+#3718 in lint match_ref_pats
Fixes#3719
This conveniently also fixes#3718
The ICE occurs when the match expression was a macro call, where the macro was defined in another file. Since we don't have the ability to reproduce this behavior with our UI tests (AFAIK), I couldn't add a test reproducing this ICE.. However, I added a test which is related to the ICE, to show the new behavior of the lint.
I tested it with the mscheme repo locally and the ICE didn't happen anymore.
r? @matthiaskrgr
Fix ICE #3747
I'm not sure if this was the correct approach.
I don't know if I put tests/ui/crashses/ice-3747.rs in correct place because the test always passed when I ran it with `cargo test`, even without the fix applied.
If I run that test with `env CLIPPY_TESTS=true cargo run --bin clippy-driver -- -L ./target/debug tests/ui/crashes/ice-3747.rs` then the test correctly fails without the fix applied
fixes#3747
Extract diagnostics module and document some functions
This moves the lint building functions from `utils/mod.rs` to their own
`utils/diagnostics.rs` file. Also adds documentation for three of them.
Make needless_range_loop not applicable to structures without iter method
Fixes https://github.com/rust-lang/rust-clippy/issues/3788
Now we will start lint indexed structure only if it has known iter or iter_mut method implemented.
Don't warn about an argument that is moved into a closure.
ExprUseVisitor doesn't walk into nested bodies so use a new
visitor that collects the variables that are moved into closures.
Fixes#3739
Don't check [print/write]_with_newline on raw strings
Some tests for #3778 and some maybe-not-the-greatest code that passes those tests!
I didn't run `fmt` because a) it doesn't seem to install on nightly for me, and b) on stable it wanted to apply formatting to over 90 files. Happy to make any tweaks though!
I suspect this contribution may require more than just tweaks. I'm still sort of new to rust so it may not be idiomatic, and the specific approach I took feels a little heavy-handed and brittle. I'm happy to make changes with some guidance, or equally happy if this gives a starting place for someone else to do it better :)
Add a lint to warn on `T: Drop` bounds
**What it does:** Checks for generics with `std::ops::Drop` as bounds.
**Why is this bad?** `Drop` bounds do not really accomplish anything.
A type may have compiler-generated drop glue without implementing the
`Drop` trait itself. The `Drop` trait also only has one method,
`Drop::drop`, and that function is by fiat not callable in user code.
So there is really no use case for using `Drop` in trait bounds.
**Known problems:** None.
**Example:**
```rust
fn foo<T: Drop>() {}
```
Fixes#3773
Both regular strings and raw strings can contain literal newlines. This commit
extends the lint to also warn about terminating strings with these.
Behaviour handling for raw strings is also moved into `check_newlines` by
passing in the `is_raw` boolean from `check_tts` as
[suggested](https://github.com/rust-lang/rust-clippy/pull/3781#pullrequestreview-204663732)
Pass tests for #3778, {print,write}_with_newline false positive
This change guards the lint from checking newlines with a sort of complicated
check to see if it's a raw string. Raw strings shouldn't be newline-checked,
since r"\n" is literally \-n, not a newline. I think it's ok not to check for
_literal_ newlines at the end of raw strings, but maybe that's debatable.
I... don't think this code is that great. I wanted to write the check after
`check_tts`, but that was too late -- raw string type info is lost (or I
couldn't find it). Putting it inside `check_tts` feels heavy-duty and the check
itself feels like a brittle reach possibly into places it shouldn't.
Maybe someone can fix this up :)
**What it does:** Checks for generics with `std::ops::Drop` as bounds.
**Why is this bad?** `Drop` bounds do not really accomplish anything.
A type may have compiler-generated drop glue without implementing the
`Drop` trait itself. The `Drop` trait also only has one method,
`Drop::drop`, and that function is by fiat not callable in user code.
So there is really no use case for using `Drop` in trait bounds.
**Known problems:** None.
**Example:**
```rust
fn foo<T: Drop>() {}
```
Fix ICE in needless_pass_by_value lint
If I understand it correctly, we were first creating a type with a
`RegionKind::ReErased` region and then deleted it again in
`util::implements_trait` with:
cx.tcx.erase_regions(&ty);
causing the type query to fail.
It looks like using `ReEmpty` works around that deletion.
Fixes#3144
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.
Add a uitest subcommand to simplify UI test invocation
This makes running single tests a lot easier.
It's now
`TESTNAME=xxx cargo uitest`
instead of
`TESTNAME=xxx cargo test --test compile-test`
Start making clippy easier to invoke in non-cargo contexts
Clippy (clippy-driver) currently has a couple of strong but unnecessary couplings with cargo. This series:
1. makes detection of check builds more robust, and
2. make clippy-driver use the --sysroot specified on the command line as its internal sysroot.
If I understand it correctly, we were first creating a type with a
`RegionKind::ReErased` region and then deleted it again in
`util::implements_trait` with:
cx.tcx.erase_regions(&ty);
causing the type query to fail.
It looks like using `ReEmpty` works around that deletion.
Fix `cast_sign_loss` false positive
This checks if the value is a non-negative constant before linting about
losing the sign.
Because the `constant` function doesn't handle const functions, we check if
the value is from a call to a `max_value` function directly. A utility method
called `get_def_path` was added to make checking for the function paths
easier.
Fixes#2728
The rustc change added HirId to a few nodes. As I understand it, the plan is
to remove the NodeId from these nodes eventually. Where the NodeId was
not being matched, I used `..` to try and avoid further breakage. Where it
was, I used `_` to make the fix easier when NodeId is removed.
Adding lint test for excessive LOC.
This is a WIP for #2377. Just wanted to pull in because I had a few questions:
1. Is it okay that I'm approaching this via counting by looking at each line in the snippet instead of looking at the AST tree? If there's another way to do it, I want to make sure I'm doing the correct way, but I wasn't sure since the output AST JSON doesn't seem to contain whitespace.
2. My function is definitely going to trigger the lint, so also wanted to see if there was something obvious I could do to reduce it.
3. Are the two tests fine, or is there something obvious I'm missing?
4. Obviously bigger question - am I approaching the line count correctly. Current strategy is count a line if it contains some code, so skip if it's just comments or empty.
`hir::Ty` doesn't seem to know anything about type bounds and
`cx.tcx.type_of(def_id)` caused an ICE when it was passed a generic type
with a bound:
```
src/librustc_typeck/collect.rs:1311: unexpected non-type Node::GenericParam: Type { default: None, synthetic: None }
```
Converting it to a proper `Ty` fixes the ICE and catches a few more
places where the lint applies.
This checks if the value is a non-negative constant before linting about
losing the sign.
Because the `constant` function doesn't handle const functions, we check if
the value is from a call to a `max_value` function directly. A utility method
called `get_def_path` was added to make checking for the function paths
easier.
Fixes#2728
Add initial version of const_fn lint
This adds an initial version of a lint that can tell if a function could be `const`.
TODO:
- [x] Finish up the docs
- [x] Fix the ICE
cc #2440
Prevent incorrect cast_lossless suggestion in const_fn
`::from` is not a const fn, so applying the suggestion of
`cast_lossless` would fail to compile. The fix is to skip the lint if
the cast is found inside a const fn.
Fixes#3656
Fix documentation for `slow_vector_initialization`
This PR fixes the documentation for the lint `slow_vector_initialization`. The documentation recommended writing `vec![len; 0]` but the correct solution is `vec![0; len]`.
for file in `fd \.rs$` ; do sed -i s/span_suggestion_with_applicability/span_suggestion/g $file ; done
for file in `fd \.rs$` ; do sed -i s/span_suggestion_short_with_applicability/span_suggestion_short/g $file ; done
for file in `fd \.rs$` ; do sed -i s/span_suggestions_with_applicability/span_suggestions/g $file ; done
Fix `expect_fun_call` lint suggestions
This commit corrects some bad suggestions produced by the
`expect_fun_call` lint and enables `rust-fix` checking on the tests.
Addresses #3630
`::from` is not a const fn, so applying the suggestion of
`cast_lossless` would fail to compile. The fix is to skip the lint if
the cast is found inside a const fn.
* 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
...
Fix automatic suggestion on `use_self`.
In an example like this:
```rust
impl Example {
fn fun_1() { }
fn fun_2() {
Example::fun_1();
}
}
```
Clippy tries to replace `Example::fun_1` with `Self`, loosing `::fun_1` in the process, it should rather try to replace `Example` with `Self`.
**Question**
- There may be other paths that need the same treatment, but I'm not sure I understand them fully:
- e648adf086/clippy_lints/src/use_self.rs (L94-L96)
- e648adf086/clippy_lints/src/use_self.rs (L225-L229)
Fix `implicit_return` false positives.
Fixes the following false positives:
- linting on `if let` without `else` in a `loop` even with a present `return`
- linting on `unreachable!()`
Catch up with `format_args` change
Catches up with a change in rust-lang/rust#57537. (Since the optimization is optional, this clippy PR can be merged before the rustc PR.)
Happened to fix a bug in `expect_fun_call`, that is the lint ignores more than
one arguments to `format`.
```
warning: use of `expect` followed by a function call
--> src/main.rs:2:17
|
2 | Some("foo").expect(format!("{} {}", 1, 2).as_ref());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| panic!("{} {}", 1))`
|
```
Catches up with a change in rust-lang/rust#57537
Happened to fix a bug in `expect_fun_call`, that is the lint ignores more than
one arguments to `format`.
`cloned` requires that the elements of the iterator must be references. This
change determines if that is the case by examining the type of the closure
argument and suggesting `.cloned` only if it is a reference. When the closure
argument is not a reference, it suggests removing the `map` call instead.
A minor problem with this change is that the new check sometimes overlaps
with the `clone_on_copy` lint.
Fixes#498
Add several run rustfix annotations
Adds `run-rustfix` to 18 of the tests from the tracking issue #3630.
Each test has its own commit, to make reviewing easier (hopefully this is easier to review than 18 separate PRs).
## Changes
- `cfg_attr_rustfmt`: Custom inner attributes are unstable. Let's disable the lint for inner attributes until [#54726](https://github.com/rust-lang/rust/issues/54726) stabilizes
- `collapsible_if`: unrelated cyclomatic_complexity warning that can be ignored
- `duration_subsec`: Simply needed `#![allow(dead_code)]`
- `excessive_precision`: Fixed by `#!allow(dead_code,unused_variables)`
- `explicit_write`: Fixed by `#![allow(unused_imports)]`
- `inconsistent_digit_grouping`: Avoid triggering `clippy::excessive_precision` lint
- `infallible_destructuring_match`: Fixed by `#![allow(dead_code, unreachable_code, unused_variables)]`
- `into_iter_on_ref`: Triggered unrelated `clippy::useless_vec` lint
- `large_digit_groups`: Avoid triggering `clippy::excessive_precision` lint
- `map_clone`: Fixed by `#![allow(clippy::iter_cloned_collect)]`
- `mem_replace`: Suggestion causes import to be unused, fixed by `#![allow(unused_imports)]`
- `precedence`: Allow some unrelated lints, and change out-of-range `0b1111_1111i8` literal
- `redundant_field_names`: Allow dead code, and remove stabilized feature toggles
- `replace_consts`: Fixed by `#![allow(unused_variables)]`
- `starts_ends_with`: Fixed by `#![allow(unused_must_use)]`
- `types`: Fixed by `#![allow(dead_code, unused_variables)]`
- `unit_arg`: Fixed by `#[allow(unused_must_use)]`
- `unnecessary_fold`: Fixed by adding type annotations and adding `#![allow(dead_code)]`
Restrict `use_self` on nested items
Fixes#3637Fixes#3463
These changes make it so that nested items aren't visited any more by the `use_self` lint.
I think visiting nested items should be possible (so that it uses a different `item_path` for the nested item), but I'm not sure whether it's viable and what the best approach would be.
- Can `item_path` be changed to a new `Self` path before visiting the item, and then changing it back afterwards?
- Alternatively, could a new visitor be created, re-using `check_trait_method_impl_decl`?
cast_ref_to_mut lint
I see this pattern way too often, and it's completely wrong. In fact, due to how common this incorrect pattern is, [the Rustonomicon specifically points this out](https://doc.rust-lang.org/nomicon/transmutes.html).
> - Transmuting an & to &mut is UB
> - Transmuting an & to &mut is always UB
> - No you can't do it
> - No you're not special
This is my first lint.
Trigger `use_self` lint in local macros
Closes#2098
The test currently only covers local macros. #2098 suggested this:
> You could add the macro in question into the `mini_macro` subcrate
But that doesn't work for a `macro_rules`:
```
error: cannot export macro_rules! macros from a `proc-macro` crate type currently
```
So I suggest leaving out the test for external macros, as using `in_external_macro` seems straigtforward enough. Alternatives would be to use to add an additional crate (overkill if you ask me), or test with a `proc-macro`.
Integrate rustfix into Clippy test suite
Once the [PR to compiletest-rs](https://github.com/laumann/compiletest-rs/pull/151) is reviewed and merged this fixes#2376.
I will create a separate tracking issue for adding `run-rustfix` to all tests.
Only trigger `infinite_iter` lint for infinitely allocating `collect()` calls
Fixes #3538
~Oh, I guess this should actually check other methods like `count` as well, not only `collect()`.~
Never mind, `collect` is the only of these functions that allocates a data structure.
Fix if_same_then_else false positive
This fixes false positive in #3559.
The problem was that `SpanlessEq` does not check patterns in declarations. So this two blocks considered equal.
```rust
if true {
let (x, y) = foo();
} else {
let (y, x) = foo();
}
```
Not sure if the proposed change is safe as `SpanlessEq` is used extensively in other lints, but I tried hard to come up with counterexample and failed.
Merge new_without_default_derive into new_without_default
Closes#3525, deprecating new_without_default_derive and moving both lints into new_without_default.
Fix false positives for `implicit_return` and `empty_loop` on macro expansion.
This PR only fixes `implicit_return` and `empty_loop`.
But I suspect this bug may affect a lot of other lints.
Teach `suspicious_else_formatting` about `if .. {..} {..}`
We essentially treat bare blocks `{..}` identically to `if .. {..}`, except for different lint messages.
Fixes#3044
Implements lint for order comparisons against bool (#3438)
As described on issue #3438, this change implements linting for `>` and `<` comparisons against both `boolean` literals and between expressions.
Adds lint for Vec<Box<T: Sized>>
This adds, and subsequently closes#3530. This is the first time I've ever worked with anything remotely close to internal Rust code, so I'm very much unsure about the if_chain! to figure this out!
I can't get rustfmt working on WSL with nightly 2018-12-07:
`error: component 'rustfmt' for target 'x86_64-unknown-linux-gnu' is unavailable for download`
Lint redundant clone of fields
Makes `redundant_clone` warn on unnecessary `foo.field.clone()` sometimes (it can detect an unnecessary clone only if the base of projection, `foo` in this case, is not used at all after that). This is enough for cases like `returns_tuple().0.clone()`.
If there are more than one such assignment, the last one may be
the one supplied to `clone` method.
Makes `find_stmt_assigns_to` internally reverses the iterator to make
the intent to "iterate statements backward" clear.
The path of `libc::c_void` has changes in 5c1a6b8a6d
The DefId path is now always platform specific like
`libc::windows::c_void`. This fixes our c_void detection to only check
the first and last elements.
