Skip to content

Commit ab5d3a3

Browse files
samueltardieusamueltardieu
committed
Add collapse_let_chains option to collapsible_if lint
Since `collapsible_if` is an early lint, it is not possible to automatically check whether the `let_chains` unstable rustc feature is enabled or not, as features are not available in the `EarlyContext` object.
2 parents 419a0a2 + 6e26ad1 commit ab5d3a3

17 files changed

+173
-47
lines changed

Diff for: ‎.github/workflows/lintcheck.yml

+2-2
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ jobs:
6666

6767
- name: Run lintcheck
6868
if: steps.cache-json.outputs.cache-hit != 'true'
69-
run: ./target/debug/lintcheck --format json --all-lints --crates-toml ./lintcheck/ci_crates.toml
69+
run: env CLIPPY_CONF_DIR="$PWD/lintcheck/ci-config" ./target/debug/lintcheck --format json --all-lints --crates-toml ./lintcheck/ci_crates.toml
7070

7171
- name: Upload base JSON
7272
uses: actions/upload-artifact@v4
@@ -97,7 +97,7 @@ jobs:
9797
run: cargo build --manifest-path=lintcheck/Cargo.toml
9898

9999
- name: Run lintcheck
100-
run: ./target/debug/lintcheck --format json --all-lints --crates-toml ./lintcheck/ci_crates.toml
100+
run: env CLIPPY_CONF_DIR="$PWD/lintcheck/ci-config" ./target/debug/lintcheck --format json --all-lints --crates-toml ./lintcheck/ci_crates.toml
101101

102102
- name: Upload head JSON
103103
uses: actions/upload-artifact@v4

Diff for: ‎CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -6299,6 +6299,7 @@ Released 2018-09-13
62996299
[`cargo-ignore-publish`]: https://doc.rust-lang.org/clippy/lint_configuration.html#cargo-ignore-publish
63006300
[`check-private-items`]: https://doc.rust-lang.org/clippy/lint_configuration.html#check-private-items
63016301
[`cognitive-complexity-threshold`]: https://doc.rust-lang.org/clippy/lint_configuration.html#cognitive-complexity-threshold
6302+
[`collapse-let-chains`]: https://doc.rust-lang.org/clippy/lint_configuration.html#collapse-let-chains
63026303
[`disallowed-macros`]: https://doc.rust-lang.org/clippy/lint_configuration.html#disallowed-macros
63036304
[`disallowed-methods`]: https://doc.rust-lang.org/clippy/lint_configuration.html#disallowed-methods
63046305
[`disallowed-names`]: https://doc.rust-lang.org/clippy/lint_configuration.html#disallowed-names

Diff for: ‎book/src/lint_configuration.md

+11
Original file line numberDiff line numberDiff line change
@@ -437,6 +437,17 @@ The maximum cognitive complexity a function can have
437437
* [`cognitive_complexity`](https://rust-lang.github.io/rust-clippy/master/index.html#cognitive_complexity)
438438

439439

440+
## `collapse-let-chains`
441+
Whether `if let` chains should be collapsed. This requires the use of the unstable
442+
`let_chains` rustc feature.
443+
444+
**Default Value:** `false`
445+
446+
---
447+
**Affected lints:**
448+
* [`collapsible_if`](https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_if)
449+
450+
440451
## `disallowed-macros`
441452
The list of disallowed macros, written as fully qualified paths.
442453

Diff for: ‎clippy_config/src/conf.rs

+4
Original file line numberDiff line numberDiff line change
@@ -469,6 +469,10 @@ define_Conf! {
469469
/// The maximum cognitive complexity a function can have
470470
#[lints(cognitive_complexity)]
471471
cognitive_complexity_threshold: u64 = 25,
472+
/// Whether `if let` chains should be collapsed. This requires the use of the unstable
473+
/// `let_chains` rustc feature.
474+
#[lints(collapsible_if)]
475+
collapse_let_chains: bool = false,
472476
/// DEPRECATED LINT: CYCLOMATIC_COMPLEXITY.
473477
///
474478
/// Use the Cognitive Complexity lint instead.

Diff for: ‎clippy_lints/src/collapsible_if.rs

+10-4
Original file line numberDiff line numberDiff line change
@@ -78,16 +78,24 @@ declare_clippy_lint! {
7878
}
7979

8080
pub struct CollapsibleIf {
81+
collapse_let_chains: bool,
8182
lint_commented_code: bool,
8283
}
8384

8485
impl CollapsibleIf {
8586
pub fn new(conf: &'static Conf) -> Self {
8687
Self {
88+
collapse_let_chains: conf.collapse_let_chains,
8789
lint_commented_code: conf.lint_commented_code,
8890
}
8991
}
9092

93+
/// Prevent triggering on `if c { if let a = b { .. } }` unless the
94+
/// `collapse_let_chains` config option is set.
95+
fn is_collapsible(&self, cond: &ast::Expr) -> bool {
96+
self.collapse_let_chains || !matches!(cond.kind, ast::ExprKind::Let(..))
97+
}
98+
9199
fn check_collapsible_else_if(cx: &EarlyContext<'_>, then_span: Span, else_: &ast::Expr) {
92100
if let ast::ExprKind::Block(ref block, _) = else_.kind
93101
&& !block_starts_with_comment(cx, block)
@@ -126,8 +134,7 @@ impl CollapsibleIf {
126134
if let Some(inner) = expr_block(then)
127135
&& inner.attrs.is_empty()
128136
&& let ast::ExprKind::If(ref check_inner, ref content, None) = inner.kind
129-
// Prevent triggering on `if c { if let a = b { .. } }`.
130-
&& !matches!(check_inner.kind, ast::ExprKind::Let(..))
137+
&& self.is_collapsible(check_inner)
131138
&& let ctxt = expr.span.ctxt()
132139
&& inner.span.ctxt() == ctxt
133140
&& let contains_comment = span_contains_comment(cx.sess().source_map(), check.span.to(check_inner.span))
@@ -171,8 +178,7 @@ impl EarlyLintPass for CollapsibleIf {
171178
{
172179
if let Some(else_) = else_ {
173180
Self::check_collapsible_else_if(cx, then.span, else_);
174-
} else if !matches!(cond.kind, ast::ExprKind::Let(..)) {
175-
// Prevent triggering on `if c { if let a = b { .. } }`.
181+
} else if self.is_collapsible(cond) {
176182
self.check_collapsible_if_if(cx, expr, cond, then);
177183
}
178184
}

Diff for: ‎clippy_lints/src/escape.rs

+5-4
Original file line numberDiff line numberDiff line change
@@ -94,11 +94,12 @@ impl<'tcx> LateLintPass<'tcx> for BoxedLocal {
9494
// find `self` ty for this trait if relevant
9595
if let ItemKind::Trait(_, _, _, _, items) = item.kind {
9696
for trait_item in items {
97-
if trait_item.id.owner_id.def_id == fn_def_id
97+
if trait_item.id.owner_id.def_id == fn_def_id {
9898
// be sure we have `self` parameter in this function
99-
&& trait_item.kind == (AssocItemKind::Fn { has_self: true })
100-
{
101-
trait_self_ty = Some(TraitRef::identity(cx.tcx, trait_item.id.owner_id.to_def_id()).self_ty());
99+
if trait_item.kind == (AssocItemKind::Fn { has_self: true }) {
100+
trait_self_ty =
101+
Some(TraitRef::identity(cx.tcx, trait_item.id.owner_id.to_def_id()).self_ty());
102+
}
102103
}
103104
}
104105
}

Diff for: ‎clippy_lints/src/item_name_repetitions.rs

+11-10
Original file line numberDiff line numberDiff line change
@@ -282,21 +282,22 @@ fn check_struct_name_repetition(cx: &LateContext<'_>, item: &Item<'_>, fields: &
282282
"field name starts with the struct's name",
283283
);
284284
}
285-
if field_words.len() > item_name_words.len()
285+
if field_words.len() > item_name_words.len() {
286286
// lint only if the end is not covered by the start
287-
&& field_words
287+
if field_words
288288
.iter()
289289
.rev()
290290
.zip(item_name_words.iter().rev())
291291
.all(|(a, b)| a == b)
292-
{
293-
span_lint_hir(
294-
cx,
295-
STRUCT_FIELD_NAMES,
296-
field.hir_id,
297-
field.span,
298-
"field name ends with the struct's name",
299-
);
292+
{
293+
span_lint_hir(
294+
cx,
295+
STRUCT_FIELD_NAMES,
296+
field.hir_id,
297+
field.span,
298+
"field name ends with the struct's name",
299+
);
300+
}
300301
}
301302
}
302303
}

Diff for: ‎clippy_lints/src/mem_replace.rs

+5-4
Original file line numberDiff line numberDiff line change
@@ -305,13 +305,14 @@ impl<'tcx> LateLintPass<'tcx> for MemReplace {
305305
&& let ExprKind::Path(ref func_qpath) = func.kind
306306
&& let Some(def_id) = cx.qpath_res(func_qpath, func.hir_id).opt_def_id()
307307
&& cx.tcx.is_diagnostic_item(sym::mem_replace, def_id)
308-
308+
{
309309
// Check that second argument is `Option::None`
310-
&& !check_replace_option_with_none(cx, src, dest, expr.span)
310+
if !check_replace_option_with_none(cx, src, dest, expr.span)
311311
&& !check_replace_option_with_some(cx, src, dest, expr.span, &self.msrv)
312312
&& !check_replace_with_default(cx, src, dest, expr, &self.msrv)
313-
{
314-
check_replace_with_uninit(cx, src, dest, expr.span);
313+
{
314+
check_replace_with_uninit(cx, src, dest, expr.span);
315+
}
315316
}
316317
}
317318
extract_msrv_attr!(LateContext);

Diff for: ‎clippy_lints/src/methods/map_clone.rs

+5-3
Original file line numberDiff line numberDiff line change
@@ -114,17 +114,19 @@ fn handle_path(
114114
) {
115115
if let Some(path_def_id) = cx.qpath_res(qpath, arg.hir_id).opt_def_id()
116116
&& cx.tcx.lang_items().get(LangItem::CloneFn) == Some(path_def_id)
117+
{
117118
// The `copied` and `cloned` methods are only available on `&T` and `&mut T` in `Option`
118119
// and `Result`.
119-
&& let ty::Adt(_, args) = cx.typeck_results().expr_ty(recv).kind()
120+
if let ty::Adt(_, args) = cx.typeck_results().expr_ty(recv).kind()
120121
&& let args = args.as_slice()
121122
&& let Some(ty) = args.iter().find_map(|generic_arg| generic_arg.as_type())
122123
&& let ty::Ref(_, ty, Mutability::Not) = ty.kind()
123124
&& let ty::FnDef(_, lst) = cx.typeck_results().expr_ty(arg).kind()
124125
&& lst.iter().all(|l| l.as_type() == Some(*ty))
125126
&& !should_call_clone_as_function(cx, *ty)
126-
{
127-
lint_path(cx, e.span, recv.span, is_copy(cx, ty.peel_refs()));
127+
{
128+
lint_path(cx, e.span, recv.span, is_copy(cx, ty.peel_refs()));
129+
}
128130
}
129131
}
130132

Diff for: ‎clippy_lints/src/methods/seek_from_current.rs

+5-3
Original file line numberDiff line numberDiff line change
@@ -38,11 +38,13 @@ fn arg_is_seek_from_current<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>)
3838
&& let ExprKind::Path(ref path) = f.kind
3939
&& let Some(ctor_call_id) = cx.qpath_res(path, f.hir_id).opt_def_id()
4040
&& is_enum_variant_ctor(cx, sym::SeekFrom, sym!(Current), ctor_call_id)
41+
{
4142
// check if argument of `SeekFrom::Current` is `0`
42-
&& let ExprKind::Lit(lit) = arg.kind
43+
if let ExprKind::Lit(lit) = arg.kind
4344
&& let LitKind::Int(Pu128(0), LitIntType::Unsuffixed) = lit.node
44-
{
45-
return true;
45+
{
46+
return true;
47+
}
4648
}
4749

4850
false

Diff for: ‎clippy_lints/src/mixed_read_write_in_expression.rs

+15-15
Original file line numberDiff line numberDiff line change
@@ -327,22 +327,22 @@ impl<'tcx> Visitor<'tcx> for ReadVisitor<'_, 'tcx> {
327327
return;
328328
}
329329

330-
if path_to_local_id(expr, self.var)
330+
if path_to_local_id(expr, self.var) {
331331
// Check that this is a read, not a write.
332-
&& !is_in_assignment_position(self.cx, expr)
333-
{
334-
span_lint_and_then(
335-
self.cx,
336-
MIXED_READ_WRITE_IN_EXPRESSION,
337-
expr.span,
338-
format!("unsequenced read of `{}`", self.cx.tcx.hir().name(self.var)),
339-
|diag| {
340-
diag.span_note(
341-
self.write_expr.span,
342-
"whether read occurs before this write depends on evaluation order",
343-
);
344-
},
345-
);
332+
if !is_in_assignment_position(self.cx, expr) {
333+
span_lint_and_then(
334+
self.cx,
335+
MIXED_READ_WRITE_IN_EXPRESSION,
336+
expr.span,
337+
format!("unsequenced read of `{}`", self.cx.tcx.hir().name(self.var)),
338+
|diag| {
339+
diag.span_note(
340+
self.write_expr.span,
341+
"whether read occurs before this write depends on evaluation order",
342+
);
343+
},
344+
);
345+
}
346346
}
347347
match expr.kind {
348348
// We're about to descend a closure. Since we don't know when (or

Diff for: ‎lintcheck/ci-config/clippy.toml

+4
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
# Configuration applied when running lintcheck from the CI
2+
#
3+
# The CI will set the `CLIPPY_CONF_DIR` environment variable
4+
# to `$PWD/lintcheck/ci-config`.

Diff for: ‎tests/ui-toml/collapsible_if/clippy.toml

+1
Original file line numberDiff line numberDiff line change
@@ -1 +1,2 @@
1+
collapse-let-chains = true
12
lint-commented-code = false

Diff for: ‎tests/ui-toml/collapsible_if/collapsible_if.fixed

+16
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
#![feature(let_chains)]
12
#![allow(clippy::eq_op, clippy::nonminimal_bool)]
23

34
#[rustfmt::skip]
@@ -14,4 +15,19 @@ fn main() {
1415
println!("Hello world!");
1516
}
1617
}
18+
19+
if let Some(a) = Some(3) && let Some(b) = Some(4) {
20+
let _ = a + b;
21+
}
22+
//~^^^^^ collapsible_if
23+
24+
if let Some(a) = Some(3) && a + 1 == 4 {
25+
let _ = a;
26+
}
27+
//~^^^^^ collapsible_if
28+
29+
if Some(3) == Some(4).map(|x| x - 1) && let Some(b) = Some(4) {
30+
let _ = b;
31+
}
32+
//~^^^^^ collapsible_if
1733
}

Diff for: ‎tests/ui-toml/collapsible_if/collapsible_if.rs

+22
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
#![feature(let_chains)]
12
#![allow(clippy::eq_op, clippy::nonminimal_bool)]
23

34
#[rustfmt::skip]
@@ -16,4 +17,25 @@ fn main() {
1617
println!("Hello world!");
1718
}
1819
}
20+
21+
if let Some(a) = Some(3) {
22+
if let Some(b) = Some(4) {
23+
let _ = a + b;
24+
}
25+
}
26+
//~^^^^^ collapsible_if
27+
28+
if let Some(a) = Some(3) {
29+
if a + 1 == 4 {
30+
let _ = a;
31+
}
32+
}
33+
//~^^^^^ collapsible_if
34+
35+
if Some(3) == Some(4).map(|x| x - 1) {
36+
if let Some(b) = Some(4) {
37+
let _ = b;
38+
}
39+
}
40+
//~^^^^^ collapsible_if
1941
}

Diff for: ‎tests/ui-toml/collapsible_if/collapsible_if.stderr

+53-2
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: this `if` statement can be collapsed
2-
--> tests/ui-toml/collapsible_if/collapsible_if.rs:6:5
2+
--> tests/ui-toml/collapsible_if/collapsible_if.rs:7:5
33
|
44
LL | / if true {
55
LL | | if true {
@@ -17,5 +17,56 @@ LL + println!("No comment, linted");
1717
LL + }
1818
|
1919

20-
error: aborting due to 1 previous error
20+
error: this `if` statement can be collapsed
21+
--> tests/ui-toml/collapsible_if/collapsible_if.rs:21:5
22+
|
23+
LL | / if let Some(a) = Some(3) {
24+
LL | | if let Some(b) = Some(4) {
25+
LL | | let _ = a + b;
26+
LL | | }
27+
LL | | }
28+
| |_____^
29+
|
30+
help: collapse nested if block
31+
|
32+
LL ~ if let Some(a) = Some(3) && let Some(b) = Some(4) {
33+
LL + let _ = a + b;
34+
LL + }
35+
|
36+
37+
error: this `if` statement can be collapsed
38+
--> tests/ui-toml/collapsible_if/collapsible_if.rs:28:5
39+
|
40+
LL | / if let Some(a) = Some(3) {
41+
LL | | if a + 1 == 4 {
42+
LL | | let _ = a;
43+
LL | | }
44+
LL | | }
45+
| |_____^
46+
|
47+
help: collapse nested if block
48+
|
49+
LL ~ if let Some(a) = Some(3) && a + 1 == 4 {
50+
LL + let _ = a;
51+
LL + }
52+
|
53+
54+
error: this `if` statement can be collapsed
55+
--> tests/ui-toml/collapsible_if/collapsible_if.rs:35:5
56+
|
57+
LL | / if Some(3) == Some(4).map(|x| x - 1) {
58+
LL | | if let Some(b) = Some(4) {
59+
LL | | let _ = b;
60+
LL | | }
61+
LL | | }
62+
| |_____^
63+
|
64+
help: collapse nested if block
65+
|
66+
LL ~ if Some(3) == Some(4).map(|x| x - 1) && let Some(b) = Some(4) {
67+
LL + let _ = b;
68+
LL + }
69+
|
70+
71+
error: aborting due to 4 previous errors
2172

Diff for: ‎tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr

+3
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ error: error reading Clippy's configuration file: unknown field `foobar`, expect
3333
cargo-ignore-publish
3434
check-private-items
3535
cognitive-complexity-threshold
36+
collapse-let-chains
3637
cyclomatic-complexity-threshold
3738
disallowed-macros
3839
disallowed-methods
@@ -125,6 +126,7 @@ error: error reading Clippy's configuration file: unknown field `barfoo`, expect
125126
cargo-ignore-publish
126127
check-private-items
127128
cognitive-complexity-threshold
129+
collapse-let-chains
128130
cyclomatic-complexity-threshold
129131
disallowed-macros
130132
disallowed-methods
@@ -217,6 +219,7 @@ error: error reading Clippy's configuration file: unknown field `allow_mixed_uni
217219
cargo-ignore-publish
218220
check-private-items
219221
cognitive-complexity-threshold
222+
collapse-let-chains
220223
cyclomatic-complexity-threshold
221224
disallowed-macros
222225
disallowed-methods

0 commit comments

Comments
 (0)