suggest mkdir() instead of system("mkdir ...")

3 files changed, 30 insertions, 8 deletions

diff --git a/perl_checker.src/parser_helper.ml b/perl_checker.src/parser_helper.ml
index c317f98..b4afdaf 100644
--- a/perl_checker.src/parser_helper.ml
+++ b/perl_checker.src/parser_helper.ml
@@ -840,6 +840,22 @@ msgstr \"\"
   ) sorted_pot_strings ;      
   close_out fd
 
+let fake_string_from_String_l l = String.concat "$foo" (List.map fst l)
+let fake_string_option_from_expr = function
+  | String(l, _) -> Some(fake_string_from_String_l l)
+  | Raw_string(s, _) -> Some s
+  | _ -> None
+
+let check_system_call = function
+  | "mkdir" :: l ->
+      let has_p = List.exists (str_begins_with "-p") l in
+      let has_m = List.exists (str_begins_with "-m") l in
+      if has_p && has_m then ()
+      else if has_p then warn_rule "you can replace system(\"mkdir -p ...\") with mkdir_p(...)"
+      else if has_m then warn_rule "you can replace system(\"mkdir -m <mode> ...\") with mkdir(..., <mode>)"
+      else warn_rule "you can replace system(\"mkdir ...\") with mkdir(...)"
+  | _ -> ()
+
 let call_raw force_non_builtin_func (e, para) =
   let check_anonymous_block f = function
   | [ Anonymous_sub _ ; Deref (I_hash, _) ] ->
@@ -946,12 +962,17 @@ let call_raw force_non_builtin_func (e, para) =
 
       | "system" ->
 	  (match un_parenthesize_full_l para with
-	  | [ String(l, _) ] -> 
-	      if List.exists (fun (s, _) -> String.contains s '\'' || String.contains s '"') l && 
-		not (List.exists (fun (s, _) -> List.exists (String.contains s) [ '<' ; '>' ; '&' ; ';']) l) then
-		warn_rule "instead of quoting parameters you should give a list of arguments"
-	  | _ -> ())
-
+	  | [ e ] ->
+	      (match fake_string_option_from_expr e with
+	      | Some s ->
+		  if List.exists (String.contains s) [ '\'' ; char_quote ] &&
+	            not (List.exists (String.contains s) [ '<' ; '>' ; '&' ; ';']) then
+		    warn_rule "instead of quoting parameters you should give a list of arguments";
+		  check_system_call (split_at ' ' s)
+	      | None -> ())
+	  | l -> 
+	      let l' = filter_some_with fake_string_option_from_expr l in
+	      check_system_call l')
       | _ -> ()
       );
 
diff --git a/perl_checker.src/test/suggest_better.t b/perl_checker.src/test/suggest_better.t
index 4b8d2d5..e98e64f 100644
--- a/perl_checker.src/test/suggest_better.t
+++ b/perl_checker.src/test/suggest_better.t
@@ -100,3 +100,6 @@ $xxx ? $yyy : ()                         you may use if_() here
                                            beware that the short-circuit semantic of ?: is not kept
                                            if you want to keep the short-circuit behaviour, replace () with @{[]} and there will be no warning anymore
 
+system(qq(foo "$xxx"))                   instead of quoting parameters you should give a list of arguments
+
+system("mkdir", $xxx)                    you can replace system("mkdir ...") with mkdir(...)
diff --git a/perl_checker.src/test/various_errors.t b/perl_checker.src/test/various_errors.t
index f9c4089..dfac926 100644
--- a/perl_checker.src/test/various_errors.t
+++ b/perl_checker.src/test/various_errors.t
@@ -39,8 +39,6 @@ pop @l, 1                                pop is expecting an array and nothing e
 
 pop $xxx                                 pop is expecting an array and nothing else
 
-system(qq(foo "$xxx"))                   instead of quoting parameters you should give a list of arguments
-
 my (@l2, $xxx) = @l;                     @l2 takes all the arguments, $xxx is undef in any case
 
 $bad                                     undeclared variable $bad