Merge pull request #7436 from iMichka/audit-block

on_os_blocks: add audit

Library/Homebrew/rubocops/components_order.rb

@@ -10,6 +10,11 @@ module RuboCop
       # - `component_precedence_list` has component hierarchy in a nested list
       #   where each sub array contains components' details which are at same precedence level
       class ComponentsOrder < FormulaCop
+        # `aspell`: options and resources should be grouped by language
+        COMPONENT_WHITELIST = %w[
+          aspell
+        ].freeze
+
         def audit_formula(_node, _class_node, _parent_class_node, body_node)
           component_precedence_list = [
             [{ name: :include,   type: :method_call }],
@@ -34,6 +39,8 @@ module RuboCop
             [{ name: :deprecated_option, type: :method_call }],
             [{ name: :depends_on, type: :method_call }],
             [{ name: :uses_from_macos, type: :method_call }],
+            [{ name: :on_macos, type: :block_call }],
+            [{ name: :on_linux, type: :block_call }],
             [{ name: :conflicts_with, type: :method_call }],
             [{ name: :skip_clean, type: :method_call }],
             [{ name: :cxxstdlib_check, type: :method_call }],
@@ -49,50 +56,115 @@ module RuboCop
             [{ name: :test, type: :block_call }],
           ]
 
-          @present_components = component_precedence_list.map do |components|
+          @present_components, @offensive_nodes = check_order(component_precedence_list, body_node)
-            relevant_components = []
+
-            components.each do |component|
+          component_problem @offensive_nodes[0], @offensive_nodes[1] if @offensive_nodes
-              case component[:type]
+
-              when :method_call
+          component_precedence_list = [
-                relevant_components += find_method_calls_by_name(body_node, component[:name]).to_a
+            [{ name: :depends_on, type: :method_call }],
-              when :block_call
+            [{ name: :resource, type: :block_call }],
-                relevant_components += find_blocks(body_node, component[:name]).to_a
+            [{ name: :patch, type: :method_call }, { name: :patch, type: :block_call }],
-              when :method_definition
+          ]
-                relevant_components << find_method_def(body_node, component[:name])
+
-              end
+          on_macos_blocks = find_blocks(body_node, :on_macos)
-            end
+
-            relevant_components.delete_if(&:nil?)
+          if on_macos_blocks.length > 1
+            @offensive_node = on_macos_blocks.second
+            @offense_source_range = on_macos_blocks.second.source_range
+            problem "there can only be one `on_macos` block in a formula."
           end
 
-          # Check if each present_components is above rest of the present_components
+          check_on_os_block_content(component_precedence_list, on_macos_blocks.first) if on_macos_blocks.any?
-          @present_components.take(@present_components.size - 1).each_with_index do |preceding_component, p_idx|
-            next if preceding_component.empty?
 
-            @present_components.drop(p_idx + 1).each do |succeeding_component|
+          on_linux_blocks = find_blocks(body_node, :on_linux)
-              next if succeeding_component.empty?
 
-              @offensive_nodes = check_precedence(preceding_component, succeeding_component)
+          if on_linux_blocks.length > 1
-              component_problem @offensive_nodes[0], @offensive_nodes[1] if @offensive_nodes
+            @offensive_node = on_linux_blocks.second
+            @offense_source_range = on_linux_blocks.second.source_range
+            problem "there can only be one `on_linux` block in a formula."
+          end
+
+          check_on_os_block_content(component_precedence_list, on_linux_blocks.first) if on_linux_blocks.any?
+
+          resource_blocks = find_blocks(body_node, :resource)
+          resource_blocks.each do |resource_block|
+            on_macos_blocks = find_blocks(resource_block.body, :on_macos)
+            on_linux_blocks = find_blocks(resource_block.body, :on_linux)
+
+            if on_macos_blocks.length.zero? && on_linux_blocks.length.zero?
+              # Found nothing. Try without .body as depending on the code,
+              # on_macos or on_linux might be in .body or not ...
+              on_macos_blocks = find_blocks(resource_block, :on_macos)
+              on_linux_blocks = find_blocks(resource_block, :on_linux)
+
+              next if on_macos_blocks.length.zero? && on_linux_blocks.length.zero?
+            end
+
+            @offensive_node = resource_block
+            @offense_source_range = resource_block.source_range
+
+            if on_macos_blocks.length > 1
+              problem "there can only be one `on_macos` block in a resource block."
+              next
+            end
+
+            if on_linux_blocks.length > 1
+              problem "there can only be one `on_linux` block in a resource block."
+              next
+            end
+
+            if on_macos_blocks.length == 1 && on_linux_blocks.length.zero?
+              problem "you need to define an `on_linux` block within your resource block."
+              next
+            end
+
+            if on_macos_blocks.length.zero? && on_linux_blocks.length == 1
+              problem "you need to define an `on_macos` block within your resource block."
+              next
+            end
+
+            on_macos_block = on_macos_blocks.first
+            on_linux_block = on_linux_blocks.first
+
+            child_nodes = on_macos_block.body.child_nodes
+            if child_nodes[0].method_name.to_s != "url" && child_nodes[1].method_name.to_s != "sha256"
+              problem "only an url and a sha256 (in the right order) are allowed in a `on_macos` " \
+                      "block within a resource block."
+              next
+            end
+
+            child_nodes = on_linux_block.body.child_nodes
+            if child_nodes[0].method_name.to_s != "url" && child_nodes[1].method_name.to_s != "sha256"
+              problem "only an url and a sha256 (in the right order) are allowed in a `on_linux` " \
+                      "block within a resource block."
             end
           end
         end
 
-        # `aspell`: options and resources should be grouped by language
+        def check_on_os_block_content(component_precedence_list, on_os_block)
-        WHITELIST = %w[
+          _, offensive_node = check_order(component_precedence_list, on_os_block.body)
-          aspell
+          component_problem(*offensive_node) if offensive_node
-        ].freeze
+          on_os_block.body.child_nodes.each do |child|
+            valid_node = depends_on_node?(child)
+            # Check for RuboCop::AST::SendNode instances only, as we are checking the
+            # method_name for patches and resources.
+            next unless child.instance_of? RuboCop::AST::SendNode
 
-        # Method to format message for reporting component precedence violations
+            valid_node ||= child.method_name.to_s == "patch"
-        def component_problem(c1, c2)
+            valid_node ||= child.method_name.to_s == "resource"
-          return if WHITELIST.include?(@formula_name)
 
-          problem "`#{format_component(c1)}` (line #{line_number(c1)}) " \
+            @offensive_node = on_os_block
-                  "should be put before `#{format_component(c2)}` " \
+            @offense_source_range = on_os_block.source_range
-                  "(line #{line_number(c2)})"
+            unless valid_node
+              problem "`#{on_os_block.method_name}` can only include `depends_on`, `patch` and `resource` nodes."
+            end
+          end
         end
 
         # autocorrect method gets called just after component_problem method call
         def autocorrect(_node)
+          return if @offensive_nodes.nil?
+
           succeeding_node = @offensive_nodes[0]
           preceding_node = @offensive_nodes[1]
           lambda do |corrector|
@@ -130,6 +202,52 @@ module RuboCop
             return [idx, comp.index(node1), comp] if comp.member?(node1)
           end
         end
+
+        def check_order(component_precedence_list, body_node)
+          present_components = component_precedence_list.map do |components|
+            components.flat_map do |component|
+              case component[:type]
+              when :method_call
+                find_method_calls_by_name(body_node, component[:name]).to_a
+              when :block_call
+                find_blocks(body_node, component[:name]).to_a
+              when :method_definition
+                find_method_def(body_node, component[:name])
+              end
+            end.compact
+          end
+
+          # Check if each present_components is above rest of the present_components
+          offensive_nodes = nil
+          present_components.take(present_components.size - 1).each_with_index do |preceding_component, p_idx|
+            next if preceding_component.empty?
+
+            present_components.drop(p_idx + 1).each do |succeeding_component|
+              next if succeeding_component.empty?
+
+              offensive_nodes = check_precedence(preceding_component, succeeding_component)
+              break if offensive_nodes
+            end
+          end
+
+          [present_components, offensive_nodes]
+        end
+
+        # Method to format message for reporting component precedence violations
+        def component_problem(c1, c2)
+          return if COMPONENT_WHITELIST.include?(@formula_name)
+
+          problem "`#{format_component(c1)}` (line #{line_number(c1)}) " \
+                  "should be put before `#{format_component(c2)}` " \
+                  "(line #{line_number(c2)})"
+        end
+
+        # Node pattern method to match
+        # `depends_on` variants
+        def_node_matcher :depends_on_node?, <<~EOS
+          {(if _ (send nil? :depends_on ...) nil?)
+           (send nil? :depends_on ...)}
+        EOS
       end
     end
   end

Library/Homebrew/test/rubocops/components_order_spec.rb

@@ -90,6 +90,47 @@ describe RuboCop::Cop::FormulaAudit::ComponentsOrder do
         end
       RUBY
     end
+
+    it "the on_macos block is not after uses_from_macos" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          url "https://brew.sh/foo-1.0.tgz"
+          on_macos do
+            depends_on "readline"
+          end
+          uses_from_macos "bar"
+          ^^^^^^^^^^^^^^^^^^^^^ `uses_from_macos` (line 6) should be put before `on_macos` (line 3)
+        end
+      RUBY
+    end
+
+    it "the on_linux block is not after uses_from_macos" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          url "https://brew.sh/foo-1.0.tgz"
+          on_linux do
+            depends_on "readline"
+          end
+          uses_from_macos "bar"
+          ^^^^^^^^^^^^^^^^^^^^^ `uses_from_macos` (line 6) should be put before `on_linux` (line 3)
+        end
+      RUBY
+    end
+
+    it "the on_linux block is not after the on_macos block" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          url "https://brew.sh/foo-1.0.tgz"
+          on_linux do
+            depends_on "vim"
+          end
+          on_macos do
+          ^^^^^^^^^^^ `on_macos` (line 6) should be put before `on_linux` (line 3)
+            depends_on "readline"
+          end
+        end
+      RUBY
+    end
   end
 
   context "When auditing formula components order with autocorrect" do
@@ -141,4 +182,301 @@ describe RuboCop::Cop::FormulaAudit::ComponentsOrder do
       expect(corrected_source).to eq(correct_source)
     end
   end
+
+  context "no on_os_block" do
+    it "does not fail when there is no on_os block" do
+      expect_no_offenses(<<~RUBY)
+        class Foo < Formula
+          homepage "https://brew.sh"
+
+          depends_on "pkg-config" => :build
+
+          def install
+          end
+        end
+      RUBY
+    end
+  end
+
+  context "on_os_block" do
+    it "correctly uses on_macos and on_linux blocks" do
+      expect_no_offenses(<<~RUBY)
+        class Foo < Formula
+          homepage "https://brew.sh"
+
+          depends_on "pkg-config" => :build
+
+          uses_from_macos "libxml2"
+
+          on_macos do
+            depends_on "perl"
+
+            resource "resource1" do
+              url "https://brew.sh/resource1.tar.gz"
+              sha256 "a2f5650770e1c87fb335af19a9b7eb73fc05ccf22144eb68db7d00cd2bcb0902"
+
+              patch do
+                url "https://raw.githubusercontent.com/Homebrew/formula-patches/0ae366e6/patch3.diff"
+                sha256 "89fa3c95c329ec326e2e76493471a7a974c673792725059ef121e6f9efb05bf4"
+              end
+            end
+
+            resource "resource2" do
+              url "https://brew.sh/resource2.tar.gz"
+              sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35"
+            end
+
+            patch do
+              url "https://raw.githubusercontent.com/Homebrew/formula-patches/0ae366e6/patch1.diff"
+              sha256 "89fa3c95c329ec326e2e76493471a7a974c673792725059ef121e6f9efb05bf4"
+            end
+
+            patch do
+              url "https://raw.githubusercontent.com/Homebrew/formula-patches/0ae366e6/patch2.diff"
+              sha256 "89fa3c95c329ec326e2e76493471a7a974c673792725059ef121e6f9efb05bf4"
+            end
+          end
+
+          on_linux do
+            depends_on "readline"
+          end
+
+          def install
+          end
+        end
+      RUBY
+    end
+  end
+
+  context "on_macos_block" do
+    it "correctly uses as single on_macos block" do
+      expect_no_offenses(<<~RUBY)
+        class Foo < Formula
+          homepage "https://brew.sh"
+
+          on_macos do
+            depends_on "readline"
+          end
+
+          def install
+          end
+        end
+      RUBY
+    end
+  end
+
+  context "on_linux_block" do
+    it "correctly uses as single on_linux block" do
+      expect_no_offenses(<<~RUBY)
+        class Foo < Formula
+          homepage "https://brew.sh"
+
+          on_linux do
+            depends_on "readline"
+          end
+
+          def install
+          end
+        end
+      RUBY
+    end
+  end
+
+  it "there can only be one on_macos block" do
+    expect_offense(<<~RUBY)
+      class Foo < Formula
+        url "https://brew.sh/foo-1.0.tgz"
+        on_macos do
+          depends_on "readline"
+        end
+
+        on_macos do
+        ^^^^^^^^^^^ there can only be one `on_macos` block in a formula.
+          depends_on "foo"
+        end
+      end
+    RUBY
+  end
+
+  it "there can only be one on_linux block" do
+    expect_offense(<<~RUBY)
+      class Foo < Formula
+        url "https://brew.sh/foo-1.0.tgz"
+        on_linux do
+          depends_on "readline"
+        end
+
+        on_linux do
+        ^^^^^^^^^^^ there can only be one `on_linux` block in a formula.
+          depends_on "foo"
+        end
+      end
+    RUBY
+  end
+
+  it "the on_macos block can only contain depends_on, patch and resource nodes" do
+    expect_offense(<<~RUBY)
+      class Foo < Formula
+        url "https://brew.sh/foo-1.0.tgz"
+        on_macos do
+        ^^^^^^^^^^^ `on_macos` can only include `depends_on`, `patch` and `resource` nodes.
+          depends_on "readline"
+          uses_from_macos "ncurses"
+        end
+      end
+    RUBY
+  end
+
+  it "the on_linux block can only contain depends_on, patch and resource nodes" do
+    expect_offense(<<~RUBY)
+      class Foo < Formula
+        url "https://brew.sh/foo-1.0.tgz"
+        on_linux do
+        ^^^^^^^^^^^ `on_linux` can only include `depends_on`, `patch` and `resource` nodes.
+          depends_on "readline"
+          uses_from_macos "ncurses"
+        end
+      end
+    RUBY
+  end
+
+  context "resource" do
+    it "correctly uses an on_macos and on_linux block" do
+      expect_no_offenses(<<~RUBY)
+        class Foo < Formula
+          homepage "https://brew.sh"
+
+          resource do
+            on_macos do
+              url "https://brew.sh/resource1.tar.gz"
+              sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35"
+            end
+
+            on_linux do
+              url "https://brew.sh/resource2.tar.gz"
+              sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35"
+            end
+          end
+        end
+      RUBY
+    end
+
+    it "there are two on_macos blocks, which is not allowed" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          url "https://brew.sh/foo-1.0.tgz"
+
+          resource do
+          ^^^^^^^^^^^ there can only be one `on_macos` block in a resource block.
+            on_macos do
+              url "https://brew.sh/resource1.tar.gz"
+              sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35"
+            end
+
+            on_macos do
+              url "https://brew.sh/resource2.tar.gz"
+              sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35"
+            end
+          end
+        end
+      RUBY
+    end
+
+    it "there are two on_linux blocks, which is not allowed" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          url "https://brew.sh/foo-1.0.tgz"
+
+          resource do
+          ^^^^^^^^^^^ there can only be one `on_linux` block in a resource block.
+            on_linux do
+              url "https://brew.sh/resource1.tar.gz"
+              sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35"
+            end
+
+            on_linux do
+              url "https://brew.sh/resource2.tar.gz"
+              sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35"
+            end
+          end
+        end
+      RUBY
+    end
+
+    it "there is a on_macos block but no on_linux block" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          url "https://brew.sh/foo-1.0.tgz"
+
+          resource do
+          ^^^^^^^^^^^ you need to define an `on_linux` block within your resource block.
+            on_macos do
+              url "https://brew.sh/resource1.tar.gz"
+              sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35"
+            end
+          end
+        end
+      RUBY
+    end
+
+    it "there is a on_linux block but no on_macos block" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          url "https://brew.sh/foo-1.0.tgz"
+
+          resource do
+          ^^^^^^^^^^^ you need to define an `on_macos` block within your resource block.
+            on_linux do
+              url "https://brew.sh/resource1.tar.gz"
+              sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35"
+            end
+          end
+        end
+      RUBY
+    end
+
+    it "the content of the on_macos block is wrong in a resource block" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          url "https://brew.sh/foo-1.0.tgz"
+
+          resource do
+          ^^^^^^^^^^^ only an url and a sha256 (in the right order) are allowed in a `on_macos` block within a resource block.
+            on_macos do
+              sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35"
+              url "https://brew.sh/resource2.tar.gz"
+            end
+
+            on_linux do
+              url "https://brew.sh/resource2.tar.gz"
+              sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35"
+            end
+          end
+        end
+      RUBY
+    end
+
+    it "the content of the on_linux block is wrong in a resource block" do
+      expect_offense(<<~RUBY)
+        class Foo < Formula
+          url "https://brew.sh/foo-1.0.tgz"
+
+          resource do
+          ^^^^^^^^^^^ only an url and a sha256 (in the right order) are allowed in a `on_linux` block within a resource block.
+            on_macos do
+              url "https://brew.sh/resource2.tar.gz"
+              sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35"
+            end
+
+            on_linux do
+              sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35"
+              url "https://brew.sh/resource2.tar.gz"
+            end
+          end
+        end
+      RUBY
+    end
+
+    include_examples "formulae exist", described_class::COMPONENT_WHITELIST
+  end
 end