Merge pull request #7909 from Rylan12/audit-rubocop-migration

Audit to RuboCop migration
This commit is contained in:
Rylan Polster 2020-07-10 15:37:44 -04:00 committed by GitHub
commit e9932a601f
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 370 additions and 168 deletions

View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -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'

View File

@ -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"

View File

@ -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

View File

@ -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