From af99dfba003d38ba248fb62267f70789c17f19e4 Mon Sep 17 00:00:00 2001 From: Rylan Polster Date: Sun, 14 Aug 2022 15:39:20 -0400 Subject: [PATCH] Refactor `on_system` rubocops for use in casks --- .../rubocops/cask/on_system_conditionals.rb | 52 +++++ Library/Homebrew/rubocops/lines.rb | 207 ++---------------- Library/Homebrew/rubocops/rubocop-cask.rb | 1 + .../shared/on_system_conditionals_helper.rb | 196 +++++++++++++++++ .../cask/on_system_conditionals_spec.rb | 140 ++++++++++++ 5 files changed, 403 insertions(+), 193 deletions(-) create mode 100644 Library/Homebrew/rubocops/cask/on_system_conditionals.rb create mode 100644 Library/Homebrew/rubocops/shared/on_system_conditionals_helper.rb create mode 100644 Library/Homebrew/test/rubocops/cask/on_system_conditionals_spec.rb diff --git a/Library/Homebrew/rubocops/cask/on_system_conditionals.rb b/Library/Homebrew/rubocops/cask/on_system_conditionals.rb new file mode 100644 index 0000000000..42c741aa63 --- /dev/null +++ b/Library/Homebrew/rubocops/cask/on_system_conditionals.rb @@ -0,0 +1,52 @@ +# typed: true +# frozen_string_literal: true + +require "forwardable" +require "rubocops/shared/on_system_conditionals_helper" + +module RuboCop + module Cop + module Cask + # This cop makes sure that OS conditionals are consistent. + # + # @example + # # bad + # cask 'foo' do + # if MacOS.version == :high_sierra + # sha256 "..." + # end + # end + # + # # good + # cask 'foo' do + # on_high_sierra do + # sha256 "..." + # end + # end + class OnSystemConditionals < Base + extend Forwardable + extend AutoCorrector + include OnSystemConditionalsHelper + include CaskHelp + + FLIGHT_STANZA_NAMES = [:preflight, :postflight, :uninstall_preflight, :uninstall_postflight].freeze + + def on_cask(cask_block) + @cask_block = cask_block + + toplevel_stanzas.each do |stanza| + next unless FLIGHT_STANZA_NAMES.include? stanza.stanza_name + + audit_on_system_blocks(stanza.stanza_node, stanza.stanza_name) + end + end + + private + + attr_reader :cask_block + + def_delegators :cask_block, :toplevel_stanzas, :cask_body + end + end + end +end diff --git a/Library/Homebrew/rubocops/lines.rb b/Library/Homebrew/rubocops/lines.rb index d8fd97bead..215100829f 100644 --- a/Library/Homebrew/rubocops/lines.rb +++ b/Library/Homebrew/rubocops/lines.rb @@ -3,6 +3,7 @@ require "macos_versions" require "rubocops/extend/formula" +require "rubocops/shared/on_system_conditionals_helper" module RuboCop module Cop @@ -377,104 +378,20 @@ module RuboCop # # @api private class OnSystemConditionals < FormulaCop + include OnSystemConditionalsHelper extend AutoCorrector NO_ON_SYSTEM_METHOD_NAMES = [:install, :post_install].freeze NO_ON_SYSTEM_BLOCK_NAMES = [:service, :test].freeze - ON_ARCH_OPTIONS = [:intel, :arm].freeze - ON_BASE_OS_OPTIONS = [:macos, :linux].freeze - ON_MACOS_VERSION_OPTIONS = MacOSVersions::SYMBOLS.keys.freeze - ALL_SYSTEM_OPTIONS = [*ON_ARCH_OPTIONS, *ON_BASE_OS_OPTIONS, *ON_MACOS_VERSION_OPTIONS, :system].freeze - - MACOS_VERSION_CONDITIONALS = { - "==" => nil, - "<=" => :or_older, - ">=" => :or_newer, - }.freeze - - ON_SYSTEM_CONDITIONALS = [:<, :<=].freeze - - def on_system_method_info(on_system_option) - info = {} - info[:method] = :"on_#{on_system_option}" - info[:if_module], info[:if_method] = if ON_ARCH_OPTIONS.include?(on_system_option) - ["Hardware::CPU", :"#{on_system_option}?"] - elsif ON_BASE_OS_OPTIONS.include?(on_system_option) - ["OS", (on_system_option == :macos) ? :mac? : :linux?] - else - ["MacOS", :version] - end - info[:on_system_string] = "on_#{on_system_option}" - info[:if_string] = if on_system_option == :system - "if OS.linux? || MacOS.version" - else - "if #{info[:if_module]}.#{info[:if_method]}" - end - - info - end - def audit_formula(_node, _class_node, _parent_class_node, body_node) - top_level_nodes_to_check = [] NO_ON_SYSTEM_METHOD_NAMES.each do |formula_method_name| method_node = find_method_def(body_node, formula_method_name) - top_level_nodes_to_check << [formula_method_name, method_node] if method_node + audit_on_system_blocks(method_node, formula_method_name) if method_node end NO_ON_SYSTEM_BLOCK_NAMES.each do |formula_block_name| block_node = find_block(body_node, formula_block_name) - top_level_nodes_to_check << [formula_block_name, block_node] if block_node - end - - ALL_SYSTEM_OPTIONS.each do |on_system_option| - method_info = on_system_method_info(on_system_option) - - top_level_nodes_to_check.each do |top_level_name, top_level_node| - top_level_node_string = if top_level_node.def_type? - "def #{top_level_name}" - else - "#{top_level_name} do" - end - - find_every_method_call_by_name(top_level_node, method_info[:method]).each do |method| - if ON_MACOS_VERSION_OPTIONS.include?(on_system_option) - on_macos_version_method_call(method, on_method: method_info[:method]) do |on_method_parameters| - if on_method_parameters.empty? - method_info[:if_string] = "if MacOS.version == :#{on_system_option}" - else - method_info[:on_system_string] = "#{method_info[:method]} :#{on_method_parameters.first}" - if_condition_operator = MACOS_VERSION_CONDITIONALS.key(on_method_parameters.first) - method_info[:if_string] = "if MacOS.version #{if_condition_operator} :#{on_system_option}" - end - end - elsif method_info[:method] == :on_system - on_system_method_call(method) do |macos_symbol| - base_os, condition = macos_symbol.to_s.split(/_(?=or_)/).map(&:to_sym) - method_info[:on_system_string] = if condition.present? - "on_system :linux, macos: :#{base_os}_#{condition}" - else - "on_system :linux, macos: :#{base_os}" - end - if_condition_operator = MACOS_VERSION_CONDITIONALS.key(condition) - method_info[:if_string] = "if OS.linux? || MacOS.version #{if_condition_operator} :#{base_os}" - end - end - - offending_node(method) - - problem "Don't use `#{method_info[:on_system_string]}` in `#{top_level_node_string}`, " \ - "use `#{method_info[:if_string]}` instead." do |corrector| - block_node = offending_node.parent - next if block_node.type != :block - - # TODO: could fix corrector to handle this but punting for now. - next if block_node.single_line? - - source_range = offending_node.source_range.join(offending_node.parent.loc.begin) - corrector.replace(source_range, method_info[:if_string]) - end - end - end + audit_on_system_blocks(block_node, formula_block_name) if block_node end # Don't restrict OS.mac? or OS.linux? usage in taps; they don't care @@ -483,115 +400,19 @@ module RuboCop # that case. return if formula_tap != "homebrew-core" - ALL_SYSTEM_OPTIONS.each do |on_system_option| - method_info = on_system_method_info(on_system_option) + audit_arch_conditionals(body_node, + allowed_methods: NO_ON_SYSTEM_METHOD_NAMES, + allowed_blocks: NO_ON_SYSTEM_BLOCK_NAMES) - if_nodes_to_check = [] + audit_base_os_conditionals(body_node, + allowed_methods: NO_ON_SYSTEM_METHOD_NAMES, + allowed_blocks: NO_ON_SYSTEM_BLOCK_NAMES) - if ON_ARCH_OPTIONS.include?(on_system_option) - if_arch_node_search(body_node, arch: method_info[:if_method]) do |if_node, else_node| - else_info = if else_node.present? - { - can_autocorrect: true, - on_system_method: (on_system_option == :intel) ? "on_arm" : "on_intel", - node: else_node, - } - end - - if_nodes_to_check << [if_node, else_info] - end - elsif ON_BASE_OS_OPTIONS.include?(on_system_option) - if_base_os_node_search(body_node, base_os: method_info[:if_method]) do |if_node, else_node| - else_info = if else_node.present? - { - can_autocorrect: true, - on_system_method: (on_system_option == :macos) ? "on_linux" : "on_macos", - node: else_node, - } - end - - if_nodes_to_check << [if_node, else_info] - end - else - if_macos_version_node_search(body_node, os_name: on_system_option) do |if_node, operator, else_node| - if operator == :< - method_info[:on_system_string] = "on_system" - elsif operator == :<= - method_info[:on_system_string] = "on_system :linux, macos: :#{on_system_option}_or_older" - elsif operator != :== && MACOS_VERSION_CONDITIONALS.key?(operator.to_s) - method_info[:on_system_string] = "#{method_info[:method]} " \ - ":#{MACOS_VERSION_CONDITIONALS[operator.to_s]}" - end - method_info[:if_string] = "if #{method_info[:if_module]}.#{method_info[:if_method]} #{operator} " \ - ":#{on_system_option}" - if else_node.present? || !MACOS_VERSION_CONDITIONALS.key?(operator.to_s) - else_info = { can_autocorrect: false } - end - - if_nodes_to_check << [if_node, else_info] - end - end - - if_nodes_to_check.each do |if_node, else_info| - # TODO: check to see if it's legal - valid = T.let(false, T::Boolean) - if_node.each_ancestor do |ancestor| - valid_method_names = case ancestor.type - when :def - NO_ON_SYSTEM_METHOD_NAMES - when :block - NO_ON_SYSTEM_BLOCK_NAMES - else - next - end - next unless valid_method_names.include?(ancestor.method_name) - - valid = true - break - end - next if valid - - offending_node(if_node) - - problem "Don't use `#{method_info[:if_string]}`, " \ - "use `#{method_info[:on_system_string]} do` instead." do |corrector| - # TODO: could fix corrector to handle this but punting for now. - next if if_node.unless? - - if else_info.present? - next unless else_info[:can_autocorrect] - - corrector.replace(if_node.source_range, - "#{method_info[:on_system_string]} do\n#{if_node.body.source}\nend\n" \ - "#{else_info[:on_system_method]} do\n#{else_info[:node].source}\nend") - else - corrector.replace(if_node.source_range, - "#{method_info[:on_system_string]} do\n#{if_node.body.source}\nend") - end - end - end - end + audit_macos_version_conditionals(body_node, + allowed_methods: NO_ON_SYSTEM_METHOD_NAMES, + allowed_blocks: NO_ON_SYSTEM_BLOCK_NAMES, + recommend_on_system: true) end - - def_node_matcher :on_macos_version_method_call, <<~PATTERN - (send nil? %on_method (sym ${:or_newer :or_older})?) - PATTERN - - def_node_matcher :on_system_method_call, <<~PATTERN - (send nil? :on_system (sym :linux) (hash (pair (sym :macos) (sym $_)))) - PATTERN - - def_node_search :if_arch_node_search, <<~PATTERN - $(if (send (const (const nil? :Hardware) :CPU) %arch) _ $_) - PATTERN - - def_node_search :if_base_os_node_search, <<~PATTERN - $(if (send (const nil? :OS) %base_os) _ $_) - PATTERN - - def_node_search :if_macos_version_node_search, <<~PATTERN - $(if (send (send (const nil? :MacOS) :version) ${:== :<= :< :>= :> :!=} (sym %os_name)) _ $_) - PATTERN end # This cop checks for other miscellaneous style violations. diff --git a/Library/Homebrew/rubocops/rubocop-cask.rb b/Library/Homebrew/rubocops/rubocop-cask.rb index 673b6a5231..b17c9c4494 100644 --- a/Library/Homebrew/rubocops/rubocop-cask.rb +++ b/Library/Homebrew/rubocops/rubocop-cask.rb @@ -16,6 +16,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/on_system_conditionals" require_relative "cask/stanza_order" require_relative "cask/stanza_grouping" require_relative "cask/url_legacy_comma_separators" diff --git a/Library/Homebrew/rubocops/shared/on_system_conditionals_helper.rb b/Library/Homebrew/rubocops/shared/on_system_conditionals_helper.rb new file mode 100644 index 0000000000..f8cfafaceb --- /dev/null +++ b/Library/Homebrew/rubocops/shared/on_system_conditionals_helper.rb @@ -0,0 +1,196 @@ +# typed: false +# frozen_string_literal: true + +require "macos_versions" +require "rubocops/shared/helper_functions" + +module RuboCop + module Cop + # This module performs common checks on `on_{system}` blocks in both formulae and casks. + # + # @api private + module OnSystemConditionalsHelper + extend NodePattern::Macros + include HelperFunctions + + ARCH_OPTIONS = [:arm, :intel].freeze + BASE_OS_OPTIONS = [:macos, :linux].freeze + MACOS_VERSION_OPTIONS = MacOSVersions::SYMBOLS.keys.freeze + ON_SYSTEM_OPTIONS = [*ARCH_OPTIONS, *BASE_OS_OPTIONS, *MACOS_VERSION_OPTIONS, :system].freeze + + MACOS_VERSION_CONDITIONALS = { + "==" => nil, + "<=" => :or_older, + ">=" => :or_newer, + }.freeze + + def audit_on_system_blocks(body_node, parent_name) + parent_string = if body_node.def_type? + "def #{parent_name}" + else + "#{parent_name} do" + end + + ON_SYSTEM_OPTIONS.each do |on_system_option| + on_system_method = :"on_#{on_system_option}" + if_statement_string = if ARCH_OPTIONS.include?(on_system_option) + "if Hardware::CPU.#{on_system_option}?" + elsif BASE_OS_OPTIONS.include?(on_system_option) + "if OS.#{(on_system_option == :macos) ? "mac" : "linux"}?" + elsif on_system_option == :system + "if OS.linux? || MacOS.version" + else + "if MacOS.version" + end + + find_every_method_call_by_name(body_node, on_system_method).each do |on_system_node| + if_conditional = "" + if MACOS_VERSION_OPTIONS.include? on_system_option + on_macos_version_method_call(on_system_node, on_method: on_system_method) do |on_method_parameters| + if on_method_parameters.empty? + if_conditional = " == :#{on_system_option}" + else + if_condition_operator = MACOS_VERSION_CONDITIONALS.key(on_method_parameters.first) + if_conditional = " #{if_condition_operator} :#{on_system_option}" + end + end + elsif on_system_option == :system + on_system_method_call(on_system_node) do |macos_symbol| + base_os, condition = macos_symbol.to_s.split(/_(?=or_)/).map(&:to_sym) + if_condition_operator = MACOS_VERSION_CONDITIONALS.key(condition) + if_conditional = " #{if_condition_operator} :#{base_os}" + end + end + + offending_node(on_system_node) + problem "Don't use `#{on_system_node.source}` in `#{parent_string}`, " \ + "use `#{if_statement_string}#{if_conditional}` instead." do |corrector| + block_node = offending_node.parent + next if block_node.type != :block + + # TODO: could fix corrector to handle this but punting for now. + next if block_node.single_line? + + source_range = offending_node.source_range.join(offending_node.parent.loc.begin) + corrector.replace(source_range, "#{if_statement_string}#{if_conditional}") + end + end + end + end + + def audit_arch_conditionals(body_node, allowed_methods: [], allowed_blocks: []) + ARCH_OPTIONS.each do |arch_option| + else_method = (arch_option == :arm) ? :on_intel : :on_arm + if_arch_node_search(body_node, arch: :"#{arch_option}?") do |if_node, else_node| + next if if_node_is_allowed?(if_node, allowed_methods: allowed_methods, allowed_blocks: allowed_blocks) + + if_statement_problem(if_node, "if Hardware::CPU.#{arch_option}?", "on_#{arch_option}", + else_method: else_method, else_node: else_node) + end + end + end + + def audit_base_os_conditionals(body_node, allowed_methods: [], allowed_blocks: []) + BASE_OS_OPTIONS.each do |base_os_option| + os_method, else_method = if base_os_option == :macos + [:mac?, :on_linux] + else + [:linux?, :on_macos] + end + if_base_os_node_search(body_node, base_os: os_method) do |if_node, else_node| + next if if_node_is_allowed?(if_node, allowed_methods: allowed_methods, allowed_blocks: allowed_blocks) + + if_statement_problem(if_node, "if OS.#{os_method}", "on_#{base_os_option}", + else_method: else_method, else_node: else_node) + end + end + end + + def audit_macos_version_conditionals(body_node, allowed_methods: [], allowed_blocks: [], + recommend_on_system: true) + MACOS_VERSION_OPTIONS.each do |macos_version_option| + if_macos_version_node_search(body_node, os_version: macos_version_option) do |if_node, operator, else_node| + next if if_node_is_allowed?(if_node, allowed_methods: allowed_methods, allowed_blocks: allowed_blocks) + + autocorrect = else_node.blank? && MACOS_VERSION_CONDITIONALS.key?(operator.to_s) + on_system_method_string = if recommend_on_system && operator == :< + "on_system" + elsif recommend_on_system && operator == :<= + "on_system :linux, macos: :#{macos_version_option}_or_older" + elsif operator != :== && MACOS_VERSION_CONDITIONALS.key?(operator.to_s) + "on_#{macos_version_option} :#{MACOS_VERSION_CONDITIONALS[operator.to_s]}" + else + "on_#{macos_version_option}" + end + + if_statement_problem(if_node, "if MacOS.version #{operator} :#{macos_version_option}", + on_system_method_string, autocorrect: autocorrect) + end + end + end + + private + + def if_statement_problem(if_node, if_statement_string, on_system_method_string, + else_method: nil, else_node: nil, autocorrect: true) + offending_node(if_node) + problem "Don't use `#{if_statement_string}`, " \ + "use `#{on_system_method_string} do` instead." do |corrector| + next unless autocorrect + # TODO: could fix corrector to handle this but punting for now. + next if if_node.unless? + + if else_method.present? && else_node.present? + corrector.replace(if_node.source_range, + "#{on_system_method_string} do\n#{if_node.body.source}\nend\n" \ + "#{else_method} do\n#{else_node.source}\nend") + else + corrector.replace(if_node.source_range, "#{on_system_method_string} do\n#{if_node.body.source}\nend") + end + end + end + + def if_node_is_allowed?(if_node, allowed_methods: [], allowed_blocks: []) + # TODO: check to see if it's legal + valid = false + if_node.each_ancestor do |ancestor| + valid_method_names = case ancestor.type + when :def + allowed_methods + when :block + allowed_blocks + else + next + end + next unless valid_method_names.include?(ancestor.method_name) + + valid = true + break + end + return true if valid + + false + end + + def_node_matcher :on_macos_version_method_call, <<~PATTERN + (send nil? %on_method (sym ${:or_newer :or_older})?) + PATTERN + + def_node_matcher :on_system_method_call, <<~PATTERN + (send nil? :on_system (sym :linux) (hash (pair (sym :macos) (sym $_)))) + PATTERN + + def_node_search :if_arch_node_search, <<~PATTERN + $(if (send (const (const nil? :Hardware) :CPU) %arch) _ $_) + PATTERN + + def_node_search :if_base_os_node_search, <<~PATTERN + $(if (send (const nil? :OS) %base_os) _ $_) + PATTERN + + def_node_search :if_macos_version_node_search, <<~PATTERN + $(if (send (send (const nil? :MacOS) :version) ${:== :<= :< :>= :> :!=} (sym %os_version)) _ $_) + PATTERN + end + end +end diff --git a/Library/Homebrew/test/rubocops/cask/on_system_conditionals_spec.rb b/Library/Homebrew/test/rubocops/cask/on_system_conditionals_spec.rb new file mode 100644 index 0000000000..c1c7b1aa33 --- /dev/null +++ b/Library/Homebrew/test/rubocops/cask/on_system_conditionals_spec.rb @@ -0,0 +1,140 @@ +# typed: false +# frozen_string_literal: true + +require "rubocops/rubocop-cask" +require "test/rubocops/cask/shared_examples/cask_cop" + +describe RuboCop::Cop::Cask::OnSystemConditionals do + include CaskCop + + subject(:cop) { described_class.new } + + context "when auditing `postflight` stanzas" do + context "when there are no on_system blocks" do + let(:source) do + <<-CASK.undent + postflight do + foobar + end + CASK + end + + include_examples "does not report any offenses" + end + + context "when there is an `on_intel` block" do + let(:source) do + <<-CASK.undent + cask 'foo' do + postflight do + on_intel do + foobar + end + end + end + CASK + end + let(:correct_source) do + <<-CASK.undent + cask 'foo' do + postflight do + if Hardware::CPU.intel? + foobar + end + end + end + CASK + end + let(:expected_offenses) do + [{ + message: "Don't use `on_intel` in `postflight do`, use `if Hardware::CPU.intel?` instead.", + severity: :convention, + line: 3, + column: 4, + source: "on_intel", + }] + end + + include_examples "reports offenses" + + include_examples "autocorrects source" + end + + context "when there is an `on_monterey` block" do + let(:source) do + <<-CASK.undent + cask 'foo' do + postflight do + on_monterey do + foobar + end + end + end + CASK + end + let(:correct_source) do + <<-CASK.undent + cask 'foo' do + postflight do + if MacOS.version == :monterey + foobar + end + end + end + CASK + end + let(:expected_offenses) do + [{ + message: "Don't use `on_monterey` in `postflight do`, use `if MacOS.version == :monterey` instead.", + severity: :convention, + line: 3, + column: 4, + source: "on_monterey", + }] + end + + include_examples "reports offenses" + + include_examples "autocorrects source" + end + + context "when there is an `on_monterey :or_older` block" do + let(:source) do + <<-CASK.undent + cask 'foo' do + postflight do + on_monterey :or_older do + foobar + end + end + end + CASK + end + let(:correct_source) do + <<-CASK.undent + cask 'foo' do + postflight do + if MacOS.version <= :monterey + foobar + end + end + end + CASK + end + let(:expected_offenses) do + [{ + message: "Don't use `on_monterey :or_older` in `postflight do`, " \ + "use `if MacOS.version <= :monterey` instead.", + severity: :convention, + line: 3, + column: 4, + source: "on_monterey :or_older", + }] + end + + include_examples "reports offenses" + + include_examples "autocorrects source" + end + end +end