From 9b49561066792b11e8246e3d30f29ff1ba08bba6 Mon Sep 17 00:00:00 2001 From: Rylan Polster Date: Thu, 30 Jun 2022 13:36:16 -0400 Subject: [PATCH] Update `components_order` cop to check other `on_{system}` methods --- Library/Homebrew/ast_constants.rb | 7 + Library/Homebrew/rubocops/components_order.rb | 114 ++++--- .../test/rubocops/components_order_spec.rb | 313 +++++++++++++++++- manpages/brew.1 | 2 +- 4 files changed, 365 insertions(+), 71 deletions(-) diff --git a/Library/Homebrew/ast_constants.rb b/Library/Homebrew/ast_constants.rb index a957c7ffd0..c7a8fa88d7 100644 --- a/Library/Homebrew/ast_constants.rb +++ b/Library/Homebrew/ast_constants.rb @@ -1,6 +1,8 @@ # typed: true # frozen_string_literal: true +require "macos_versions" + FORMULA_COMPONENT_PRECEDENCE_LIST = [ [{ name: :include, type: :method_call }], [{ name: :desc, type: :method_call }], @@ -28,6 +30,11 @@ FORMULA_COMPONENT_PRECEDENCE_LIST = [ [{ name: :uses_from_macos, type: :method_call }], [{ name: :on_macos, type: :block_call }], [{ name: :on_linux, type: :block_call }], + [{ name: :on_arm, type: :block_call }], + [{ name: :on_intel, type: :block_call }], + *MacOSVersions::SYMBOLS.keys.map do |os_name| + [{ name: :"on_#{os_name}", type: :block_call }] + end, [{ name: :conflicts_with, type: :method_call }], [{ name: :skip_clean, type: :method_call }], [{ name: :cxxstdlib_check, type: :method_call }], diff --git a/Library/Homebrew/rubocops/components_order.rb b/Library/Homebrew/rubocops/components_order.rb index d8c5c7c147..094ef0bfce 100644 --- a/Library/Homebrew/rubocops/components_order.rb +++ b/Library/Homebrew/rubocops/components_order.rb @@ -14,6 +14,12 @@ module RuboCop class ComponentsOrder < FormulaCop extend AutoCorrector + def on_system_methods + @on_system_methods ||= [:intel, :arm, :macos, :linux, *MacOSVersions::SYMBOLS.keys].map do |m| + :"on_#{m}" + end + end + def audit_formula(_node, _class_node, _parent_class_node, body_node) @present_components, @offensive_nodes = check_order(FORMULA_COMPONENT_PRECEDENCE_LIST, body_node) @@ -25,48 +31,45 @@ module RuboCop [{ name: :patch, type: :method_call }, { name: :patch, type: :block_call }], ] - on_macos_blocks = find_blocks(body_node, :on_macos) + on_system_methods.each do |on_method| + on_method_blocks = find_blocks(body_node, on_method) + next if on_method_blocks.empty? - if on_macos_blocks.length > 1 - @offensive_node = on_macos_blocks.second - problem "there can only be one `on_macos` block in a formula." + if on_method_blocks.length > 1 + @offensive_node = on_method_blocks.second + problem "there can only be one `#{on_method}` block in a formula." + end + + check_on_system_block_content(component_precedence_list, on_method_blocks.first) end - check_on_os_block_content(component_precedence_list, on_macos_blocks.first) if on_macos_blocks.any? - - on_linux_blocks = find_blocks(body_node, :on_linux) - - if on_linux_blocks.length > 1 - @offensive_node = on_linux_blocks.second - 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) + on_system_blocks = {} - 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? + on_system_methods.each do |on_method| + on_system_blocks[on_method] = find_blocks(resource_block.body, on_method) end + if on_system_blocks.empty? + # Found nothing. Try without .body as depending on the code, + # on_{system} might be in .body or not ... + on_system_methods.each do |on_method| + on_system_blocks[on_method] = find_blocks(resource_block, on_method) + end + end + next if on_system_blocks.empty? + @offensive_node = resource_block - next if on_macos_blocks.length.zero? && on_linux_blocks.length.zero? + on_system_bodies = [] - on_os_bodies = [] - - (on_macos_blocks + on_linux_blocks).each do |on_os_block| - on_os_body = on_os_block.body - branches = on_os_body.if_type? ? on_os_body.branches : [on_os_body] - on_os_bodies += branches.map { |branch| [on_os_block, branch] } + on_system_blocks.each_value do |blocks| + blocks.each do |on_system_block| + on_system_body = on_system_block.body + branches = on_system_body.if_type? ? on_system_body.branches : [on_system_body] + on_system_bodies += branches.map { |branch| [on_system_block, branch] } + end end message = nil @@ -79,14 +82,20 @@ module RuboCop minimum_methods = allowed_methods.first.map { |m| "`#{m}`" }.to_sentence maximum_methods = allowed_methods.last.map { |m| "`#{m}`" }.to_sentence - on_os_bodies.each do |on_os_block, on_os_body| - method_name = on_os_block.method_name - child_nodes = on_os_body.begin_type? ? on_os_body.child_nodes : [on_os_body] - if child_nodes.all? { |n| n.send_type? || n.block_type? } - method_names = child_nodes.map(&:method_name) - next if allowed_methods.include? method_names + on_system_bodies.each do |on_system_block, on_system_body| + method_name = on_system_block.method_name + child_nodes = on_system_body.begin_type? ? on_system_body.child_nodes : [on_system_body] + if child_nodes.all? { |n| n.send_type? || n.block_type? || n.lvasgn_type? } + method_names = child_nodes.map do |node| + next if node.lvasgn_type? + next if node.method_name == :patch + next if on_system_methods.include? node.method_name + + node.method_name + end.compact + next if method_names.empty? || allowed_methods.include?(method_names) end - offending_node(on_os_block) + offending_node(on_system_block) message = "`#{method_name}` blocks within `resource` blocks must contain at least " \ "#{minimum_methods} and at most #{maximum_methods} (in order)." break @@ -97,32 +106,31 @@ module RuboCop next end - 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 + on_system_blocks.each do |on_method, blocks| + if blocks.length > 1 + problem "there can only be one `#{on_method}` block in a resource block." + next + end end end end - def check_on_os_block_content(component_precedence_list, on_os_block) - on_os_allowed_methods = %w[ + def check_on_system_block_content(component_precedence_list, on_system_block) + on_system_allowed_methods = %w[ depends_on patch resource deprecate! disable! conflicts_with + fails_with keg_only ignore_missing_libraries ] - _, offensive_node = check_order(component_precedence_list, on_os_block.body) + on_system_allowed_methods += on_system_methods.map(&:to_s) + _, offensive_node = check_order(component_precedence_list, on_system_block.body) component_problem(*offensive_node) if offensive_node - child_nodes = on_os_block.body.begin_type? ? on_os_block.body.child_nodes : [on_os_block.body] + child_nodes = on_system_block.body.begin_type? ? on_system_block.body.child_nodes : [on_system_block.body] child_nodes.each do |child| valid_node = depends_on_node?(child) # Check for RuboCop::AST::SendNode and RuboCop::AST::BlockNode instances @@ -130,13 +138,13 @@ module RuboCop method_type = child.send_type? || child.block_type? next unless method_type - valid_node ||= on_os_allowed_methods.include? child.method_name.to_s + valid_node ||= on_system_allowed_methods.include? child.method_name.to_s @offensive_node = child next if valid_node - problem "`#{on_os_block.method_name}` cannot include `#{child.method_name}`. " \ - "Only #{on_os_allowed_methods.map { |m| "`#{m}`" }.to_sentence} are allowed." + problem "`#{on_system_block.method_name}` cannot include `#{child.method_name}`. " \ + "Only #{on_system_allowed_methods.map { |m| "`#{m}`" }.to_sentence} are allowed." end end diff --git a/Library/Homebrew/test/rubocops/components_order_spec.rb b/Library/Homebrew/test/rubocops/components_order_spec.rb index 42ce5b9759..7b8e2a813a 100644 --- a/Library/Homebrew/test/rubocops/components_order_spec.rb +++ b/Library/Homebrew/test/rubocops/components_order_spec.rb @@ -337,7 +337,7 @@ describe RuboCop::Cop::FormulaAudit::ComponentsOrder do RUBY end - context "when formula has no OS-specific blocks" do + context "when formula has no system-specific blocks" do it "reports no offenses" do expect_no_offenses(<<~RUBY) class Foo < Formula @@ -352,7 +352,7 @@ describe RuboCop::Cop::FormulaAudit::ComponentsOrder do end end - context "when formula has OS-specific block(s)" do + context "when formula has system-specific block(s)" do it "reports no offenses when `on_macos` and `on_linux` are used correctly" do expect_no_offenses(<<~RUBY) class Foo < Formula @@ -363,7 +363,13 @@ describe RuboCop::Cop::FormulaAudit::ComponentsOrder do uses_from_macos "libxml2" on_macos do - depends_on "perl" + on_arm do + depends_on "perl" + end + + on_intel do + depends_on "python" + end resource "resource1" do url "https://brew.sh/resource1.tar.gz" @@ -433,6 +439,70 @@ describe RuboCop::Cop::FormulaAudit::ComponentsOrder do RUBY end + it "reports no offenses when `on_intel` is used correctly" do + expect_no_offenses(<<~RUBY) + class Foo < Formula + homepage "https://brew.sh" + + on_intel do + disable! because: :does_not_build + depends_on "readline" + end + + def install + end + end + RUBY + end + + it "reports no offenses when `on_arm` is used correctly" do + expect_no_offenses(<<~RUBY) + class Foo < Formula + homepage "https://brew.sh" + + on_arm do + deprecate! because: "it's deprecated" + depends_on "readline" + end + + def install + end + end + RUBY + end + + it "reports no offenses when `on_monterey` is used correctly" do + expect_no_offenses(<<~RUBY) + class Foo < Formula + homepage "https://brew.sh" + + on_monterey do + disable! because: :does_not_build + depends_on "readline" + end + + def install + end + end + RUBY + end + + it "reports no offenses when `on_monterey :or_older` is used correctly" do + expect_no_offenses(<<~RUBY) + class Foo < Formula + homepage "https://brew.sh" + + on_monterey :or_older do + deprecate! because: "it's deprecated" + depends_on "readline" + end + + def install + end + end + RUBY + end + it "reports an offense when there are multiple `on_macos` blocks" do expect_offense(<<~RUBY) class Foo < Formula @@ -465,6 +535,70 @@ describe RuboCop::Cop::FormulaAudit::ComponentsOrder do RUBY end + it "reports an offense when there are multiple `on_intel` blocks" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + on_intel do + depends_on "readline" + end + + on_intel do + ^^^^^^^^^^^ there can only be one `on_intel` block in a formula. + depends_on "foo" + end + end + RUBY + end + + it "reports an offense when there are multiple `on_arm` blocks" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + on_arm do + depends_on "readline" + end + + on_arm do + ^^^^^^^^^ there can only be one `on_arm` block in a formula. + depends_on "foo" + end + end + RUBY + end + + it "reports an offense when there are multiple `on_monterey` blocks" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + on_monterey do + depends_on "readline" + end + + on_monterey do + ^^^^^^^^^^^^^^ there can only be one `on_monterey` block in a formula. + depends_on "foo" + end + end + RUBY + end + + it "reports an offense when there are multiple `on_monterey` blocks with parameters" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + on_monterey do + depends_on "readline" + end + + on_monterey :or_older do + ^^^^^^^^^^^^^^^^^^^^^^^^ there can only be one `on_monterey` block in a formula. + depends_on "foo" + end + end + RUBY + end + it "reports an offense when the `on_macos` block contains nodes other than `depends_on`, `patch` or `resource`" do expect_offense(<<~RUBY) class Foo < Formula @@ -491,6 +625,60 @@ describe RuboCop::Cop::FormulaAudit::ComponentsOrder do RUBY end + it "reports an offense when the `on_intel` block contains nodes other than `depends_on`, `patch` or `resource`" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + on_intel do + depends_on "readline" + uses_from_macos "ncurses" + ^^^^^^^^^^^^^^^^^^^^^^^^^ `on_intel` cannot include `uses_from_macos`. [...] + end + end + RUBY + end + + it "reports an offense when the `on_arm` block contains nodes other than `depends_on`, `patch` or `resource`" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + on_arm do + depends_on "readline" + uses_from_macos "ncurses" + ^^^^^^^^^^^^^^^^^^^^^^^^^ `on_arm` cannot include `uses_from_macos`. [...] + end + end + RUBY + end + + it "reports an offense when the `on_monterey` block contains nodes other than " \ + "`depends_on`, `patch` or `resource`" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + on_monterey do + depends_on "readline" + uses_from_macos "ncurses" + ^^^^^^^^^^^^^^^^^^^^^^^^^ `on_monterey` cannot include `uses_from_macos`. [...] + end + end + RUBY + end + + it "reports an offense when the `on_monterey :or_older` block contains nodes other than " \ + "`depends_on`, `patch` or `resource`" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + on_monterey :or_older do + depends_on "readline" + uses_from_macos "ncurses" + ^^^^^^^^^^^^^^^^^^^^^^^^^ `on_monterey` cannot include `uses_from_macos`. [...] + end + end + RUBY + end + context "when in a resource block" do it "reports no offenses for a valid `on_macos` and `on_linux` block" do expect_no_offenses(<<~RUBY) @@ -512,19 +700,19 @@ describe RuboCop::Cop::FormulaAudit::ComponentsOrder do RUBY end - it "reports no offenses for a valid `on_macos` and `on_linux` block (with `version`)" do + it "reports no offenses for a valid `on_arm` and `on_intel` block (with `version`)" do expect_no_offenses(<<~RUBY) class Foo < Formula homepage "https://brew.sh" resource do - on_macos do + on_arm do url "https://brew.sh/resource1.tar.gz" version "1.2.3" sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35" end - on_linux do + on_intel do url "https://brew.sh/resource2.tar.gz" version "1.2.3" sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35" @@ -576,6 +764,69 @@ describe RuboCop::Cop::FormulaAudit::ComponentsOrder do RUBY end + it "reports an offense if there are two `on_intel` blocks" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + + resource do + ^^^^^^^^^^^ there can only be one `on_intel` block in a resource block. + on_intel do + url "https://brew.sh/resource1.tar.gz" + sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35" + end + + on_intel do + url "https://brew.sh/resource2.tar.gz" + sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35" + end + end + end + RUBY + end + + it "reports an offense if there are two `on_arm` blocks" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + + resource do + ^^^^^^^^^^^ there can only be one `on_arm` block in a resource block. + on_arm do + url "https://brew.sh/resource1.tar.gz" + sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35" + end + + on_arm do + url "https://brew.sh/resource2.tar.gz" + sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35" + end + end + end + RUBY + end + + it "reports an offense if there are two `on_monterey` blocks" do + expect_offense(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + + resource do + ^^^^^^^^^^^ there can only be one `on_monterey` block in a resource block. + on_monterey do + url "https://brew.sh/resource1.tar.gz" + sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35" + end + + on_monterey :or_older do + url "https://brew.sh/resource2.tar.gz" + sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35" + end + end + end + RUBY + end + it "reports no offenses if there is an `on_macos` block but no `on_linux` block" do expect_no_offenses(<<~RUBY) class Foo < Formula @@ -604,6 +855,34 @@ describe RuboCop::Cop::FormulaAudit::ComponentsOrder do RUBY end + it "reports no offenses if there is an `on_intel` block but no `on_arm` block" do + expect_no_offenses(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + resource do + on_intel do + url "https://brew.sh/resource1.tar.gz" + sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35" + end + end + end + RUBY + end + + it "reports no offenses if there is an `on_arm` block but no `on_intel` block" do + expect_no_offenses(<<~RUBY) + class Foo < Formula + url "https://brew.sh/foo-1.0.tgz" + resource do + on_arm do + url "https://brew.sh/resource1.tar.gz" + sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35" + end + end + end + RUBY + end + it "reports an offense if the content of an `on_macos` block is improperly formatted" do expect_offense(<<~RUBY) class Foo < Formula @@ -697,19 +976,19 @@ describe RuboCop::Cop::FormulaAudit::ComponentsOrder do RUBY end - it "reports an offense if the content of an `on_linux` block is improperly formatted" do + it "reports an offense if the content of an `on_arm` block is improperly formatted" do expect_offense(<<~RUBY) class Foo < Formula url "https://brew.sh/foo-1.0.tgz" resource do - on_macos do + on_intel do url "https://brew.sh/resource2.tar.gz" sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35" end - on_linux do - ^^^^^^^^^^^ `on_linux` blocks within `resource` blocks must contain at least `url` and `sha256` and at most `url`, `mirror`, `version` and `sha256` (in order). + on_arm do + ^^^^^^^^^ `on_arm` blocks within `resource` blocks must contain at least `url` and `sha256` and at most `url`, `mirror`, `version` and `sha256` (in order). sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35" url "https://brew.sh/resource2.tar.gz" end @@ -718,18 +997,18 @@ describe RuboCop::Cop::FormulaAudit::ComponentsOrder do RUBY end - it "reports no offenses if an `on_linux` block has if-else branches that are properly formatted" do + it "reports no offenses if an `on_arm` block has if-else branches that are properly formatted" do expect_no_offenses(<<~RUBY) class Foo < Formula url "https://brew.sh/foo-1.0.tgz" resource do - on_macos do + on_intel do url "https://brew.sh/resource2.tar.gz" sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35" end - on_linux do + on_arm do if foo == :bar url "https://brew.sh/resource2.tar.gz" sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35" @@ -743,19 +1022,19 @@ describe RuboCop::Cop::FormulaAudit::ComponentsOrder do RUBY end - it "reports an offense if an `on_linux` block has if-else branches that aren't properly formatted" do + it "reports an offense if an `on_arm` block has if-else branches that aren't properly formatted" do expect_offense(<<~RUBY) class Foo < Formula url "https://brew.sh/foo-1.0.tgz" resource do - on_macos do + on_intel do url "https://brew.sh/resource2.tar.gz" sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35" end - on_linux do - ^^^^^^^^^^^ `on_linux` blocks within `resource` blocks must contain at least `url` and `sha256` and at most `url`, `mirror`, `version` and `sha256` (in order). + on_arm do + ^^^^^^^^^ `on_arm` blocks within `resource` blocks must contain at least `url` and `sha256` and at most `url`, `mirror`, `version` and `sha256` (in order). if foo == :bar url "https://brew.sh/resource2.tar.gz" sha256 "586372eb92059873e29eba4f9dec8381541b4d3834660707faf8ba59146dfc35" diff --git a/manpages/brew.1 b/manpages/brew.1 index f6e18f541b..87c2999563 100644 --- a/manpages/brew.1 +++ b/manpages/brew.1 @@ -1,7 +1,7 @@ .\" generated with Ronn/v0.7.3 .\" http://github.com/rtomayko/ronn/tree/0.7.3 . -.TH "BREW" "1" "June 2022" "Homebrew" "brew" +.TH "BREW" "1" "July 2022" "Homebrew" "brew" . .SH "NAME" \fBbrew\fR \- The Missing Package Manager for macOS (or Linux)