Auto merge of rust-lang#6717 - booleancoercion:master, r=llogiq

Add the from_str_radix_10 lint

changelog: added the new `from_str_radix_10` which sometimes replaces calls to `primitive::from_str_radix` to `str::parse`

This is ready to be merged, although maybe the category should be `pedantic` instead of `style`? I'm not sure where it fits better.

Closes rust-lang#6713.

Author: bors

CHANGELOG.md

@@ -2093,6 +2093,7 @@ Released 2018-09-13
 [`forget_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#forget_ref
 [`from_iter_instead_of_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#from_iter_instead_of_collect
 [`from_over_into`]: https://rust-lang.github.io/rust-clippy/master/index.html#from_over_into
+[`from_str_radix_10`]: https://rust-lang.github.io/rust-clippy/master/index.html#from_str_radix_10
 [`future_not_send`]: https://rust-lang.github.io/rust-clippy/master/index.html#future_not_send
 [`get_last_with_len`]: https://rust-lang.github.io/rust-clippy/master/index.html#get_last_with_len
 [`get_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#get_unwrap

clippy_lints/src/from_str_radix_10.rs

@@ -0,0 +1,101 @@
+use if_chain::if_chain;
+use rustc_errors::Applicability;
+use rustc_hir::{def, Expr, ExprKind, PrimTy, QPath, TyKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty::Ty;
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::symbol::sym;
+
+use crate::utils::is_type_diagnostic_item;
+use crate::utils::span_lint_and_sugg;
+use crate::utils::sugg::Sugg;
+
+declare_clippy_lint! {
+    /// **What it does:**
+    /// Checks for function invocations of the form `primitive::from_str_radix(s, 10)`
+    ///
+    /// **Why is this bad?**
+    /// This specific common use case can be rewritten as `s.parse::<primitive>()`
+    /// (and in most cases, the turbofish can be removed), which reduces code length
+    /// and complexity.
+    ///
+    /// **Known problems:**
+    /// This lint may suggest using (&<expression>).parse() instead of <expression>.parse() directly
+    /// in some cases, which is correct but adds unnecessary complexity to the code.
+    ///
+    /// **Example:**
+    ///
+    /// ```ignore
+    /// let input: &str = get_input();
+    /// let num = u16::from_str_radix(input, 10)?;
+    /// ```
+    /// Use instead:
+    /// ```ignore
+    /// let input: &str = get_input();
+    /// let num: u16 = input.parse()?;
+    /// ```
+    pub FROM_STR_RADIX_10,
+    style,
+    "from_str_radix with radix 10"
+}
+
+declare_lint_pass!(FromStrRadix10 => [FROM_STR_RADIX_10]);
+
+impl LateLintPass<'tcx> for FromStrRadix10 {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, exp: &Expr<'tcx>) {
+        if_chain! {
+            if let ExprKind::Call(maybe_path, arguments) = &exp.kind;
+            if let ExprKind::Path(QPath::TypeRelative(ty, pathseg)) = &maybe_path.kind;
+
+            // check if the first part of the path is some integer primitive
+            if let TyKind::Path(ty_qpath) = &ty.kind;
+            let ty_res = cx.qpath_res(ty_qpath, ty.hir_id);
+            if let def::Res::PrimTy(prim_ty) = ty_res;
+            if matches!(prim_ty, PrimTy::Int(_) | PrimTy::Uint(_));
+
+            // check if the second part of the path indeed calls the associated
+            // function `from_str_radix`
+            if pathseg.ident.name.as_str() == "from_str_radix";
+
+            // check if the second argument is a primitive `10`
+            if arguments.len() == 2;
+            if let ExprKind::Lit(lit) = &arguments[1].kind;
+            if let rustc_ast::ast::LitKind::Int(10, _) = lit.node;
+
+            then {
+                let expr = if let ExprKind::AddrOf(_, _, expr) = &arguments[0].kind {
+                    let ty = cx.typeck_results().expr_ty(expr);
+                    if is_ty_stringish(cx, ty) {
+                        expr
+                    } else {
+                        &arguments[0]
+                    }
+                } else {
+                    &arguments[0]
+                };
+
+                let sugg = Sugg::hir_with_applicability(
+                    cx,
+                    expr,
+                    "<string>",
+                    &mut Applicability::MachineApplicable
+                ).maybe_par();
+
+                span_lint_and_sugg(
+                    cx,
+                    FROM_STR_RADIX_10,
+                    exp.span,
+                    "this call to `from_str_radix` can be replaced with a call to `str::parse`",
+                    "try",
+                    format!("{}.parse::<{}>()", sugg, prim_ty.name_str()),
+                    Applicability::MaybeIncorrect
+                );
+            }
+        }
+    }
+}
+
+/// Checks if a Ty is `String` or `&str`
+fn is_ty_stringish(cx: &LateContext<'_>, ty: Ty<'_>) -> bool {
+    is_type_diagnostic_item(cx, ty, sym::string_type) || is_type_diagnostic_item(cx, ty, sym::str)
+}

clippy_lints/src/lib.rs

@@ -211,6 +211,7 @@ mod floating_point_arithmetic;
 mod format;
 mod formatting;
 mod from_over_into;
+mod from_str_radix_10;
 mod functions;
 mod future_not_send;
 mod get_last_with_len;
@@ -639,6 +640,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &formatting::SUSPICIOUS_ELSE_FORMATTING,
         &formatting::SUSPICIOUS_UNARY_OP_FORMATTING,
         &from_over_into::FROM_OVER_INTO,
+        &from_str_radix_10::FROM_STR_RADIX_10,
         &functions::DOUBLE_MUST_USE,
         &functions::MUST_USE_CANDIDATE,
         &functions::MUST_USE_UNIT,
@@ -1259,6 +1261,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(move || box types::PtrAsPtr::new(msrv));
     store.register_late_pass(|| box case_sensitive_file_extension_comparisons::CaseSensitiveFileExtensionComparisons);
     store.register_late_pass(|| box redundant_slicing::RedundantSlicing);
+    store.register_late_pass(|| box from_str_radix_10::FromStrRadix10);
 
     store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
         LintId::of(&arithmetic::FLOAT_ARITHMETIC),
@@ -1472,6 +1475,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&formatting::SUSPICIOUS_ELSE_FORMATTING),
         LintId::of(&formatting::SUSPICIOUS_UNARY_OP_FORMATTING),
         LintId::of(&from_over_into::FROM_OVER_INTO),
+        LintId::of(&from_str_radix_10::FROM_STR_RADIX_10),
         LintId::of(&functions::DOUBLE_MUST_USE),
         LintId::of(&functions::MUST_USE_UNIT),
         LintId::of(&functions::NOT_UNSAFE_PTR_ARG_DEREF),
@@ -1728,6 +1732,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&formatting::SUSPICIOUS_ELSE_FORMATTING),
         LintId::of(&formatting::SUSPICIOUS_UNARY_OP_FORMATTING),
         LintId::of(&from_over_into::FROM_OVER_INTO),
+        LintId::of(&from_str_radix_10::FROM_STR_RADIX_10),
         LintId::of(&functions::DOUBLE_MUST_USE),
         LintId::of(&functions::MUST_USE_UNIT),
         LintId::of(&functions::RESULT_UNIT_ERR),

tests/ui/from_str_radix_10.rs

@@ -0,0 +1,52 @@
+#![warn(clippy::from_str_radix_10)]
+
+mod some_mod {
+    // fake function that shouldn't trigger the lint
+    pub fn from_str_radix(_: &str, _: u32) -> Result<(), std::num::ParseIntError> {
+        unimplemented!()
+    }
+}
+
+// fake function that shouldn't trigger the lint
+fn from_str_radix(_: &str, _: u32) -> Result<(), std::num::ParseIntError> {
+    unimplemented!()
+}
+
+// to test parenthesis addition
+struct Test;
+
+impl std::ops::Add<Test> for Test {
+    type Output = &'static str;
+
+    fn add(self, _: Self) -> Self::Output {
+        "304"
+    }
+}
+
+fn main() -> Result<(), Box<dyn std::error::Error>> {
+    // all of these should trigger the lint
+    u32::from_str_radix("30", 10)?;
+    i64::from_str_radix("24", 10)?;
+    isize::from_str_radix("100", 10)?;
+    u8::from_str_radix("7", 10)?;
+    u16::from_str_radix(&("10".to_owned() + "5"), 10)?;
+    i128::from_str_radix(Test + Test, 10)?;
+
+    let string = "300";
+    i32::from_str_radix(string, 10)?;
+
+    let stringier = "400".to_string();
+    i32::from_str_radix(&stringier, 10)?;
+
+    // none of these should trigger the lint
+    u16::from_str_radix("20", 3)?;
+    i32::from_str_radix("45", 12)?;
+    usize::from_str_radix("10", 16)?;
+    i128::from_str_radix("10", 13)?;
+    some_mod::from_str_radix("50", 10)?;
+    some_mod::from_str_radix("50", 6)?;
+    from_str_radix("50", 10)?;
+    from_str_radix("50", 6)?;
+
+    Ok(())
+}

tests/ui/from_str_radix_10.stderr

@@ -0,0 +1,52 @@
+error: this call to `from_str_radix` can be replaced with a call to `str::parse`
+  --> $DIR/from_str_radix_10.rs:28:5
+   |
+LL |     u32::from_str_radix("30", 10)?;
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `"30".parse::<u32>()`
+   |
+   = note: `-D clippy::from-str-radix-10` implied by `-D warnings`
+
+error: this call to `from_str_radix` can be replaced with a call to `str::parse`
+  --> $DIR/from_str_radix_10.rs:29:5
+   |
+LL |     i64::from_str_radix("24", 10)?;
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `"24".parse::<i64>()`
+
+error: this call to `from_str_radix` can be replaced with a call to `str::parse`
+  --> $DIR/from_str_radix_10.rs:30:5
+   |
+LL |     isize::from_str_radix("100", 10)?;
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `"100".parse::<isize>()`
+
+error: this call to `from_str_radix` can be replaced with a call to `str::parse`
+  --> $DIR/from_str_radix_10.rs:31:5
+   |
+LL |     u8::from_str_radix("7", 10)?;
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `"7".parse::<u8>()`
+
+error: this call to `from_str_radix` can be replaced with a call to `str::parse`
+  --> $DIR/from_str_radix_10.rs:32:5
+   |
+LL |     u16::from_str_radix(&("10".to_owned() + "5"), 10)?;
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `(("10".to_owned() + "5")).parse::<u16>()`
+
+error: this call to `from_str_radix` can be replaced with a call to `str::parse`
+  --> $DIR/from_str_radix_10.rs:33:5
+   |
+LL |     i128::from_str_radix(Test + Test, 10)?;
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `(Test + Test).parse::<i128>()`
+
+error: this call to `from_str_radix` can be replaced with a call to `str::parse`
+  --> $DIR/from_str_radix_10.rs:36:5
+   |
+LL |     i32::from_str_radix(string, 10)?;
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `string.parse::<i32>()`
+
+error: this call to `from_str_radix` can be replaced with a call to `str::parse`
+  --> $DIR/from_str_radix_10.rs:39:5
+   |
+LL |     i32::from_str_radix(&stringier, 10)?;
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `stringier.parse::<i32>()`
+
+error: aborting due to 8 previous errors
+