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