diff --git a/Library/.rubocop.yml b/Library/.rubocop.yml index cb065a1a4b..a2e3cc9c95 100644 --- a/Library/.rubocop.yml +++ b/Library/.rubocop.yml @@ -118,6 +118,10 @@ Style/Documentation: Style/Encoding: Enabled: true +# use spaces for indentation; detect tabs +Style/Tab: + Enabled: true + # dashes in filenames are typical Style/FileName: Regex: !ruby/regexp /^[\w\@\-\+\.]+(\.rb)?$/ diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index db99656cb1..4195cdf2b1 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -849,23 +849,8 @@ class FormulaAuditor 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/ - # 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)}\"" @@ -879,74 +864,14 @@ class FormulaAuditor 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 - - 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 - return unless @strict problem "`#{Regexp.last_match(1)}` in formulae is deprecated" if line =~ /(env :(std|userpaths))/ @@ -993,18 +918,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 diff --git a/Library/Homebrew/rubocops/extend/formula_cop.rb b/Library/Homebrew/rubocops/extend/formula_cop.rb index 991551585d..08c9e6eb84 100644 --- a/Library/Homebrew/rubocops/extend/formula_cop.rb +++ b/Library/Homebrew/rubocops/extend/formula_cop.rb @@ -55,6 +55,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 @@ -100,7 +101,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 @@ -114,6 +117,7 @@ module RuboCop next unless method.receiver && (method.receiver.const_name == instance || method.receiver.method_name == instance) @offense_source_range = method.source_range @offensive_node = method + return true unless block_given? yield method end end @@ -165,6 +169,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_child_node(:const) do |const_node| + next if 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 + # To compare node with appropriate Ruby variable def node_equals?(node, var) node == Parser::CurrentRuby.parse(var.inspect) @@ -204,11 +222,12 @@ 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 diff --git a/Library/Homebrew/rubocops/lines_cop.rb b/Library/Homebrew/rubocops/lines_cop.rb index 338a3256ab..4c357f6ab3 100644 --- a/Library/Homebrew/rubocops/lines_cop.rb +++ b/Library/Homebrew/rubocops/lines_cop.rb @@ -88,6 +88,59 @@ module RuboCop end end + find_every_method_call_by_name(body_node, :depends_on).each do |m| + next unless modifier?(m) + dep, option = hash_dep(m) + next if dep.nil? || option.nil? + problem "Dependency #{string_content(dep)} should not use option #{string_content(option)}" + end + + find_instance_method_call(body_node, :version, :==) do |m| + next unless parameters_passed?(m, "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 |m| + param = parameters(m).first + next unless match = regex_match_group(param, %r{--(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 + + dependency(body_node) do |m| + # handle symbols and shit: WIP + next unless modifier?(m.parent) + dep = parameters(m).first + condition = m.parent.condition + if (condition.if? && condition.method_name == :include? && parameters_passed(condition, /with-#{string_content(dep)}$/))|| + (condition.if? && condition.method_name == :with? && parameters_passed?(condition, /#{string_content(dep)}$/)) + problem "Replace #{m.parent.source} with #{dep.source} => :optional" + end + if (condition.unless? && condition.method_name == :include? && parameters_passed?(condition, /without-#{string_content(dep)}$/))|| + (condition.unless? && condition.method_name == :without? && parameters_passed?(condition, /#{string_content(dep)}$/)) + problem "Replace #{m.parent.source} with #{dep.source} => :recommended" + end + end + + find_every_method_call_by_name(body_node, :depends_on).each do |m| + next unless modifier?(m.parent) + dep = parameters(m).first + next if dep.hash_type? + condition = m.parent.node_parts + 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 @@ -154,9 +207,81 @@ module RuboCop problem "Use the `#{method}` Ruby method instead of `#{m.source}`" 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, :without?) do |m| + next unless unless_modifier?(m.parent) + correct = m.source.gsub("out?", "?").gsub("unless", "if") + problem "Use #{correct} instead of unless #{m.source}" + end + + find_instance_method_call(body_node, :build, :with?) do |m| + next unless unless_modifier?(m.parent) + correct = m.source.gsub("?", "out?").gsub("unless", "if") + problem "Use #{correct} instead of unless #{m.source}" + end + + find_instance_method_call(body_node, :build, :with?) do |m| + next unless negation?(m) + problem "Don't negate 'build.with?': use 'build.without?'" + end + + find_instance_method_call(body_node, :build, :without?) do |m| + next unless negation?(m) + problem "Don't negate 'build.without?': use 'build.with?'" + end + + find_instance_method_call(body_node, :build, :without?) do |m| + arg = parameters(m).first + next unless match = regex_match_group(arg, %r{-?-?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 |m| + arg = parameters(m).first + next unless match = regex_match_group(arg, %r{-?-?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 |m| + arg = parameters(m).first + next unless match = regex_match_group(arg, %r{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 |m| + arg = parameters(m).first + next unless match = regex_match_group(arg, %r{\-\-(.*)}) + problem "Reference '#{match[1]}' without dashes" + end end + def unless_modifier?(node) + node.modifier_form? && node.unless? + end + + def modifier?(node) + node.modifier_form? + end + + def_node_search :condition, <<-EOS.undent + (send (send nil :build) $_ $({str sym} _)) + EOS + + def_node_search :dependency, <<-EOS.undent + (send nil :depends_on ({str sym} _)) + EOS + + # Match depends_on with hash as argument + def_node_search :hash_dep, <<-EOS.undent + {$(hash (pair $(str _) $(str _))) + $(hash (pair $(str _) (array $(str _) ...)))} + EOS + + def_node_matcher :negation?, '(send ... :!)' # This is Pattern Matching method for AST # Takes the AST node as argument and yields matching node if block given # Else returns boolean for the match