rubocops/cask: Check for correct stanza ordering within on_* blocks

- Now that we detect correct stanza _grouping_ within `on_*` blocks in Casks (PR 15211), correct stanza _ordering_ in `on_*` blocks was the next logical step. For example, `url` has to come after `version` and `sha256` in an `on_macos` or `on_intel` block for consistency with the top-level stanza order we enforce elsewhere. - Still not doing the nested `on_os` inside `on_arch`, that felt excessive for an edge case that isn't present in any actual real Casks we have. I removed the test with that specific TODO.
2023-04-13 19:10:38 +01:00 · 2023-04-13 19:10:38 +01:00 · bd6e9e72a1
commit bd6e9e72a1
parent 1120812378
3 changed files with 76 additions and 78 deletions
--- a/Library/Homebrew/rubocops/cask/ast/cask_block.rb
+++ b/Library/Homebrew/rubocops/cask/ast/cask_block.rb
@ -51,6 +51,10 @@ module RuboCop
          @sorted_toplevel_stanzas ||= sort_stanzas(toplevel_stanzas)
        end

+        def sorted_inner_stanzas(stanzas)
+          sort_stanzas(stanzas)
+        end
+
        private

        def sort_stanzas(stanzas)
--- a/Library/Homebrew/rubocops/cask/stanza_order.rb
+++ b/Library/Homebrew/rubocops/cask/stanza_order.rb
@ -13,11 +13,23 @@ module RuboCop
        extend AutoCorrector
        include CaskHelp

+        ON_SYSTEM_METHODS = RuboCop::Cask::Constants::ON_SYSTEM_METHODS
        MESSAGE = "`%<stanza>s` stanza out of order"

        def on_cask(cask_block)
          @cask_block = cask_block
-          add_offenses
+          add_offenses(toplevel_stanzas)
+
+          return unless (on_blocks = toplevel_stanzas.select { |s| ON_SYSTEM_METHODS.include?(s.stanza_name) }).any?
+
+          on_blocks.map(&:method_node).each do |on_block|
+            next unless on_block.block_type?
+
+            block_contents = on_block.child_nodes.select(&:begin_type?)
+            inner_nodes = block_contents.map(&:child_nodes).flatten.select(&:send_type?)
+            inner_stanzas = inner_nodes.map { |node| RuboCop::Cask::AST::Stanza.new(node, processed_source.comments) }
+            add_offenses(inner_stanzas, inner: true)
+          end
        end

        private
@ -25,22 +37,24 @@ module RuboCop
        attr_reader :cask_block

        def_delegators :cask_block, :cask_node, :toplevel_stanzas,
-                       :sorted_toplevel_stanzas
+                       :sorted_toplevel_stanzas, :sorted_inner_stanzas

-        def add_offenses
-          offending_stanzas.each do |stanza|
+        def add_offenses(stanzas, inner: false)
+          sorted_stanzas = inner ? sorted_inner_stanzas(stanzas) : sorted_toplevel_stanzas
+          offending_stanzas(stanzas, inner: inner).each do |stanza|
            message = format(MESSAGE, stanza: stanza.stanza_name)
            add_offense(stanza.source_range_with_comments, message: message) do |corrector|
-              correct_stanza_index = toplevel_stanzas.index(stanza)
-              correct_stanza = sorted_toplevel_stanzas[correct_stanza_index]
+              correct_stanza_index = stanzas.index(stanza)
+              correct_stanza = sorted_stanzas[correct_stanza_index]
              corrector.replace(stanza.source_range_with_comments,
                                correct_stanza.source_with_comments)
            end
          end
        end

-        def offending_stanzas
-          stanza_pairs = toplevel_stanzas.zip(sorted_toplevel_stanzas)
+        def offending_stanzas(stanzas, inner: false)
+          sorted_stanzas = inner ? sorted_inner_stanzas(stanzas) : sorted_toplevel_stanzas
+          stanza_pairs = stanzas.zip(sorted_stanzas)
          stanza_pairs.each_with_object([]) do |stanza_pair, offending_stanzas|
            stanza, sorted_stanza = *stanza_pair
            offending_stanzas << stanza if stanza != sorted_stanza
--- a/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb
+++ b/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb
@ -458,39 +458,22 @@ describe RuboCop::Cop::Cask::StanzaOrder do
    include_examples "autocorrects source"
  end

-  # TODO: detect out-of-order stanzas in nested expressions
-  context "when stanzas are nested in a conditional expression" do
-    let(:source) do
-      <<~CASK
-        cask 'foo' do
-          if true
-            sha256 :no_check
-            version :latest
-          end
-        end
-      CASK
-    end
-
-    include_examples "does not report any offenses"
-  end
-
  context "when `on_arch` blocks are out of order" do
    let(:source) do
      <<~CASK
        cask 'foo' do
          on_intel do
+            version :latest
+            sha256 :no_check
+
            url "https://foo.brew.sh/foo-intel.zip"
-            sha256 :no_check
-            version :latest
          end
-
          on_arm do
-            url "https://foo.brew.sh/foo-arm.zip"
-            sha256 :no_check
            version :latest
-          end
+            sha256 :no_check

-          name "Foo"
+            url "https://foo.brew.sh/foo-arm.zip"
+          end
        end
      CASK
    end
@ -501,13 +484,13 @@ describe RuboCop::Cop::Cask::StanzaOrder do
        severity: :convention,
        line:     2,
        column:   2,
-        source:   "on_intel do\n    url \"https://foo.brew.sh/foo-intel.zip\"\n    sha256 :no_check\n    version :latest\n  end", # rubocop:disable Layout/LineLength
+        source:   "on_intel do\n    version :latest\n    sha256 :no_check\n\n    url \"https://foo.brew.sh/foo-intel.zip\"\n  end", # rubocop:disable Layout/LineLength
      }, {
        message:  "Cask/StanzaOrder: `on_arm` stanza out of order",
        severity: :convention,
        line:     8,
        column:   2,
-        source:   "on_arm do\n    url \"https://foo.brew.sh/foo-arm.zip\"\n    sha256 :no_check\n    version :latest\n  end", # rubocop:disable Layout/LineLength
+        source:   "on_arm do\n    version :latest\n    sha256 :no_check\n\n    url \"https://foo.brew.sh/foo-arm.zip\"\n  end", # rubocop:disable Layout/LineLength
      }]
    end

@ -515,18 +498,17 @@ describe RuboCop::Cop::Cask::StanzaOrder do
      <<~CASK
        cask 'foo' do
          on_arm do
+            version :latest
+            sha256 :no_check
+
            url "https://foo.brew.sh/foo-arm.zip"
-            sha256 :no_check
-            version :latest
          end
-
          on_intel do
-            url "https://foo.brew.sh/foo-intel.zip"
-            sha256 :no_check
            version :latest
-          end
+            sha256 :no_check

-          name "Foo"
+            url "https://foo.brew.sh/foo-intel.zip"
+          end
        end
      CASK
    end
@ -535,44 +517,42 @@ describe RuboCop::Cop::Cask::StanzaOrder do
    include_examples "autocorrects source"
  end

-  # TODO: detect out-of-order stanzas in nested expressions
-  context "when the on_arch and on_os stanzas are nested" do
+  context "when the `on_arch` blocks contents are out of order" do
    let(:source) do
      <<~CASK
        cask 'foo' do
          on_arm do
-            url "https://foo.brew.sh/foo-arm-all.zip"
-            sha256 :no_check
-            version :latest
+            url "https://foo.brew.sh/foo-arm.zip"
+            sha256 "123abc"
+            version "1.0"
          end
-
          on_intel do
-            on_ventura do
-              url "https://foo.brew.sh/foo-intel-ventura.zip"
-              sha256 :no_check
-            end
-            on_mojave do
-              url "https://foo.brew.sh/foo-intel-mojave.zip"
-              sha256 :no_check
-            end
-            on_catalina do
-              url "https://foo.brew.sh/foo-intel-catalina.zip"
-              sha256 :no_check
-            end
-            on_big_sur do
-              url "https://foo.brew.sh/foo-intel-big-sur.zip"
-              sha256 :no_check
-            end
-
-            version :latest
+            url "https://foo.brew.sh/foo-intel.zip"
+            sha256 "abc123"
+            version "0.9" # comment here
          end
-
-          name "Foo"
        end
      CASK
    end

-    include_examples "does not report any offenses"
+    let(:correct_source) do
+      <<~CASK
+        cask 'foo' do
+          on_arm do
+            version "1.0"
+            sha256 "123abc"
+            url "https://foo.brew.sh/foo-arm.zip"
+          end
+          on_intel do
+            version "0.9" # comment here
+            sha256 "abc123"
+            url "https://foo.brew.sh/foo-intel.zip"
+          end
+        end
+      CASK
+    end
+
+    include_examples "autocorrects source"
  end

  context "when the on_os stanzas are out of order" do
@ -580,20 +560,20 @@ describe RuboCop::Cop::Cask::StanzaOrder do
      <<~CASK
        cask "foo" do
          on_ventura do
-            url "https://foo.brew.sh/foo-ventura.zip"
            sha256 :no_check
+            url "https://foo.brew.sh/foo-ventura.zip"
          end
          on_catalina do
-            url "https://foo.brew.sh/foo-catalina.zip"
            sha256 :no_check
+            url "https://foo.brew.sh/foo-catalina.zip"
          end
          on_mojave do
-            url "https://foo.brew.sh/foo-mojave.zip"
            sha256 :no_check
+            url "https://foo.brew.sh/foo-mojave.zip"
          end
          on_big_sur do
-            url "https://foo.brew.sh/foo-big-sur.zip"
            sha256 :no_check
+            url "https://foo.brew.sh/foo-big-sur.zip"
          end

          name "Foo"
@ -607,19 +587,19 @@ describe RuboCop::Cop::Cask::StanzaOrder do
        severity: :convention,
        line:     2,
        column:   2,
-        source:   "on_ventura do\n    url \"https://foo.brew.sh/foo-ventura.zip\"\n    sha256 :no_check\n  end",
+        source:   "on_ventura do\n    sha256 :no_check\n    url \"https://foo.brew.sh/foo-ventura.zip\"\n  end",
      }, {
        message:  "Cask/StanzaOrder: `on_mojave` stanza out of order",
        severity: :convention,
        line:     10,
        column:   2,
-        source:   "on_mojave do\n    url \"https://foo.brew.sh/foo-mojave.zip\"\n    sha256 :no_check\n  end",
+        source:   "on_mojave do\n    sha256 :no_check\n    url \"https://foo.brew.sh/foo-mojave.zip\"\n  end",
      }, {
        message:  "Cask/StanzaOrder: `on_big_sur` stanza out of order",
        severity: :convention,
        line:     14,
        column:   2,
-        source:   "on_big_sur do\n    url \"https://foo.brew.sh/foo-big-sur.zip\"\n    sha256 :no_check\n  end",
+        source:   "on_big_sur do\n    sha256 :no_check\n    url \"https://foo.brew.sh/foo-big-sur.zip\"\n  end",
      }]
    end

@ -627,20 +607,20 @@ describe RuboCop::Cop::Cask::StanzaOrder do
      <<~CASK
        cask "foo" do
          on_mojave do
-            url "https://foo.brew.sh/foo-mojave.zip"
            sha256 :no_check
+            url "https://foo.brew.sh/foo-mojave.zip"
          end
          on_catalina do
-            url "https://foo.brew.sh/foo-catalina.zip"
            sha256 :no_check
+            url "https://foo.brew.sh/foo-catalina.zip"
          end
          on_big_sur do
-            url "https://foo.brew.sh/foo-big-sur.zip"
            sha256 :no_check
+            url "https://foo.brew.sh/foo-big-sur.zip"
          end
          on_ventura do
-            url "https://foo.brew.sh/foo-ventura.zip"
            sha256 :no_check
+            url "https://foo.brew.sh/foo-ventura.zip"
          end

          name "Foo"