diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 820f4c858..adb696ad5 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -3420,11 +3420,12 @@ declare_clippy_lint! { declare_clippy_lint! { /// ### What it does /// Looks for calls to [`Stdin::read_line`] to read a line from the standard input - /// into a string, then later attempting to parse this string into a type without first trimming it, which will - /// always fail because the string has a trailing newline in it. + /// into a string, then later attempting to use that string for an operation that will never + /// work for strings with a trailing newline character in it (e.g. parsing into a `i32`). /// /// ### Why is this bad? - /// The `.parse()` call will always fail. + /// The operation will always fail at runtime no matter what the user enters, thus + /// making it a useless operation. /// /// ### Example /// ```rust,ignore diff --git a/clippy_lints/src/methods/read_line_without_trim.rs b/clippy_lints/src/methods/read_line_without_trim.rs index 3b903ec5c..0c8b62842 100644 --- a/clippy_lints/src/methods/read_line_without_trim.rs +++ b/clippy_lints/src/methods/read_line_without_trim.rs @@ -5,15 +5,26 @@ use clippy_utils::source::snippet; use clippy_utils::ty::is_type_diagnostic_item; use clippy_utils::visitors::for_each_local_use_after_expr; use clippy_utils::{get_parent_expr, match_def_path}; +use rustc_ast::LitKind; use rustc_errors::Applicability; use rustc_hir::def::Res; -use rustc_hir::{Expr, ExprKind, QPath}; +use rustc_hir::{BinOpKind, Expr, ExprKind, QPath}; use rustc_lint::LateContext; use rustc_middle::ty::{self, Ty}; use rustc_span::sym; use super::READ_LINE_WITHOUT_TRIM; +fn expr_is_string_literal_without_trailing_newline(expr: &Expr<'_>) -> bool { + if let ExprKind::Lit(lit) = expr.kind + && let LitKind::Str(sym, _) = lit.node + { + !sym.as_str().ends_with('\n') + } else { + false + } +} + /// Will a `.parse::()` call fail if the input has a trailing newline? fn parse_fails_on_trailing_newline(ty: Ty<'_>) -> bool { // only allow a very limited set of types for now, for which we 100% know parsing will fail @@ -27,30 +38,66 @@ pub fn check(cx: &LateContext<'_>, call: &Expr<'_>, recv: &Expr<'_>, arg: &Expr< && let Res::Local(local_id) = path.res { // We've checked that `call` is a call to `Stdin::read_line()` with the right receiver, - // now let's check if the first use of the string passed to `::read_line()` is - // parsed into a type that will always fail if it has a trailing newline. + // now let's check if the first use of the string passed to `::read_line()` + // is used for operations that will always fail (e.g. parsing "6\n" into a number) for_each_local_use_after_expr(cx, local_id, call.hir_id, |expr| { - if let Some(parent) = get_parent_expr(cx, expr) - && let ExprKind::MethodCall(segment, .., span) = parent.kind - && segment.ident.name == sym!(parse) - && let parse_result_ty = cx.typeck_results().expr_ty(parent) - && is_type_diagnostic_item(cx, parse_result_ty, sym::Result) - && let ty::Adt(_, args) = parse_result_ty.kind() - && let Some(ok_ty) = args[0].as_type() - && parse_fails_on_trailing_newline(ok_ty) - { - let local_snippet = snippet(cx, expr.span, ""); - span_lint_and_then( - cx, - READ_LINE_WITHOUT_TRIM, - span, - "calling `.parse()` without trimming the trailing newline character", - |diag| { + if let Some(parent) = get_parent_expr(cx, expr) { + let data = if let ExprKind::MethodCall(segment, recv, args, span) = parent.kind { + if segment.ident.name == sym!(parse) + && let parse_result_ty = cx.typeck_results().expr_ty(parent) + && is_type_diagnostic_item(cx, parse_result_ty, sym::Result) + && let ty::Adt(_, substs) = parse_result_ty.kind() + && let Some(ok_ty) = substs[0].as_type() + && parse_fails_on_trailing_newline(ok_ty) + { + // Called `s.parse::()` where `T` is a type we know for certain will fail + // if the input has a trailing newline + Some(( + span, + "calling `.parse()` on a string without trimming the trailing newline character", + "checking", + )) + } else if segment.ident.name == sym!(ends_with) + && recv.span == expr.span + && let [arg] = args + && expr_is_string_literal_without_trailing_newline(arg) + { + // Called `s.ends_with()` where the argument is a string literal that does + // not end with a newline, thus always evaluating to false + Some(( + parent.span, + "checking the end of a string without trimming the trailing newline character", + "parsing", + )) + } else { + None + } + } else if let ExprKind::Binary(binop, left, right) = parent.kind + && let BinOpKind::Eq = binop.node + && (expr_is_string_literal_without_trailing_newline(left) + || expr_is_string_literal_without_trailing_newline(right)) + { + // `s == ` where the string literal does not end with a newline + Some(( + parent.span, + "comparing a string literal without trimming the trailing newline character", + "comparison", + )) + } else { + None + }; + + if let Some((primary_span, lint_message, operation)) = data { + span_lint_and_then(cx, READ_LINE_WITHOUT_TRIM, primary_span, lint_message, |diag| { + let local_snippet = snippet(cx, expr.span, ""); + diag.span_note( call.span, - "call to `.read_line()` here, \ - which leaves a trailing newline character in the buffer, \ - which in turn will cause `.parse()` to fail", + format!( + "call to `.read_line()` here, \ + which leaves a trailing newline character in the buffer, \ + which in turn will cause the {operation} to always fail" + ), ); diag.span_suggestion( @@ -59,8 +106,8 @@ pub fn check(cx: &LateContext<'_>, call: &Expr<'_>, recv: &Expr<'_>, arg: &Expr< format!("{local_snippet}.trim_end()"), Applicability::MachineApplicable, ); - }, - ); + }); + } } // only consider the first use to prevent this scenario: diff --git a/tests/ui/read_line_without_trim.fixed b/tests/ui/read_line_without_trim.fixed index 03a99b17d..523ad5552 100644 --- a/tests/ui/read_line_without_trim.fixed +++ b/tests/ui/read_line_without_trim.fixed @@ -31,4 +31,17 @@ fn main() { std::io::stdin().read_line(&mut input).unwrap(); // this is actually ok, so don't lint here let _x = input.parse::().unwrap(); + + // comparing with string literals + let mut input = String::new(); + std::io::stdin().read_line(&mut input).unwrap(); + if input.trim_end() == "foo" { + println!("This will never ever execute!"); + } + + let mut input = String::new(); + std::io::stdin().read_line(&mut input).unwrap(); + if input.trim_end().ends_with("foo") { + println!("Neither will this"); + } } diff --git a/tests/ui/read_line_without_trim.rs b/tests/ui/read_line_without_trim.rs index 65510aea0..e31ff0cde 100644 --- a/tests/ui/read_line_without_trim.rs +++ b/tests/ui/read_line_without_trim.rs @@ -31,4 +31,17 @@ fn main() { std::io::stdin().read_line(&mut input).unwrap(); // this is actually ok, so don't lint here let _x = input.parse::().unwrap(); + + // comparing with string literals + let mut input = String::new(); + std::io::stdin().read_line(&mut input).unwrap(); + if input == "foo" { + println!("This will never ever execute!"); + } + + let mut input = String::new(); + std::io::stdin().read_line(&mut input).unwrap(); + if input.ends_with("foo") { + println!("Neither will this"); + } } diff --git a/tests/ui/read_line_without_trim.stderr b/tests/ui/read_line_without_trim.stderr index 7d0ac26c3..b54229f76 100644 --- a/tests/ui/read_line_without_trim.stderr +++ b/tests/ui/read_line_without_trim.stderr @@ -1,4 +1,4 @@ -error: calling `.parse()` without trimming the trailing newline character +error: calling `.parse()` on a string without trimming the trailing newline character --> tests/ui/read_line_without_trim.rs:12:25 | LL | let _x: i32 = input.parse().unwrap(); @@ -6,7 +6,7 @@ LL | let _x: i32 = input.parse().unwrap(); | | | help: try: `input.trim_end()` | -note: call to `.read_line()` here, which leaves a trailing newline character in the buffer, which in turn will cause `.parse()` to fail +note: call to `.read_line()` here, which leaves a trailing newline character in the buffer, which in turn will cause the checking to always fail --> tests/ui/read_line_without_trim.rs:11:5 | LL | std::io::stdin().read_line(&mut input).unwrap(); @@ -14,7 +14,7 @@ LL | std::io::stdin().read_line(&mut input).unwrap(); = note: `-D clippy::read-line-without-trim` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::read_line_without_trim)]` -error: calling `.parse()` without trimming the trailing newline character +error: calling `.parse()` on a string without trimming the trailing newline character --> tests/ui/read_line_without_trim.rs:16:20 | LL | let _x = input.parse::().unwrap(); @@ -22,13 +22,13 @@ LL | let _x = input.parse::().unwrap(); | | | help: try: `input.trim_end()` | -note: call to `.read_line()` here, which leaves a trailing newline character in the buffer, which in turn will cause `.parse()` to fail +note: call to `.read_line()` here, which leaves a trailing newline character in the buffer, which in turn will cause the checking to always fail --> tests/ui/read_line_without_trim.rs:15:5 | LL | std::io::stdin().read_line(&mut input).unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: calling `.parse()` without trimming the trailing newline character +error: calling `.parse()` on a string without trimming the trailing newline character --> tests/ui/read_line_without_trim.rs:20:20 | LL | let _x = input.parse::().unwrap(); @@ -36,13 +36,13 @@ LL | let _x = input.parse::().unwrap(); | | | help: try: `input.trim_end()` | -note: call to `.read_line()` here, which leaves a trailing newline character in the buffer, which in turn will cause `.parse()` to fail +note: call to `.read_line()` here, which leaves a trailing newline character in the buffer, which in turn will cause the checking to always fail --> tests/ui/read_line_without_trim.rs:19:5 | LL | std::io::stdin().read_line(&mut input).unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: calling `.parse()` without trimming the trailing newline character +error: calling `.parse()` on a string without trimming the trailing newline character --> tests/ui/read_line_without_trim.rs:24:20 | LL | let _x = input.parse::().unwrap(); @@ -50,13 +50,13 @@ LL | let _x = input.parse::().unwrap(); | | | help: try: `input.trim_end()` | -note: call to `.read_line()` here, which leaves a trailing newline character in the buffer, which in turn will cause `.parse()` to fail +note: call to `.read_line()` here, which leaves a trailing newline character in the buffer, which in turn will cause the checking to always fail --> tests/ui/read_line_without_trim.rs:23:5 | LL | std::io::stdin().read_line(&mut input).unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: calling `.parse()` without trimming the trailing newline character +error: calling `.parse()` on a string without trimming the trailing newline character --> tests/ui/read_line_without_trim.rs:28:20 | LL | let _x = input.parse::().unwrap(); @@ -64,11 +64,39 @@ LL | let _x = input.parse::().unwrap(); | | | help: try: `input.trim_end()` | -note: call to `.read_line()` here, which leaves a trailing newline character in the buffer, which in turn will cause `.parse()` to fail +note: call to `.read_line()` here, which leaves a trailing newline character in the buffer, which in turn will cause the checking to always fail --> tests/ui/read_line_without_trim.rs:27:5 | LL | std::io::stdin().read_line(&mut input).unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 5 previous errors +error: comparing a string literal without trimming the trailing newline character + --> tests/ui/read_line_without_trim.rs:38:8 + | +LL | if input == "foo" { + | -----^^^^^^^^^ + | | + | help: try: `input.trim_end()` + | +note: call to `.read_line()` here, which leaves a trailing newline character in the buffer, which in turn will cause the comparison to always fail + --> tests/ui/read_line_without_trim.rs:37:5 + | +LL | std::io::stdin().read_line(&mut input).unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: checking the end of a string without trimming the trailing newline character + --> tests/ui/read_line_without_trim.rs:44:8 + | +LL | if input.ends_with("foo") { + | -----^^^^^^^^^^^^^^^^^ + | | + | help: try: `input.trim_end()` + | +note: call to `.read_line()` here, which leaves a trailing newline character in the buffer, which in turn will cause the parsing to always fail + --> tests/ui/read_line_without_trim.rs:43:5 + | +LL | std::io::stdin().read_line(&mut input).unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 7 previous errors