Auto merge of #4490 - mikerite:fix-4364, r=flip1995

Fix `too_many_lines` false positive changelog: Fix `too_many_lines` false positive
rust-lang · Sep 4, 2019 · a2c4b2b · a2c4b2b
2 parents 8239b76 + 232dd43
commit a2c4b2b
Show file tree

Hide file tree

Showing 7 changed files with 167 additions and 142 deletions.
diff --git a/clippy_lints/src/functions.rs b/clippy_lints/src/functions.rs
@@ -200,7 +200,7 @@ impl<'a, 'tcx> Functions {
             Some(i) => i + 1,
             None => 0,
         };
-        let end_brace_idx = match code_snippet.find('}') {
+        let end_brace_idx = match code_snippet.rfind('}') {
             Some(i) => i,
             None => code_snippet.len(),
         };

diff --git a/clippy_lints/src/ranges.rs b/clippy_lints/src/ranges.rs
@@ -151,75 +151,82 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Ranges {
             }
         }
 
-        // exclusive range plus one: `x..(y+1)`
-        if_chain! {
-            if let Some(higher::Range {
-                start,
-                end: Some(end),
-                limits: RangeLimits::HalfOpen
-            }) = higher::range(cx, expr);
-            if let Some(y) = y_plus_one(end);
-            then {
-                let span = if expr.span.from_expansion() {
-                    expr.span
-                        .ctxt()
-                        .outer_expn_data()
-                        .call_site
-                } else {
-                    expr.span
-                };
-                span_lint_and_then(
-                    cx,
-                    RANGE_PLUS_ONE,
-                    span,
-                    "an inclusive range would be more readable",
-                    |db| {
-                        let start = start.map_or(String::new(), |x| Sugg::hir(cx, x, "x").to_string());
-                        let end = Sugg::hir(cx, y, "y");
-                        if let Some(is_wrapped) = &snippet_opt(cx, span) {
-                            if is_wrapped.starts_with('(') && is_wrapped.ends_with(')') {
-                                db.span_suggestion(
-                                    span,
-                                    "use",
-                                    format!("({}..={})", start, end),
-                                    Applicability::MaybeIncorrect,
-                                );
-                            } else {
-                                db.span_suggestion(
-                                    span,
-                                    "use",
-                                    format!("{}..={}", start, end),
-                                    Applicability::MachineApplicable, // snippet
-                                );
-                            }
+        check_exclusive_range_plus_one(cx, expr);
+        check_inclusive_range_minus_one(cx, expr);
+    }
+}
+
+// exclusive range plus one: `x..(y+1)`
+fn check_exclusive_range_plus_one(cx: &LateContext<'_, '_>, expr: &Expr) {
+    if_chain! {
+        if let Some(higher::Range {
+            start,
+            end: Some(end),
+            limits: RangeLimits::HalfOpen
+        }) = higher::range(cx, expr);
+        if let Some(y) = y_plus_one(end);
+        then {
+            let span = if expr.span.from_expansion() {
+                expr.span
+                    .ctxt()
+                    .outer_expn_data()
+                    .call_site
+            } else {
+                expr.span
+            };
+            span_lint_and_then(
+                cx,
+                RANGE_PLUS_ONE,
+                span,
+                "an inclusive range would be more readable",
+                |db| {
+                    let start = start.map_or(String::new(), |x| Sugg::hir(cx, x, "x").to_string());
+                    let end = Sugg::hir(cx, y, "y");
+                    if let Some(is_wrapped) = &snippet_opt(cx, span) {
+                        if is_wrapped.starts_with('(') && is_wrapped.ends_with(')') {
+                            db.span_suggestion(
+                                span,
+                                "use",
+                                format!("({}..={})", start, end),
+                                Applicability::MaybeIncorrect,
+                            );
+                        } else {
+                            db.span_suggestion(
+                                span,
+                                "use",
+                                format!("{}..={}", start, end),
+                                Applicability::MachineApplicable, // snippet
+                            );
                         }
-                    },
-                );
-            }
+                    }
+                },
+            );
         }
+    }
+}
 
-        // inclusive range minus one: `x..=(y-1)`
-        if_chain! {
-            if let Some(higher::Range { start, end: Some(end), limits: RangeLimits::Closed }) = higher::range(cx, expr);
-            if let Some(y) = y_minus_one(end);
-            then {
-                span_lint_and_then(
-                    cx,
-                    RANGE_MINUS_ONE,
-                    expr.span,
-                    "an exclusive range would be more readable",
-                    |db| {
-                        let start = start.map_or(String::new(), |x| Sugg::hir(cx, x, "x").to_string());
-                        let end = Sugg::hir(cx, y, "y");
-                        db.span_suggestion(
-                            expr.span,
-                            "use",
-                            format!("{}..{}", start, end),
-                            Applicability::MachineApplicable, // snippet
-                        );
-                    },
-                );
-            }
+// inclusive range minus one: `x..=(y-1)`
+fn check_inclusive_range_minus_one(cx: &LateContext<'_, '_>, expr: &Expr) {
+    if_chain! {
+        if let Some(higher::Range { start, end: Some(end), limits: RangeLimits::Closed }) = higher::range(cx, expr);
+        if let Some(y) = y_minus_one(end);
+        then {
+            span_lint_and_then(
+                cx,
+                RANGE_MINUS_ONE,
+                expr.span,
+                "an exclusive range would be more readable",
+                |db| {
+                    let start = start.map_or(String::new(), |x| Sugg::hir(cx, x, "x").to_string());
+                    let end = Sugg::hir(cx, y, "y");
+                    db.span_suggestion(
+                        expr.span,
+                        "use",
+                        format!("{}..{}", start, end),
+                        Applicability::MachineApplicable, // snippet
+                    );
+                },
+            );
         }
     }
 }

diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs
@@ -1159,83 +1159,97 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Casts {
                 }
             }
             if cast_from.is_numeric() && cast_to.is_numeric() && !in_external_macro(cx.sess(), expr.span) {
-                match (cast_from.is_integral(), cast_to.is_integral()) {
-                    (true, false) => {
-                        let from_nbits = int_ty_to_nbits(cast_from, cx.tcx);
-                        let to_nbits = if let ty::Float(FloatTy::F32) = cast_to.sty {
-                            32
-                        } else {
-                            64
-                        };
-                        if is_isize_or_usize(cast_from) || from_nbits >= to_nbits {
-                            span_precision_loss_lint(cx, expr, cast_from, to_nbits == 64);
-                        }
-                        if from_nbits < to_nbits {
-                            span_lossless_lint(cx, expr, ex, cast_from, cast_to);
-                        }
-                    },
-                    (false, true) => {
-                        span_lint(
-                            cx,
-                            CAST_POSSIBLE_TRUNCATION,
-                            expr.span,
-                            &format!("casting {} to {} may truncate the value", cast_from, cast_to),
-                        );
-                        if !cast_to.is_signed() {
-                            span_lint(
-                                cx,
-                                CAST_SIGN_LOSS,
-                                expr.span,
-                                &format!("casting {} to {} may lose the sign of the value", cast_from, cast_to),
-                            );
-                        }
-                    },
-                    (true, true) => {
-                        check_loss_of_sign(cx, expr, ex, cast_from, cast_to);
-                        check_truncation_and_wrapping(cx, expr, cast_from, cast_to);
-                        check_lossless(cx, expr, ex, cast_from, cast_to);
-                    },
-                    (false, false) => {
-                        if let (&ty::Float(FloatTy::F64), &ty::Float(FloatTy::F32)) = (&cast_from.sty, &cast_to.sty) {
-                            span_lint(
-                                cx,
-                                CAST_POSSIBLE_TRUNCATION,
-                                expr.span,
-                                "casting f64 to f32 may truncate the value",
-                            );
-                        }
-                        if let (&ty::Float(FloatTy::F32), &ty::Float(FloatTy::F64)) = (&cast_from.sty, &cast_to.sty) {
-                            span_lossless_lint(cx, expr, ex, cast_from, cast_to);
-                        }
-                    },
-                }
+                lint_numeric_casts(cx, expr, ex, cast_from, cast_to);
             }
 
-            if_chain! {
-                if let ty::RawPtr(from_ptr_ty) = &cast_from.sty;
-                if let ty::RawPtr(to_ptr_ty) = &cast_to.sty;
-                if let Ok(from_layout) = cx.layout_of(from_ptr_ty.ty);
-                if let Ok(to_layout) = cx.layout_of(to_ptr_ty.ty);
-                if from_layout.align.abi < to_layout.align.abi;
-                // with c_void, we inherently need to trust the user
-                if !is_c_void(cx, from_ptr_ty.ty);
-                // when casting from a ZST, we don't know enough to properly lint
-                if !from_layout.is_zst();
-                then {
-                    span_lint(
-                        cx,
-                        CAST_PTR_ALIGNMENT,
-                        expr.span,
-                        &format!(
-                            "casting from `{}` to a more-strictly-aligned pointer (`{}`) ({} < {} bytes)",
-                            cast_from,
-                            cast_to,
-                            from_layout.align.abi.bytes(),
-                            to_layout.align.abi.bytes(),
-                        ),
-                    );
-                }
+            lint_cast_ptr_alignment(cx, expr, cast_from, cast_to);
+        }
+    }
+}
+
+fn lint_numeric_casts<'tcx>(
+    cx: &LateContext<'_, 'tcx>,
+    expr: &Expr,
+    cast_expr: &Expr,
+    cast_from: Ty<'tcx>,
+    cast_to: Ty<'tcx>,
+) {
+    match (cast_from.is_integral(), cast_to.is_integral()) {
+        (true, false) => {
+            let from_nbits = int_ty_to_nbits(cast_from, cx.tcx);
+            let to_nbits = if let ty::Float(FloatTy::F32) = cast_to.sty {
+                32
+            } else {
+                64
+            };
+            if is_isize_or_usize(cast_from) || from_nbits >= to_nbits {
+                span_precision_loss_lint(cx, expr, cast_from, to_nbits == 64);
             }
+            if from_nbits < to_nbits {
+                span_lossless_lint(cx, expr, cast_expr, cast_from, cast_to);
+            }
+        },
+        (false, true) => {
+            span_lint(
+                cx,
+                CAST_POSSIBLE_TRUNCATION,
+                expr.span,
+                &format!("casting {} to {} may truncate the value", cast_from, cast_to),
+            );
+            if !cast_to.is_signed() {
+                span_lint(
+                    cx,
+                    CAST_SIGN_LOSS,
+                    expr.span,
+                    &format!("casting {} to {} may lose the sign of the value", cast_from, cast_to),
+                );
+            }
+        },
+        (true, true) => {
+            check_loss_of_sign(cx, expr, cast_expr, cast_from, cast_to);
+            check_truncation_and_wrapping(cx, expr, cast_from, cast_to);
+            check_lossless(cx, expr, cast_expr, cast_from, cast_to);
+        },
+        (false, false) => {
+            if let (&ty::Float(FloatTy::F64), &ty::Float(FloatTy::F32)) = (&cast_from.sty, &cast_to.sty) {
+                span_lint(
+                    cx,
+                    CAST_POSSIBLE_TRUNCATION,
+                    expr.span,
+                    "casting f64 to f32 may truncate the value",
+                );
+            }
+            if let (&ty::Float(FloatTy::F32), &ty::Float(FloatTy::F64)) = (&cast_from.sty, &cast_to.sty) {
+                span_lossless_lint(cx, expr, cast_expr, cast_from, cast_to);
+            }
+        },
+    }
+}
+
+fn lint_cast_ptr_alignment<'tcx>(cx: &LateContext<'_, 'tcx>, expr: &Expr, cast_from: Ty<'tcx>, cast_to: Ty<'tcx>) {
+    if_chain! {
+        if let ty::RawPtr(from_ptr_ty) = &cast_from.sty;
+        if let ty::RawPtr(to_ptr_ty) = &cast_to.sty;
+        if let Ok(from_layout) = cx.layout_of(from_ptr_ty.ty);
+        if let Ok(to_layout) = cx.layout_of(to_ptr_ty.ty);
+        if from_layout.align.abi < to_layout.align.abi;
+        // with c_void, we inherently need to trust the user
+        if !is_c_void(cx, from_ptr_ty.ty);
+        // when casting from a ZST, we don't know enough to properly lint
+        if !from_layout.is_zst();
+        then {
+            span_lint(
+                cx,
+                CAST_PTR_ALIGNMENT,
+                expr.span,
+                &format!(
+                    "casting from `{}` to a more-strictly-aligned pointer (`{}`) ({} < {} bytes)",
+                    cast_from,
+                    cast_to,
+                    from_layout.align.abi.bytes(),
+                    to_layout.align.abi.bytes(),
+                ),
+            );
         }
     }
 }

diff --git a/clippy_lints/src/utils/inspector.rs b/clippy_lints/src/utils/inspector.rs
@@ -144,6 +144,7 @@ fn has_attr(sess: &Session, attrs: &[Attribute]) -> bool {
 }
 
 #[allow(clippy::similar_names)]
+#[allow(clippy::too_many_lines)]
 fn print_expr(cx: &LateContext<'_, '_>, expr: &hir::Expr, indent: usize) {
     let ind = "  ".repeat(indent);
     println!("{}+", ind);
@@ -396,6 +397,7 @@ fn print_item(cx: &LateContext<'_, '_>, item: &hir::Item) {
 }
 
 #[allow(clippy::similar_names)]
+#[allow(clippy::too_many_lines)]
 fn print_pat(cx: &LateContext<'_, '_>, pat: &hir::Pat, indent: usize) {
     let ind = "  ".repeat(indent);
     println!("{}+", ind);

diff --git a/clippy_lints/src/write.rs b/clippy_lints/src/write.rs
@@ -320,6 +320,7 @@ impl FmtStr {
 /// ```rust,ignore
 /// (Some("string to write: {}"), Some(buf))
 /// ```
+#[allow(clippy::too_many_lines)]
 fn check_tts<'a>(cx: &EarlyContext<'a>, tts: &TokenStream, is_write: bool) -> (Option<FmtStr>, Option<Expr>) {
     use fmt_macros::*;
     let tts = tts.clone();

diff --git a/tests/ui/functions_maxlines.rs b/tests/ui/functions_maxlines.rs
@@ -56,6 +56,7 @@ fn good_lines() {
 }
 
 fn bad_lines() {
+    println!("Dont get confused by braces: {{}}");
     println!("This is bad.");
     println!("This is bad.");
     println!("This is bad.");

diff --git a/tests/ui/functions_maxlines.stderr b/tests/ui/functions_maxlines.stderr
@@ -2,7 +2,7 @@ error: This function has a large number of lines.
   --> $DIR/functions_maxlines.rs:58:1
    |
 LL | / fn bad_lines() {
-LL | |     println!("This is bad.");
+LL | |     println!("Dont get confused by braces: {{}}");
 LL | |     println!("This is bad.");
 LL | |     println!("This is bad.");
 ...  |