@@ -5,8 +5,9 @@ use crate::utils::usage::{is_unused, mutated_variables};
55use crate :: utils:: {
66 get_enclosing_block, get_parent_expr, get_trait_def_id, has_iter_method, higher, implements_trait,
77 is_integer_const, is_no_std_crate, is_refutable, is_type_diagnostic_item, last_path_segment, match_trait_method,
8- match_type, match_var, multispan_sugg, qpath_res, snippet, snippet_opt, snippet_with_applicability, span_lint,
9- span_lint_and_help, span_lint_and_sugg, span_lint_and_then, sugg, SpanlessEq ,
8+ match_type, match_var, multispan_sugg, qpath_res, snippet, snippet_opt, snippet_with_applicability,
9+ snippet_with_macro_callsite, span_lint, span_lint_and_help, span_lint_and_sugg, span_lint_and_then, sugg,
10+ SpanlessEq ,
1011} ;
1112use if_chain:: if_chain;
1213use rustc_ast:: ast;
@@ -419,6 +420,39 @@ declare_clippy_lint! {
419420 "variables used within while expression are not mutated in the body"
420421}
421422
423+ declare_clippy_lint ! {
424+ /// **What it does:** Checks whether a for loop is being used to push a constant
425+ /// value into a Vec.
426+ ///
427+ /// **Why is this bad?** This kind of operation can be expressed more succinctly with
428+ /// `vec![item;SIZE]` or `vec.resize(NEW_SIZE, item)` and using these alternatives may also
429+ /// have better performance.
430+ /// **Known problems:** None
431+ ///
432+ /// **Example:**
433+ /// ```rust
434+ /// let item1 = 2;
435+ /// let item2 = 3;
436+ /// let mut vec: Vec<u8> = Vec::new();
437+ /// for _ in 0..20 {
438+ /// vec.push(item1);
439+ /// }
440+ /// for _ in 0..30 {
441+ /// vec.push(item2);
442+ /// }
443+ /// ```
444+ /// could be written as
445+ /// ```rust
446+ /// let item1 = 2;
447+ /// let item2 = 3;
448+ /// let mut vec: Vec<u8> = vec![item1; 20];
449+ /// vec.resize(20 + 30, item2);
450+ /// ```
451+ pub SAME_ITEM_PUSH ,
452+ style,
453+ "the same item is pushed inside of a for loop"
454+ }
455+
422456declare_lint_pass ! ( Loops => [
423457 MANUAL_MEMCPY ,
424458 NEEDLESS_RANGE_LOOP ,
@@ -435,6 +469,7 @@ declare_lint_pass!(Loops => [
435469 NEVER_LOOP ,
436470 MUT_RANGE_BOUND ,
437471 WHILE_IMMUTABLE_CONDITION ,
472+ SAME_ITEM_PUSH ,
438473] ) ;
439474
440475impl < ' tcx > LateLintPass < ' tcx > for Loops {
@@ -740,6 +775,7 @@ fn check_for_loop<'tcx>(
740775 check_for_loop_over_map_kv ( cx, pat, arg, body, expr) ;
741776 check_for_mut_range_bound ( cx, arg, body) ;
742777 detect_manual_memcpy ( cx, pat, arg, body, expr) ;
778+ detect_same_item_push ( cx, pat, arg, body, expr) ;
743779}
744780
745781fn same_var < ' tcx > ( cx : & LateContext < ' tcx > , expr : & Expr < ' _ > , var : HirId ) -> bool {
@@ -1016,6 +1052,117 @@ fn detect_manual_memcpy<'tcx>(
10161052 }
10171053}
10181054
1055+ // Scans the body of the for loop and determines whether lint should be given
1056+ struct SameItemPushVisitor < ' a , ' tcx > {
1057+ should_lint : bool ,
1058+ // this field holds the last vec push operation visited, which should be the only push seen
1059+ vec_push : Option < ( & ' tcx Expr < ' tcx > , & ' tcx Expr < ' tcx > ) > ,
1060+ cx : & ' a LateContext < ' tcx > ,
1061+ }
1062+
1063+ impl < ' a , ' tcx > Visitor < ' tcx > for SameItemPushVisitor < ' a , ' tcx > {
1064+ type Map = Map < ' tcx > ;
1065+
1066+ fn visit_expr ( & mut self , expr : & ' tcx Expr < ' _ > ) {
1067+ match & expr. kind {
1068+ // Non-determinism may occur ... don't give a lint
1069+ ExprKind :: Loop ( _, _, _) | ExprKind :: Match ( _, _, _) => self . should_lint = false ,
1070+ ExprKind :: Block ( block, _) => self . visit_block ( block) ,
1071+ _ => { } ,
1072+ }
1073+ }
1074+
1075+ fn visit_block ( & mut self , b : & ' tcx Block < ' _ > ) {
1076+ for stmt in b. stmts . iter ( ) {
1077+ self . visit_stmt ( stmt) ;
1078+ }
1079+ }
1080+
1081+ fn visit_stmt ( & mut self , s : & ' tcx Stmt < ' _ > ) {
1082+ let vec_push_option = get_vec_push ( self . cx , s) ;
1083+ if vec_push_option. is_none ( ) {
1084+ // Current statement is not a push so visit inside
1085+ match & s. kind {
1086+ StmtKind :: Expr ( expr) | StmtKind :: Semi ( expr) => self . visit_expr ( & expr) ,
1087+ _ => { } ,
1088+ }
1089+ } else {
1090+ // Current statement is a push ...check whether another
1091+ // push had been previously done
1092+ if self . vec_push . is_none ( ) {
1093+ self . vec_push = vec_push_option;
1094+ } else {
1095+ // There are multiple pushes ... don't lint
1096+ self . should_lint = false ;
1097+ }
1098+ }
1099+ }
1100+
1101+ fn nested_visit_map ( & mut self ) -> NestedVisitorMap < Self :: Map > {
1102+ NestedVisitorMap :: None
1103+ }
1104+ }
1105+
1106+ // Given some statement, determine if that statement is a push on a Vec. If it is, return
1107+ // the Vec being pushed into and the item being pushed
1108+ fn get_vec_push < ' tcx > ( cx : & LateContext < ' tcx > , stmt : & ' tcx Stmt < ' _ > ) -> Option < ( & ' tcx Expr < ' tcx > , & ' tcx Expr < ' tcx > ) > {
1109+ if_chain ! {
1110+ // Extract method being called
1111+ if let StmtKind :: Semi ( semi_stmt) = & stmt. kind;
1112+ if let ExprKind :: MethodCall ( path, _, args, _) = & semi_stmt. kind;
1113+ // Figure out the parameters for the method call
1114+ if let Some ( self_expr) = args. get( 0 ) ;
1115+ if let Some ( pushed_item) = args. get( 1 ) ;
1116+ // Check that the method being called is push() on a Vec
1117+ if match_type( cx, cx. typeck_results( ) . expr_ty( self_expr) , & paths:: VEC ) ;
1118+ if path. ident. name. as_str( ) == "push" ;
1119+ then {
1120+ return Some ( ( self_expr, pushed_item) )
1121+ }
1122+ }
1123+ None
1124+ }
1125+
1126+ /// Detects for loop pushing the same item into a Vec
1127+ fn detect_same_item_push < ' tcx > (
1128+ cx : & LateContext < ' tcx > ,
1129+ pat : & ' tcx Pat < ' _ > ,
1130+ _: & ' tcx Expr < ' _ > ,
1131+ body : & ' tcx Expr < ' _ > ,
1132+ _: & ' tcx Expr < ' _ > ,
1133+ ) {
1134+ // Determine whether it is safe to lint the body
1135+ let mut same_item_push_visitor = SameItemPushVisitor {
1136+ should_lint : true ,
1137+ vec_push : None ,
1138+ cx,
1139+ } ;
1140+ walk_expr ( & mut same_item_push_visitor, body) ;
1141+ if same_item_push_visitor. should_lint {
1142+ if let Some ( ( vec, pushed_item) ) = same_item_push_visitor. vec_push {
1143+ // Make sure that the push does not involve possibly mutating values
1144+ if mutated_variables ( pushed_item, cx) . map_or ( false , |mutvars| mutvars. is_empty ( ) ) {
1145+ if let PatKind :: Wild = pat. kind {
1146+ let vec_str = snippet_with_macro_callsite ( cx, vec. span , "" ) ;
1147+ let item_str = snippet_with_macro_callsite ( cx, pushed_item. span , "" ) ;
1148+
1149+ span_lint_and_help (
1150+ cx,
1151+ SAME_ITEM_PUSH ,
1152+ vec. span ,
1153+ "it looks like the same item is being pushed into this Vec" ,
1154+ None ,
1155+ & format ! (
1156+ "try using vec![{};SIZE] or {}.resize(NEW_SIZE, {})" ,
1157+ item_str, vec_str, item_str
1158+ ) ,
1159+ )
1160+ }
1161+ }
1162+ }
1163+ }
1164+ }
1165+
10191166/// Checks for looping over a range and then indexing a sequence with it.
10201167/// The iteratee must be a range literal.
10211168#[ allow( clippy:: too_many_lines) ]
0 commit comments