admin/brew
1
0
Fork 0
Code Issues Packages Projects Releases 6 Wiki Activity

Merge pull request #3313 from GauthamGoli/audit_line_rubocop_part_4_rebase_attempt_1

Browse Source 
audit: Port line_problems to rubocop and add tests part 4
This commit is contained in:
Mike McQuaid 2017-10-24 10:36:31 +01:00 committed by GitHub
commit 7f46dcfa35
5 changed files with 1291 additions and 312 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
Library/.rubocop.yml
View File

@ -123,6 +123,10 @@ Style/Documentation:
Style/Encoding:
Enabled: true
# use spaces for indentation; detect tabs
Layout/Tab:
Enabled: true
# dashes in filenames are typical
Naming/FileName:
Regex: !ruby/regexp /^[\w\@\-\+\.]+(\.rb)?$/

168
Library/Homebrew/dev-cmd/audit.rb
View File

@ -820,168 +820,12 @@ class FormulaAuditor
problem "\"(#{Regexp.last_match(1)}...#{Regexp.last_match(2)})\" should be \"(#{Regexp.last_match(3).downcase}+...)\""
end
if line =~ /((man)\s*\+\s*(['"])(man[1-8])(['"]))/
problem "\"#{Regexp.last_match(1)}\" should be \"#{Regexp.last_match(4)}\""
end
# Prefer formula path shortcuts in strings
if line =~ %r[(\#\{prefix\}/(bin|include|libexec|lib|sbin|share|Frameworks))]
problem "\"#{Regexp.last_match(1)}\" should be \"\#{#{Regexp.last_match(2).downcase}}\""
end
if line =~ %r[((\#\{prefix\}/share/man/|\#\{man\}/)(man[1-8]))]
problem "\"#{Regexp.last_match(1)}\" should be \"\#{#{Regexp.last_match(3)}}\""
end
if line =~ %r[((\#\{share\}/(man)))[/'"]]
problem "\"#{Regexp.last_match(1)}\" should be \"\#{#{Regexp.last_match(3)}}\""
end
if line =~ %r[(\#\{prefix\}/share/(info|man))]
problem "\"#{Regexp.last_match(1)}\" should be \"\#{#{Regexp.last_match(2)}}\""
end
if line =~ /depends_on\s+['"](.+)['"]\s+=>\s+:(lua|perl|python|ruby)(\d*)/
problem "#{Regexp.last_match(2)} modules should be vendored rather than use deprecated `depends_on \"#{Regexp.last_match(1)}\" => :#{Regexp.last_match(2)}#{Regexp.last_match(3)}`"
end
if line =~ /depends_on\s+['"](.+)['"]\s+=>\s+(.*)/
dep = Regexp.last_match(1)
Regexp.last_match(2).split(" ").map do |o|
break if ["if", "unless"].include?(o)
next unless o =~ /^\[?['"](.*)['"]/
problem "Dependency #{dep} should not use option #{Regexp.last_match(1)}"
end
end
if line =~ /if\s+ARGV\.include\?\s+'--(HEAD|devel)'/
problem "Use \"if build.#{Regexp.last_match(1).downcase}?\" instead"
end
problem "Use separate make calls" if line.include?("make && make")
problem "Use spaces instead of tabs for indentation" if line =~ /^[ ]*\t/
if line.include?("ENV.java_cache")
problem "In-formula ENV.java_cache usage has been deprecated & should be removed."
end
# Avoid hard-coding compilers
if line =~ %r{(system|ENV\[.+\]\s?=)\s?['"](/usr/bin/)?(gcc|llvm-gcc|clang)['" ]}
problem "Use \"\#{ENV.cc}\" instead of hard-coding \"#{Regexp.last_match(3)}\""
end
if line =~ %r{(system|ENV\[.+\]\s?=)\s?['"](/usr/bin/)?((g|llvm-g|clang)\+\+)['" ]}
problem "Use \"\#{ENV.cxx}\" instead of hard-coding \"#{Regexp.last_match(3)}\""
end
if line =~ /system\s+['"](env|export)(\s+|['"])/
problem "Use ENV instead of invoking '#{Regexp.last_match(1)}' to modify the environment"
end
if line =~ /version == ['"]HEAD['"]/
problem "Use 'build.head?' instead of inspecting 'version'"
end
if line =~ /build\.include\?[\s\(]+['"]\-\-(.*)['"]/
problem "Reference '#{Regexp.last_match(1)}' without dashes"
end
if line =~ /build\.include\?[\s\(]+['"]with(out)?-(.*)['"]/
problem "Use build.with#{Regexp.last_match(1)}? \"#{Regexp.last_match(2)}\" instead of build.include? 'with#{Regexp.last_match(1)}-#{Regexp.last_match(2)}'"
end
if line =~ /build\.with\?[\s\(]+['"]-?-?with-(.*)['"]/
problem "Don't duplicate 'with': Use `build.with? \"#{Regexp.last_match(1)}\"` to check for \"--with-#{Regexp.last_match(1)}\""
end
if line =~ /build\.without\?[\s\(]+['"]-?-?without-(.*)['"]/
problem "Don't duplicate 'without': Use `build.without? \"#{Regexp.last_match(1)}\"` to check for \"--without-#{Regexp.last_match(1)}\""
end
if line =~ /unless build\.with\?(.*)/
problem "Use if build.without?#{Regexp.last_match(1)} instead of unless build.with?#{Regexp.last_match(1)}"
end
if line =~ /unless build\.without\?(.*)/
problem "Use if build.with?#{Regexp.last_match(1)} instead of unless build.without?#{Regexp.last_match(1)}"
end
if line =~ /(not\s|!)\s*build\.with?\?/
problem "Don't negate 'build.with?': use 'build.without?'"
end
if line =~ /(not\s|!)\s*build\.without?\?/
problem "Don't negate 'build.without?': use 'build.with?'"
end
if line =~ /ARGV\.(?!(debug\?|verbose\?|value[\(\s]))/
problem "Use build instead of ARGV to check options"
end
if line.include?("MACOS_VERSION")
problem "Use MacOS.version instead of MACOS_VERSION"
end
if line.include?("MACOS_FULL_VERSION")
problem "Use MacOS.full_version instead of MACOS_FULL_VERSION"
end
cats = %w[leopard snow_leopard lion mountain_lion].join("|")
if line =~ /MacOS\.(?:#{cats})\?/
problem "\"#{$&}\" is deprecated, use a comparison to MacOS.version instead"
end
if line =~ /depends_on [A-Z][\w:]+\.new$/
problem "`depends_on` can take requirement classes instead of instances"
end
if line =~ /^def (\w+).*$/
problem "Define method #{Regexp.last_match(1).inspect} in the class body, not at the top-level"
end
if line.include?("ENV.fortran") && !formula.requirements.map(&:class).include?(FortranRequirement)
problem "Use `depends_on :fortran` instead of `ENV.fortran`"
end
if line =~ /JAVA_HOME/i && !formula.requirements.map(&:class).include?(JavaRequirement)
problem "Use `depends_on :java` to set JAVA_HOME"
end
if line =~ /depends_on :(.+) (if.+|unless.+)$/
conditional_dep_problems(Regexp.last_match(1).to_sym, Regexp.last_match(2), $&)
end
if line =~ /depends_on ['"](.+)['"] (if.+|unless.+)$/
conditional_dep_problems(Regexp.last_match(1), Regexp.last_match(2), $&)
end
if line =~ /(Dir\[("[^\*{},]+")\])/
problem "#{Regexp.last_match(1)} is unnecessary; just use #{Regexp.last_match(2)}"
end
if line =~ /system (["'](#{FILEUTILS_METHODS})["' ])/o
system = Regexp.last_match(1)
method = Regexp.last_match(2)
problem "Use the `#{method}` Ruby method instead of `system #{system}`"
end
if line =~ /assert [^!]+\.include?/
problem "Use `assert_match` instead of `assert ...include?`"
end
if line =~ /(assert File\.exist\?|assert \(.*\)\.exist\?)/
problem "Use `assert_predicate <path_to_file>, :exist?` instead of `#{Regexp.last_match(1)}`"
end
if line =~ /(assert !File\.exist\?|assert !\(.*\)\.exist\?)/
problem "Use `refute_predicate <path_to_file>, :exist?` instead of `#{Regexp.last_match(1)}`"
end
if line =~ /(assert File\.executable\?|assert \(.*\)\.executable\?)/
problem "Use `assert_predicate <path_to_file>, :executable?` instead of `#{Regexp.last_match(1)}`"
end
return unless @strict
problem "`#{Regexp.last_match(1)}` in formulae is deprecated" if line =~ /(env :(std|userpaths))/
@ -1028,18 +872,6 @@ class FormulaAuditor
EOS
end
def conditional_dep_problems(dep, condition, line)
quoted_dep = quote_dep(dep)
dep = Regexp.escape(dep.to_s)
case condition
when /if build\.include\? ['"]with-#{dep}['"]$/, /if build\.with\? ['"]#{dep}['"]$/
problem %Q(Replace #{line.inspect} with "depends_on #{quoted_dep} => :optional")
when /unless build\.include\? ['"]without-#{dep}['"]$/, /unless build\.without\? ['"]#{dep}['"]$/
problem %Q(Replace #{line.inspect} with "depends_on #{quoted_dep} => :recommended")
end
end
def quote_dep(dep)
dep.is_a?(Symbol) ? dep.inspect : "'#{dep}'"
end

39
Library/Homebrew/rubocops/extend/formula_cop.rb
View File

@ -56,6 +56,7 @@ module RuboCop
# Returns all string nodes among the descendants of given node
def find_strings(node)
return [] if node.nil?
return node if node.str_type?
node.each_descendant(:str)
end
@ -101,7 +102,9 @@ module RuboCop
def find_method_with_args(node, method_name, *args)
methods = find_every_method_call_by_name(node, method_name)
methods.each do |method|
yield method if parameters_passed?(method, *args)
next unless parameters_passed?(method, *args)
return true unless block_given?
yield method
end
end
@ -119,6 +122,7 @@ module RuboCop
method.receiver.method_name != instance
@offense_source_range = method.source_range
@offensive_node = method
return true unless block_given?
yield method
end
end
@ -167,6 +171,20 @@ module RuboCop
type_match && name_match
end
# Find CONSTANTs in the source
# if block given, yield matching nodes
def find_const(node, const_name)
return if node.nil?
node.each_descendant(:const) do |const_node|
next unless const_node.const_name == const_name
@offensive_node = const_node
@offense_source_range = const_node.source_range
yield const_node if block_given?
return true
end
nil
end
def_node_search :required_dependency?, <<~EOS
(send nil :depends_on ({str sym} _))
EOS
@ -222,15 +240,17 @@ module RuboCop
end
# Returns a method definition node with method_name
def find_method_def(node, method_name)
# Returns first method def if method_name is nil
def find_method_def(node, method_name = nil)
return if node.nil?
node.each_child_node(:def) do |def_node|
def_method_name = method_name(def_node)
next unless method_name == def_method_name
next unless method_name == def_method_name || method_name.nil?
@offensive_node = def_node
@offense_source_range = def_node.source_range
return def_node
end
return if node.parent.nil?
# If not found then, parent node becomes the offensive node
@offensive_node = node.parent
@offense_source_range = node.parent.source_range
@ -250,11 +270,15 @@ module RuboCop
end
# Check if method_name is called among the direct children nodes in the given node
# Check if the node itself is the method
def method_called?(node, method_name)
if node.send_type? && node.method_name == method_name
offending_node(node)
return true
end
node.each_child_node(:send) do |call_node|
next unless call_node.method_name == method_name
@offensive_node = call_node
@offense_source_range = call_node.source_range
offending_node(call_node)
return true
end
false
@ -291,6 +315,11 @@ module RuboCop
true
end
# Check if negation is present in the given node
def negated?(node)
method_called?(node, :!)
end
# Return all the caveats' string nodes in an array
def caveats_strings
find_strings(find_method_def(@body, :caveats))

259
Library/Homebrew/rubocops/lines_cop.rb
View File

@ -21,7 +21,7 @@ module RuboCop
begin_pos = start_column(parent_class_node)
end_pos = end_column(class_node)
return unless begin_pos-end_pos != 3
problem "Use a space in class inheritance: class #{class_name(class_node)} < #{class_name(parent_class_node)}"
problem "Use a space in class inheritance: class #{@formula_name.capitalize} < #{class_name(parent_class_node)}"
end
end
@ -53,6 +53,87 @@ module RuboCop
end
end
class AssertStatements < FormulaCop
def audit_formula(_node, _class_node, _parent_class_node, body_node)
find_every_method_call_by_name(body_node, :assert).each do |method|
if method_called_ever?(method, :include?) && !method_called_ever?(method, :!)
problem "Use `assert_match` instead of `assert ...include?`"
end
if method_called_ever?(method, :exist?) && !method_called_ever?(method, :!)
problem "Use `assert_predicate <path_to_file>, :exist?` instead of `#{method.source}`"
end
if method_called_ever?(method, :exist?) && method_called_ever?(method, :!)
problem "Use `refute_predicate <path_to_file>, :exist?` instead of `#{method.source}`"
end
if method_called_ever?(method, :executable?) && !method_called_ever?(method, :!)
problem "Use `assert_predicate <path_to_file>, :executable?` instead of `#{method.source}`"
end
end
end
end
class OptionDeclarations < FormulaCop
def audit_formula(_node, _class_node, _parent_class_node, body_node)
if find_method_def(body_node, :options)
problem "Use new-style option definitions"
end
find_instance_method_call(body_node, :build, :without?) do |method|
next unless unless_modifier?(method.parent)
correct = method.source.gsub("out?", "?")
problem "Use if #{correct} instead of unless #{method.source}"
end
find_instance_method_call(body_node, :build, :with?) do |method|
next unless unless_modifier?(method.parent)
correct = method.source.gsub("?", "out?")
problem "Use if #{correct} instead of unless #{method.source}"
end
find_instance_method_call(body_node, :build, :with?) do |method|
next unless negated?(method.parent)
problem "Don't negate 'build.with?': use 'build.without?'"
end
find_instance_method_call(body_node, :build, :without?) do |method|
next unless negated?(method.parent)
problem "Don't negate 'build.without?': use 'build.with?'"
end
find_instance_method_call(body_node, :build, :without?) do |method|
arg = parameters(method).first
next unless match = regex_match_group(arg, /-?-?without-(.*)/)
problem "Don't duplicate 'without': Use `build.without? \"#{match[1]}\"` to check for \"--without-#{match[1]}\""
end
find_instance_method_call(body_node, :build, :with?) do |method|
arg = parameters(method).first
next unless match = regex_match_group(arg, /-?-?with-(.*)/)
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
end
def unless_modifier?(node)
return false unless node.if_type?
node.modifier_form? && node.unless?
end
end
class Miscellaneous < FormulaCop
def audit_formula(_node, _class_node, _parent_class_node, body_node)
# FileUtils is included in Formula
@ -81,18 +162,128 @@ module RuboCop
end
end
[:debug?, :verbose?, :value].each do |method_name|
find_instance_method_call(body_node, "ARGV", method_name) do
problem "Use build instead of ARGV to check options"
end
end
find_instance_method_call(body_node, :man, :+) do |method|
next unless match = regex_match_group(parameters(method).first, /man[1-8]/)
problem "\"#{method.source}\" should be \"#{match[0]}\""
end
# Avoid hard-coding compilers
find_every_method_call_by_name(body_node, :system).each do |method|
param = parameters(method).first
if match = regex_match_group(param, %r{(/usr/bin/)?(gcc|llvm-gcc|clang)\s?})
problem "Use \"\#{ENV.cc}\" instead of hard-coding \"#{match[2]}\""
elsif match = regex_match_group(param, %r{(/usr/bin/)?((g|llvm-g|clang)\+\+)\s?})
problem "Use \"\#{ENV.cxx}\" instead of hard-coding \"#{match[2]}\""
end
end
find_instance_method_call(body_node, "ENV", :[]=) do |method|
param = parameters(method)[1]
if match = regex_match_group(param, %r{(/usr/bin/)?(gcc|llvm-gcc|clang)\s?})
problem "Use \"\#{ENV.cc}\" instead of hard-coding \"#{match[2]}\""
elsif match = regex_match_group(param, %r{(/usr/bin/)?((g|llvm-g|clang)\+\+)\s?})
problem "Use \"\#{ENV.cxx}\" instead of hard-coding \"#{match[2]}\""
end
end
# Prefer formula path shortcuts in strings
formula_path_strings(body_node, :share) do |p|
next unless match = regex_match_group(p, %r{(/(man))/?})
problem "\"\#{share}#{match[1]}\" should be \"\#{#{match[2]}}\""
end
formula_path_strings(body_node, :prefix) do |p|
if match = regex_match_group(p, %r{(/share/(info|man))$})
problem "\"\#\{prefix}#{match[1]}\" should be \"\#{#{match[2]}}\""
end
if match = regex_match_group(p, %r{((/share/man/)(man[1-8]))})
problem "\"\#\{prefix}#{match[1]}\" should be \"\#{#{match[3]}}\""
end
if match = regex_match_group(p, %r{(/(bin|include|libexec|lib|sbin|share|Frameworks))}i)
problem "\"\#\{prefix}#{match[1]}\" should be \"\#{#{match[2].downcase}}\""
end
end
find_every_method_call_by_name(body_node, :depends_on).each do |method|
key, value = destructure_hash(parameters(method).first)
next if key.nil? || value.nil?
next unless match = regex_match_group(value, /(lua|perl|python|ruby)(\d*)/)
problem "#{match[1]} modules should be vendored rather than use deprecated #{method.source}`"
end
find_every_method_call_by_name(body_node, :system).each do |method|
next unless match = regex_match_group(parameters(method).first, /(env|export)(\s+)?/)
problem "Use ENV instead of invoking '#{match[1]}' to modify the environment"
end
find_every_method_call_by_name(body_node, :depends_on).each do |method|
next if modifier?(method.parent)
param = parameters(method).first
dep, option = hash_dep(param)
next if dep.nil? || option.nil?
offending_node(param)
problem "Dependency #{string_content(dep)} should not use option #{string_content(option)}"
end
find_instance_method_call(body_node, :version, :==) do |method|
next unless parameters_passed?(method, "HEAD")
problem "Use 'build.head?' instead of inspecting 'version'"
end
find_instance_method_call(body_node, "ENV", :fortran) do
next if depends_on?(:fortran)
problem "Use `depends_on :fortran` instead of `ENV.fortran`"
end
find_instance_method_call(body_node, "ARGV", :include?) do |method|
param = parameters(method).first
next unless match = regex_match_group(param, /--(HEAD|devel)/)
problem "Use \"if build.#{match[1].downcase}?\" instead"
end
find_const(body_node, "MACOS_VERSION") do
problem "Use MacOS.version instead of MACOS_VERSION"
end
find_const(body_node, "MACOS_FULL_VERSION") do
problem "Use MacOS.full_version instead of MACOS_FULL_VERSION"
end
conditional_dependencies(body_node) do |node, method, param, dep_node|
dep = string_content(dep_node)
if node.if?
if (method == :include? && regex_match_group(param, /with-#{dep}$/)) ||
(method == :with? && regex_match_group(param, /#{dep}$/))
offending_node(dep_node.parent)
problem "Replace #{node.source} with #{dep_node.parent.source} => :optional"
end
elsif node.unless?
if (method == :include? && regex_match_group(param, /without-#{dep}$/)) ||
(method == :without? && regex_match_group(param, /#{dep}$/))
offending_node(dep_node.parent)
problem "Replace #{node.source} with #{dep_node.parent.source} => :recommended"
end
end
end
find_method_with_args(body_node, :fails_with, :llvm) do
problem "'fails_with :llvm' is now a no-op so should be removed"
end
find_method_with_args(body_node, :system, /^(otool|install_name_tool|lipo)$/) do
find_method_with_args(body_node, :system, /(otool|install_name_tool|lipo)/) do
next if @formula_name == "cctools"
problem "Use ruby-macho instead of calling #{@offensive_node.source}"
end
find_every_method_call_by_name(body_node, :system).each do |method_node|
# Skip Kibana: npm cache edge (see formula for more details)
next if @formula_name =~ /^kibana(\@\d+(\.\d+)?)?$/
next if @formula_name =~ /^kibana(\@\d+(\.\d+)?)?$/i
first_param, second_param = parameters(method_node)
next if !node_equals?(first_param, "npm") ||
!node_equals?(second_param, "install")
@ -104,10 +295,6 @@ module RuboCop
problem "Use new-style test definitions (test do)"
end
if find_method_def(body_node, :options)
problem "Use new-style option definitions"
end
find_method_with_args(body_node, :skip_clean, :all) do
problem <<~EOS.chomp
`skip_clean :all` is deprecated; brew no longer strips symbols
@ -115,6 +302,10 @@ module RuboCop
EOS
end
if find_method_def(@processed_source.ast)
problem "Define method #{method_name(@offensive_node)} in the class body, not at the top-level"
end
find_instance_method_call(body_node, :build, :universal?) do
next if @formula_name == "wine"
problem "macOS has been 64-bit only since 10.6 so build.universal? is deprecated."
@ -128,8 +319,62 @@ module RuboCop
find_instance_method_call(body_node, "ENV", :x11) do
problem 'Use "depends_on :x11" instead of "ENV.x11"'
end
find_every_method_call_by_name(body_node, :depends_on).each do |method|
next unless method_called?(method, :new)
problem "`depends_on` can take requirement classes instead of instances"
end
os = [:leopard?, :snow_leopard?, :lion?, :mountain_lion?]
os.each do |version|
find_instance_method_call(body_node, "MacOS", version) do |method|
problem "\"#{method.source}\" is deprecated, use a comparison to MacOS.version instead"
end
end
find_instance_method_call(body_node, "Dir", :[]) do |method|
path = parameters(method).first
next unless path.str_type?
next unless match = regex_match_group(path, /^[^\*{},]+$/)
problem "Dir([\"#{string_content(path)}\"]) is unnecessary; just use \"#{match[0]}\""
end
fileutils_methods = Regexp.new(FileUtils.singleton_methods(false).map { |m| "(?-mix:^" + Regexp.escape(m) + "$)" }.join("|"))
find_every_method_call_by_name(body_node, :system).each do |method|
param = parameters(method).first
next unless match = regex_match_group(param, fileutils_methods)
problem "Use the `#{match}` Ruby method instead of `#{method.source}`"
end
end
def modifier?(node)
return false unless node.if_type?
node.modifier_form?
end
def_node_search :conditional_dependencies, <<~EOS
{$(if (send (send nil :build) ${:include? :with? :without?} $(str _))
(send nil :depends_on $({str sym} _)) nil)
$(if (send (send nil :build) ${:include? :with? :without?} $(str _)) nil
(send nil :depends_on $({str sym} _)))}
EOS
# Match depends_on with hash as argument
def_node_matcher :hash_dep, <<~EOS
{(hash (pair $(str _) $(str _)))
(hash (pair $(str _) (array $(str _) ...)))}
EOS
def_node_matcher :destructure_hash, <<~EOS
(hash (pair $(str _) $(sym _)))
EOS
def_node_search :formula_path_strings, <<~EOS
{(dstr (begin (send nil %1)) $(str _ ))
(dstr _ (begin (send nil %1)) $(str _ ))}
EOS
# Node Pattern search for Language::Node
def_node_search :languageNodeModule?, <<~EOS
(const (const nil :Language) :Node)

1133
Library/Homebrew/test/rubocops/lines_cop_spec.rb
View File

File diff suppressed because it is too large Load Diff