-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Auto merge of #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 #6713.
- Loading branch information
Showing
5 changed files
with
211 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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(()) | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
|
|