Skip to content

Commit c2783b6

Browse files
profetiaprofetia
committed
Lint more cases in collapsible_else_if 
1 parent 5290b1e commit c2783b6

File tree

7 files changed

+262
-41
lines changed

7 files changed

+262
-41
lines changed

‎clippy_lints/src/collapsible_if.rs‎

Lines changed: 64 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
use clippy_config::Conf;
2-
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
2+
use clippy_utils::diagnostics::span_lint_and_then;
33
use clippy_utils::msrvs::{self, Msrv};
4-
use clippy_utils::source::{IntoSpan as _, SpanRangeExt, snippet, snippet_block, snippet_block_with_applicability};
5-
use clippy_utils::span_contains_non_comment;
4+
use clippy_utils::source::{IntoSpan as _, SpanRangeExt, snippet, snippet_block_with_applicability};
5+
use clippy_utils::span_contains_non_whitespace;
66
use rustc_ast::BinOpKind;
77
use rustc_errors::Applicability;
88
use rustc_hir::{Block, Expr, ExprKind, Stmt, StmtKind};
@@ -91,37 +91,63 @@ impl CollapsibleIf {
9191
}
9292
}
9393

94-
fn check_collapsible_else_if(cx: &LateContext<'_>, then_span: Span, else_block: &Block<'_>) {
95-
if !block_starts_with_comment(cx, else_block)
96-
&& let Some(else_) = expr_block(else_block)
94+
fn check_collapsible_else_if(&self, cx: &LateContext<'_>, then_span: Span, else_block: &Block<'_>) {
95+
if let Some(else_) = expr_block(else_block)
9796
&& cx.tcx.hir_attrs(else_.hir_id).is_empty()
9897
&& !else_.span.from_expansion()
9998
&& let ExprKind::If(..) = else_.kind
100-
&& let up_to_if = else_block.span.until(else_.span)
101-
&& !span_contains_non_comment(cx, up_to_if.with_lo(BytePos(up_to_if.lo().0 + 1)))
99+
&& !block_starts_with_significant_tokens(cx, else_block, else_, self.lint_commented_code)
102100
{
103-
// Prevent "elseif"
104-
// Check that the "else" is followed by whitespace
105-
let up_to_else = then_span.between(else_block.span);
106-
let requires_space = if let Some(c) = snippet(cx, up_to_else, "..").chars().last() {
107-
!c.is_whitespace()
108-
} else {
109-
false
110-
};
111-
112-
let mut applicability = Applicability::MachineApplicable;
113-
span_lint_and_sugg(
101+
span_lint_and_then(
114102
cx,
115103
COLLAPSIBLE_ELSE_IF,
116104
else_block.span,
117105
"this `else { if .. }` block can be collapsed",
118-
"collapse nested if block",
119-
format!(
120-
"{}{}",
121-
if requires_space { " " } else { "" },
122-
snippet_block_with_applicability(cx, else_.span, "..", Some(else_block.span), &mut applicability)
123-
),
124-
applicability,
106+
|diag| {
107+
if self.lint_commented_code {
108+
let else_open_bracket = else_block.span.split_at(1).0.with_leading_whitespace(cx).into_span();
109+
let else_closing_bracket = {
110+
let end = else_block.span.shrink_to_hi();
111+
end.with_lo(end.lo() - BytePos(1))
112+
.with_leading_whitespace(cx)
113+
.into_span()
114+
};
115+
let sugg = vec![
116+
// Remove the outer else block `{`
117+
(else_open_bracket, String::new()),
118+
// Remove the outer else block '}'
119+
(else_closing_bracket, String::new()),
120+
];
121+
diag.multipart_suggestion("collapse nested if block", sugg, Applicability::MachineApplicable);
122+
return;
123+
}
124+
125+
// Prevent "elseif"
126+
// Check that the "else" is followed by whitespace
127+
let up_to_else = then_span.between(else_block.span);
128+
let requires_space = if let Some(c) = snippet(cx, up_to_else, "..").chars().last() {
129+
!c.is_whitespace()
130+
} else {
131+
false
132+
};
133+
let mut applicability = Applicability::MachineApplicable;
134+
diag.span_suggestion(
135+
else_block.span,
136+
"collapse nested if block",
137+
format!(
138+
"{}{}",
139+
if requires_space { " " } else { "" },
140+
snippet_block_with_applicability(
141+
cx,
142+
else_.span,
143+
"..",
144+
Some(else_block.span),
145+
&mut applicability
146+
)
147+
),
148+
applicability,
149+
);
150+
},
125151
);
126152
}
127153
}
@@ -133,7 +159,7 @@ impl CollapsibleIf {
133159
&& self.eligible_condition(cx, check_inner)
134160
&& let ctxt = expr.span.ctxt()
135161
&& inner.span.ctxt() == ctxt
136-
&& (self.lint_commented_code || !block_starts_with_comment(cx, then))
162+
&& !block_starts_with_significant_tokens(cx, then, inner, self.lint_commented_code)
137163
{
138164
span_lint_and_then(
139165
cx,
@@ -182,7 +208,7 @@ impl LateLintPass<'_> for CollapsibleIf {
182208
if let Some(else_) = else_
183209
&& let ExprKind::Block(else_, None) = else_.kind
184210
{
185-
Self::check_collapsible_else_if(cx, then.span, else_);
211+
self.check_collapsible_else_if(cx, then.span, else_);
186212
} else if else_.is_none()
187213
&& self.eligible_condition(cx, cond)
188214
&& let ExprKind::Block(then, None) = then.kind
@@ -193,12 +219,16 @@ impl LateLintPass<'_> for CollapsibleIf {
193219
}
194220
}
195221

196-
fn block_starts_with_comment(cx: &LateContext<'_>, block: &Block<'_>) -> bool {
197-
// We trim all opening braces and whitespaces and then check if the next string is a comment.
198-
let trimmed_block_text = snippet_block(cx, block.span, "..", None)
199-
.trim_start_matches(|c: char| c.is_whitespace() || c == '{')
200-
.to_owned();
201-
trimmed_block_text.starts_with("//") || trimmed_block_text.starts_with("/*")
222+
// Check that nothing significant can be found but whitespaces between the initial `{` of `block`
223+
// and the beginning of `stop_at`.
224+
fn block_starts_with_significant_tokens(
225+
cx: &LateContext<'_>,
226+
block: &Block<'_>,
227+
stop_at: &Expr<'_>,
228+
lint_commented_code: bool,
229+
) -> bool {
230+
let span = block.span.split_at(1).1.until(stop_at.span);
231+
span_contains_non_whitespace(cx, span, lint_commented_code)
202232
}
203233

204234
/// If `block` is a block with either one expression or a statement containing an expression,

‎clippy_utils/src/lib.rs‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2788,16 +2788,19 @@ pub fn span_contains_comment(sm: &SourceMap, span: Span) -> bool {
27882788
});
27892789
}
27902790

2791-
/// Checks whether a given span has any non-comment token. This checks for all types of token other
2792-
/// than line comment "//", block comment "/**", doc "///" "//!" and whitespace
2791+
/// Checks whether a given span has any significant token. A significant token is a non-whitespace
2792+
/// token, including comments unless `skip_comments` is set.
27932793
/// This is useful to determine if there are any actual code tokens in the span that are omitted in
27942794
/// the late pass, such as platform-specific code.
2795-
pub fn span_contains_non_comment(cx: &impl source::HasSession, span: Span) -> bool {
2796-
matches!(span.get_source_text(cx), Some(snippet) if tokenize_with_text(&snippet).any(|(token, _, _)| {
2797-
!matches!(token, TokenKind::LineComment { .. } | TokenKind::BlockComment { .. } | TokenKind::Whitespace)
2798-
}))
2795+
pub fn span_contains_non_whitespace(cx: &impl source::HasSession, span: Span, skip_comments: bool) -> bool {
2796+
matches!(span.get_source_text(cx), Some(snippet) if tokenize_with_text(&snippet).any(|(token, _, _)|
2797+
match token {
2798+
TokenKind::Whitespace => false,
2799+
TokenKind::BlockComment { .. } | TokenKind::LineComment { .. } => !skip_comments,
2800+
_ => true,
2801+
}
2802+
))
27992803
}
2800-
28012804
/// Returns all the comments a given span contains
28022805
///
28032806
/// Comments are returned wrapped with their relevant delimiters

‎tests/ui-toml/collapsible_if/collapsible_else_if.fixed‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
#![allow(clippy::eq_op, clippy::nonminimal_bool)]
2+
3+
#[rustfmt::skip]
4+
#[warn(clippy::collapsible_if)]
5+
fn main() {
6+
let (x, y) = ("hello", "world");
7+
8+
if x == "hello" {
9+
todo!()
10+
} else
11+
// Comment must be kept
12+
if y == "world" {
13+
println!("Hello world!");
14+
}
15+
//~^^^^^^ collapsible_else_if
16+
17+
if x == "hello" {
18+
todo!()
19+
} else // Inner comment
20+
if y == "world" {
21+
println!("Hello world!");
22+
}
23+
//~^^^^^ collapsible_else_if
24+
25+
if x == "hello" {
26+
todo!()
27+
} else
28+
/* Inner comment */
29+
if y == "world" {
30+
println!("Hello world!");
31+
}
32+
//~^^^^^^ collapsible_else_if
33+
34+
if x == "hello" {
35+
todo!()
36+
} else /* Inner comment */
37+
if y == "world" {
38+
println!("Hello world!");
39+
}
40+
//~^^^^^ collapsible_else_if
41+
}

‎tests/ui-toml/collapsible_if/collapsible_else_if.rs‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
#![allow(clippy::eq_op, clippy::nonminimal_bool)]
2+
3+
#[rustfmt::skip]
4+
#[warn(clippy::collapsible_if)]
5+
fn main() {
6+
let (x, y) = ("hello", "world");
7+
8+
if x == "hello" {
9+
todo!()
10+
} else {
11+
// Comment must be kept
12+
if y == "world" {
13+
println!("Hello world!");
14+
}
15+
}
16+
//~^^^^^^ collapsible_else_if
17+
18+
if x == "hello" {
19+
todo!()
20+
} else { // Inner comment
21+
if y == "world" {
22+
println!("Hello world!");
23+
}
24+
}
25+
//~^^^^^ collapsible_else_if
26+
27+
if x == "hello" {
28+
todo!()
29+
} else {
30+
/* Inner comment */
31+
if y == "world" {
32+
println!("Hello world!");
33+
}
34+
}
35+
//~^^^^^^ collapsible_else_if
36+
37+
if x == "hello" {
38+
todo!()
39+
} else { /* Inner comment */
40+
if y == "world" {
41+
println!("Hello world!");
42+
}
43+
}
44+
//~^^^^^ collapsible_else_if
45+
}

‎tests/ui-toml/collapsible_if/collapsible_else_if.stderr‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
error: this `else { if .. }` block can be collapsed
2+
--> tests/ui-toml/collapsible_if/collapsible_else_if.rs:10:12
3+
|
4+
LL | } else {
5+
| ____________^
6+
LL | | // Comment must be kept
7+
LL | | if y == "world" {
8+
LL | | println!("Hello world!");
9+
LL | | }
10+
LL | | }
11+
| |_____^
12+
|
13+
= note: `-D clippy::collapsible-else-if` implied by `-D warnings`
14+
= help: to override `-D warnings` add `#[allow(clippy::collapsible_else_if)]`
15+
help: collapse nested if block
16+
|
17+
LL ~ } else
18+
LL | // Comment must be kept
19+
LL | if y == "world" {
20+
LL | println!("Hello world!");
21+
LL ~ }
22+
|
23+
24+
error: this `else { if .. }` block can be collapsed
25+
--> tests/ui-toml/collapsible_if/collapsible_else_if.rs:20:12
26+
|
27+
LL | } else { // Inner comment
28+
| ____________^
29+
LL | | if y == "world" {
30+
LL | | println!("Hello world!");
31+
LL | | }
32+
LL | | }
33+
| |_____^
34+
|
35+
help: collapse nested if block
36+
|
37+
LL ~ } else // Inner comment
38+
LL | if y == "world" {
39+
LL | println!("Hello world!");
40+
LL ~ }
41+
|
42+
43+
error: this `else { if .. }` block can be collapsed
44+
--> tests/ui-toml/collapsible_if/collapsible_else_if.rs:29:12
45+
|
46+
LL | } else {
47+
| ____________^
48+
LL | | /* Inner comment */
49+
LL | | if y == "world" {
50+
LL | | println!("Hello world!");
51+
LL | | }
52+
LL | | }
53+
| |_____^
54+
|
55+
help: collapse nested if block
56+
|
57+
LL ~ } else
58+
LL | /* Inner comment */
59+
LL | if y == "world" {
60+
LL | println!("Hello world!");
61+
LL ~ }
62+
|
63+
64+
error: this `else { if .. }` block can be collapsed
65+
--> tests/ui-toml/collapsible_if/collapsible_else_if.rs:39:12
66+
|
67+
LL | } else { /* Inner comment */
68+
| ____________^
69+
LL | | if y == "world" {
70+
LL | | println!("Hello world!");
71+
LL | | }
72+
LL | | }
73+
| |_____^
74+
|
75+
help: collapse nested if block
76+
|
77+
LL ~ } else /* Inner comment */
78+
LL | if y == "world" {
79+
LL | println!("Hello world!");
80+
LL ~ }
81+
|
82+
83+
error: aborting due to 4 previous errors
84+

‎tests/ui/collapsible_if.fixed‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,3 +154,12 @@ fn issue14722() {
154154
None
155155
};
156156
}
157+
158+
fn issue14799() {
159+
if true {
160+
#[cfg(target_os = "freebsd")]
161+
todo!();
162+
163+
if true {}
164+
};
165+
}

‎tests/ui/collapsible_if.rs‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,3 +164,12 @@ fn issue14722() {
164164
None
165165
};
166166
}
167+
168+
fn issue14799() {
169+
if true {
170+
#[cfg(target_os = "freebsd")]
171+
todo!();
172+
173+
if true {}
174+
};
175+
}

0 commit comments

Comments
 (0)