Merge pull request #7696 from Rylan12/audit-shell-variables

style: set shell variables in hash

Library/Homebrew/rubocops/lines.rb

@@ -195,7 +195,7 @@ module RuboCop
         end
       end
 
-      class ShellCmd < FormulaCop
+      class SafePopenCommands < FormulaCop
         def audit_formula(_node, _class_node, _parent_class_node, body_node)
           test = find_block(body_node, :test)
 
@@ -223,6 +223,35 @@ module RuboCop
         end
       end
 
+      class ShellVariables < FormulaCop
+        def audit_formula(_node, _class_node, _parent_class_node, body_node)
+          popen_commands = [
+            :popen,
+            :popen_read,
+            :safe_popen_read,
+            :popen_write,
+            :safe_popen_write,
+          ]
+
+          popen_commands.each do |command|
+            find_instance_method_call(body_node, "Utils", command) do |method|
+              next unless match = regex_match_group(parameters(method).first, /^([^"' ]+)=([^"' ]+)(?: (.*))?$/)
+
+              good_args = "Utils.#{command}({ \"#{match[1]}\" => \"#{match[2]}\" }, \"#{match[3]}\")"
+
+              problem "Use `#{good_args}` instead of `#{method.source}`"
+            end
+          end
+        end
+
+        def autocorrect(node)
+          lambda do |corrector|
+            match = regex_match_group(node, /^([^"' ]+)=([^"' ]+)(?: (.*))?$/)
+            corrector.replace(node.source_range, "{ \"#{match[1]}\" => \"#{match[2]}\" }, \"#{match[3]}\"")
+          end
+        end
+      end
+
       class Miscellaneous < FormulaCop
         def audit_formula(_node, _class_node, _parent_class_node, body_node)
           # FileUtils is included in Formula

Library/Homebrew/test/rubocops/lines_spec.rb

@@ -345,10 +345,10 @@ describe RuboCop::Cop::FormulaAudit::MpiCheck do
   end
 end
 
-describe RuboCop::Cop::FormulaAudit::ShellCmd do
+describe RuboCop::Cop::FormulaAudit::SafePopenCommands do
   subject(:cop) { described_class.new }
 
-  context "When auditing shell commands" do
+  context "When auditing popen commands" do
     it "Utils.popen_read should become Utils.safe_popen_read" do
       expect_offense(<<~RUBY)
         class Foo < Formula
@@ -440,6 +440,140 @@ describe RuboCop::Cop::FormulaAudit::ShellCmd do
   end
 end
 
+describe RuboCop::Cop::FormulaAudit::ShellVariables do
+  subject(:cop) { described_class.new }
+
+  context "When auditing shell variables" do
+    it "Shell variables should be expanded in Utils.popen" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          def install
+            Utils.popen "SHELL=bash foo"
+                         ^^^^^^^^^^^^^^ Use `Utils.popen({ "SHELL" => "bash" }, "foo")` instead of `Utils.popen "SHELL=bash foo"`
+          end
+        end
+      RUBY
+    end
+
+    it "Shell variables should be expanded in Utils.safe_popen_read" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          def install
+            Utils.safe_popen_read "SHELL=bash foo"
+                                   ^^^^^^^^^^^^^^ Use `Utils.safe_popen_read({ "SHELL" => "bash" }, "foo")` instead of `Utils.safe_popen_read "SHELL=bash foo"`
+          end
+        end
+      RUBY
+    end
+
+    it "Shell variables should be expanded in Utils.safe_popen_write" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          def install
+            Utils.safe_popen_write "SHELL=bash foo"
+                                    ^^^^^^^^^^^^^^ Use `Utils.safe_popen_write({ "SHELL" => "bash" }, "foo")` instead of `Utils.safe_popen_write "SHELL=bash foo"`
+          end
+        end
+      RUBY
+    end
+
+    it "Shell variables should be expanded and keep inline string variables in the arguments" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          def install
+            Utils.popen "SHELL=bash \#{bin}/foo"
+                         ^^^^^^^^^^^^^^^^^^^^^ Use `Utils.popen({ "SHELL" => "bash" }, "\#{bin}/foo")` instead of `Utils.popen "SHELL=bash \#{bin}/foo"`
+          end
+        end
+      RUBY
+    end
+
+    it "corrects shell variables in Utils.popen" do
+      source = <<~RUBY
+        class Foo < Formula
+          def install
+            Utils.popen("SHELL=bash foo")
+          end
+        end
+      RUBY
+
+      corrected_source = <<~RUBY
+        class Foo < Formula
+          def install
+            Utils.popen({ "SHELL" => "bash" }, "foo")
+          end
+        end
+      RUBY
+
+      new_source = autocorrect_source(source)
+      expect(new_source).to eq(corrected_source)
+    end
+
+    it "corrects shell variables in Utils.safe_popen_read" do
+      source = <<~RUBY
+        class Foo < Formula
+          def install
+            Utils.safe_popen_read("SHELL=bash foo")
+          end
+        end
+      RUBY
+
+      corrected_source = <<~RUBY
+        class Foo < Formula
+          def install
+            Utils.safe_popen_read({ "SHELL" => "bash" }, "foo")
+          end
+        end
+      RUBY
+
+      new_source = autocorrect_source(source)
+      expect(new_source).to eq(corrected_source)
+    end
+
+    it "corrects shell variables in Utils.safe_popen_write" do
+      source = <<~RUBY
+        class Foo < Formula
+          def install
+            Utils.safe_popen_write("SHELL=bash foo")
+          end
+        end
+      RUBY
+
+      corrected_source = <<~RUBY
+        class Foo < Formula
+          def install
+            Utils.safe_popen_write({ "SHELL" => "bash" }, "foo")
+          end
+        end
+      RUBY
+
+      new_source = autocorrect_source(source)
+      expect(new_source).to eq(corrected_source)
+    end
+
+    it "corrects shell variables with inline string variable in arguments" do
+      source = <<~RUBY
+        class Foo < Formula
+          def install
+            Utils.popen("SHELL=bash \#{bin}/foo")
+          end
+        end
+      RUBY
+
+      corrected_source = <<~RUBY
+        class Foo < Formula
+          def install
+            Utils.popen({ "SHELL" => "bash" }, "\#{bin}/foo")
+          end
+        end
+      RUBY
+
+      new_source = autocorrect_source(source)
+      expect(new_source).to eq(corrected_source)
+    end
+  end
+end
+
 describe RuboCop::Cop::FormulaAudit::Miscellaneous do
   subject(:cop) { described_class.new }