diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index b1f3462b07..81ae3b5026 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -231,11 +231,6 @@ module Homebrew end def audit_file - # TODO: check could be in RuboCop - if text.to_s.match?(/inreplace [^\n]* do [^\n]*\n[^\n]*\.gsub![^\n]*\n\ *end/m) - problem "'inreplace ... do' was used for a single substitution (use the non-block form instead)." - end - if formula.core_formula? && @versioned_formula unversioned_formula = begin # build this ourselves as we want e.g. homebrew/core to be present @@ -830,73 +825,6 @@ module Homebrew end end - def audit_lines - text.without_patch.split("\n").each_with_index do |line, lineno| - line_problems(line, lineno + 1) - end - end - - def line_problems(line, _lineno) - # Check for string interpolation of single values. - if line =~ /(system|inreplace|gsub!|change_make_var!).*[ ,]"#\{([\w.]+)\}"/ - # TODO: check could be in RuboCop - problem "Don't need to interpolate \"#{Regexp.last_match(2)}\" with #{Regexp.last_match(1)}" - end - - # Check for string concatenation; prefer interpolation - if line =~ /(#\{\w+\s*\+\s*['"][^}]+\})/ - # TODO: check could be in RuboCop - problem "Try not to concatenate paths in string interpolation:\n #{Regexp.last_match(1)}" - end - - # Prefer formula path shortcuts in Pathname+ - if line =~ %r{\(\s*(prefix\s*\+\s*(['"])(bin|include|libexec|lib|sbin|share|Frameworks)[/'"])} - # TODO: check could be in RuboCop - problem( - "\"(#{Regexp.last_match(1)}...#{Regexp.last_match(2)})\" should" \ - " be \"(#{Regexp.last_match(3).downcase}+...)\"", - ) - end - - # TODO: check could be in RuboCop - problem "Use separate make calls" if line.include?("make && make") - - if line =~ /JAVA_HOME/i && - [formula.name, *formula.deps.map(&:name)].none? { |name| name.match?(/^openjdk(@|$)/) } && - formula.requirements.none? { |req| req.is_a?(JavaRequirement) } - # TODO: check could be in RuboCop - problem "Use `depends_on :java` to set JAVA_HOME" - end - - return unless @strict - - # TODO: check could be in RuboCop - problem "`env :userpaths` in formulae is deprecated" if line.include?("env :userpaths") - - # TODO: check could be in RuboCop - problem "`#{Regexp.last_match(1)}` is now unnecessary" if line =~ /(require ["']formula["'])/ - - if line.match?(%r{#\{share\}/#{Regexp.escape(formula.name)}[/'"]}) - # TODO: check could be in RuboCop - problem "Use \#{pkgshare} instead of \#{share}/#{formula.name}" - end - - if !@core_tap && line =~ /depends_on .+ if build\.with(out)?\?\(?["']\w+["']\)?/ - # TODO: check could be in RuboCop - problem "`Use :optional` or `:recommended` instead of `#{Regexp.last_match(0)}`" - end - - if line =~ %r{share(\s*[/+]\s*)(['"])#{Regexp.escape(formula.name)}(?:\2|/)} - # TODO: check could be in RuboCop - problem "Use pkgshare instead of (share#{Regexp.last_match(1)}\"#{formula.name}\")" - end - - return unless @core_tap - - # TODO: check could be in RuboCop - problem "`env :std` in homebrew/core formulae is deprecated" if line.include?("env :std") - end - def audit_reverse_migration # Only enforce for new formula being re-added to core return unless @strict diff --git a/Library/Homebrew/rubocops/extend/formula.rb b/Library/Homebrew/rubocops/extend/formula.rb index 34b8a4a7f1..b7e5bfccb8 100644 --- a/Library/Homebrew/rubocops/extend/formula.rb +++ b/Library/Homebrew/rubocops/extend/formula.rb @@ -35,7 +35,7 @@ module RuboCop # Checks for regex match of pattern in the node and # sets the appropriate instance variables to report the match def regex_match_group(node, pattern) - string_repr = string_content(node) + string_repr = string_content(node).encode("UTF-8", invalid: :replace) match_object = string_repr.match(pattern) return unless match_object diff --git a/Library/Homebrew/rubocops/lines.rb b/Library/Homebrew/rubocops/lines.rb index 93c9cd3ba9..0310b94348 100644 --- a/Library/Homebrew/rubocops/lines.rb +++ b/Library/Homebrew/rubocops/lines.rb @@ -102,6 +102,34 @@ module RuboCop def audit_formula(_node, _class_node, _parent_class_node, body_node) problem "Use new-style option definitions" if find_method_def(body_node, :options) + if formula_tap == "homebrew-core" + # Use of build.with? implies options, which are forbidden in homebrew/core + find_instance_method_call(body_node, :build, :without?) do + problem "Formulae in homebrew/core should not use `build.without?`." + end + find_instance_method_call(body_node, :build, :with?) do + problem "Formulae in homebrew/core should not use `build.with?`." + end + + return + end + + # Matches `depends_on "foo" if build.with?("bar")` or depends_on "foo" if build.without?("bar")` + depends_on_build_regex = /depends_on .+ (if build\.with(out)?\?\(["']\w+["']\))/ + + find_instance_method_call(body_node, :build, :with?) do |n| + # TODO: this should be refactored to a direct method match + next unless match = n.parent.source.match(depends_on_build_regex) + + problem "Use `:optional` or `:recommended` instead of `#{match[1]}`" + end + + find_instance_method_call(body_node, :build, :without?) do |n| + next unless match = n.parent.source.match(depends_on_build_regex) + + problem "Use `:optional` or `:recommended` instead of `#{match[1]}`" + end + find_instance_method_call(body_node, :build, :without?) do |method| next unless unless_modifier?(method.parent) @@ -143,29 +171,8 @@ module RuboCop problem "Don't duplicate 'with': Use `build.with? \"#{match[1]}\"` to check for \"--with-#{match[1]}\"" end - find_instance_method_call(body_node, :build, :include?) do |method| - arg = parameters(method).first - next unless match = regex_match_group(arg, /^with(out)?-(.*)/) - - problem "Use build.with#{match[1]}? \"#{match[2]}\" instead of " \ - "build.include? 'with#{match[1]}-#{match[2]}'" - end - - find_instance_method_call(body_node, :build, :include?) do |method| - arg = parameters(method).first - next unless match = regex_match_group(arg, /^--(.*)$/) - - problem "Reference '#{match[1]}' without dashes" - end - - return if formula_tap != "homebrew-core" - - # Use of build.with? implies options, which are forbidden in homebrew/core - find_instance_method_call(body_node, :build, :without?) do - problem "Formulae in homebrew/core should not use `build.without?`." - end - find_instance_method_call(body_node, :build, :with?) do - problem "Formulae in homebrew/core should not use `build.with?`." + find_instance_method_call(body_node, :build, :include?) do + problem "`build.include?` is deprecated" end end diff --git a/Library/Homebrew/rubocops/text.rb b/Library/Homebrew/rubocops/text.rb index 6938d5f5fc..85f0a89ba4 100644 --- a/Library/Homebrew/rubocops/text.rb +++ b/Library/Homebrew/rubocops/text.rb @@ -6,7 +6,19 @@ module RuboCop module Cop module FormulaAudit class Text < FormulaCop - def audit_formula(_node, _class_node, _parent_class_node, body_node) + def audit_formula(node, _class_node, _parent_class_node, body_node) + @full_source_content = source_buffer(node).source + + if match = @full_source_content.match(/^require ['"]formula['"]$/) + @offensive_node = node + @source_buf = source_buffer(node) + @line_no = match.pre_match.count("\n") + 1 + @column = 0 + @length = match[0].length + @offense_source_range = source_range(@source_buf, @line_no, @column, @length) + problem "`#{match}` is now unnecessary" + end + if !find_node_method_by_name(body_node, :plist_options) && find_method_def(body_node, :plist) problem "Please set plist_options when using a formula-defined plist." @@ -62,6 +74,51 @@ module RuboCop find_method_with_args(body_node, :system, "cargo", "build") do problem "use \"cargo\", \"install\", *std_cargo_args" end + + find_every_method_call_by_name(body_node, :system).each do |m| + next unless parameters_passed?(m, /make && make/) + + offending_node(m) + problem "Use separate `make` calls" + end + + body_node.each_descendant(:dstr) do |dstr_node| + dstr_node.each_descendant(:begin) do |interpolation_node| + next unless interpolation_node.source.match?(/#\{\w+\s*\+\s*['"][^}]+\}/) + + offending_node(interpolation_node) + problem "Do not concatenate paths in string interpolation" + end + end + + find_strings(body_node).each do |n| + next unless regex_match_group(n, /JAVA_HOME/i) + + next if @formula_name.match?(/^openjdk(@|$)/) + + next if find_every_method_call_by_name(body_node, :depends_on).any? do |dependency| + dependency.each_descendant(:str).count.zero? || + regex_match_group(dependency.each_descendant(:str).first, /^openjdk(@|$)/) || + depends_on?(:java) + end + + offending_node(n) + problem "Use `depends_on :java` to set JAVA_HOME" + end + + find_strings(body_node).each do |n| + # Skip strings that don't start with one of the keywords + next unless regex_match_group(n, %r{^(bin|include|libexec|lib|sbin|share|Frameworks)/?}) + + parent = n.parent + # Only look at keywords that have `prefix` before them + # TODO: this should be refactored to a direct method match + prefix_keyword_regex = %r{(prefix\s*\+\s*["'](bin|include|libexec|lib|sbin|share|Frameworks))["'/]} + if match = parent.source.match(prefix_keyword_regex) + offending_node(parent) + problem "Use `#{match[2].downcase}` instead of `#{match[1]}\"`" + end + end end end end @@ -72,6 +129,30 @@ module RuboCop find_method_with_args(body_node, :go_resource) do problem "`go_resource`s are deprecated. Please ask upstream to implement Go vendoring" end + + find_method_with_args(body_node, :env, :userpaths) do + problem "`env :userpaths` in homebrew/core formulae is deprecated" + end + + body_node.each_descendant(:dstr) do |dstr_node| + next unless match = dstr_node.source.match(%r{(\#{share}/#{Regexp.escape(@formula_name)})[ /"]}) + + offending_node(dstr_node) + problem "Use `\#{pkgshare}` instead of `#{match[1]}`" + end + + find_every_method_call_by_name(body_node, :share).each do |share_node| + if match = share_node.parent.source.match(%r{(share\s*[/+]\s*"#{Regexp.escape(@formula_name)})[/"]}) + offending_node(share_node.parent) + problem "Use `pkgshare` instead of `#{match[1]}\"`" + end + end + + return unless formula_tap == "homebrew-core" + + find_method_with_args(body_node, :env, :std) do + problem "`env :std` in homebrew/core formulae is deprecated" + end end end end diff --git a/Library/Homebrew/test/.rubocop_todo.yml b/Library/Homebrew/test/.rubocop_todo.yml index 18986235db..59e8c081af 100644 --- a/Library/Homebrew/test/.rubocop_todo.yml +++ b/Library/Homebrew/test/.rubocop_todo.yml @@ -21,7 +21,7 @@ RSpec/InstanceVariable: - 'utils/git_spec.rb' - 'version_spec.rb' -# Offense count: 74 +# Offense count: 75 RSpec/MultipleDescribes: Exclude: - 'ENV_spec.rb' @@ -94,6 +94,7 @@ RSpec/MultipleDescribes: - 'rubocops/class_spec.rb' - 'rubocops/formula_desc_spec.rb' - 'rubocops/lines_spec.rb' + - 'rubocops/text_spec.rb' - 'rubocops/urls_spec.rb' - 'software_spec_spec.rb' - 'tap_spec.rb' diff --git a/Library/Homebrew/test/dev-cmd/audit_spec.rb b/Library/Homebrew/test/dev-cmd/audit_spec.rb index a7b770592f..75a5fa8af8 100644 --- a/Library/Homebrew/test/dev-cmd/audit_spec.rb +++ b/Library/Homebrew/test/dev-cmd/audit_spec.rb @@ -179,60 +179,6 @@ module Homebrew end end - # Intentionally outputted non-interpolated strings - # rubocop:disable Lint/InterpolationCheck - describe "#line_problems" do - specify "pkgshare" do - fa = formula_auditor "foo", <<~RUBY, strict: true - class Foo < Formula - url "https://brew.sh/foo-1.0.tgz" - end - RUBY - - fa.line_problems 'ohai "#{share}/foo"', 3 - expect(fa.problems.shift).to eq("Use \#{pkgshare} instead of \#{share}/foo") - - fa.line_problems 'ohai "#{share}/foo/bar"', 3 - expect(fa.problems.shift).to eq("Use \#{pkgshare} instead of \#{share}/foo") - - fa.line_problems 'ohai share/"foo"', 3 - expect(fa.problems.shift).to eq('Use pkgshare instead of (share/"foo")') - - fa.line_problems 'ohai share/"foo/bar"', 3 - expect(fa.problems.shift).to eq('Use pkgshare instead of (share/"foo")') - - fa.line_problems 'ohai "#{share}/foo-bar"', 3 - expect(fa.problems).to eq([]) - - fa.line_problems 'ohai share/"foo-bar"', 3 - expect(fa.problems).to eq([]) - - fa.line_problems 'ohai share/"bar"', 3 - expect(fa.problems).to eq([]) - end - - # Regression test for https://github.com/Homebrew/legacy-homebrew/pull/48744 - # Formulae with "++" in their name would break various audit regexps: - # Error: nested *?+ in regexp: /^libxml++3\s/ - specify "++ in name" do - fa = formula_auditor "foolibc++", <<~RUBY, strict: true - class Foolibcxx < Formula - desc "foolibc++ is a test" - url "https://brew.sh/foo-1.0.tgz" - end - RUBY - - fa.line_problems 'ohai "#{share}/foolibc++"', 3 - expect(fa.problems.shift) - .to eq("Use \#{pkgshare} instead of \#{share}/foolibc++") - - fa.line_problems 'ohai share/"foolibc++"', 3 - expect(fa.problems.shift) - .to eq('Use pkgshare instead of (share/"foolibc++")') - end - end - # rubocop:enable Lint/InterpolationCheck - describe "#audit_github_repository" do specify "#audit_github_repository when HOMEBREW_NO_GITHUB_API is set" do ENV["HOMEBREW_NO_GITHUB_API"] = "1" diff --git a/Library/Homebrew/test/rubocops/lines_spec.rb b/Library/Homebrew/test/rubocops/lines_spec.rb index 95ad56c311..46d24fab94 100644 --- a/Library/Homebrew/test/rubocops/lines_spec.rb +++ b/Library/Homebrew/test/rubocops/lines_spec.rb @@ -201,6 +201,24 @@ describe RuboCop::Cop::FormulaAudit::OptionDeclarations do RUBY end + it "build.without? in dependencies" do + expect_offense(<<~RUBY) + class Foo < Formula + depends_on "bar" if build.without?("baz") + ^^^^^^^^^^^^^^^^^^^^^ Use `:optional` or `:recommended` instead of `if build.without?("baz")` + end + RUBY + end + + it "build.with? in dependencies" do + expect_offense(<<~RUBY) + class Foo < Formula + depends_on "bar" if build.with?("baz") + ^^^^^^^^^^^^^^^^^^ Use `:optional` or `:recommended` instead of `if build.with?("baz")` + end + RUBY + end + it "unless build.without? conditional" do expect_offense(<<~RUBY) class Foo < Formula @@ -279,27 +297,14 @@ describe RuboCop::Cop::FormulaAudit::OptionDeclarations do RUBY end - it "build.include? conditional" do + it "build.include? deprecated" do expect_offense(<<~RUBY) class Foo < Formula desc "foo" url 'https://brew.sh/foo-1.0.tgz' def post_install - return if build.include? "without-bar" - ^^^^^^^^^^^ Use build.without? \"bar\" instead of build.include? 'without-bar' - end - end - RUBY - end - - it "build.include? with dashed args conditional" do - expect_offense(<<~RUBY) - class Foo < Formula - desc "foo" - url 'https://brew.sh/foo-1.0.tgz' - def post_install - return if build.include? "--bar" - ^^^^^ Reference 'bar' without dashes + return if build.include? "foo" + ^^^^^^^^^^^^^^^^^^^^ `build.include?` is deprecated end end RUBY diff --git a/Library/Homebrew/test/rubocops/text_spec.rb b/Library/Homebrew/test/rubocops/text_spec.rb index 38204d4ef6..4ccb551145 100644 --- a/Library/Homebrew/test/rubocops/text_spec.rb +++ b/Library/Homebrew/test/rubocops/text_spec.rb @@ -6,6 +6,17 @@ describe RuboCop::Cop::FormulaAudit::Text do subject(:cop) { described_class.new } context "When auditing formula text" do + it "with `require \"formula\"` is present" do + expect_offense(<<~RUBY) + require "formula" + ^^^^^^^^^^^^^^^^^ `require "formula"` is now unnecessary + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + homepage "https://brew.sh" + end + RUBY + end + it "with both openssl and libressl optional dependencies" do expect_offense(<<~RUBY) class Foo < Formula @@ -215,5 +226,228 @@ describe RuboCop::Cop::FormulaAudit::Text do end RUBY end + + it "When make calls are not separated" do + expect_offense(<<~RUBY) + class Foo < Formula + def install + system "make && make install" + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use separate `make` calls + end + end + RUBY + end + + it "When concatenating in string interpolation" do + expect_offense(<<~RUBY) + class Foo < Formula + def install + ohai "foo \#{bar + "baz"}" + ^^^^^^^^^^^^^^ Do not concatenate paths in string interpolation + end + end + RUBY + end + + it "When using JAVA_HOME without a java dependency" do + expect_offense(<<~RUBY) + class Foo < Formula + def install + ohai "JAVA_HOME" + ^^^^^^^^^^^ Use `depends_on :java` to set JAVA_HOME + end + end + RUBY + end + + it "When using JAVA_HOME with an openjdk dependency" do + expect_no_offenses(<<~RUBY) + class Foo < Formula + depends_on "openjdk" + def install + ohai "JAVA_HOME" + end + end + RUBY + end + + it "When using JAVA_HOME with an openjdk build dependency" do + expect_no_offenses(<<~RUBY) + class Foo < Formula + depends_on "openjdk" => :build + def install + ohai "JAVA_HOME" + end + end + RUBY + end + + it "When using JAVA_HOME with a java dependency" do + expect_no_offenses(<<~RUBY) + class Foo < Formula + depends_on :java + def install + ohai "JAVA_HOME" + end + end + RUBY + end + + it "When using JAVA_HOME with a java build dependency" do + expect_no_offenses(<<~RUBY) + class Foo < Formula + depends_on :java => :build + def install + ohai "JAVA_HOME" + end + end + RUBY + end + + it "When using `prefix + \"bin\"` instead of `bin`" do + expect_offense(<<~RUBY) + class Foo < Formula + def install + ohai prefix + "bin" + ^^^^^^^^^^^^^^ Use `bin` instead of `prefix + "bin"` + end + end + RUBY + end + + it "When using `prefix + \"bin/foo\"` instead of `bin`" do + expect_offense(<<~RUBY) + class Foo < Formula + def install + ohai prefix + "bin/foo" + ^^^^^^^^^^^^^^^^^^ Use `bin` instead of `prefix + "bin"` + end + end + RUBY + end + end +end + +describe RuboCop::Cop::FormulaAuditStrict::Text do + subject(:cop) { described_class.new } + + context "When auditing formula text" do + it "when deprecated `env :userpaths` is present" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + + env :userpaths + ^^^^^^^^^^^^^^ `env :userpaths` in homebrew/core formulae is deprecated + end + RUBY + end + + it "when deprecated `env :std` is present in homebrew-core" do + expect_offense(<<~RUBY, "/homebrew-core/") + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + + env :std + ^^^^^^^^ `env :std` in homebrew/core formulae is deprecated + end + RUBY + end + + it "when `\#{share}/foo` is used instead of `\#{pkgshare}`" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + def install + ohai "\#{share}/foo" + ^^^^^^^^^^^^^^ Use `\#{pkgshare}` instead of `\#{share}/foo` + end + end + RUBY + end + + it "when `\#{share}/foo/bar` is used instead of `\#{pkgshare}/bar`" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + def install + ohai "\#{share}/foo/bar" + ^^^^^^^^^^^^^^^^^^ Use `\#{pkgshare}` instead of `\#{share}/foo` + end + end + RUBY + end + + it "when `\#{share}/foolibc++` is used instead of `\#{pkgshare}/foolibc++`" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foolibc++.rb") + class Foo < Formula + def install + ohai "\#{share}/foolibc++" + ^^^^^^^^^^^^^^^^^^^^ Use `\#{pkgshare}` instead of `\#{share}/foolibc++` + end + end + RUBY + end + + it "when `share/\"foo\"` is used instead of `pkgshare`" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + def install + ohai share/"foo" + ^^^^^^^^^^^ Use `pkgshare` instead of `share/"foo"` + end + end + RUBY + end + + it "when `share/\"foo/bar\"` is used instead of `pkgshare`" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + def install + ohai share/"foo/bar" + ^^^^^^^^^^^^^^^ Use `pkgshare` instead of `share/"foo"` + end + end + RUBY + end + + it "when `share/\"foolibc++\"` is used instead of `pkgshare`" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foolibc++.rb") + class Foo < Formula + def install + ohai share/"foolibc++" + ^^^^^^^^^^^^^^^^^ Use `pkgshare` instead of `share/"foolibc++"` + end + end + RUBY + end + + it "when `\#{share}/foo-bar` doesn't match formula name" do + expect_no_offenses(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + def install + ohai "\#{share}/foo-bar" + end + end + RUBY + end + + it "when `share/foo-bar` doesn't match formula name" do + expect_no_offenses(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + def install + ohai share/"foo-bar" + end + end + RUBY + end + + it "when `share/bar` doesn't match formula name" do + expect_no_offenses(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + def install + ohai share/"bar" + end + end + RUBY + end end end