Auto merge of #10970 - y21:read_line_without_trim, r=giraffate

new lint: `read_line_without_trim`

This adds a new lint that checks for calls to `Stdin::read_line` with a reference to a string that is then attempted to parse into an integer type without first trimming it, which is always going to fail at runtime.
This is something that I've seen happen a lot to beginners, because it's easy to run into when following the example of chapter 2 in the book where it shows how to program a guessing game.
It would be nice if we could point beginners to clippy and tell them "let's see what clippy has to say" and have clippy explain to them why it fails 👀

I think this lint can later be "generalized" to work not just for `Stdin` but also any `BufRead` (which seems to be where the guarantee about the trailing newline comes from) and also, matching/comparing it to a string slice that doesn't end in a newline character (e.g. `input == "foo"` is always going to fail)

changelog: new lint: [`read_line_without_trim`]

Author: bors

CHANGELOG.md

@@ -5134,6 +5134,7 @@ Released 2018-09-13
 [`rc_buffer`]: https://rust-lang.github.io/rust-clippy/master/index.html#rc_buffer
 [`rc_clone_in_vec_init`]: https://rust-lang.github.io/rust-clippy/master/index.html#rc_clone_in_vec_init
 [`rc_mutex`]: https://rust-lang.github.io/rust-clippy/master/index.html#rc_mutex
+[`read_line_without_trim`]: https://rust-lang.github.io/rust-clippy/master/index.html#read_line_without_trim
 [`read_zero_byte_vec`]: https://rust-lang.github.io/rust-clippy/master/index.html#read_zero_byte_vec
 [`recursive_format_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#recursive_format_impl
 [`redundant_allocation`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_allocation

clippy_lints/src/declared_lints.rs

@@ -389,6 +389,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::methods::OR_THEN_UNWRAP_INFO,
     crate::methods::PATH_BUF_PUSH_OVERWRITE_INFO,
     crate::methods::RANGE_ZIP_WITH_LEN_INFO,
+    crate::methods::READ_LINE_WITHOUT_TRIM_INFO,
     crate::methods::REPEAT_ONCE_INFO,
     crate::methods::RESULT_MAP_OR_INTO_OPTION_INFO,
     crate::methods::SEARCH_IS_SOME_INFO,

clippy_lints/src/methods/mod.rs

@@ -72,6 +72,7 @@ mod or_fun_call;
 mod or_then_unwrap;
 mod path_buf_push_overwrite;
 mod range_zip_with_len;
+mod read_line_without_trim;
 mod repeat_once;
 mod search_is_some;
 mod seek_from_current;
@@ -3348,6 +3349,35 @@ declare_clippy_lint! {
     "checks for usage of `Iterator::fold` with a type that implements `Try`"
 }
 
+declare_clippy_lint! {
+    /// 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.
+    ///
+    /// ### Why is this bad?
+    /// The `.parse()` call will always fail.
+    ///
+    /// ### Example
+    /// ```rust,ignore
+    /// let mut input = String::new();
+    /// std::io::stdin().read_line(&mut input).expect("Failed to read a line");
+    /// let num: i32 = input.parse().expect("Not a number!");
+    /// assert_eq!(num, 42); // we never even get here!
+    /// ```
+    /// Use instead:
+    /// ```rust,ignore
+    /// let mut input = String::new();
+    /// std::io::stdin().read_line(&mut input).expect("Failed to read a line");
+    /// let num: i32 = input.trim_end().parse().expect("Not a number!");
+    /// //                  ^^^^^^^^^^^ remove the trailing newline
+    /// assert_eq!(num, 42);
+    /// ```
+    #[clippy::version = "1.72.0"]
+    pub READ_LINE_WITHOUT_TRIM,
+    correctness,
+    "calling `Stdin::read_line`, then trying to parse it without first trimming"
+}
+
 pub struct Methods {
     avoid_breaking_exported_api: bool,
     msrv: Msrv,
@@ -3468,6 +3498,7 @@ impl_lint_pass!(Methods => [
     REPEAT_ONCE,
     STABLE_SORT_PRIMITIVE,
     UNIT_HASH,
+    READ_LINE_WITHOUT_TRIM,
     UNNECESSARY_SORT_BY,
     VEC_RESIZE_TO_ZERO,
     VERBOSE_FILE_READS,
@@ -3879,6 +3910,9 @@ impl Methods {
                 ("read_to_string", [_]) => {
                     verbose_file_reads::check(cx, expr, recv, verbose_file_reads::READ_TO_STRING_MSG);
                 },
+                ("read_line", [arg]) => {
+                    read_line_without_trim::check(cx, expr, recv, arg);
+                }
                 ("repeat", [arg]) => {
                     repeat_once::check(cx, expr, recv, arg);
                 },

clippy_lints/src/methods/read_line_without_trim.rs

@@ -0,0 +1,74 @@
+use std::ops::ControlFlow;
+
+use clippy_utils::{
+    diagnostics::span_lint_and_then, get_parent_expr, match_def_path, source::snippet, ty::is_type_diagnostic_item,
+    visitors::for_each_local_use_after_expr,
+};
+use rustc_errors::Applicability;
+use rustc_hir::Expr;
+use rustc_hir::QPath;
+use rustc_hir::{def::Res, ExprKind};
+use rustc_lint::LateContext;
+use rustc_middle::ty::{self, Ty};
+use rustc_span::sym;
+
+use super::READ_LINE_WITHOUT_TRIM;
+
+/// Will a `.parse::<ty>()` 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
+    matches!(ty.kind(), ty::Float(_) | ty::Bool | ty::Int(_) | ty::Uint(_))
+}
+
+pub fn check(cx: &LateContext<'_>, call: &Expr<'_>, recv: &Expr<'_>, arg: &Expr<'_>) {
+    if let Some(recv_adt) = cx.typeck_results().expr_ty(recv).ty_adt_def()
+        && match_def_path(cx, recv_adt.did(), &["std", "io", "stdio", "Stdin"])
+        && let ExprKind::Path(QPath::Resolved(_, path)) = arg.peel_borrows().kind
+        && 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.
+        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(_, substs) = parse_result_ty.kind()
+                && let Some(ok_ty) = substs[0].as_type()
+                && parse_fails_on_trailing_newline(ok_ty)
+            {
+                let local_snippet = snippet(cx, expr.span, "<expr>");
+                span_lint_and_then(
+                    cx,
+                    READ_LINE_WITHOUT_TRIM,
+                    span,
+                    "calling `.parse()` without trimming the trailing newline character",
+                    |diag| {
+                        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");
+
+                        diag.span_suggestion(
+                            expr.span,
+                            "try",
+                            format!("{local_snippet}.trim_end()"),
+                            Applicability::MachineApplicable,
+                        );
+                    }
+                );
+            }
+
+            // only consider the first use to prevent this scenario:
+            // ```
+            // let mut s = String::new();
+            // std::io::stdin().read_line(&mut s);
+            // s.pop();
+            // let _x: i32 = s.parse().unwrap();
+            // ```
+            // this is actually fine, because the pop call removes the trailing newline.
+            ControlFlow::<(), ()>::Break(())
+        });
+    }
+}

tests/ui/read_line_without_trim.fixed

@@ -0,0 +1,36 @@
+//@run-rustfix
+
+#![allow(unused)]
+#![warn(clippy::read_line_without_trim)]
+
+fn main() {
+    let mut input = String::new();
+    std::io::stdin().read_line(&mut input).unwrap();
+    input.pop();
+    let _x: i32 = input.parse().unwrap(); // don't trigger here, newline character is popped
+
+    let mut input = String::new();
+    std::io::stdin().read_line(&mut input).unwrap();
+    let _x: i32 = input.trim_end().parse().unwrap();
+
+    let mut input = String::new();
+    std::io::stdin().read_line(&mut input).unwrap();
+    let _x = input.trim_end().parse::<i32>().unwrap();
+
+    let mut input = String::new();
+    std::io::stdin().read_line(&mut input).unwrap();
+    let _x = input.trim_end().parse::<u32>().unwrap();
+
+    let mut input = String::new();
+    std::io::stdin().read_line(&mut input).unwrap();
+    let _x = input.trim_end().parse::<f32>().unwrap();
+
+    let mut input = String::new();
+    std::io::stdin().read_line(&mut input).unwrap();
+    let _x = input.trim_end().parse::<bool>().unwrap();
+
+    let mut input = String::new();
+    std::io::stdin().read_line(&mut input).unwrap();
+    // this is actually ok, so don't lint here
+    let _x = input.parse::<String>().unwrap();
+}

tests/ui/read_line_without_trim.rs

@@ -0,0 +1,36 @@
+//@run-rustfix
+
+#![allow(unused)]
+#![warn(clippy::read_line_without_trim)]
+
+fn main() {
+    let mut input = String::new();
+    std::io::stdin().read_line(&mut input).unwrap();
+    input.pop();
+    let _x: i32 = input.parse().unwrap(); // don't trigger here, newline character is popped
+
+    let mut input = String::new();
+    std::io::stdin().read_line(&mut input).unwrap();
+    let _x: i32 = input.parse().unwrap();
+
+    let mut input = String::new();
+    std::io::stdin().read_line(&mut input).unwrap();
+    let _x = input.parse::<i32>().unwrap();
+
+    let mut input = String::new();
+    std::io::stdin().read_line(&mut input).unwrap();
+    let _x = input.parse::<u32>().unwrap();
+
+    let mut input = String::new();
+    std::io::stdin().read_line(&mut input).unwrap();
+    let _x = input.parse::<f32>().unwrap();
+
+    let mut input = String::new();
+    std::io::stdin().read_line(&mut input).unwrap();
+    let _x = input.parse::<bool>().unwrap();
+
+    let mut input = String::new();
+    std::io::stdin().read_line(&mut input).unwrap();
+    // this is actually ok, so don't lint here
+    let _x = input.parse::<String>().unwrap();
+}

tests/ui/read_line_without_trim.stderr

@@ -0,0 +1,73 @@
+error: calling `.parse()` without trimming the trailing newline character
+  --> $DIR/read_line_without_trim.rs:14:25
+   |
+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
+  --> $DIR/read_line_without_trim.rs:13:5
+   |
+LL |     std::io::stdin().read_line(&mut input).unwrap();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   = note: `-D clippy::read-line-without-trim` implied by `-D warnings`
+
+error: calling `.parse()` without trimming the trailing newline character
+  --> $DIR/read_line_without_trim.rs:18:20
+   |
+LL |     let _x = input.parse::<i32>().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
+  --> $DIR/read_line_without_trim.rs:17:5
+   |
+LL |     std::io::stdin().read_line(&mut input).unwrap();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: calling `.parse()` without trimming the trailing newline character
+  --> $DIR/read_line_without_trim.rs:22:20
+   |
+LL |     let _x = input.parse::<u32>().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
+  --> $DIR/read_line_without_trim.rs:21:5
+   |
+LL |     std::io::stdin().read_line(&mut input).unwrap();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: calling `.parse()` without trimming the trailing newline character
+  --> $DIR/read_line_without_trim.rs:26:20
+   |
+LL |     let _x = input.parse::<f32>().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
+  --> $DIR/read_line_without_trim.rs:25:5
+   |
+LL |     std::io::stdin().read_line(&mut input).unwrap();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: calling `.parse()` without trimming the trailing newline character
+  --> $DIR/read_line_without_trim.rs:30:20
+   |
+LL |     let _x = input.parse::<bool>().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
+  --> $DIR/read_line_without_trim.rs:29:5
+   |
+LL |     std::io::stdin().read_line(&mut input).unwrap();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 5 previous errors
+