diff --git a/Library/.rubocop.yml b/Library/.rubocop.yml index a782c11172..276e4aabf6 100644 --- a/Library/.rubocop.yml +++ b/Library/.rubocop.yml @@ -14,6 +14,12 @@ Homebrew/CorrectBottleBlock: Homebrew/FormulaDesc: Enabled: true +Homebrew/FormulaComponentsOrder: + Enabled: true + +Homebrew/ComponentsRedundancy: + Enabled: true + Metrics/AbcSize: Enabled: false diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index 8d76c2075d..32130f343b 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -248,70 +248,6 @@ class FormulaAuditor end end - def component_problem(before, after, offset = 0) - problem "`#{before[1]}` (line #{before[0] + offset}) should be put before `#{after[1]}` (line #{after[0] + offset})" - end - - # scan in the reverse direction for remaining problems but report problems - # in the forward direction so that contributors don't reverse the order of - # lines in the file simply by following instructions - def audit_components(reverse = true, previous_pair = nil) - component_list = [ - [/^ include Language::/, "include directive"], - [/^ desc ["'][\S\ ]+["']/, "desc"], - [/^ homepage ["'][\S\ ]+["']/, "homepage"], - [/^ url ["'][\S\ ]+["']/, "url"], - [/^ mirror ["'][\S\ ]+["']/, "mirror"], - [/^ version ["'][\S\ ]+["']/, "version"], - [/^ (sha1|sha256) ["'][\S\ ]+["']/, "checksum"], - [/^ revision/, "revision"], - [/^ version_scheme/, "version_scheme"], - [/^ head ["'][\S\ ]+["']/, "head"], - [/^ stable do/, "stable block"], - [/^ bottle do/, "bottle block"], - [/^ devel do/, "devel block"], - [/^ head do/, "head block"], - [/^ bottle (:unneeded|:disable)/, "bottle modifier"], - [/^ keg_only/, "keg_only"], - [/^ option/, "option"], - [/^ depends_on/, "depends_on"], - [/^ conflicts_with/, "conflicts_with"], - [/^ (go_)?resource/, "resource"], - [/^ def install/, "install method"], - [/^ def caveats/, "caveats method"], - [/^ (plist_options|def plist)/, "plist block"], - [/^ test do/, "test block"], - ] - if previous_pair - previous_before = previous_pair[0] - previous_after = previous_pair[1] - end - offset = previous_after && previous_after[0] && previous_after[0] >= 1 ? previous_after[0] - 1 : 0 - present = component_list.map do |regex, name| - lineno = if reverse - text.reverse_line_number regex - else - text.line_number regex, offset - end - next unless lineno - [lineno, name] - end.compact - no_problem = true - present.each_cons(2) do |c1, c2| - if reverse - # scan in the forward direction from the offset - audit_components(false, [c1, c2]) if c1[0] > c2[0] # at least one more offense - elsif c1[0] > c2[0] && (offset.zero? || previous_pair.nil? || (c1[0] + offset) != previous_before[0] || (c2[0] + offset) != previous_after[0]) - component_problem c1, c2, offset - no_problem = false - end - end - if no_problem && previous_pair - component_problem previous_before, previous_after - end - present - end - def audit_file # Under normal circumstances (umask 0022), we expect a file mode of 644. If # the user's umask is more restrictive, respect that by masking out the @@ -374,26 +310,6 @@ class FormulaAuditor EOS end end - - return unless @strict - - present = audit_components - - present.map!(&:last) - if present.include?("stable block") - %w[url checksum mirror].each do |component| - if present.include?(component) - problem "`#{component}` should be put inside `stable block`" - end - end - end - - if present.include?("head") && present.include?("head block") - problem "Should not have both `head` and `head do`" - end - - return unless present.include?("bottle modifier") && present.include?("bottle block") - problem "Should not have `bottle :unneeded/:disable` and `bottle do`" end def audit_class diff --git a/Library/Homebrew/rubocops.rb b/Library/Homebrew/rubocops.rb index 3625f20049..9dd365ee86 100644 --- a/Library/Homebrew/rubocops.rb +++ b/Library/Homebrew/rubocops.rb @@ -1,2 +1,4 @@ require_relative "./rubocops/bottle_block_cop" require_relative "./rubocops/formula_desc_cop" +require_relative "./rubocops/components_order_cop" +require_relative "./rubocops/components_redundancy_cop" diff --git a/Library/Homebrew/rubocops/bottle_block_cop.rb b/Library/Homebrew/rubocops/bottle_block_cop.rb index 4d7a944616..141d87b358 100644 --- a/Library/Homebrew/rubocops/bottle_block_cop.rb +++ b/Library/Homebrew/rubocops/bottle_block_cop.rb @@ -13,7 +13,7 @@ module RuboCop def audit_formula(_node, _class_node, _parent_class_node, formula_class_body_node) bottle = find_block(formula_class_body_node, :bottle) return if bottle.nil? || block_size(bottle).zero? - problem "Use rebuild instead of revision in bottle block" if method_called?(bottle, :revision) + problem "Use rebuild instead of revision in bottle block" if method_called_in_block?(bottle, :revision) end private diff --git a/Library/Homebrew/rubocops/components_order_cop.rb b/Library/Homebrew/rubocops/components_order_cop.rb new file mode 100644 index 0000000000..c66b5614ea --- /dev/null +++ b/Library/Homebrew/rubocops/components_order_cop.rb @@ -0,0 +1,71 @@ +require_relative "./extend/formula_cop" + +module RuboCop + module Cop + module Homebrew + # This cop checks for correct order of components in a Formula + # + # - component_precedence_list has component hierarchy in a nested list + # where each sub array contains components' details which are at same precedence level + class FormulaComponentsOrder < FormulaCop + def audit_formula(_node, _class_node, _parent_class_node, formula_class_body_node) + component_precedence_list = [ + [{ name: :include, type: :method_call }], + [{ name: :desc, type: :method_call }], + [{ name: :homepage, type: :method_call }], + [{ name: :url, type: :method_call }], + [{ name: :mirror, type: :method_call }], + [{ name: :version, type: :method_call }], + [{ name: :sha256, type: :method_call }], + [{ name: :revision, type: :method_call }], + [{ name: :version_scheme, type: :method_call }], + [{ name: :head, type: :method_call }], + [{ name: :stable, type: :block_call }], + [{ name: :bottle, type: :block_call }], + [{ name: :devel, type: :block_call }], + [{ name: :head, type: :block_call }], + [{ name: :bottle, type: :method_call }], + [{ name: :keg_only, type: :method_call }], + [{ name: :option, type: :method_call }], + [{ name: :depends_on, type: :method_call }], + [{ name: :conflicts_with, type: :method_call }], + [{ name: :go_resource, type: :block_call }, { name: :resource, type: :block_call }], + [{ name: :install, type: :method_definition }], + [{ name: :caveats, type: :method_definition }], + [{ name: :plist_options, type: :method_call }, { name: :plist, type: :method_definition }], + [{ name: :test, type: :block_call }], + ] + + present_components = component_precedence_list.map do |components| + relevant_components = [] + components.each do |component| + case component[:type] + when :method_call + relevant_components += find_method_calls_by_name(formula_class_body_node, component[:name]).to_a + when :block_call + relevant_components += find_blocks(formula_class_body_node, component[:name]).to_a + when :method_definition + relevant_components << find_method_def(formula_class_body_node, component[:name]) + end + end + relevant_components.delete_if(&:nil?) + end + + present_components = present_components.delete_if(&:empty?) + + present_components.each_cons(2) do |preceding_component, succeeding_component| + offensive_nodes = check_precedence(preceding_component, succeeding_component) + component_problem offensive_nodes[0], offensive_nodes[1] if offensive_nodes + end + end + + private + + def component_problem(c1, c2) + # Method to format message for reporting component precedence violations + problem "`#{format_component(c1)}` (line #{line_number(c1)}) should be put before `#{format_component(c2)}` (line #{line_number(c2)})" + end + end + end + end +end diff --git a/Library/Homebrew/rubocops/components_redundancy_cop.rb b/Library/Homebrew/rubocops/components_redundancy_cop.rb new file mode 100644 index 0000000000..3d95d6c053 --- /dev/null +++ b/Library/Homebrew/rubocops/components_redundancy_cop.rb @@ -0,0 +1,33 @@ +require_relative "./extend/formula_cop" + +module RuboCop + module Cop + module Homebrew + # This cop checks if redundant components are present and other component errors + # + # - `url|checksum|mirror` should be inside `stable` block + # - `head` and `head do` should not be simultaneously present + # - `bottle :unneeded/:disable` and `bottle do` should not be simultaneously present + + class ComponentsRedundancy < FormulaCop + HEAD_MSG = "`head` and `head do` should not be simultaneously present".freeze + BOTTLE_MSG = "`bottle :modifier` and `bottle do` should not be simultaneously present".freeze + + def audit_formula(_node, _class_node, _parent_class_node, formula_class_body_node) + stable_block = find_block(formula_class_body_node, :stable) + if stable_block + [:url, :sha256, :mirror].each do |method_name| + problem "`#{method_name}` should be put inside `stable` block" if method_called?(formula_class_body_node, method_name) + end + end + + problem HEAD_MSG if method_called?(formula_class_body_node, :head) && + find_block(formula_class_body_node, :head) + + problem BOTTLE_MSG if method_called?(formula_class_body_node, :bottle) && + find_block(formula_class_body_node, :bottle) + end + end + end + end +end diff --git a/Library/Homebrew/rubocops/extend/formula_cop.rb b/Library/Homebrew/rubocops/extend/formula_cop.rb index 49108bd48a..11e5d93d39 100644 --- a/Library/Homebrew/rubocops/extend/formula_cop.rb +++ b/Library/Homebrew/rubocops/extend/formula_cop.rb @@ -4,8 +4,8 @@ module RuboCop class FormulaCop < Cop @registry = Cop.registry + # This method is called by RuboCop and is the main entry point def on_class(node) - # This method is called by RuboCop and is the main entry point file_path = processed_source.buffer.name return unless file_path_allowed?(file_path) class_node, parent_class_node, body = *node @@ -15,9 +15,9 @@ module RuboCop audit_formula(node, class_node, parent_class_node, body) end + # 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) - # Checks for regex match of pattern in the node and - # Sets the appropriate instance variables to report the match string_repr = string_content(node) match_object = string_repr.match(pattern) return unless match_object @@ -32,8 +32,8 @@ module RuboCop match_object end + # Returns method_node matching method_name def find_node_method_by_name(node, method_name) - # Returns method_node matching method_name return if node.nil? node.each_child_node(:send) do |method_node| next unless method_node.method_name == method_name @@ -47,8 +47,14 @@ module RuboCop nil end + # Returns an array of method call nodes matching method_name inside node + def find_method_calls_by_name(node, method_name) + return if node.nil? + node.each_child_node(:send).select { |method_node| method_name == method_node.method_name } + end + + # Returns a block named block_name inside node def find_block(node, block_name) - # Returns a block named block_name inside node return if node.nil? node.each_child_node(:block) do |block_node| next if block_node.method_name != block_name @@ -62,8 +68,30 @@ module RuboCop nil end - def method_called?(node, method_name) - # Check if a method is called inside a block + # Returns an array of block nodes named block_name inside node + def find_blocks(node, block_name) + return if node.nil? + node.each_child_node(:block).select { |block_node| block_name == block_node.method_name } + end + + # Returns a method definition node with method_name + def find_method_def(node, method_name) + 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 + @offensive_node = def_node + @offense_source_range = def_node.source_range + return def_node + end + # If not found then, parent node becomes the offensive node + @offensive_node = node.parent + @offense_source_range = node.parent.source_range + nil + end + + # Check if a method is called inside a block + def method_called_in_block?(node, method_name) block_body = node.children[2] block_body.each_child_node(:send) do |call_node| next unless call_node.method_name == method_name @@ -74,54 +102,96 @@ module RuboCop false end + # Check if method_name is called among the direct children nodes in the given node + def method_called?(node, method_name) + 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 + return true + end + false + end + + # Checks for precedence, returns the first pair of precedence violating nodes + def check_precedence(first_nodes, next_nodes) + next_nodes.each do |each_next_node| + first_nodes.each do |each_first_node| + if component_precedes?(each_first_node, each_next_node) + return [each_first_node, each_next_node] + end + end + end + nil + end + + # If first node does not precede next_node, sets appropriate instance variables for reporting + def component_precedes?(first_node, next_node) + return false if line_number(first_node) < line_number(next_node) + @offense_source_range = first_node.source_range + @offensive_node = first_node + true + end + + # Returns the array of arguments of the method_node def parameters(method_node) - # Returns the array of arguments of the method_node return unless method_node.send_type? method_node.method_args end + # Returns the begin position of the node's line in source code def line_start_column(node) - # Returns the begin position of the node's line in source code node.source_range.source_buffer.line_range(node.loc.line).begin_pos end + # Returns the begin position of the node in source code def start_column(node) - # Returns the begin position of the node in source code node.source_range.begin_pos end + # Returns the line number of the node def line_number(node) - # Returns the line number of the node node.loc.line end + # Returns the class node's name, nil if not a class node def class_name(node) - # Returns the class node's name, nil if not a class node @offensive_node = node @offense_source_range = node.source_range node.const_name end + # Returns the method name for a def node + def method_name(node) + node.children[0] if node.def_type? + end + + # Returns the node size in the source code def size(node) - # Returns the node size in the source code node.source_range.size end + # Returns the block length of the block node def block_size(block) - # Returns the block length of the block node block_length(block) end + # Source buffer is required as an argument to report style violations def source_buffer(node) - # Source buffer is required as an argument to report style violations node.source_range.source_buffer end + # Returns the string representation if node is of type str def string_content(node) - # Returns the string representation if node is of type str node.str_content if node.type == :str end + # Returns printable component name + def format_component(component_node) + return component_node.method_name if component_node.send_type? || component_node.block_type? + method_name(component_node) if component_node.def_type? + end + def problem(msg) add_offense(@offensive_node, @offense_source_range, msg) end diff --git a/Library/Homebrew/test/dev-cmd/audit_spec.rb b/Library/Homebrew/test/dev-cmd/audit_spec.rb index d07d9d72b7..9afb7954ae 100644 --- a/Library/Homebrew/test/dev-cmd/audit_spec.rb +++ b/Library/Homebrew/test/dev-cmd/audit_spec.rb @@ -148,100 +148,6 @@ describe FormulaAuditor do fa.audit_file expect(fa.problems).to eq([]) end - - specify "strict: ordering issue" do - fa = formula_auditor "foo", <<-EOS.undent, strict: true - class Foo < Formula - url "http://example.com/foo-1.0.tgz" - homepage "http://example.com" - end - EOS - - fa.audit_file - expect(fa.problems) - .to eq(["`homepage` (line 3) should be put before `url` (line 2)"]) - end - - specify "strict: resource placement" do - fa = formula_auditor "foo", <<-EOS.undent, strict: true - class Foo < Formula - url "https://example.com/foo-1.0.tgz" - - resource "foo2" do - url "https://example.com/foo-2.0.tgz" - end - - depends_on "openssl" - end - EOS - - fa.audit_file - expect(fa.problems) - .to eq(["`depends_on` (line 8) should be put before `resource` (line 4)"]) - end - - specify "strict: plist placement" do - fa = formula_auditor "foo", <<-EOS.undent, strict: true - class Foo < Formula - url "https://example.com/foo-1.0.tgz" - - test do - expect(shell_output("./dogs")).to match("Dogs are terrific") - end - - def plist - end - end - EOS - - fa.audit_file - expect(fa.problems) - .to eq(["`plist block` (line 8) should be put before `test block` (line 4)"]) - end - - specify "strict: url outside of stable block" do - fa = formula_auditor "foo", <<-EOS.undent, strict: true - class Foo < Formula - url "http://example.com/foo-1.0.tgz" - stable do - # stuff - end - end - EOS - - fa.audit_file - expect(fa.problems).to eq(["`url` should be put inside `stable block`"]) - end - - specify "strict: head and head do" do - fa = formula_auditor "foo", <<-EOS.undent, strict: true - class Foo < Formula - head "http://example.com/foo.git" - head do - # stuff - end - end - EOS - - fa.audit_file - expect(fa.problems).to eq(["Should not have both `head` and `head do`"]) - end - - specify "strict: bottle and bottle do" do - fa = formula_auditor "foo", <<-EOS.undent, strict: true - class Foo < Formula - url "http://example.com/foo-1.0.tgz" - bottle do - # bottles go here - end - bottle :unneeded - end - EOS - - fa.audit_file - expect(fa.problems) - .to eq(["Should not have `bottle :unneeded/:disable` and `bottle do`"]) - end end describe "#audit_class" do diff --git a/Library/Homebrew/test/rubocops/components_order_cop_spec.rb b/Library/Homebrew/test/rubocops/components_order_cop_spec.rb new file mode 100644 index 0000000000..a424da8635 --- /dev/null +++ b/Library/Homebrew/test/rubocops/components_order_cop_spec.rb @@ -0,0 +1,116 @@ +require "rubocop" +require "rubocop/rspec/support" +require_relative "../../extend/string" +require_relative "../../rubocops/components_order_cop" + +describe RuboCop::Cop::Homebrew::FormulaComponentsOrder do + subject(:cop) { described_class.new } + + context "When auditing formula components order" do + it "When url precedes homepage" do + source = <<-EOS.undent + class Foo < Formula + url "http://example.com/foo-1.0.tgz" + homepage "http://example.com" + end + EOS + + expected_offenses = [{ message: "`homepage` (line 3) should be put before `url` (line 2)", + severity: :convention, + line: 3, + column: 2, + source: source }] + + inspect_source(cop, source) + + expected_offenses.zip(cop.offenses).each do |expected, actual| + expect_offense(expected, actual) + end + end + + it "When `resource` precedes `depends_on`" do + source = <<-EOS.undent + class Foo < Formula + url "https://example.com/foo-1.0.tgz" + + resource "foo2" do + url "https://example.com/foo-2.0.tgz" + end + + depends_on "openssl" + end + EOS + + expected_offenses = [{ message: "`depends_on` (line 8) should be put before `resource` (line 4)", + severity: :convention, + line: 8, + column: 2, + source: source }] + + inspect_source(cop, source) + + expected_offenses.zip(cop.offenses).each do |expected, actual| + expect_offense(expected, actual) + end + end + + it "When `test` precedes `plist`" do + source = <<-EOS.undent + class Foo < Formula + url "https://example.com/foo-1.0.tgz" + + test do + expect(shell_output("./dogs")).to match("Dogs are terrific") + end + + def plist + end + end + EOS + + expected_offenses = [{ message: "`plist` (line 8) should be put before `test` (line 4)", + severity: :convention, + line: 8, + column: 2, + source: source }] + + inspect_source(cop, source) + + expected_offenses.zip(cop.offenses).each do |expected, actual| + expect_offense(expected, actual) + end + end + + it "When only one of many `depends_on` precedes `conflicts_with`" do + source = <<-EOS.undent + class Foo < Formula + depends_on "autoconf" => :build + conflicts_with "visionmedia-watch" + depends_on "automake" => :build + depends_on "libtool" => :build + depends_on "pkg-config" => :build + depends_on "gettext" + end + EOS + + expected_offenses = [{ message: "`depends_on` (line 4) should be put before `conflicts_with` (line 3)", + severity: :convention, + line: 4, + column: 2, + source: source }] + + inspect_source(cop, source) + + expected_offenses.zip(cop.offenses).each do |expected, actual| + expect_offense(expected, actual) + end + end + + def expect_offense(expected, actual) + expect(actual.message).to eq(expected[:message]) + expect(actual.severity).to eq(expected[:severity]) + expect(actual.line).to eq(expected[:line]) + expect(actual.column).to eq(expected[:column]) + end + end +end diff --git a/Library/Homebrew/test/rubocops/components_redundancy_cop_spec.rb b/Library/Homebrew/test/rubocops/components_redundancy_cop_spec.rb new file mode 100644 index 0000000000..5637330d83 --- /dev/null +++ b/Library/Homebrew/test/rubocops/components_redundancy_cop_spec.rb @@ -0,0 +1,87 @@ +require "rubocop" +require "rubocop/rspec/support" +require_relative "../../extend/string" +require_relative "../../rubocops/components_redundancy_cop" + +describe RuboCop::Cop::Homebrew::ComponentsRedundancy do + subject(:cop) { described_class.new } + + context "When auditing formula components common errors" do + it "When url outside stable block" do + source = <<-EOS.undent + class Foo < Formula + url "http://example.com/foo-1.0.tgz" + stable do + # stuff + end + end + EOS + + expected_offenses = [{ message: "`url` should be put inside `stable` block", + severity: :convention, + line: 2, + column: 2, + source: source }] + + inspect_source(cop, source) + + expected_offenses.zip(cop.offenses).each do |expected, actual| + expect_offense(expected, actual) + end + end + + it "When both `head` and `head do` are present" do + source = <<-EOS.undent + class Foo < Formula + head "http://example.com/foo.git" + head do + # stuff + end + end + EOS + + expected_offenses = [{ message: "`head` and `head do` should not be simultaneously present", + severity: :convention, + line: 3, + column: 2, + source: source }] + + inspect_source(cop, source) + + expected_offenses.zip(cop.offenses).each do |expected, actual| + expect_offense(expected, actual) + end + end + + it "When both `bottle :modifier` and `bottle do` are present" do + source = <<-EOS.undent + class Foo < Formula + url "http://example.com/foo-1.0.tgz" + bottle do + # bottles go here + end + bottle :unneeded + end + EOS + + expected_offenses = [{ message: "`bottle :modifier` and `bottle do` should not be simultaneously present", + severity: :convention, + line: 3, + column: 2, + source: source }] + + inspect_source(cop, source) + + expected_offenses.zip(cop.offenses).each do |expected, actual| + expect_offense(expected, actual) + end + end + + def expect_offense(expected, actual) + expect(actual.message).to eq(expected[:message]) + expect(actual.severity).to eq(expected[:severity]) + expect(actual.line).to eq(expected[:line]) + expect(actual.column).to eq(expected[:column]) + end + end +end