Merge pull request #15013 from issyl0/rubocops-cask-no-overrides

2023-03-25 13:44:19 +00:00 · 2023-03-25 13:44:19 +00:00 · d43ba7c306
commit d43ba7c306
parent c271a9ae75 e4156909d4
4 changed files with 342 additions and 6 deletions
--- a/Library/Homebrew/rubocops/cask/no_overrides.rb
+++ b/Library/Homebrew/rubocops/cask/no_overrides.rb
@ -0,0 +1,79 @@
+# typed: true
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Cask
+      class NoOverrides < Base
+        extend T::Sig
+        include CaskHelp
+
+        ON_SYSTEM_METHODS = RuboCop::Cask::Constants::ON_SYSTEM_METHODS
+        # These stanzas can be overridden by `on_*` blocks, so take them into account.
+        # TODO: Update this list if new stanzas are added to `Cask::DSL` that call `set_unique_stanza`.
+        OVERRIDEABLE_METHODS = [
+          :appcast, :arch, :auto_updates, :conflicts_with, :container,
+          :desc, :homepage, :sha256, :url, :version
+        ].freeze
+        MESSAGE = <<~EOS
+          Do not use a top-level `%<stanza>s` stanza as the default. Add it to an `on_{system}` block instead.
+          Use `:or_older` or `:or_newer` to specify a range of macOS versions.
+        EOS
+
+        def on_cask(cask_block)
+          cask_stanzas = cask_block.toplevel_stanzas
+
+          # Skip if there are no `on_*` blocks.
+          return unless (on_blocks = cask_stanzas.select { |s| ON_SYSTEM_METHODS.include?(s.stanza_name) }).any?
+
+          stanzas_in_blocks = on_system_stanzas(on_blocks)
+
+          cask_stanzas.each do |stanza|
+            # Skip if the stanza is not allowed to be overridden.
+            next unless OVERRIDEABLE_METHODS.include?(stanza.stanza_name)
+            # Skip if the stanza outside of a block is not also in an `on_*` block.
+            next unless stanzas_in_blocks.include?(stanza.stanza_name)
+
+            add_offense(stanza.source_range, message: format(MESSAGE, stanza: stanza.stanza_name))
+          end
+        end
+
+        def on_system_stanzas(on_system)
+          names = Set.new
+          method_nodes = on_system.map(&:method_node)
+          method_nodes.each do |node|
+            next unless node.block_type?
+
+            node.child_nodes.each do |child|
+              child.each_node(:send) do |send_node|
+                # Skip (nested) livecheck blocks as its `url` is different to a download `url`.
+                next if send_node.method_name == :livecheck || inside_livecheck_block?(send_node)
+                # Skip string interpolations.
+                if send_node.ancestors.drop_while { |a| !a.begin_type? }.any? { |a| a.dstr_type? || a.regexp_type? }
+                  next
+                end
+                next if ON_SYSTEM_METHODS.include?(send_node.method_name)
+
+                names.add(send_node.method_name)
+              end
+            end
+          end
+          names
+        end
+
+        def inside_livecheck_block?(node)
+          single_stanza_livecheck_block?(node) || multi_stanza_livecheck_block?(node)
+        end
+
+        def single_stanza_livecheck_block?(node)
+          node.parent.block_type? && node.parent.method_name == :livecheck
+        end
+
+        def multi_stanza_livecheck_block?(node)
+          grandparent_node = node.parent.parent
+          node.parent.begin_type? && grandparent_node.block_type? && grandparent_node.method_name == :livecheck
+        end
+      end
+    end
+  end
+end
--- a/Library/Homebrew/rubocops/rubocop-cask.rb
+++ b/Library/Homebrew/rubocops/rubocop-cask.rb
@ -15,6 +15,7 @@ require_relative "cask/mixin/on_url_stanza"
 require_relative "cask/desc"
 require_relative "cask/homepage_url_trailing_slash"
 require_relative "cask/no_dsl_version"
+require_relative "cask/no_overrides"
 require_relative "cask/on_system_conditionals"
 require_relative "cask/stanza_order"
 require_relative "cask/stanza_grouping"
--- a/Library/Homebrew/test/rubocops/cask/no_overrides_spec.rb
+++ b/Library/Homebrew/test/rubocops/cask/no_overrides_spec.rb
@ -0,0 +1,255 @@
+# typed: false
+# frozen_string_literal: true
+
+require "rubocops/rubocop-cask"
+require "test/rubocops/cask/shared_examples/cask_cop"
+
+describe RuboCop::Cop::Cask::NoOverrides do
+  include CaskCop
+
+  subject(:cop) { described_class.new }
+
+  context "when there are no on_system blocks" do
+    let(:source) do
+      <<~CASK
+        cask 'foo' do
+          version '1.2.3'
+          url 'https://brew.sh/foo.pkg'
+
+          name 'Foo'
+        end
+      CASK
+    end
+
+    include_examples "does not report any offenses"
+  end
+
+  context "when there are no top-level standalone stanzas" do
+    let(:source) do
+      <<~CASK
+        cask 'foo' do
+          on_mojave :or_later do
+            version :latest
+          end
+        end
+      CASK
+    end
+
+    include_examples "does not report any offenses"
+  end
+
+  context "when there are top-level stanzas also in `on_*` blocks that should not override" do
+    let(:source) do
+      <<~CASK
+        cask 'foo' do
+          version '1.2.3'
+
+          on_arm do
+            binary "foo-\#{version}-arm64"
+          end
+
+          app "foo-\#{version}.app"
+
+          binary "foo-\#{version}"
+        end
+      CASK
+    end
+
+    include_examples "does not report any offenses"
+  end
+
+  context "when there are `arch` variables in the `url` in the `on_*` blocks" do
+    let(:source) do
+      <<~CASK
+        cask 'foo' do
+          arch arm: "arm64", intel: "x86"
+          version '1.2.3'
+          on_mojave :or_later do
+            url "https://brew.sh/foo-\#{version}-\#{arch}.pkg"
+            sha256 "aaa"
+          end
+        end
+      CASK
+    end
+
+    include_examples "does not report any offenses"
+  end
+
+  context "when there are `version` interpolations in `on_*` blocks with methods called on them" do
+    let(:source) do
+      <<~CASK
+        cask 'foo' do
+          version 0.99,123.3
+
+          on_mojave :or_later do
+            url "https://brew.sh/foo-\#{version.csv.first}-\#{version.csv.second}.pkg"
+          end
+        end
+      CASK
+    end
+
+    include_examples "does not report any offenses"
+  end
+
+  context "when there are `arch` interpolations in regexps in `on_*` blocks" do
+    let(:source) do
+      <<~CASK
+        cask 'foo' do
+          arch arm: "arm64", intel: "x86"
+
+          version 0.99,123.3
+
+          on_mojave :or_later do
+            url "https://brew.sh/foo-\#{arch}-\#{version.csv.first}-\#{version.csv.last}.pkg"
+
+            livecheck do
+              url "https://brew.sh/foo/releases.html"
+              regex(/href=.*?foo[._-]v?(\d+(?:.\d+)+)-\#{arch}.pkg/i)
+            end
+          end
+        end
+      CASK
+    end
+
+    include_examples "does not report any offenses"
+  end
+
+  context "when there are single-line livecheck blocks within `on_*` blocks, ignore their contents" do
+    let(:source) do
+      <<~CASK
+        cask 'foo' do
+          on_intel do
+            livecheck do
+              url 'https://brew.sh/foo' # Livecheck should be allowed since it's a different "kind" of URL.
+            end
+            version '1.2.3'
+          end
+          on_arm do
+            version '2.3.4'
+          end
+
+          url 'https://brew.sh/foo.pkg'
+          sha256 "bbb"
+        end
+      CASK
+    end
+
+    include_examples "does not report any offenses"
+  end
+
+  context "when there are multi-line livecheck blocks within `on_*` blocks, ignore their contents" do
+    let(:source) do
+      <<~CASK
+        cask 'foo' do
+          on_intel do
+            livecheck do
+              url 'https://brew.sh/foo' # Livecheck should be allowed since it's a different "kind" of URL.
+              strategy :sparkle
+            end
+            version '1.2.3'
+          end
+          on_arm do
+            version '2.3.4'
+          end
+
+          url 'https://brew.sh/foo.pkg'
+          sha256 "bbb"
+        end
+      CASK
+    end
+
+    include_examples "does not report any offenses"
+  end
+
+  context "when there's only one difference between the `on_*` blocks" do
+    let(:source) do
+      <<~CASK
+        cask "foo" do
+          version "1.2.3"
+
+          on_big_sur :or_older do
+            sha256 "bbb"
+            url "https://brew.sh/legacy/foo-2.3.4.dmg"
+          end
+          on_monterey :or_newer do
+            sha256 "aaa"
+            url "https://brew.sh/foo-2.3.4.dmg"
+          end
+        end
+      CASK
+    end
+
+    include_examples "does not report any offenses"
+  end
+
+  context "when there are multiple differences between the `on_*` blocks" do
+    let(:source) do
+      <<~CASK
+        cask "foo" do
+          version "1.2.3"
+          sha256 "aaa"
+          url "https://brew.sh/foo-2.3.4.dmg"
+
+          on_big_sur :or_older do
+            sha256 "bbb"
+            url "https://brew.sh/legacy/foo-2.3.4.dmg"
+          end
+        end
+      CASK
+    end
+
+    let(:expected_offenses) do
+      [{
+        message:  <<~EOS,
+          Do not use a top-level `sha256` stanza as the default. Add it to an `on_{system}` block instead.
+          Use `:or_older` or `:or_newer` to specify a range of macOS versions.
+        EOS
+        severity: :convention,
+        line:     3,
+        column:   2,
+        source:   "sha256 \"aaa\"",
+      }, {
+        message:  <<~EOS,
+          Do not use a top-level `url` stanza as the default. Add it to an `on_{system}` block instead.
+          Use `:or_older` or `:or_newer` to specify a range of macOS versions.
+        EOS
+        severity: :convention,
+        line:     4,
+        column:   2,
+        source:   "url \"https://brew.sh/foo-2.3.4.dmg\"",
+      }]
+    end
+
+    include_examples "reports offenses"
+  end
+
+  context "when there are top-level standalone stanzas" do
+    let(:source) do
+      <<~CASK
+        cask 'foo' do
+          version '2.3.4'
+          on_mojave :or_older do
+            version '1.2.3'
+          end
+
+          url 'https://brew.sh/foo-2.3.4.dmg'
+        end
+      CASK
+    end
+
+    let(:expected_offenses) do
+      [{
+        message:  <<~EOS,
+          Do not use a top-level `version` stanza as the default. Add it to an `on_{system}` block instead.
+          Use `:or_older` or `:or_newer` to specify a range of macOS versions.
+        EOS
+        severity: :convention,
+        line:     2,
+        column:   2,
+        source:   "version '2.3.4'",
+      }]
+    end
+
+    include_examples "reports offenses"
+  end
+end
--- a/Library/Homebrew/test/support/fixtures/cask/Casks/multiple-versions.rb
+++ b/Library/Homebrew/test/support/fixtures/cask/Casks/multiple-versions.rb
@ -2,16 +2,17 @@ cask "multiple-versions" do
  arch arm: "arm", intel: "intel"
  platform = on_arch_conditional arm: "darwin-arm64", intel: "darwin"

-  version "1.2.3"
-  sha256 "67cdb8a02803ef37fdbf7e0be205863172e41a561ca446cd84f0d7ab35a99d94"
-
+  on_catalina :or_older do
+    version "1.0.0"
+    sha256 "1866dfa833b123bb8fe7fa7185ebf24d28d300d0643d75798bc23730af734216"
+  end
  on_big_sur do
    version "1.2.0"
    sha256 "8c62a2b791cf5f0da6066a0a4b6e85f62949cd60975da062df44adf887f4370b"
  end
-  on_catalina :or_older do
-    version "1.0.0"
-    sha256 "1866dfa833b123bb8fe7fa7185ebf24d28d300d0643d75798bc23730af734216"
+  on_monterey :or_newer do
+    version "1.2.3"
+    sha256 "67cdb8a02803ef37fdbf7e0be205863172e41a561ca446cd84f0d7ab35a99d94"
  end

  url "file://#{TEST_FIXTURE_DIR}/cask/caffeine/#{platform}/#{version}/#{arch}.zip"