From bd6e9e72a1d50b58e338eca543f34d07fbaad853 Mon Sep 17 00:00:00 2001 From: Issy Long Date: Thu, 13 Apr 2023 19:10:38 +0100 Subject: [PATCH 01/10] 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. --- .../Homebrew/rubocops/cask/ast/cask_block.rb | 4 + .../Homebrew/rubocops/cask/stanza_order.rb | 30 +++-- .../test/rubocops/cask/stanza_order_spec.rb | 120 ++++++++---------- 3 files changed, 76 insertions(+), 78 deletions(-) diff --git a/Library/Homebrew/rubocops/cask/ast/cask_block.rb b/Library/Homebrew/rubocops/cask/ast/cask_block.rb index 1b4090ab49..929a4aeb67 100644 --- 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) diff --git a/Library/Homebrew/rubocops/cask/stanza_order.rb b/Library/Homebrew/rubocops/cask/stanza_order.rb index 83264020bb..6423fc613b 100644 --- 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 = "`%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 diff --git a/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb b/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb index b7357cd744..b1da650dfe 100644 --- 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" From 9457a23430d92ffdfd7c8baf18f548b70ee83fbb Mon Sep 17 00:00:00 2001 From: Issy Long Date: Sat, 15 Apr 2023 22:55:12 +0100 Subject: [PATCH 02/10] Extract methods for checking for `on_*` blocks and their contents - The same code to do the same thing was getting a bit repetitive in `Cask/StanzaOrder`, `Cask/StanzaGrouping` and `Cask/NoOverrides` cops. --- .../Homebrew/rubocops/cask/mixin/cask_help.rb | 10 ++++++++++ Library/Homebrew/rubocops/cask/no_overrides.rb | 10 +++------- .../Homebrew/rubocops/cask/stanza_grouping.rb | 17 +++++------------ Library/Homebrew/rubocops/cask/stanza_order.rb | 15 +++++---------- 4 files changed, 23 insertions(+), 29 deletions(-) diff --git a/Library/Homebrew/rubocops/cask/mixin/cask_help.rb b/Library/Homebrew/rubocops/cask/mixin/cask_help.rb index c4e76607cb..f4ba80c8f1 100644 --- a/Library/Homebrew/rubocops/cask/mixin/cask_help.rb +++ b/Library/Homebrew/rubocops/cask/mixin/cask_help.rb @@ -22,6 +22,16 @@ module RuboCop cask_block = RuboCop::Cask::AST::CaskBlock.new(block_node, comments) on_cask(cask_block) end + + def on_system_methods(cask_stanzas) + cask_stanzas.select { |s| RuboCop::Cask::Constants::ON_SYSTEM_METHODS.include?(s.stanza_name) } + end + + def inner_stanzas(block_node, comments) + block_contents = block_node.child_nodes.select(&:begin_type?) + inner_nodes = block_contents.map(&:child_nodes).flatten.select(&:send_type?) + inner_nodes.map { |n| RuboCop::Cask::AST::Stanza.new(n, comments) } + end end end end diff --git a/Library/Homebrew/rubocops/cask/no_overrides.rb b/Library/Homebrew/rubocops/cask/no_overrides.rb index 484d8696d0..dc704f8575 100644 --- a/Library/Homebrew/rubocops/cask/no_overrides.rb +++ b/Library/Homebrew/rubocops/cask/no_overrides.rb @@ -7,7 +7,6 @@ module RuboCop class NoOverrides < Base 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 = [ @@ -22,8 +21,7 @@ module RuboCop def on_cask(cask_block) cask_stanzas = cask_block.toplevel_stanzas - # Skip if there are no `on_*` blocks. - return if (on_blocks = cask_stanzas.select { |s| ON_SYSTEM_METHODS.include?(s.stanza_name) }).none? + return unless (on_blocks = on_system_methods(cask_stanzas)).any? stanzas_in_blocks = on_system_stanzas(on_blocks) @@ -40,9 +38,7 @@ module RuboCop 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? - + method_nodes.select(&:block_type?).each do |node| 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`. @@ -51,7 +47,7 @@ module RuboCop 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) + next if RuboCop::Cask::Constants::ON_SYSTEM_METHODS.include?(send_node.method_name) names.add(send_node.method_name) end diff --git a/Library/Homebrew/rubocops/cask/stanza_grouping.rb b/Library/Homebrew/rubocops/cask/stanza_grouping.rb index 9fa5a5b35c..489c7ba061 100644 --- a/Library/Homebrew/rubocops/cask/stanza_grouping.rb +++ b/Library/Homebrew/rubocops/cask/stanza_grouping.rb @@ -6,7 +6,7 @@ require "forwardable" module RuboCop module Cop module Cask - # This cop checks that a cask's stanzas are grouped correctly. + # This cop checks that a cask's stanzas are grouped correctly, including nested within `on_*` blocks. # @see https://docs.brew.sh/Cask-Cookbook#stanza-order class StanzaGrouping < Base extend Forwardable @@ -14,7 +14,6 @@ module RuboCop include CaskHelp include RangeHelp - ON_SYSTEM_METHODS = RuboCop::Cask::Constants::ON_SYSTEM_METHODS MISSING_LINE_MSG = "stanza groups should be separated by a single empty line" EXTRA_LINE_MSG = "stanzas within the same group should have no lines between them" @@ -24,17 +23,11 @@ module RuboCop cask_stanzas = cask_block.toplevel_stanzas add_offenses(cask_stanzas) - # If present, check grouping of stanzas within `on_*` blocks. - return if (on_blocks = cask_stanzas.select { |s| ON_SYSTEM_METHODS.include?(s.stanza_name) }).none? + return unless (on_blocks = on_system_methods(cask_stanzas)).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) + on_blocks.map(&:method_node).select(&:block_type?).each do |on_block| + stanzas = inner_stanzas(on_block, processed_source.comments) + add_offenses(stanzas) end end diff --git a/Library/Homebrew/rubocops/cask/stanza_order.rb b/Library/Homebrew/rubocops/cask/stanza_order.rb index 6423fc613b..4b5ea9cb7d 100644 --- a/Library/Homebrew/rubocops/cask/stanza_order.rb +++ b/Library/Homebrew/rubocops/cask/stanza_order.rb @@ -6,29 +6,24 @@ require "forwardable" module RuboCop module Cop module Cask - # This cop checks that a cask's stanzas are ordered correctly. + # This cop checks that a cask's stanzas are ordered correctly, including nested within `on_*` blocks. # @see https://docs.brew.sh/Cask-Cookbook#stanza-order class StanzaOrder < Base extend Forwardable extend AutoCorrector include CaskHelp - ON_SYSTEM_METHODS = RuboCop::Cask::Constants::ON_SYSTEM_METHODS MESSAGE = "`%s` stanza out of order" def on_cask(cask_block) @cask_block = cask_block add_offenses(toplevel_stanzas) - return unless (on_blocks = toplevel_stanzas.select { |s| ON_SYSTEM_METHODS.include?(s.stanza_name) }).any? + return unless (on_blocks = on_system_methods(toplevel_stanzas)).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) + on_blocks.map(&:method_node).select(&:block_type?).each do |on_block| + stanzas = inner_stanzas(on_block, processed_source.comments) + add_offenses(stanzas, inner: true) end end From 40bc235cb62c7cf72969c1355aa4d6c7ae4e80ee Mon Sep 17 00:00:00 2001 From: Issy Long Date: Sun, 16 Apr 2023 22:03:29 +0100 Subject: [PATCH 03/10] Simplify `offending_stanzas` method - pass `sorted_stanzas` in directly - Otherwise we're doing the same "if inner" check twice, for no gain. --- Library/Homebrew/rubocops/cask/stanza_order.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Library/Homebrew/rubocops/cask/stanza_order.rb b/Library/Homebrew/rubocops/cask/stanza_order.rb index 4b5ea9cb7d..fb56e9572b 100644 --- a/Library/Homebrew/rubocops/cask/stanza_order.rb +++ b/Library/Homebrew/rubocops/cask/stanza_order.rb @@ -36,7 +36,7 @@ module RuboCop def add_offenses(stanzas, inner: false) sorted_stanzas = inner ? sorted_inner_stanzas(stanzas) : sorted_toplevel_stanzas - offending_stanzas(stanzas, inner: inner).each do |stanza| + offending_stanzas(stanzas, sorted_stanzas).each do |stanza| message = format(MESSAGE, stanza: stanza.stanza_name) add_offense(stanza.source_range_with_comments, message: message) do |corrector| correct_stanza_index = stanzas.index(stanza) @@ -47,8 +47,7 @@ module RuboCop end end - def offending_stanzas(stanzas, inner: false) - sorted_stanzas = inner ? sorted_inner_stanzas(stanzas) : sorted_toplevel_stanzas + def offending_stanzas(stanzas, sorted_stanzas) stanza_pairs = stanzas.zip(sorted_stanzas) stanza_pairs.each_with_object([]) do |stanza_pair, offending_stanzas| stanza, sorted_stanza = *stanza_pair From 704482d8153498f4714076fa5ae31397d034a5ce Mon Sep 17 00:00:00 2001 From: Issy Long Date: Sun, 16 Apr 2023 23:46:37 +0100 Subject: [PATCH 04/10] wip a test for ordering of both on_* blocks and their contents - This is very broken, "detected clobbering" errors. --- .../Homebrew/rubocops/cask/stanza_order.rb | 5 +- .../test/rubocops/cask/stanza_order_spec.rb | 114 ++++-------------- 2 files changed, 24 insertions(+), 95 deletions(-) diff --git a/Library/Homebrew/rubocops/cask/stanza_order.rb b/Library/Homebrew/rubocops/cask/stanza_order.rb index fb56e9572b..efb15a7977 100644 --- a/Library/Homebrew/rubocops/cask/stanza_order.rb +++ b/Library/Homebrew/rubocops/cask/stanza_order.rb @@ -37,12 +37,13 @@ module RuboCop def add_offenses(stanzas, inner: false) sorted_stanzas = inner ? sorted_inner_stanzas(stanzas) : sorted_toplevel_stanzas offending_stanzas(stanzas, sorted_stanzas).each do |stanza| + puts "offending stanza: #{stanza.stanza_name}" message = format(MESSAGE, stanza: stanza.stanza_name) add_offense(stanza.source_range_with_comments, message: message) do |corrector| 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) + puts "correct stanza: #{correct_stanza.stanza_name}" + corrector.replace(stanza.source_range_with_comments, correct_stanza.source_with_comments) end end end diff --git a/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb b/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb index b1da650dfe..bbf7582ee1 100644 --- a/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb +++ b/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb @@ -458,15 +458,15 @@ describe RuboCop::Cop::Cask::StanzaOrder do include_examples "autocorrects source" end - context "when `on_arch` blocks are out of order" do + context "when `on_arch` blocks and their contents are out of order" do let(:source) do <<~CASK cask 'foo' do on_intel do + url "https://foo.brew.sh/foo-intel.zip" + version :latest sha256 :no_check - - url "https://foo.brew.sh/foo-intel.zip" end on_arm do version :latest @@ -478,22 +478,6 @@ describe RuboCop::Cop::Cask::StanzaOrder do CASK end - let(:expected_offenses) do - [{ - message: "Cask/StanzaOrder: `on_intel` stanza out of order", - severity: :convention, - line: 2, - column: 2, - 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 version :latest\n sha256 :no_check\n\n url \"https://foo.brew.sh/foo-arm.zip\"\n end", # rubocop:disable Layout/LineLength - }] - end - let(:correct_source) do <<~CASK cask 'foo' do @@ -513,122 +497,66 @@ describe RuboCop::Cop::Cask::StanzaOrder do CASK end - include_examples "reports offenses" - include_examples "autocorrects source" - end - - 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.zip" - sha256 "123abc" - version "1.0" - end - on_intel do - url "https://foo.brew.sh/foo-intel.zip" - sha256 "abc123" - version "0.9" # comment here - end - end - CASK - end - - 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 + context "when the `on_os` stanzas and their contents are out of order" do let(:source) do <<~CASK cask "foo" do on_ventura do - sha256 :no_check + sha256 "abc123" + version :latest url "https://foo.brew.sh/foo-ventura.zip" end on_catalina do - sha256 :no_check + sha256 "def456" + version "0.7" url "https://foo.brew.sh/foo-catalina.zip" end on_mojave do - sha256 :no_check + version :latest + sha256 "ghi789" url "https://foo.brew.sh/foo-mojave.zip" end on_big_sur do - sha256 :no_check + sha256 "jkl012" + version :latest + url "https://foo.brew.sh/foo-big-sur.zip" end - - name "Foo" end CASK end - let(:expected_offenses) do - [{ - message: "Cask/StanzaOrder: `on_ventura` stanza out of order", - severity: :convention, - line: 2, - column: 2, - 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 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 sha256 :no_check\n url \"https://foo.brew.sh/foo-big-sur.zip\"\n end", - }] - end - let(:correct_source) do <<~CASK cask "foo" do on_mojave do - sha256 :no_check + version "0.6" + sha256 "ghi789" url "https://foo.brew.sh/foo-mojave.zip" end on_catalina do - sha256 :no_check + sha256 "def456" + version "0.7" url "https://foo.brew.sh/foo-catalina.zip" end on_big_sur do - sha256 :no_check + version "0.8" + sha256 "jkl012" + url "https://foo.brew.sh/foo-big-sur.zip" end on_ventura do + version :latest sha256 :no_check url "https://foo.brew.sh/foo-ventura.zip" end - - name "Foo" end CASK end - include_examples "reports offenses" include_examples "autocorrects source" end end From a4937871259b2688410c5fd2f3a0b02d0f47c28d Mon Sep 17 00:00:00 2001 From: Issy Long Date: Thu, 20 Apr 2023 15:41:04 +0000 Subject: [PATCH 05/10] Regressed a bit, but at least it's not clobbering anymore? --- .../Homebrew/rubocops/cask/ast/cask_block.rb | 7 +--- .../Homebrew/rubocops/cask/stanza_order.rb | 37 ++++++++++--------- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/Library/Homebrew/rubocops/cask/ast/cask_block.rb b/Library/Homebrew/rubocops/cask/ast/cask_block.rb index 929a4aeb67..06825c26bb 100644 --- a/Library/Homebrew/rubocops/cask/ast/cask_block.rb +++ b/Library/Homebrew/rubocops/cask/ast/cask_block.rb @@ -51,10 +51,6 @@ module RuboCop @sorted_toplevel_stanzas ||= sort_stanzas(toplevel_stanzas) end - def sorted_inner_stanzas(stanzas) - sort_stanzas(stanzas) - end - private def sort_stanzas(stanzas) @@ -70,7 +66,8 @@ module RuboCop end def stanza_order_index(stanza) - Constants::STANZA_ORDER.index(stanza.stanza_name) + stanza_name = stanza.respond_to?(:method_name) ? stanza.method_name : stanza.stanza_name + Constants::STANZA_ORDER.index(stanza_name) end end end diff --git a/Library/Homebrew/rubocops/cask/stanza_order.rb b/Library/Homebrew/rubocops/cask/stanza_order.rb index efb15a7977..c1b9545ea9 100644 --- a/Library/Homebrew/rubocops/cask/stanza_order.rb +++ b/Library/Homebrew/rubocops/cask/stanza_order.rb @@ -17,14 +17,17 @@ module RuboCop def on_cask(cask_block) @cask_block = cask_block - add_offenses(toplevel_stanzas) + stanzas = [toplevel_stanzas] - return unless (on_blocks = on_system_methods(toplevel_stanzas)).any? - - on_blocks.map(&:method_node).select(&:block_type?).each do |on_block| - stanzas = inner_stanzas(on_block, processed_source.comments) - add_offenses(stanzas, inner: true) + puts "before on blocks: #{stanzas.first.map(&:stanza_name)}" + if (on_blocks = on_system_methods(stanzas.first)).any? + on_blocks.map(&:method_node).select(&:block_type?).each do |on_block| + stanzas.push(inner_stanzas(on_block, processed_source.comments)) + end end + + puts "after on blocks: #{stanzas.last.map(&:method_node).select(&:send_type?).map(&:method_name) }" if on_blocks + add_offenses(stanzas) end private @@ -32,18 +35,18 @@ module RuboCop attr_reader :cask_block def_delegators :cask_block, :cask_node, :toplevel_stanzas, - :sorted_toplevel_stanzas, :sorted_inner_stanzas + :sorted_toplevel_stanzas - def add_offenses(stanzas, inner: false) - sorted_stanzas = inner ? sorted_inner_stanzas(stanzas) : sorted_toplevel_stanzas - offending_stanzas(stanzas, sorted_stanzas).each do |stanza| - puts "offending stanza: #{stanza.stanza_name}" - message = format(MESSAGE, stanza: stanza.stanza_name) - add_offense(stanza.source_range_with_comments, message: message) do |corrector| - correct_stanza_index = stanzas.index(stanza) - correct_stanza = sorted_stanzas[correct_stanza_index] - puts "correct stanza: #{correct_stanza.stanza_name}" - corrector.replace(stanza.source_range_with_comments, correct_stanza.source_with_comments) + def add_offenses(outer_and_inner_stanzas) + outer_and_inner_stanzas.map do |stanza_types| + offending_stanzas(stanza_types, sorted_toplevel_stanzas).flatten.compact.each do |stanza| + name = stanza.respond_to?(:method_name) ? stanza.method_name : stanza.stanza_name + message = format(MESSAGE, stanza: name) + add_offense(stanza.source_range_with_comments, message: message) do |corrector| + correct_stanza_index = outer_and_inner_stanzas.flatten.index(stanza) + correct_stanza = sorted_toplevel_stanzas[correct_stanza_index] + corrector.replace(stanza&.source_range_with_comments, correct_stanza&.source_with_comments) + end end end end From 8035d46dfe238027bd821f6fa81bf9221e05e65e Mon Sep 17 00:00:00 2001 From: Issy Long Date: Fri, 21 Apr 2023 00:30:45 +0100 Subject: [PATCH 06/10] very wip but reimagined - Thanks to Markus on Slack for saying "the cop should only apply to the content of the blocks, or more specifically only to stanzas that are direct children of cask or on_* blocks", which made me realize that I was overcomplicating things. --- .../Homebrew/rubocops/cask/stanza_order.rb | 42 ++++++++----------- 1 file changed, 17 insertions(+), 25 deletions(-) diff --git a/Library/Homebrew/rubocops/cask/stanza_order.rb b/Library/Homebrew/rubocops/cask/stanza_order.rb index c1b9545ea9..3bb27d366b 100644 --- a/Library/Homebrew/rubocops/cask/stanza_order.rb +++ b/Library/Homebrew/rubocops/cask/stanza_order.rb @@ -17,46 +17,38 @@ module RuboCop def on_cask(cask_block) @cask_block = cask_block - stanzas = [toplevel_stanzas] - puts "before on blocks: #{stanzas.first.map(&:stanza_name)}" - if (on_blocks = on_system_methods(stanzas.first)).any? - on_blocks.map(&:method_node).select(&:block_type?).each do |on_block| - stanzas.push(inner_stanzas(on_block, processed_source.comments)) - end + # Find all the stanzas that are direct children of the cask block or one of its `on_*` blocks. + puts "toplevel_stanzas: #{toplevel_stanzas.map(&:stanza_name).inspect}" + outer_and_inner_stanzas = toplevel_stanzas + toplevel_stanzas.map do |stanza| + return stanza unless stanza.method_node&.block_type? + + inner_stanzas(stanza.method_node, stanza.comments) end - puts "after on blocks: #{stanzas.last.map(&:method_node).select(&:send_type?).map(&:method_name) }" if on_blocks - add_offenses(stanzas) + puts "outer_and_inner_stanzas: #{outer_and_inner_stanzas.flatten.map(&:stanza_name).inspect}" + add_offenses(outer_and_inner_stanzas.flatten) end private attr_reader :cask_block - def_delegators :cask_block, :cask_node, :toplevel_stanzas, - :sorted_toplevel_stanzas + def_delegators :cask_block, :cask_node, :toplevel_stanzas def add_offenses(outer_and_inner_stanzas) - outer_and_inner_stanzas.map do |stanza_types| - offending_stanzas(stanza_types, sorted_toplevel_stanzas).flatten.compact.each do |stanza| - name = stanza.respond_to?(:method_name) ? stanza.method_name : stanza.stanza_name - message = format(MESSAGE, stanza: name) - add_offense(stanza.source_range_with_comments, message: message) do |corrector| - correct_stanza_index = outer_and_inner_stanzas.flatten.index(stanza) - correct_stanza = sorted_toplevel_stanzas[correct_stanza_index] - corrector.replace(stanza&.source_range_with_comments, correct_stanza&.source_with_comments) - end + outer_and_inner_stanzas.each_cons(2) do |stanza1, stanza2| + next if stanza_order_index(stanza1.stanza_name) < stanza_order_index(stanza2.stanza_name) + + puts "#{stanza2.stanza_name} should come before #{stanza1.stanza_name}" + add_offense(stanza1.method_node, message: format(MESSAGE, stanza: stanza1.stanza_name)) do |corrector| + # TODO: Move the stanza to the correct location. end end end - def offending_stanzas(stanzas, sorted_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 - end + def stanza_order_index(stanza_name) + RuboCop::Cask::Constants::STANZA_ORDER.index(stanza_name) end end end From 2dea4f2370c88eea158996d8a4006779fce0c29d Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Sat, 22 Apr 2023 04:00:04 +0200 Subject: [PATCH 07/10] Add `on_cask_stanza_block`. --- Library/Homebrew/rubocops/cask/extend/node.rb | 3 ++- .../Homebrew/rubocops/cask/mixin/cask_help.rb | 16 +++++++++++----- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/Library/Homebrew/rubocops/cask/extend/node.rb b/Library/Homebrew/rubocops/cask/extend/node.rb index ec79aa4f3f..2fa13eabc7 100644 --- a/Library/Homebrew/rubocops/cask/extend/node.rb +++ b/Library/Homebrew/rubocops/cask/extend/node.rb @@ -14,7 +14,8 @@ module RuboCop def_node_matcher :key_node, "{(pair $_ _) (hash (pair $_ _) ...)}" def_node_matcher :val_node, "{(pair _ $_) (hash (pair _ $_) ...)}" - def_node_matcher :cask_block?, "(block (send nil? :cask _) args ...)" + def_node_matcher :cask_block?, "(block (send nil? :cask ...) args ...)" + def_node_matcher :on_system_block?, "(block (send nil? {#{ON_SYSTEM_METHODS.map(&:inspect).join(" ")}} ...) args ...)" def_node_matcher :arch_variable?, "(lvasgn _ (send nil? :on_arch_conditional ...))" def_node_matcher :begin_block?, "(begin ...)" diff --git a/Library/Homebrew/rubocops/cask/mixin/cask_help.rb b/Library/Homebrew/rubocops/cask/mixin/cask_help.rb index f4ba80c8f1..8126c33660 100644 --- a/Library/Homebrew/rubocops/cask/mixin/cask_help.rb +++ b/Library/Homebrew/rubocops/cask/mixin/cask_help.rb @@ -13,14 +13,20 @@ module RuboCop sig { abstract.params(cask_block: RuboCop::Cask::AST::CaskBlock).void } def on_cask(cask_block); end + def on_cask_stanza_block(cask_stanza_block); end + def on_block(block_node) super if defined? super - return unless respond_to?(:on_cask) - return unless block_node.cask_block? - comments = processed_source.comments - cask_block = RuboCop::Cask::AST::CaskBlock.new(block_node, comments) - on_cask(cask_block) + if respond_to?(:on_cask_stanza_block) && (block_node.cask_block? || block_node.on_system_block?) + on_cask_stanza_block(block_node) + end + + if respond_to?(:on_cask) && block_node.cask_block? + comments = processed_source.comments + cask_block = RuboCop::Cask::AST::CaskBlock.new(block_node, comments) + on_cask(cask_block) + end end def on_system_methods(cask_stanzas) From 81fdb3716ea390be9ea49b6b49ef8ca8817c2090 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Sun, 7 May 2023 08:23:19 +0200 Subject: [PATCH 08/10] Implement `StanzaOrder` cop using `on_cask_stanza_block`. --- .../Homebrew/rubocops/cask/ast/cask_block.rb | 70 +++++++++++-------- Library/Homebrew/rubocops/cask/ast/stanza.rb | 6 +- Library/Homebrew/rubocops/cask/extend/node.rb | 10 ++- .../Homebrew/rubocops/cask/mixin/cask_help.rb | 34 +++++---- .../Homebrew/rubocops/cask/stanza_order.rb | 60 +++++++++------- .../sorbet/rbi/hidden-definitions/hidden.rbi | 51 ++++++++++++++ .../rubocops/cask/shared_examples/cask_cop.rb | 20 +++++- .../test/rubocops/cask/stanza_order_spec.rb | 10 +-- 8 files changed, 184 insertions(+), 77 deletions(-) diff --git a/Library/Homebrew/rubocops/cask/ast/cask_block.rb b/Library/Homebrew/rubocops/cask/ast/cask_block.rb index 06825c26bb..9d19220365 100644 --- a/Library/Homebrew/rubocops/cask/ast/cask_block.rb +++ b/Library/Homebrew/rubocops/cask/ast/cask_block.rb @@ -6,27 +6,58 @@ require "forwardable" module RuboCop module Cask module AST - # This class wraps the AST block node that represents the entire cask - # definition. It includes various helper methods to aid cops in their - # analysis. - class CaskBlock - extend Forwardable + class StanzaBlock + extend T::Helpers + extend T::Sig + sig { returns(RuboCop::AST::BlockNode) } + attr_reader :block_node + + sig { returns(T::Array[Parser::Source::Comment]) } + attr_reader :comments + + sig { params(block_node: RuboCop::AST::BlockNode, comments: T::Array[Parser::Source::Comment]).void } def initialize(block_node, comments) @block_node = block_node @comments = comments end - attr_reader :block_node, :comments + sig { returns(T::Array[Stanza]) } + def stanzas + return [] unless (block_body = block_node.block_body) - alias cask_node block_node + # If a block only contains one stanza, it is that stanza's direct parent, otherwise + # stanzas are grouped in a nested block and the block is that nested block's parent. + is_stanza = if block_body.begin_block? + ->(node) { node.parent.parent == block_node } + else + ->(node) { node.parent == block_node } + end + + @stanzas ||= block_body.each_node + .select(&:stanza?) + .select(&is_stanza) + .map { |node| Stanza.new(node, comments) } + end + end + + # This class wraps the AST block node that represents the entire cask + # definition. It includes various helper methods to aid cops in their + # analysis. + class CaskBlock < StanzaBlock + extend Forwardable + + def cask_node + block_node + end def_delegator :cask_node, :block_body, :cask_body def header - @header ||= CaskHeader.new(cask_node.method_node) + @header ||= CaskHeader.new(block_node.method_node) end + # TODO: Use `StanzaBlock#stanzas` for all cops, where possible. def stanzas return [] unless cask_body @@ -46,29 +77,6 @@ module RuboCop @toplevel_stanzas ||= stanzas.select(&is_toplevel_stanza) end - - def sorted_toplevel_stanzas - @sorted_toplevel_stanzas ||= sort_stanzas(toplevel_stanzas) - end - - private - - def sort_stanzas(stanzas) - stanzas.sort do |s1, s2| - i1 = stanza_order_index(s1) - i2 = stanza_order_index(s2) - if i1 == i2 || i1.blank? || i2.blank? - i1 = stanzas.index(s1) - i2 = stanzas.index(s2) - end - i1 - i2 - end - end - - def stanza_order_index(stanza) - stanza_name = stanza.respond_to?(:method_name) ? stanza.method_name : stanza.stanza_name - Constants::STANZA_ORDER.index(stanza_name) - end end end end diff --git a/Library/Homebrew/rubocops/cask/ast/stanza.rb b/Library/Homebrew/rubocops/cask/ast/stanza.rb index 68b6de8218..e9cc111b3a 100644 --- a/Library/Homebrew/rubocops/cask/ast/stanza.rb +++ b/Library/Homebrew/rubocops/cask/ast/stanza.rb @@ -23,6 +23,7 @@ module RuboCop def_delegator :stanza_node, :parent, :parent_node def_delegator :stanza_node, :arch_variable? + def_delegator :stanza_node, :on_system_block? def source_range stanza_node.location_expression @@ -48,6 +49,10 @@ module RuboCop Constants::STANZA_GROUP_HASH[stanza_name] end + def stanza_index + Constants::STANZA_ORDER.index(stanza_name) + end + def same_group?(other) stanza_group == other.stanza_group end @@ -65,7 +70,6 @@ module RuboCop def ==(other) self.class == other.class && stanza_node == other.stanza_node end - alias eql? == Constants::STANZA_ORDER.each do |stanza_name| diff --git a/Library/Homebrew/rubocops/cask/extend/node.rb b/Library/Homebrew/rubocops/cask/extend/node.rb index 2fa13eabc7..5dbd6b9770 100644 --- a/Library/Homebrew/rubocops/cask/extend/node.rb +++ b/Library/Homebrew/rubocops/cask/extend/node.rb @@ -5,6 +5,8 @@ module RuboCop module AST # Extensions for RuboCop's AST Node class. class Node + extend T::Sig + include RuboCop::Cask::Constants def_node_matcher :method_node, "{$(send ...) (block $(send ...) ...)}" @@ -15,11 +17,17 @@ module RuboCop def_node_matcher :val_node, "{(pair _ $_) (hash (pair _ $_) ...)}" def_node_matcher :cask_block?, "(block (send nil? :cask ...) args ...)" - def_node_matcher :on_system_block?, "(block (send nil? {#{ON_SYSTEM_METHODS.map(&:inspect).join(" ")}} ...) args ...)" + def_node_matcher :on_system_block?, + "(block (send nil? {#{ON_SYSTEM_METHODS.map(&:inspect).join(" ")}} ...) args ...)" def_node_matcher :arch_variable?, "(lvasgn _ (send nil? :on_arch_conditional ...))" def_node_matcher :begin_block?, "(begin ...)" + sig { returns(T::Boolean) } + def cask_on_system_block? + (on_system_block? && each_ancestor.any?(&:cask_block?)) || false + end + def stanza? return true if arch_variable? diff --git a/Library/Homebrew/rubocops/cask/mixin/cask_help.rb b/Library/Homebrew/rubocops/cask/mixin/cask_help.rb index 8126c33660..093b93ec61 100644 --- a/Library/Homebrew/rubocops/cask/mixin/cask_help.rb +++ b/Library/Homebrew/rubocops/cask/mixin/cask_help.rb @@ -6,31 +6,39 @@ module RuboCop module Cask # Common functionality for cops checking casks. module CaskHelp - extend T::Helpers + prepend CommentsHelp - abstract! - - sig { abstract.params(cask_block: RuboCop::Cask::AST::CaskBlock).void } + sig { overridable.params(cask_block: RuboCop::Cask::AST::CaskBlock).void } def on_cask(cask_block); end + sig { overridable.params(cask_stanza_block: RuboCop::Cask::AST::StanzaBlock).void } def on_cask_stanza_block(cask_stanza_block); end + # FIXME: Workaround until https://github.com/rubocop/rubocop/pull/11858 is released. + def find_end_line(node) + return node.loc.end.line if node.block_type? || node.numblock_type? + + super + end + + sig { params(block_node: RuboCop::AST::BlockNode).void } def on_block(block_node) super if defined? super - if respond_to?(:on_cask_stanza_block) && (block_node.cask_block? || block_node.on_system_block?) - on_cask_stanza_block(block_node) - end + return if !block_node.cask_block? && !block_node.cask_on_system_block? - if respond_to?(:on_cask) && block_node.cask_block? - comments = processed_source.comments - cask_block = RuboCop::Cask::AST::CaskBlock.new(block_node, comments) - on_cask(cask_block) - end + comments = comments_in_range(block_node).to_a + stanza_block = RuboCop::Cask::AST::StanzaBlock.new(block_node, comments) + on_cask_stanza_block(stanza_block) + + return unless block_node.cask_block? + + cask_block = RuboCop::Cask::AST::CaskBlock.new(block_node, comments) + on_cask(cask_block) end def on_system_methods(cask_stanzas) - cask_stanzas.select { |s| RuboCop::Cask::Constants::ON_SYSTEM_METHODS.include?(s.stanza_name) } + cask_stanzas.select(&:on_system_block?) end def inner_stanzas(block_node, comments) diff --git a/Library/Homebrew/rubocops/cask/stanza_order.rb b/Library/Homebrew/rubocops/cask/stanza_order.rb index 3bb27d366b..8ec1929138 100644 --- a/Library/Homebrew/rubocops/cask/stanza_order.rb +++ b/Library/Homebrew/rubocops/cask/stanza_order.rb @@ -9,45 +9,57 @@ module RuboCop # This cop checks that a cask's stanzas are ordered correctly, including nested within `on_*` blocks. # @see https://docs.brew.sh/Cask-Cookbook#stanza-order class StanzaOrder < Base + include IgnoredNode extend Forwardable extend AutoCorrector include CaskHelp MESSAGE = "`%s` stanza out of order" - def on_cask(cask_block) - @cask_block = cask_block + def on_cask_stanza_block(stanza_block) + stanzas = stanza_block.stanzas + ordered_stanzas = sort_stanzas(stanzas) - # Find all the stanzas that are direct children of the cask block or one of its `on_*` blocks. - puts "toplevel_stanzas: #{toplevel_stanzas.map(&:stanza_name).inspect}" - outer_and_inner_stanzas = toplevel_stanzas + toplevel_stanzas.map do |stanza| - return stanza unless stanza.method_node&.block_type? + return if stanzas == ordered_stanzas - inner_stanzas(stanza.method_node, stanza.comments) - end + stanzas.zip(ordered_stanzas).each do |stanza_before, stanza_after| + next if stanza_before == stanza_after - puts "outer_and_inner_stanzas: #{outer_and_inner_stanzas.flatten.map(&:stanza_name).inspect}" - add_offenses(outer_and_inner_stanzas.flatten) - end + add_offense( + stanza_before.method_node, + message: format(MESSAGE, stanza: stanza_before.stanza_name), + ) do |corrector| + next if part_of_ignored_node?(stanza_before.method_node) - private + corrector.replace( + stanza_before.source_range_with_comments, + stanza_after.source_with_comments, + ) - attr_reader :cask_block - - def_delegators :cask_block, :cask_node, :toplevel_stanzas - - def add_offenses(outer_and_inner_stanzas) - outer_and_inner_stanzas.each_cons(2) do |stanza1, stanza2| - next if stanza_order_index(stanza1.stanza_name) < stanza_order_index(stanza2.stanza_name) - - puts "#{stanza2.stanza_name} should come before #{stanza1.stanza_name}" - add_offense(stanza1.method_node, message: format(MESSAGE, stanza: stanza1.stanza_name)) do |corrector| - # TODO: Move the stanza to the correct location. + # Ignore node so that nested content is not auto-corrected and clobbered. + ignore_node(stanza_before.method_node) end end end - def stanza_order_index(stanza_name) + private + + def sort_stanzas(stanzas) + stanzas.sort do |stanza1, stanza2| + i1 = stanza1.stanza_index + i2 = stanza2.stanza_index + + if i1 == i2 + i1 = stanzas.index(stanza1) + i2 = stanzas.index(stanza2) + end + + i1 - i2 + end + end + + def stanza_order_index(stanza) + stanza_name = stanza.respond_to?(:method_name) ? stanza.method_name : stanza.stanza_name RuboCop::Cask::Constants::STANZA_ORDER.index(stanza_name) end end diff --git a/Library/Homebrew/sorbet/rbi/hidden-definitions/hidden.rbi b/Library/Homebrew/sorbet/rbi/hidden-definitions/hidden.rbi index 5eed0ed4c0..95271192b2 100644 --- a/Library/Homebrew/sorbet/rbi/hidden-definitions/hidden.rbi +++ b/Library/Homebrew/sorbet/rbi/hidden-definitions/hidden.rbi @@ -6373,6 +6373,8 @@ class RuboCop::AST::Node def method_node(param0=T.unsafe(nil)); end + def on_system_block?(param0=T.unsafe(nil)); end + def val_node(param0=T.unsafe(nil)); end end @@ -6504,12 +6506,61 @@ class RuboCop::Cask::AST::Stanza def zap?(); end end +module RuboCop::Cop::Cask::CaskHelp + include ::RuboCop::Cop::CommentsHelp +end + module RuboCop::Cop::Cask::CaskHelp extend ::T::Private::Abstract::Hooks extend ::T::InterfaceWrapper::Helpers end +class RuboCop::Cop::Cask::Desc + include ::RuboCop::Cop::CommentsHelp +end + +class RuboCop::Cop::Cask::HomepageUrlTrailingSlash + include ::RuboCop::Cop::CommentsHelp +end + +class RuboCop::Cop::Cask::NoDslVersion + include ::RuboCop::Cop::CommentsHelp +end + +class RuboCop::Cop::Cask::NoOverrides + include ::RuboCop::Cop::CommentsHelp +end + +module RuboCop::Cop::Cask::OnDescStanza + include ::RuboCop::Cop::CommentsHelp +end + +module RuboCop::Cop::Cask::OnHomepageStanza + include ::RuboCop::Cop::CommentsHelp +end + +class RuboCop::Cop::Cask::OnSystemConditionals + include ::RuboCop::Cop::CommentsHelp +end + +module RuboCop::Cop::Cask::OnUrlStanza + include ::RuboCop::Cop::CommentsHelp +end + +class RuboCop::Cop::Cask::StanzaGrouping + include ::RuboCop::Cop::CommentsHelp +end + +class RuboCop::Cop::Cask::StanzaOrder + include ::RuboCop::Cop::CommentsHelp +end + +class RuboCop::Cop::Cask::Url + include ::RuboCop::Cop::CommentsHelp +end + class RuboCop::Cop::Cask::Variables + include ::RuboCop::Cop::CommentsHelp def variable_assignment(param0); end end diff --git a/Library/Homebrew/test/rubocops/cask/shared_examples/cask_cop.rb b/Library/Homebrew/test/rubocops/cask/shared_examples/cask_cop.rb index 6436b09c95..cc0235647c 100644 --- a/Library/Homebrew/test/rubocops/cask/shared_examples/cask_cop.rb +++ b/Library/Homebrew/test/rubocops/cask/shared_examples/cask_cop.rb @@ -39,8 +39,24 @@ module CaskCop expect(actual.location.source).to eq(expected[:source]) end + # TODO: Replace with `expect_correction` from `rubocop-rspec`. def expect_autocorrected_source(source, correct_source) - new_source = autocorrect_source(source) - expect(new_source).to eq(Array(correct_source).join("\n")) + correct_source = Array(correct_source).join("\n") + + current_source = source + + # RuboCop runs auto-correction in a loop to handle nested offenses. + loop do + current_source = autocorrect_source(current_source) + + if (ignored_nodes = cop.instance_variable_get(:@ignored_nodes)) && ignored_nodes.any? + ignored_nodes.clear + next + end + + break + end + + expect(current_source).to eq correct_source end end diff --git a/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb b/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb index bbf7582ee1..cfa658257c 100644 --- a/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb +++ b/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb @@ -489,8 +489,8 @@ describe RuboCop::Cop::Cask::StanzaOrder do end on_intel do version :latest - sha256 :no_check + sha256 :no_check url "https://foo.brew.sh/foo-intel.zip" end end @@ -533,24 +533,24 @@ describe RuboCop::Cop::Cask::StanzaOrder do <<~CASK cask "foo" do on_mojave do - version "0.6" + version :latest sha256 "ghi789" url "https://foo.brew.sh/foo-mojave.zip" end on_catalina do - sha256 "def456" version "0.7" + sha256 "def456" url "https://foo.brew.sh/foo-catalina.zip" end on_big_sur do - version "0.8" + version :latest sha256 "jkl012" url "https://foo.brew.sh/foo-big-sur.zip" end on_ventura do version :latest - sha256 :no_check + sha256 "abc123" url "https://foo.brew.sh/foo-ventura.zip" end end From ce8788e5bcd23bb885fd7951622f93688a2abfb1 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Sun, 7 May 2023 08:34:13 +0200 Subject: [PATCH 09/10] Fix code style. --- Library/Homebrew/rubocops/cask/ast/cask_block.rb | 1 - Library/Homebrew/rubocops/cask/extend/node.rb | 2 -- Library/Homebrew/rubocops/cask/no_overrides.rb | 2 +- Library/Homebrew/rubocops/cask/stanza_grouping.rb | 2 +- 4 files changed, 2 insertions(+), 5 deletions(-) diff --git a/Library/Homebrew/rubocops/cask/ast/cask_block.rb b/Library/Homebrew/rubocops/cask/ast/cask_block.rb index 9d19220365..f04aa1033b 100644 --- a/Library/Homebrew/rubocops/cask/ast/cask_block.rb +++ b/Library/Homebrew/rubocops/cask/ast/cask_block.rb @@ -8,7 +8,6 @@ module RuboCop module AST class StanzaBlock extend T::Helpers - extend T::Sig sig { returns(RuboCop::AST::BlockNode) } attr_reader :block_node diff --git a/Library/Homebrew/rubocops/cask/extend/node.rb b/Library/Homebrew/rubocops/cask/extend/node.rb index 5dbd6b9770..54b8654df2 100644 --- a/Library/Homebrew/rubocops/cask/extend/node.rb +++ b/Library/Homebrew/rubocops/cask/extend/node.rb @@ -5,8 +5,6 @@ module RuboCop module AST # Extensions for RuboCop's AST Node class. class Node - extend T::Sig - include RuboCop::Cask::Constants def_node_matcher :method_node, "{$(send ...) (block $(send ...) ...)}" diff --git a/Library/Homebrew/rubocops/cask/no_overrides.rb b/Library/Homebrew/rubocops/cask/no_overrides.rb index dc704f8575..aa2721d1f3 100644 --- a/Library/Homebrew/rubocops/cask/no_overrides.rb +++ b/Library/Homebrew/rubocops/cask/no_overrides.rb @@ -21,7 +21,7 @@ module RuboCop def on_cask(cask_block) cask_stanzas = cask_block.toplevel_stanzas - return unless (on_blocks = on_system_methods(cask_stanzas)).any? + return if (on_blocks = on_system_methods(cask_stanzas)).none? stanzas_in_blocks = on_system_stanzas(on_blocks) diff --git a/Library/Homebrew/rubocops/cask/stanza_grouping.rb b/Library/Homebrew/rubocops/cask/stanza_grouping.rb index 489c7ba061..93fcaf39b8 100644 --- a/Library/Homebrew/rubocops/cask/stanza_grouping.rb +++ b/Library/Homebrew/rubocops/cask/stanza_grouping.rb @@ -23,7 +23,7 @@ module RuboCop cask_stanzas = cask_block.toplevel_stanzas add_offenses(cask_stanzas) - return unless (on_blocks = on_system_methods(cask_stanzas)).any? + return if (on_blocks = on_system_methods(cask_stanzas)).none? on_blocks.map(&:method_node).select(&:block_type?).each do |on_block| stanzas = inner_stanzas(on_block, processed_source.comments) From 1df501b0ac545fa97353efecab956bf0d415b380 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Sun, 7 May 2023 10:04:28 +0200 Subject: [PATCH 10/10] Tweak tests. --- .../Homebrew/rubocops/cask/stanza_order.rb | 6 + .../rubocops/cask/shared_examples/cask_cop.rb | 4 +- .../test/rubocops/cask/stanza_order_spec.rb | 143 +++++++++--------- 3 files changed, 80 insertions(+), 73 deletions(-) diff --git a/Library/Homebrew/rubocops/cask/stanza_order.rb b/Library/Homebrew/rubocops/cask/stanza_order.rb index 8ec1929138..dfbb6e9432 100644 --- a/Library/Homebrew/rubocops/cask/stanza_order.rb +++ b/Library/Homebrew/rubocops/cask/stanza_order.rb @@ -42,6 +42,12 @@ module RuboCop end end + def on_new_investigation + super + + ignored_nodes.clear + end + private def sort_stanzas(stanzas) diff --git a/Library/Homebrew/test/rubocops/cask/shared_examples/cask_cop.rb b/Library/Homebrew/test/rubocops/cask/shared_examples/cask_cop.rb index cc0235647c..c5cdcde9e8 100644 --- a/Library/Homebrew/test/rubocops/cask/shared_examples/cask_cop.rb +++ b/Library/Homebrew/test/rubocops/cask/shared_examples/cask_cop.rb @@ -27,11 +27,11 @@ module CaskCop offenses = inspect_source(source) expect(offenses.size).to eq(expected_offenses.size) expected_offenses.zip(offenses).each do |expected, actual| - expect_offense(expected, actual) + expect_offense2(expected, actual) end end - def expect_offense(expected, actual) + def expect_offense2(expected, actual) expect(actual.message).to eq(expected[:message]) expect(actual.severity).to eq(expected[:severity]) expect(actual.line).to eq(expected[:line]) diff --git a/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb b/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb index cfa658257c..9075f1f6a2 100644 --- a/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb +++ b/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb @@ -3,11 +3,9 @@ require "rubocops/rubocop-cask" require "test/rubocops/cask/shared_examples/cask_cop" -describe RuboCop::Cop::Cask::StanzaOrder do +describe RuboCop::Cop::Cask::StanzaOrder, :config do include CaskCop - subject(:cop) { described_class.new } - context "when there is only one stanza" do let(:source) do <<~CASK @@ -55,13 +53,13 @@ describe RuboCop::Cop::Cask::StanzaOrder do end let(:expected_offenses) do [{ - message: "Cask/StanzaOrder: `sha256` stanza out of order", + message: "`sha256` stanza out of order", severity: :convention, line: 2, column: 2, source: "sha256 :no_check", }, { - message: "Cask/StanzaOrder: `version` stanza out of order", + message: "`version` stanza out of order", severity: :convention, line: 3, column: 2, @@ -95,19 +93,19 @@ describe RuboCop::Cop::Cask::StanzaOrder do end let(:expected_offenses) do [{ - message: "Cask/StanzaOrder: `version` stanza out of order", + message: "`version` stanza out of order", severity: :convention, line: 2, column: 2, source: "version :latest", }, { - message: "Cask/StanzaOrder: `sha256` stanza out of order", + message: "`sha256` stanza out of order", severity: :convention, line: 3, column: 2, source: "sha256 :no_check", }, { - message: "Cask/StanzaOrder: `arch` stanza out of order", + message: "`arch` stanza out of order", severity: :convention, line: 4, column: 2, @@ -143,13 +141,13 @@ describe RuboCop::Cop::Cask::StanzaOrder do end let(:expected_offenses) do [{ - message: "Cask/StanzaOrder: `sha256` stanza out of order", + message: "`sha256` stanza out of order", severity: :convention, line: 3, column: 2, source: "sha256 :no_check", }, { - message: "Cask/StanzaOrder: `on_arch_conditional` stanza out of order", + message: "`on_arch_conditional` stanza out of order", severity: :convention, line: 5, column: 2, @@ -185,13 +183,13 @@ describe RuboCop::Cop::Cask::StanzaOrder do end let(:expected_offenses) do [{ - message: "Cask/StanzaOrder: `on_arch_conditional` stanza out of order", + message: "`on_arch_conditional` stanza out of order", severity: :convention, line: 2, column: 2, source: 'folder = on_arch_conditional arm: "darwin-arm64", intel: "darwin"', }, { - message: "Cask/StanzaOrder: `arch` stanza out of order", + message: "`arch` stanza out of order", severity: :convention, line: 3, column: 2, @@ -231,26 +229,26 @@ describe RuboCop::Cop::Cask::StanzaOrder do end let(:expected_offenses) do [{ - message: "Cask/StanzaOrder: `url` stanza out of order", + message: "`url` stanza out of order", severity: :convention, line: 2, column: 2, source: "url 'https://foo.brew.sh/foo.zip'", }, { - message: "Cask/StanzaOrder: `uninstall` stanza out of order", + message: "`uninstall` stanza out of order", severity: :convention, line: 3, column: 2, source: "uninstall :quit => 'com.example.foo'," \ "\n :kext => 'com.example.foo.kext'", }, { - message: "Cask/StanzaOrder: `version` stanza out of order", + message: "`version` stanza out of order", severity: :convention, line: 5, column: 2, source: "version :latest", }, { - message: "Cask/StanzaOrder: `sha256` stanza out of order", + message: "`sha256` stanza out of order", severity: :convention, line: 7, column: 2, @@ -500,63 +498,66 @@ describe RuboCop::Cop::Cask::StanzaOrder do include_examples "autocorrects source" end - context "when the `on_os` stanzas and their contents are out of order" do - let(:source) do - <<~CASK - cask "foo" do - on_ventura do - sha256 "abc123" - version :latest - url "https://foo.brew.sh/foo-ventura.zip" - end - on_catalina do - sha256 "def456" - version "0.7" - url "https://foo.brew.sh/foo-catalina.zip" - end - on_mojave do - version :latest - sha256 "ghi789" - url "https://foo.brew.sh/foo-mojave.zip" - end - on_big_sur do - sha256 "jkl012" - version :latest - - url "https://foo.brew.sh/foo-big-sur.zip" - end + it "registers an offense when `on_os` stanzas and their contents are out of order" do + expect_offense <<~CASK + cask "foo" do + on_ventura do + ^^^^^^^^^^^^^ `on_ventura` stanza out of order + sha256 "abc123" + ^^^^^^^^^^^^^^^ `sha256` stanza out of order + version :latest + ^^^^^^^^^^^^^^^ `version` stanza out of order + url "https://foo.brew.sh/foo-ventura.zip" end - CASK - end - - let(:correct_source) do - <<~CASK - cask "foo" do - on_mojave do - version :latest - sha256 "ghi789" - url "https://foo.brew.sh/foo-mojave.zip" - end - on_catalina do - version "0.7" - sha256 "def456" - url "https://foo.brew.sh/foo-catalina.zip" - end - on_big_sur do - version :latest - sha256 "jkl012" - - url "https://foo.brew.sh/foo-big-sur.zip" - end - on_ventura do - version :latest - sha256 "abc123" - url "https://foo.brew.sh/foo-ventura.zip" - end + on_catalina do + sha256 "def456" + ^^^^^^^^^^^^^^^ `sha256` stanza out of order + version "0.7" + ^^^^^^^^^^^^^ `version` stanza out of order + url "https://foo.brew.sh/foo-catalina.zip" end - CASK - end + on_mojave do + ^^^^^^^^^^^^ `on_mojave` stanza out of order + version :latest + sha256 "ghi789" + url "https://foo.brew.sh/foo-mojave.zip" + end + on_big_sur do + ^^^^^^^^^^^^^ `on_big_sur` stanza out of order + sha256 "jkl012" + ^^^^^^^^^^^^^^^ `sha256` stanza out of order + version :latest + ^^^^^^^^^^^^^^^ `version` stanza out of order - include_examples "autocorrects source" + url "https://foo.brew.sh/foo-big-sur.zip" + end + end + CASK + + expect_correction <<~CASK + cask "foo" do + on_mojave do + version :latest + sha256 "ghi789" + url "https://foo.brew.sh/foo-mojave.zip" + end + on_catalina do + version "0.7" + sha256 "def456" + url "https://foo.brew.sh/foo-catalina.zip" + end + on_big_sur do + version :latest + sha256 "jkl012" + + url "https://foo.brew.sh/foo-big-sur.zip" + end + on_ventura do + version :latest + sha256 "abc123" + url "https://foo.brew.sh/foo-ventura.zip" + end + end + CASK end end