diff --git a/clippy_lints/src/collapsible_if.rs b/clippy_lints/src/collapsible_if.rs index 7f6ecea99fb0..df93be77cdc6 100644 --- a/clippy_lints/src/collapsible_if.rs +++ b/clippy_lints/src/collapsible_if.rs @@ -1,13 +1,14 @@ use clippy_config::Conf; use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then}; use clippy_utils::msrvs::{self, Msrv}; -use clippy_utils::source::{IntoSpan as _, SpanRangeExt, snippet, snippet_block, snippet_block_with_applicability}; +use clippy_utils::source::{IntoSpan as _, SpanRangeExt, snippet, snippet_block_with_applicability}; +use clippy_utils::span_contains_non_whitespace; use rustc_ast::BinOpKind; use rustc_errors::Applicability; use rustc_hir::{Block, Expr, ExprKind, Stmt, StmtKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::impl_lint_pass; -use rustc_span::Span; +use rustc_span::{BytePos, Span}; declare_clippy_lint! { /// ### What it does @@ -90,12 +91,12 @@ impl CollapsibleIf { } } - fn check_collapsible_else_if(cx: &LateContext<'_>, then_span: Span, else_block: &Block<'_>) { - if !block_starts_with_comment(cx, else_block) - && let Some(else_) = expr_block(else_block) + fn check_collapsible_else_if(&self, cx: &LateContext<'_>, then_span: Span, else_block: &Block<'_>) { + if let Some(else_) = expr_block(else_block) && cx.tcx.hir_attrs(else_.hir_id).is_empty() && !else_.span.from_expansion() && let ExprKind::If(..) = else_.kind + && !block_starts_with_significant_tokens(cx, else_block, else_, self.lint_commented_code) { // Prevent "elseif" // Check that the "else" is followed by whitespace @@ -130,7 +131,7 @@ impl CollapsibleIf { && self.eligible_condition(cx, check_inner) && let ctxt = expr.span.ctxt() && inner.span.ctxt() == ctxt - && (self.lint_commented_code || !block_starts_with_comment(cx, then)) + && !block_starts_with_significant_tokens(cx, then, inner, self.lint_commented_code) { span_lint_and_then( cx, @@ -141,7 +142,7 @@ impl CollapsibleIf { let then_open_bracket = then.span.split_at(1).0.with_leading_whitespace(cx).into_span(); let then_closing_bracket = { let end = then.span.shrink_to_hi(); - end.with_lo(end.lo() - rustc_span::BytePos(1)) + end.with_lo(end.lo() - BytePos(1)) .with_leading_whitespace(cx) .into_span() }; @@ -179,7 +180,7 @@ impl LateLintPass<'_> for CollapsibleIf { if let Some(else_) = else_ && let ExprKind::Block(else_, None) = else_.kind { - Self::check_collapsible_else_if(cx, then.span, else_); + self.check_collapsible_else_if(cx, then.span, else_); } else if else_.is_none() && self.eligible_condition(cx, cond) && let ExprKind::Block(then, None) = then.kind @@ -190,12 +191,16 @@ impl LateLintPass<'_> for CollapsibleIf { } } -fn block_starts_with_comment(cx: &LateContext<'_>, block: &Block<'_>) -> bool { - // We trim all opening braces and whitespaces and then check if the next string is a comment. - let trimmed_block_text = snippet_block(cx, block.span, "..", None) - .trim_start_matches(|c: char| c.is_whitespace() || c == '{') - .to_owned(); - trimmed_block_text.starts_with("//") || trimmed_block_text.starts_with("/*") +// Check that nothing significant can be found but whitespaces between the initial `{` of `block` +// and the beginning of `stop_at`. +fn block_starts_with_significant_tokens( + cx: &LateContext<'_>, + block: &Block<'_>, + stop_at: &Expr<'_>, + lint_commented_code: bool, +) -> bool { + let span = block.span.split_at(1).1.until(stop_at.span); + span_contains_non_whitespace(cx, span, lint_commented_code) } /// If `block` is a block with either one expression or a statement containing an expression, diff --git a/clippy_lints/src/loops/same_item_push.rs b/clippy_lints/src/loops/same_item_push.rs index 388034c39f52..b5c0635a5aa3 100644 --- a/clippy_lints/src/loops/same_item_push.rs +++ b/clippy_lints/src/loops/same_item_push.rs @@ -163,15 +163,12 @@ impl<'tcx> Visitor<'tcx> for SameItemPushVisitor<'_, 'tcx> { StmtKind::Expr(expr) | StmtKind::Semi(expr) => self.visit_expr(expr), _ => {}, } + } else if self.vec_push.is_none() { + // Current statement is a push ...check whether another push had been previously done + self.vec_push = vec_push_option; } else { - // Current statement is a push ...check whether another - // push had been previously done - if self.vec_push.is_none() { - self.vec_push = vec_push_option; - } else { - // There are multiple pushes ... don't lint - self.multiple_pushes = true; - } + // There are multiple pushes ... don't lint + self.multiple_pushes = true; } } } diff --git a/clippy_lints/src/single_component_path_imports.rs b/clippy_lints/src/single_component_path_imports.rs index 62939912304b..2aa585f9ea89 100644 --- a/clippy_lints/src/single_component_path_imports.rs +++ b/clippy_lints/src/single_component_path_imports.rs @@ -219,22 +219,21 @@ impl SingleComponentPathImports { } } } - } else { + } else if segments[0].ident.name == kw::SelfLower { // keep track of `use self::some_module` usages - if segments[0].ident.name == kw::SelfLower { - // simple case such as `use self::module::SomeStruct` - if segments.len() > 1 { - imports_reused_with_self.push(segments[1].ident.name); - return; - } - // nested case such as `use self::{module1::Struct1, module2::Struct2}` - if let UseTreeKind::Nested { items, .. } = &use_tree.kind { - for tree in items { - let segments = &tree.0.prefix.segments; - if !segments.is_empty() { - imports_reused_with_self.push(segments[0].ident.name); - } + // simple case such as `use self::module::SomeStruct` + if segments.len() > 1 { + imports_reused_with_self.push(segments[1].ident.name); + return; + } + + // nested case such as `use self::{module1::Struct1, module2::Struct2}` + if let UseTreeKind::Nested { items, .. } = &use_tree.kind { + for tree in items { + let segments = &tree.0.prefix.segments; + if !segments.is_empty() { + imports_reused_with_self.push(segments[0].ident.name); } } } diff --git a/clippy_lints/src/undocumented_unsafe_blocks.rs b/clippy_lints/src/undocumented_unsafe_blocks.rs index a2938c86c76a..4ebdd320c260 100644 --- a/clippy_lints/src/undocumented_unsafe_blocks.rs +++ b/clippy_lints/src/undocumented_unsafe_blocks.rs @@ -606,32 +606,30 @@ fn span_from_macro_expansion_has_safety_comment(cx: &LateContext<'_>, span: Span let ctxt = span.ctxt(); if ctxt == SyntaxContext::root() { HasSafetyComment::Maybe - } else { - // From a macro expansion. Get the text from the start of the macro declaration to start of the - // unsafe block. + } else if let Ok(unsafe_line) = source_map.lookup_line(span.lo()) + // From a macro expansion. Get the text from the start of the macro declaration to start of + // the unsafe block. // macro_rules! foo { () => { stuff }; (x) => { unsafe { stuff } }; } // ^--------------------------------------------^ - if let Ok(unsafe_line) = source_map.lookup_line(span.lo()) - && let Ok(macro_line) = source_map.lookup_line(ctxt.outer_expn_data().def_site.lo()) - && Arc::ptr_eq(&unsafe_line.sf, ¯o_line.sf) - && let Some(src) = unsafe_line.sf.src.as_deref() - { - if macro_line.line < unsafe_line.line { - match text_has_safety_comment( - src, - &unsafe_line.sf.lines()[macro_line.line + 1..=unsafe_line.line], - unsafe_line.sf.start_pos, - ) { - Some(b) => HasSafetyComment::Yes(b), - None => HasSafetyComment::No, - } - } else { - HasSafetyComment::No + && let Ok(macro_line) = source_map.lookup_line(ctxt.outer_expn_data().def_site.lo()) + && Arc::ptr_eq(&unsafe_line.sf, ¯o_line.sf) + && let Some(src) = unsafe_line.sf.src.as_deref() + { + if macro_line.line < unsafe_line.line { + match text_has_safety_comment( + src, + &unsafe_line.sf.lines()[macro_line.line + 1..=unsafe_line.line], + unsafe_line.sf.start_pos, + ) { + Some(b) => HasSafetyComment::Yes(b), + None => HasSafetyComment::No, } } else { - // Problem getting source text. Pretend a comment was found. - HasSafetyComment::Maybe + HasSafetyComment::No } + } else { + // Problem getting source text. Pretend a comment was found. + HasSafetyComment::Maybe } } diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 42136fdbc51a..19a2816b75ab 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -122,7 +122,7 @@ use rustc_span::hygiene::{ExpnKind, MacroKind}; use rustc_span::source_map::SourceMap; use rustc_span::symbol::{Ident, Symbol, kw}; use rustc_span::{InnerSpan, Span}; -use source::walk_span_to_context; +use source::{SpanRangeExt, walk_span_to_context}; use visitors::{Visitable, for_each_unconsumed_temporary}; use crate::consts::{ConstEvalCtxt, Constant, mir_to_const}; @@ -2788,6 +2788,19 @@ pub fn span_contains_comment(sm: &SourceMap, span: Span) -> bool { }); } +/// Checks whether a given span has any significant token. A significant token is a non-whitespace +/// token, including comments unless `skip_comments` is set. +/// This is useful to determine if there are any actual code tokens in the span that are omitted in +/// the late pass, such as platform-specific code. +pub fn span_contains_non_whitespace(cx: &impl source::HasSession, span: Span, skip_comments: bool) -> bool { + matches!(span.get_source_text(cx), Some(snippet) if tokenize_with_text(&snippet).any(|(token, _, _)| + match token { + TokenKind::Whitespace => false, + TokenKind::BlockComment { .. } | TokenKind::LineComment { .. } => !skip_comments, + _ => true, + } + )) +} /// Returns all the comments a given span contains /// /// Comments are returned wrapped with their relevant delimiters diff --git a/tests/ui-toml/collapsible_if/collapsible_else_if.fixed b/tests/ui-toml/collapsible_if/collapsible_else_if.fixed new file mode 100644 index 000000000000..802dfa813488 --- /dev/null +++ b/tests/ui-toml/collapsible_if/collapsible_else_if.fixed @@ -0,0 +1,35 @@ +#![allow(clippy::eq_op, clippy::nonminimal_bool)] + +#[rustfmt::skip] +#[warn(clippy::collapsible_if)] +fn main() { + let (x, y) = ("hello", "world"); + + if x == "hello" { + todo!() + } else if y == "world" { + println!("Hello world!"); + } + //~^^^^^^ collapsible_else_if + + if x == "hello" { + todo!() + } else if y == "world" { + println!("Hello world!"); + } + //~^^^^^ collapsible_else_if + + if x == "hello" { + todo!() + } else if y == "world" { + println!("Hello world!"); + } + //~^^^^^^ collapsible_else_if + + if x == "hello" { + todo!() + } else if y == "world" { + println!("Hello world!"); + } + //~^^^^^ collapsible_else_if +} diff --git a/tests/ui-toml/collapsible_if/collapsible_else_if.rs b/tests/ui-toml/collapsible_if/collapsible_else_if.rs new file mode 100644 index 000000000000..7d01395ccdee --- /dev/null +++ b/tests/ui-toml/collapsible_if/collapsible_else_if.rs @@ -0,0 +1,45 @@ +#![allow(clippy::eq_op, clippy::nonminimal_bool)] + +#[rustfmt::skip] +#[warn(clippy::collapsible_if)] +fn main() { + let (x, y) = ("hello", "world"); + + if x == "hello" { + todo!() + } else { + // Comment must be kept + if y == "world" { + println!("Hello world!"); + } + } + //~^^^^^^ collapsible_else_if + + if x == "hello" { + todo!() + } else { // Inner comment + if y == "world" { + println!("Hello world!"); + } + } + //~^^^^^ collapsible_else_if + + if x == "hello" { + todo!() + } else { + /* Inner comment */ + if y == "world" { + println!("Hello world!"); + } + } + //~^^^^^^ collapsible_else_if + + if x == "hello" { + todo!() + } else { /* Inner comment */ + if y == "world" { + println!("Hello world!"); + } + } + //~^^^^^ collapsible_else_if +} diff --git a/tests/ui-toml/collapsible_if/collapsible_else_if.stderr b/tests/ui-toml/collapsible_if/collapsible_else_if.stderr new file mode 100644 index 000000000000..834a141d6d8c --- /dev/null +++ b/tests/ui-toml/collapsible_if/collapsible_else_if.stderr @@ -0,0 +1,78 @@ +error: this `else { if .. }` block can be collapsed + --> tests/ui-toml/collapsible_if/collapsible_else_if.rs:10:12 + | +LL | } else { + | ____________^ +LL | | // Comment must be kept +LL | | if y == "world" { +LL | | println!("Hello world!"); +LL | | } +LL | | } + | |_____^ + | + = note: `-D clippy::collapsible-else-if` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::collapsible_else_if)]` +help: collapse nested if block + | +LL ~ } else if y == "world" { +LL + println!("Hello world!"); +LL + } + | + +error: this `else { if .. }` block can be collapsed + --> tests/ui-toml/collapsible_if/collapsible_else_if.rs:20:12 + | +LL | } else { // Inner comment + | ____________^ +LL | | if y == "world" { +LL | | println!("Hello world!"); +LL | | } +LL | | } + | |_____^ + | +help: collapse nested if block + | +LL ~ } else if y == "world" { +LL + println!("Hello world!"); +LL + } + | + +error: this `else { if .. }` block can be collapsed + --> tests/ui-toml/collapsible_if/collapsible_else_if.rs:29:12 + | +LL | } else { + | ____________^ +LL | | /* Inner comment */ +LL | | if y == "world" { +LL | | println!("Hello world!"); +LL | | } +LL | | } + | |_____^ + | +help: collapse nested if block + | +LL ~ } else if y == "world" { +LL + println!("Hello world!"); +LL + } + | + +error: this `else { if .. }` block can be collapsed + --> tests/ui-toml/collapsible_if/collapsible_else_if.rs:39:12 + | +LL | } else { /* Inner comment */ + | ____________^ +LL | | if y == "world" { +LL | | println!("Hello world!"); +LL | | } +LL | | } + | |_____^ + | +help: collapse nested if block + | +LL ~ } else if y == "world" { +LL + println!("Hello world!"); +LL + } + | + +error: aborting due to 4 previous errors + diff --git a/tests/ui/collapsible_else_if.fixed b/tests/ui/collapsible_else_if.fixed index 9f530ad670a0..fed75244c6f7 100644 --- a/tests/ui/collapsible_else_if.fixed +++ b/tests/ui/collapsible_else_if.fixed @@ -86,3 +86,21 @@ fn issue_7318() { }else if false {} //~^^^ collapsible_else_if } + +fn issue14799() { + use std::ops::ControlFlow; + + let c: ControlFlow<_, ()> = ControlFlow::Break(Some(42)); + if let ControlFlow::Break(Some(_)) = c { + todo!(); + } else { + #[cfg(target_os = "freebsd")] + todo!(); + + if let ControlFlow::Break(None) = c { + todo!(); + } else { + todo!(); + } + } +} diff --git a/tests/ui/collapsible_else_if.rs b/tests/ui/collapsible_else_if.rs index 2c646cd1d4da..e50e781fb698 100644 --- a/tests/ui/collapsible_else_if.rs +++ b/tests/ui/collapsible_else_if.rs @@ -102,3 +102,21 @@ fn issue_7318() { } //~^^^ collapsible_else_if } + +fn issue14799() { + use std::ops::ControlFlow; + + let c: ControlFlow<_, ()> = ControlFlow::Break(Some(42)); + if let ControlFlow::Break(Some(_)) = c { + todo!(); + } else { + #[cfg(target_os = "freebsd")] + todo!(); + + if let ControlFlow::Break(None) = c { + todo!(); + } else { + todo!(); + } + } +} diff --git a/tests/ui/collapsible_if.fixed b/tests/ui/collapsible_if.fixed index b553182a4454..77bc791ea8e9 100644 --- a/tests/ui/collapsible_if.fixed +++ b/tests/ui/collapsible_if.fixed @@ -154,3 +154,12 @@ fn issue14722() { None }; } + +fn issue14799() { + if true { + #[cfg(target_os = "freebsd")] + todo!(); + + if true {} + }; +} diff --git a/tests/ui/collapsible_if.rs b/tests/ui/collapsible_if.rs index f5998457ca6c..d30df157d5eb 100644 --- a/tests/ui/collapsible_if.rs +++ b/tests/ui/collapsible_if.rs @@ -164,3 +164,12 @@ fn issue14722() { None }; } + +fn issue14799() { + if true { + #[cfg(target_os = "freebsd")] + todo!(); + + if true {} + }; +}