try_err: Consider Try impl for Poll when generating suggestions
There are two different implementation of `Try` trait for `Poll` type:
`Poll<Result<T, E>>` and `Poll<Option<Result<T, E>>>`. Take them into
account when generating suggestions.
For example, for `Err(e)?` suggest either `return Poll::Ready(Err(e))` or
`return Poll::Ready(Some(Err(e)))` as appropriate.
Fixes#5855
changelog: try_err: Consider Try impl for Poll when generating suggestions
Add derive_ord_xor_partial_ord lint
Fix#1621
Some remarks:
This PR follows the example of the analogous derive_hash_xor_partial_eq lint where possible.
I initially tried using the `match_path` function to identify `Ord` implementation like the derive_hash_xor_partial_eq lint currently does, for `Hash` implementations but that didn't work.
Specifically, the structs at the top level were getting paths that matched `&["$crate", "cmp", "Ord"]` instead of `&["std", "cmp", "Ord"]`. While trying to figure out what to do instead I saw the comment at the top of [clippy_lints/src/utils/paths.rs](f5d429cd76/clippy_lints/src/utils/paths.rs (L5)) that mentioned [this issue](https://github.com/rust-lang/rust-clippy/issues/5393) and suggested to use diagnostic items instead of hardcoded paths whenever possible. I looked for a way to identify `Ord` implementations with diagnostic items, but (possibly because this was the first time I had heard of diagnostic items,) I was unable to find one.
Eventually I tried using `get_trait_def_id` and comparing `DefId` values directly and that seems to work as expected. Maybe there's a better approach however?
changelog: new lint: derive_ord_xor_partial_ord
Handle mapping to Option in `map_flatten` lint
Fixes#4496
The existing [`map_flatten`](https://rust-lang.github.io/rust-clippy/master/index.html#map_flatten) lint suggests changing `expr.map(...).flatten()` to `expr.flat_map(...)` when `expr` is `Iterator`. This PR changes suggestion to `filter_map` instead of `flat_map` when mapping to `Option`, because it is more natural
Also here are some questions:
* If expression has type which implements `Iterator` trait (`match_trait_method(cx, expr, &paths::ITERATOR) == true`), how can I get type of iterator elements? Currently I use return type of closure inside `map`, but probably it is not good way
* I would like to change suggestion range to cover only `.map(...).flatten()`, that is from:
```
let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| 0..x).flatten().collect();
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `flat_map` instead: `vec![5_i8; 6].into_iter().flat_map
```
to
```
let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| 0..x).flatten().collect();
^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `flat_map` instead: `.flat_map(|x| 0..x)`
```
Is it ok?
* Is `map_flatten` lint intentionally in `pedantic` category, or could it be moved to `complexity`?
changelog: Handle mapping to Option in [`map_flatten`](https://rust-lang.github.io/rust-clippy/master/index.html#map_flatten) lint
needless_collect: catch x: Vec<_> = iter.collect(); x.into_iter() ...
changelog: Expand the needless_collect lint as suggested in #5627 (WIP).
This PR is WIP because I can't figure out how to make the multi-part suggestion include its changes in the source code (the fixed is identical to the source, despite the lint making suggestions). Aside from that one issue, I think this should be good.
For consistency with `Attribute::has_name` which doesn't mark the attribute as used either.
Replace all uses of `check_name` with `has_name` outside of rustc
There are two different implementation of Try trait for Poll type;
Poll<Result<T, E>> and Poll<Option<Result<T, E>>>. Take them into
account when generating suggestions.
For example, for Err(e)? suggest either return Poll::Ready(Err(e)) or
return Poll::Ready(Some(Err(e))) as appropriate.
Fix FP for `suspicious_arithmetic_impl` from `suspicious_trait_impl` …
As discussed in #3215, the `suspicious_trait_impl` lint causes too many false positives, as it is complex to find out if binary operations are suspicious or not.
This PR restricts the number of binary operations to at most one, otherwise we don't lint.
This can be seen as very conservative, but at least FP can be reduced to bare minimum.
Fixes: #3215
changelog: limit the `suspicious_arithmetic_impl` lint to one binop, to avoid many FPs
Use `(std::)f64::EPSILON` in the examples as suggested in the lints
`float_cmp(_const)` suggests using `{f32|f64}::EPSILON` and it'd be great if the docs mentioned it.
changelog: none
This commit modifies the `substitute_normalize_and_test_predicates`
query, renaming it to `impossible_predicates` and only checking
predicates which do not require substs. By making this change,
polymorphization doesn't have to explicitly support vtables.
Signed-off-by: David Wood <david@davidtw.co>
Ignore not really redundant clones of ManuallyDrop
"Redundant" clones of `ManuallyDrop` are sometimes used for the side effect of
invoking the clone, without running the drop implementation of the inner type.
In other words, they aren't really redundant. For example, futures-rs crate:
```rust
#[allow(clippy::redundant_clone)] // The clone here isn't actually redundant.
unsafe fn increase_refcount<T: ArcWake>(data: *const ()) {
// Retain Arc, but don't touch refcount by wrapping in ManuallyDrop
let arc = mem::ManuallyDrop::new(Arc::<T>::from_raw(data as *const T));
// Now increase refcount, but don't drop new refcount either
let _arc_clone: mem::ManuallyDrop<_> = arc.clone();
}
```
changelog: Ignore redundant clone lint for ManuallyDrop.
"Redundant" clones of `ManuallyDrop` are sometimes used for the side effect of
invoking the clone, without running the drop implementation of the inner type.
In other words, they aren't really redundant. For example, futures-rs crate:
```rust
#[allow(clippy::redundant_clone)] // The clone here isn't actually redundant.
unsafe fn increase_refcount<T: ArcWake>(data: *const ()) {
// Retain Arc, but don't touch refcount by wrapping in ManuallyDrop
let arc = mem::ManuallyDrop::new(Arc::<T>::from_raw(data as *const T));
// Now increase refcount, but don't drop new refcount either
let _arc_clone: mem::ManuallyDrop<_> = arc.clone();
}
```
Ignore redundant clone lint for ManuallyDrop.
Panic multiple args
changelog: Fixes bug with `panic` lint reported in #5767. I also did the same changes to the lints for `todo`, `unimplemented` and `unreachable`, so those lints should now also detect calls to those macros with a message.
Removing RHS snippet from SHADOW_UNRELATED message.
Fixes#5703
I am not sure if I reinvented the wheel here, but I could not really find a snippet function that did this truncation, so I created the function. Please tell me if there was a more obvious way to do this, I am new here. 😄
changelog: Truncates multi-line RHS in shadow_unrelated message if it has more than 5 lines.
improve advice in iter_nth_zero
fixes#5783
*Please keep the line below*
changelog: For iter_nth_zero, the "use .next()" replacement advice is on the last line of the code snippet, where it is vulnerable to truncation. Display that advice at the beginning instead.
This lint catches cases where the last statement of a closure expecting
an instance of Ord has a trailing semi-colon. It compiles since the
closure ends up return () which also implements Ord but causes
unexpected results in cases such as sort_by_key.
Fixes#5080
reprise: rebase, update and address all concerns
Rename collapsable_if fix suggestion to "collapse nested if block"
The name "try" is confusing when shown as quick fix by rust-analyzer
changelog: Rename `collapsable_if` fix suggestion to "collapse nested if block"
The "use .next()" replacement advice is on the last line of the code snippet,
where it is vulnerable to truncation. Display that advice at the beginning
instead.
closes#5783
Fix out of bounds access by checking length equality BEFORE accessing by index.
Fixes#5780
changelog: fix out of bounds access in unnested_or_patterns lint.
Edit: I did not bother reducing a testcase from `librustc_typeck` crate but I can confirm that with the change the crash no longer occurs.
Move range_minus_one to pedantic
This moves the range_minus_one lint to the pedantic category, so there
will not be any warnings emitted by default. This should work around
problems where the suggestion is impossible to resolve due to the range
consumer only accepting a specific range implementation, rather than the
`RangeBounds` trait (see #3307).
While it is possible to work around this by extracting the boundary into
a variable, I don't think clippy should encourage people to disable or
work around lints, but instead the lints should be fixable. So hopefully
this will help until a proper implementation checks what the range is
used for.
*Please keep the line below*
changelog: move [`range_minus_one`] to pedantic
Some accuracy lints for floating point operations
This will add some lints for accuracy on floating point operations suggested by @clarfon in #2040 (fixes#2040).
These are the remaining lints:
- [x] x.powi(2) => x * x
- [x] x.logN() / y.logN() => x.logbase(y)
- [x] x.logbase(E) => x.log()
- [x] x.logbase(10) => x.log10()
- [x] x.logbase(2) => x.log2().
- [x] x * PI / 180 => x.to_radians()
- [x] x * 180 / PI => x.to_degrees()
- [x] (x + 1).log() => x.log_1p()
- [x] sqrt(x * x + y * y) => x.hypot(y)
changelog: Included some accuracy lints for floating point operations
new lint: match_like_matches_macro
Suggests using the `matches!` macro from `std` where appropriate.
`redundant_pattern_matching` has been moved into the `matches` pass to allow suppressing the suggestion where `is_some` and friends are a better replacement.
changelog: new lint: `match_like_matches_macro`
This moves the range_minus_one lint to the pedantic category, so there
will not be any warnings emitted by default. This should work around
problems where the suggestion is impossible to resolve due to the range
consumer only accepting a specific range implementation, rather than the
`RangeBounds` trait (see #3307).
While it is possible to work around this by extracting the boundary into
a variable, I don't think clippy should encourage people to disable or
work around lints, but instead the lints should be fixable. So hopefully
this will help until a proper implementation checks what the range is
used for.
single_match_else - single expr/stmt else block corner case
One approach to fix#3489.
See discussion in the issue.
changelog: single_match_else - single expr/stmt else block corner case fix
Rollup of 14 pull requests
Successful merges:
- #70563 ([rustdoc] Page hash handling)
- #73856 (Edit librustc_lexer top-level docs)
- #73870 (typeck: adding type information to projection)
- #73953 (Audit hidden/short code suggestions)
- #73962 (libstd/net/tcp.rs: #![deny(unsafe_op_in_unsafe_fn)])
- #73969 (mir: mark mir construction temporaries as internal)
- #73974 (Move A|Rc::as_ptr from feature(weak_into_raw) to feature(rc_as_ptr))
- #74067 (rustdoc: Restore underline text decoration on hover for FQN in header)
- #74074 (Fix the return type of Windows' `OpenOptionsExt::security_qos_flags`.)
- #74078 (Always resolve type@primitive as a primitive, not a module)
- #74089 (Add rust-analyzer to the build manifest)
- #74090 (Remove unused RUSTC_DEBUG_ASSERTIONS)
- #74102 (Fix const prop ICE)
- #74112 (Expand abbreviation in core::ffi description)
Failed merges:
r? @ghost
typeck: adding type information to projection
This commit modifies the Place as follow:
* remove 'ty' from ProjectionKind
* add type information into to Projection
* replace 'ty' in Place with 'base_ty'
* introduce 'ty()' in `Place` to return the final type of the `Place`
* introduce `ty_before_projection()` in `Place` to return the type of
a `Place` before i'th projection is applied
Closes https://github.com/rust-lang/project-rfc-2229/issues/5
Added restriction lint: pattern-type-mismatch
changelog: Added a new restriction lint `pattern-type-mismatch`. This lint is especially helpful for beginners learning about the magic behind pattern matching. (This explanation might be worth to include in the next changelog.)
This commit modifies the Place as follow:
* remove 'ty' from ProjectionKind
* add type information into to Projection
* replace 'ty' in Place with 'base_ty'
* introduce 'ty()' in `Place` to return the final type of the `Place`
* introduce `ty_before_projection()` in `Place` to return the type of
a `Place` before i'th projection is applied
Closes https://github.com/rust-lang/project-rfc-2229/issues/5
Rollup of 13 pull requests
Successful merges:
- #72620 (Omit DW_AT_linkage_name when it is the same as DW_AT_name)
- #72967 (Don't move cursor in search box when using arrows to navigate results)
- #73102 (proc_macro: Stop flattening groups with dummy spans)
- #73297 (Support configurable deny-warnings for all in-tree crates.)
- #73507 (Cleanup MinGW LLVM linkage workaround)
- #73588 (Fix handling of reserved registers for ARM inline asm)
- #73597 (Record span of `const` kw in GenericParamKind)
- #73629 (Make AssocOp Copy)
- #73681 (Update Chalk to 0.14)
- #73707 (Fix links in `SliceIndex` documentation)
- #73719 (emitter: column width defaults to 140)
- #73729 (disable collectionbenches for android)
- #73748 (Add code block to code in documentation of `List::rebase_onto`)
Failed merges:
r? @ghost
Record span of `const` kw in GenericParamKind
Context: this is needed for a fix of https://github.com/rust-lang/rustfmt/issues/4263,
which currently records the span of a const generic param incorrectly
because the location of the `const` kw is not known.
I am not sure how to add tests for this; any guidance in how to do so
would be appreciated 🙂
cmp_owned: handle when PartialEq is not implemented symmetrically
changelog: Handle asymmetrical implementations of PartialEq in [`cmp_owned`].
Fixes#4874
clone_on_copy - add machine applicability
Fix#4826.
Change the applicability of the lint clone_on_copy. Split a test file and run rustfix on the clone_on_copy part.
changelog: clone_on_copy - add machine applicability
Context: this is needed to fix https://github.com/rust-lang/rustfmt/issues/4263,
which currently records the span of a const generic param incorrectly
because the location of the `const` kw is not known.
I am not sure how to add tests for this; any guidance in how to do so
would be appreciated 🙂
#5626: lint iterator.map(|x| x)
changelog: adds a new lint for iterator.map(|x| x) (see https://github.com/rust-lang/rust-clippy/issues/5626)
The code also lints for result.map(|x| x) and option.map(|x| x). Also, I'm not sure if I'm checking for type adjustments correctly and I can't think of an example where .map(|x| x) would apply type adjustments.
Downgrade unnested_or_patterns to pedantic
Even with #5704 fixed, I don't believe it is a safe bet that if someone is using or-patterns anywhere in a codebase then they want to use it as much as possible in the whole codebase. I think it would be reasonable to reevaluate after the feature is stable. I feel that a warn-by-default lint suggesting use of an unstable feature, even if already being used in one place, is questionable.
changelog: Remove unnested_or_patterns from default set of enabled lints
The code should to check that the current expression _is_ the end
expression; not that it's equal to it. The equality check seems very
wasteful in terms of performance.
New lint: suggest `ptr::read` instead of `mem::replace(..., uninitialized())`
resolves: #5575
changelog: improvements to `MEM_REPLACE_WITH_UNINIT`:
- add a new test case in `tests/ui/repl_uninit.rs` to cover the case of replacing with `mem::MaybeUninit::uninit().assume_init()`.
- modify the existing `MEM_REPLACE_WITH_UNINIT` when replacing with `mem::uninitialized` to suggest using `ptr::read` instead.
- lint with `MEM_REPLACE_WITH_UNINIT` when replacing with `mem::MaybeUninit::uninit().assume_init()`
For the following code
```rust
let c = || bar(foo.x, foo.x)
```
We generate two different `hir::Place`s for both `foo.x`.
Handling this adds overhead for analysis we need to do for RFC 2229.
We also want to store type information at each Projection to support
analysis as part of the RFC. This resembles what we have for
`mir::Place`
This commit modifies the Place as follows:
- Rename to `PlaceWithHirId`, where there `hir_id` is that of the
expressioin.
- Move any other information that describes the access out to another
struct now called `Place`.
- Removed `Span`, it can be accessed using the [hir
API](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/hir/map/struct.Map.html#method.span)
- Modify `Projection` to be a strucutre of its own, that currently only
contains the `ProjectionKind`.
Adding type information to projections wil be completed as part of https://github.com/rust-lang/project-rfc-2229/issues/5
Closes https://github.com/rust-lang/project-rfc-2229/issues/3
Co-authored-by: Aman Arora <me@aman-arora.com>
Co-authored-by: Roxane Fruytier <roxane.fruytier@hotmail.com>
Stabilize Option::zip
This PR stabilizes the following API:
```rust
impl<T> Option<T> {
pub fn zip<U>(self, other: Option<U>) -> Option<(T, U)>;
}
```
This API has real world usage as seen in <https://grep.app/search?q=-%3E%20Option%3C%5C%28T%2C%5Cs%3FU%5C%29%3E®exp=true&filter[lang][0]=Rust>.
The `zip_with` method is left unstably as this API is kinda niche
and it hasn't received much usage in Rust repositories on GitHub.
cc #70086
Clean up type alias impl trait implementation
- Removes special case for top-level impl trait
- Removes associated opaque types
- Forbid lifetime elision in let position impl trait. This is consistent with the behavior for inferred types.
- Handle lifetimes in type alias impl trait more uniformly with other parameters
cc #69323
cc #63063Closes#57188Closes#62988Closes#69136Closes#73061
let_and_return: avoid "does not live long enough" errors
EDIT: Add #3324 to the list of fixes
<details>
<summary>Description of old impl</summary>
<br>
Avoid suggesting turning the RHS expression of the last statement into the block tail expression if a temporary borrows from a local that would be destroyed before.
This is my first incursion into MIR so there's probably room for improvement!
</details>
Avoid linting if the return type of some method or function called in the last statement has a lifetime parameter.
changelog: Fix false positive in [`let_and_return`]
Fixes#3792Fixes#3324
Make `PolyTraitRef::self_ty` return `Binder<Ty>`
This came up during review of #71618. The current implementation is the same as a call to `skip_binder` but harder to audit. Make it preserve binding levels and add a call to `skip_binder` at all use sites so they can be audited as part of #72507.
Give corrected code
This PR adds corrected code for doc examples.
I did this in several commits to facilitate review.
Don't hesitate to tell me if I forgot some.
Also, sometimes I felt it was not necessary to give corrected code, but I maybe wrong.
fixes: #4829
changelog: Improve documentation examples across multiple lints.
New lint: iter_next_slice
Hello, this is a work-in-progress PR for issue: https://github.com/rust-lang/rust-clippy/issues/5572
I have implemented lint to replace `iter().next()` for `slice[index..]` and `array` with `get(index)` and `get(0)` respectively. However since I made a lot of changes, I would like to request some feedback before continuing so that I could fix mistakes.
Thank you!
---
changelog: implement `iter_next_slice` lint and test, and modify `needless_continues`, `for_loop_over_options_result` UI tests since they have `iter().next()`
Add regression test for endless loop / update `pulldown_cmark`
Closes#4917
This was fixed in pulldown_cmark 0.7.1, specifically raphlinus/pulldown-cmark#438
changelog: none
Rework suggestion generation of `unit_arg` lint
Found this bug while running `cargo fix --clippy` on quite a big codebase:
This would replace something like:
```rust
Some(fn_that_actually_does_something(&a, b))
```
with
```rust
Some(())
```
which obviously suppresses side effects.
Since pretty much every expression could have side effects, I think making this suggestion `MaybeIncorrect` is the best thing to do here.
A correct suggestion would be:
```rust
fn_that_actually_does_something(&a, b);
Some(())
```
Somehow the suggestion is not correctly applied to the arguments, when more than one argument is a unit value. I have to look into this a little more, though.
changelog: Fixes suggestion of `unit_arg` lint, so that it suggests semantic equivalent code
Fixes#4741
len_zero: skip ranges if feature `range_is_empty` is not enabled
If the feature is not enabled, calling `is_empty()` on a range is ambiguous. Moreover, the two possible resolutions are unstable methods, one inherent to the range and the other being part of the `ExactSizeIterator` trait.
Since `len_zero` only checks for existing `is_empty()` inherent methods, we only take into account the `range_is_empty` feature.
Related: https://github.com/rust-lang/rust/issues/48111#issuecomment-445132965
changelog: len_zero: avoid linting ranges without #![feature(range_is_empty)]
Fixes: #3807
Extend useless conversion
This PR extends `useless_conversion` lint with `TryFrom` and `TryInto`
fixes: #5344
changelog: Extend `useless_conversion` with `TryFrom` and `TryInto`
Make empty_line_after_outer_attr an early lint
Fixes#5567
Unfortunately I couldn't find a way to reproduce the issue without syn/quote. Considering that most real-world macros use syn and/or quote, I think it's okay to pull them in anyway.
changelog: Fix false positive in [`empty_line_after_outer_attr`]
reversed_empty_ranges: add suggestion for &slice[N..N]
As discussed in the issue thread, the user accepted this solution. Let me know if this is what we want, or if changing the way we lint the N..N case is prefered.
changelog: reversed_empty_ranges: add suggestion for &slice[N..N]
Closes#5628
ptr_arg: honor `allow` attribute on arguments
The `intravisit::Visitor` impl for `LateContextAndPass` only takes into account the attributes of a function parameter inside the `check_param` method. `ptr_arg` starts its heuristics at `check_item` / `check_impl_item` / `check_trait_item`, so the `allow` is not taken into account automatically.
changelog: ptr_arg: honor `allow` attribute on arguments
Fixes#5644
new_without_default: do not suggest deriving
---
changelog: do not suggest deriving `Default` in `new_without_default`
This commit changes the behavior of the `new_without_default` lint to not suggest deriving `Default`. This suggestion is misleading if the `new` implementation does something different than what a derived `Default` implementation would do, because then the two methods would not be equivalent.
Instead, the `can_derive_default` check is removed, and we always suggest implementing `Default` in terms of `new()`.
Clarify the documentation of the `unnecessary_mut_passed` lint
fixes#5433 by replacing "giving" with "passing"
changelog: Clarifies documentation for `unnecessary_mut_passed`
Add to the list of words clippy::doc_markdown ignores
"TypeScript" is the only one of these I actually ran into organically; I can remove the others if they're too much.
changelog: Add to the list of words `clippy::doc_markdown` ignores
New lint: `match_wildcard_for_single_variants`
changelog: Added a new lint match_wildcard_for_single_variants to warn on enum matches where a wildcard is used to match a single variant
Closes#5556
Rename lint `identity_conversion` to `useless_conversion`
Lint name `identity_conversion` was misleading, so this PR renames it to `useless_conversion`.
As decision has not really came up in the issue comments, this PR will probably need discussion.
fixes#3106
changelog: Rename lint `identity_conversion` to `useless_conversion`
Merge some lints together
This PR merges following lints:
- `block_in_if_condition_expr` and `block_in_if_condition_stmt` → `blocks_in_if_conditions`
- `option_map_unwrap_or`, `option_map_unwrap_or_else` and `result_map_unwrap_or_else` → `map_unwrap`
- `option_unwrap_used` and `result_unwrap_used` → `unwrap_used`
- `option_expect_used` and `result_expect_used` → `expect_used`
- `wrong_pub_self_convention` into `wrong_self_convention`
- `for_loop_over_option` and `for_loop_over_result` → `for_loops_over_fallibles`
Lints that have already been merged since the issue was created:
- [x] `new_without_default` and `new_without_default_derive` → `new_without_default`
Need more discussion:
- `string_add` and `string_add_assign`: do we agree to merge them or not? Is there something more to do? → **not merge finally**
- `identity_op` and `modulo_one` → `useless_arithmetic`: seems outdated, since `modulo_arithmetic` has been created.
fixes#1078
changelog: Merging some lints together:
- `block_in_if_condition_expr` and `block_in_if_condition_stmt` → `blocks_in_if_conditions`
- `option_map_unwrap_or`, `option_map_unwrap_or_else` and `result_map_unwrap_or_else` → `map_unwrap_or`
- `option_unwrap_used` and `result_unwrap_used` → `unwrap_used`
- `option_expect_used` and `result_expect_used` → `expect_used`
- `for_loop_over_option` and `for_loop_over_result` → `for_loops_over_fallibles`
Literal error reporting cleanup
While doing some performance work, I noticed some code duplication in `librustc_parser/lexer/mod.rs`, so I cleaned it up.
This PR is probably best reviewed commit by commit.
I'm not sure what the API stability practices for `librustc_lexer` are. Four public methods in `unescape.rs` can be removed, but two are used by clippy, so I left them in for now.
I could open a PR for Rust-Analyzer when this one lands.
But how do I open a PR for clippy? (Git submodules are frustrating to work with)
identity_op: allow `1 << 0`
I went for accepting `1 << 0` verbatim instead of something more general as it seems to be what everyone in the issue thread needed.
changelog: identity_op: allow `1 << 0` as it's a common pattern in bit manipulation code.
Fixes#3430
Downgrade useless_let_if_seq to nursery
I feel that this lint has the wrong balance of incorrect suggestions for a default-enabled lint.
The immediate code I faced was something like:
```rust
fn main() {
let mut good = do1();
if !do2() {
good = false;
}
if good {
println!("good");
}
}
fn do1() -> bool { println!("1"); false }
fn do2() -> bool { println!("2"); false }
```
On this code Clippy calls it unidiomatic and suggests the following diff, which has different behavior in a way that I don't necessarily want.
```diff
- let mut good = do1();
- if !do2() {
- good = false;
- }
+ let good = if !do2() {
+ false
+ } else {
+ do1()
+ };
```
On exploring issues filed about this lint, I have found that other users have also struggled with inappropriate suggestions (https://github.com/rust-lang/rust-clippy/issues/4124, https://github.com/rust-lang/rust-clippy/issues/3043, https://github.com/rust-lang/rust-clippy/issues/2918, https://github.com/rust-lang/rust-clippy/issues/2176) and suggestions that make the code worse (https://github.com/rust-lang/rust-clippy/issues/3769, https://github.com/rust-lang/rust-clippy/issues/2749). Overall I believe that this lint is still at nursery quality for now and should not be enabled.
---
changelog: Remove useless_let_if_seq from default set of enabled lints
Reversed empty ranges
This lint checks range expressions with inverted limits which result in empty ranges. This includes also the ranges used to index slices.
The lint reverse_range_loop was covering iteration of reversed ranges in a for loop, which is a subset of what this new lint covers, so it has been removed. I'm not sure if that's the best choice. It would be doable to check in the new lint that we are not in the arguments of a for loop; I went for removing it because the logic was too similar to keep them separated.
changelog: Added reversed_empty_ranges lint that checks for ranges where the limits have been inverted, resulting in empty ranges. Removed reverse_range_loop which was covering a subset of the new lint.
Closes#4192Closes#96
Extend example for the `unneeded_field_pattern` lint
Current example is incorrect (or pseudo-code) because a struct name is omitted. I have used the code from the tests instead. Perhaps this example can be made less verbose, but I think it is more convenient to see a "real" code as an example.
---
changelog: extend example for the `unneeded_field_pattern` lint
Fix match on vec items: match on vec[..]
- Added new tests
- Fixed false positive when matching on full range, which will never panic
Closes#5551
changelog: fix match_on_vec_items when matching full range
Fix `unnecessary_unwrap` lint when checks are done in parameters
Fixes a false positive in `unnecessary_unwrap` lint when checks are done in macro parameters.
FIxes#5174
changelog: Fixes a false positive in `unnecessary_unwrap` lint when checks are done in macro parameters.
Fix FP on while-let-on-iterator
- fix `is_refutable` for slice patterns
- fix `is_refutable` for bindings
- add some TODO-s for cases, which can not be fixed easily
fixes#3780
changelog: fix FP on while-let-on-iterator for arrays and bindings
Implement the manual_non_exhaustive lint
Some implementation notes:
* Not providing automatic fixups because additional changes may be needed in other parts of the code, e.g. when constructing a struct.
* Even though the attribute is valid on enum variants, it's not possible to use the manual implementation of the pattern because the visibility is always public, so the lint ignores enum variants.
* Unit structs are also ignored, it's not possible to implement the pattern manually without fields.
* The attribute is not accepted in unions, so those are ignored too.
* Even though the original issue did not mention it, tuple structs are also linted because it's possible to apply the pattern manually.
changelog: Added the manual non-exhaustive implementation lint
Closes#2017
Fix the bugs of `manual_memcpy`, simplify the suggestion and refactor it
While I’m working on the long procrastinated work to expand `manual_memcpy`(#1670), I found a few minor bugs and probably unidiomatic or old coding style. There is a brief explanation of changes to the behaviour this PR will make below. And, I have a questoin: do I need to add tests for the first and second fixed bugs? I thought it might be too rare cases to include the tests for those. I added for the last one though.
* Bug fix
* It negates resulted offsets (`src/dst_offset`) when `offset` is subtraction by 0. This PR will remove any subtraction by 0 as a part of minification.
```rust
for i in 0..5 {
dst[i - 0] = src[i];
}
```
```diff
warning: it looks like you're manually copying between slices
--> src/main.rs:2:14
|
LL | for i in 0..5 {
- | ^^^^ help: try replacing the loop by: `dst[..-5].clone_from_slice(&src[..5])`
+ | ^^^^ help: try replacing the loop by: `dst[..5].clone_from_slice(&src[..5])`
|
```
* It prints `RangeTo` or `RangeFull` when both of `end` and `offset` are 0, which have different meaning. This PR will print 0. I could reject the cases `end` is 0, but I thought I won’t catch other cases `reverse_range_loop` will trigger, and it’s over to catch every such cases.
```rust
for i in 0..0 {
dst[i] = src[i];
}
```
```diff
warning: it looks like you're manually copying between slices
--> src/main.rs:2:14
|
LL | for i in 0..0 {
- | ^^^^ help: try replacing the loop by: `dst.clone_from_slice(&src[..])`
+ | ^^^^ help: try replacing the loop by: `dst[..0].clone_from_slice(&src[..0])`
|
```
* it prints four dots when `end` is `None`. This PR will ignore any `for` loops without `end` because a `for` loop that takes `RangeFrom` as its argument and contains indexing without the statements or the expressions that end loops such as `break` will definitely panic, and `manual_memcpy` should ignore the loops with such control flow.
```rust
fn manual_copy(src: &[u32], dst: &mut [u32]) {
for i in 0.. {
dst[i] = src[i];
}
}
```
```diff
-warning: it looks like you're manually copying between slices
- --> src/main.rs:2:14
- |
-LL | for i in 0.. {
- | ^^^ help: try replacing the loop by: `dst[....].clone_from_slice(&src[....])`
- |
```
* Simplification of the suggestion
* It prints 0 when `start` or `end` and `offset` are same (from #3323). This PR will use `RangeTo`
changelog: fixed the bugs of `manual_memcpy` and also simplify the suggestion.
Current example is incorrect (or pseudo-code) because a struct name is omitted. I have used the code from the tests instead. Perhaps this example can be made less verbose, but I think it is more convenient to see a "real" code as an example.
New lint `match_vec_item`
Added new lint to warn a match on index item which can panic. It's always better to use `get(..)` instead.
Closes#5500
changelog: New lint `match_on_vec_items`
- Show just one error message with multiple suggestions in case of
using multiple times an OS in target family position
- Only suggest #[cfg(unix)] when the OS is in the Unix family
- Test all the operating systems
Don't trigger while_let_on_iterator when the iterator is recreated every iteration
r? @phansch
Fixes#1654
changelog: Fix false positive in [`while_let_on_iterator`]
Downgrade match_bool to pedantic
I don't quite buy the justification in https://rust-lang.github.io/rust-clippy/. The justification is:
> It makes the code less readable.
In the Rust codebases I've worked in, I have found people were comfortable using `match bool` (selectively) to make code more readable. For example, initializing struct fields is a place where the indentation of `match` can work better than the indentation of `if`:
```rust
let _ = Struct {
v: {
...
},
w: match doing_w {
true => ...,
false => ...,
},
x: Nested {
c: ...,
b: ...,
a: ...,
},
y: if doing_y {
...
} else { // :(
...
},
z: ...,
};
```
Or sometimes people prefer something a bit less pithy than `if` when the meaning of the bool doesn't read off clearly from the condition:
```rust
if set.insert(...) {
... // ???
} else {
...
}
match set.insert(...) {
// set.insert returns false if already present
false => ...,
true => ...,
}
```
Or `match` can be a better fit when the bool is playing the role more of a value than a branch condition:
```rust
impl ErrorCodes {
pub fn from(b: bool) -> Self {
match b {
true => ErrorCodes::Yes,
false => ErrorCodes::No,
}
}
}
```
And then there's plain old it's-1-line-shorter, which means we get 25% more content on a screen when stacking a sequence of conditions:
```rust
let old_noun = match old_binding.is_import() {
true => "import",
false => "definition",
};
let new_participle = match new_binding.is_import() {
true => "imported",
false => "defined",
};
```
Bottom line is I think this lint fits the bill better as a pedantic lint; I don't think linting on this by default is justified.
changelog: Remove match_bool from default set of enabled lints
