From e82084583a1c6c497e062d33f8602b3eb29e58de Mon Sep 17 00:00:00 2001 From: Rylan Polster Date: Thu, 4 Jun 2020 21:52:20 -0400 Subject: [PATCH] style: set shell variables in hash When running Utils.popen (or similar popen command), having a line like "SHELL=bash ..." doesn't work properly. Instead, use: `Utils.popen({ "SHELL" => "bash" }, "...")` --- Library/Homebrew/rubocops/lines.rb | 31 ++++- Library/Homebrew/test/rubocops/lines_spec.rb | 138 ++++++++++++++++++- 2 files changed, 166 insertions(+), 3 deletions(-) diff --git a/Library/Homebrew/rubocops/lines.rb b/Library/Homebrew/rubocops/lines.rb index 38e26af8b5..faa5f6ff80 100644 --- a/Library/Homebrew/rubocops/lines.rb +++ b/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 diff --git a/Library/Homebrew/test/rubocops/lines_spec.rb b/Library/Homebrew/test/rubocops/lines_spec.rb index bb31d2e935..9c92e47850 100644 --- a/Library/Homebrew/test/rubocops/lines_spec.rb +++ b/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 }