Auto merge of rust-lang#11136 - y21:enhance_read_line_without_trim, r=dswij

[`read_line_without_trim`]: detect string literal comparison and `.ends_with()` calls This lint now also realizes that a comparison like `s == "foo"` and calls such as `s.ends_with("foo")` will fail if `s` was initialized by a call to `Stdin::read_line` (because of the trailing newline). changelog: [`read_line_without_trim`]: detect string literal comparison and `.ends_with()` calls r? `@giraffate` assigning you because you reviewed rust-lang#10970 that added this lint, so this is kinda a followup PR ^^

Author: bors

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

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::<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
@@ -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, "<expr>");
- 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::<T>()` 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(<some string literal>)` 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 == <some string literal>` 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, "<expr>");
+
  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:

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::<String>().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");
+ }
 }

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::<String>().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");
+ }
 }

tests/ui/read_line_without_trim.stderr

@@ -1,74 +1,102 @@
-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();
  | ----- ^^^^^^^
  | |
  | 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();
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  = 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::<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
+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::<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
+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::<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
+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::<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
+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