diff --git a/Library/Homebrew/rubocops/lines.rb b/Library/Homebrew/rubocops/lines.rb index cbac0882a0..2b11692aa7 100644 --- a/Library/Homebrew/rubocops/lines.rb +++ b/Library/Homebrew/rubocops/lines.rb @@ -1,6 +1,7 @@ # typed: true # frozen_string_literal: true +require "macos_versions" require "rubocops/extend/formula" module RuboCop @@ -375,44 +376,79 @@ module RuboCop # This cop makes sure that OS conditionals are consistent. # # @api private - class OSConditionals < FormulaCop + class OnSystemConditionals < FormulaCop extend AutoCorrector + NO_ON_SYSTEM_METHOD_NAMES = [:install, :post_install].freeze + NO_ON_SYSTEM_BLOCK_NAMES = [:service, :test].freeze + + MACOS_VERSION_CONDITIONALS = { + "==" => nil, + "<=" => :or_older, + ">=" => :or_newer, + }.freeze + def audit_formula(_node, _class_node, _parent_class_node, body_node) - no_on_os_method_names = [:install, :post_install].freeze - no_on_os_block_names = [:service, :test].freeze - [[:on_macos, :mac?], [:on_linux, :linux?]].each do |on_method_name, if_method_name| - if_method_and_class = "if OS.#{if_method_name}" - no_on_os_method_names.each do |formula_method_name| - method_node = find_method_def(body_node, formula_method_name) - next unless method_node - next unless method_called_ever?(method_node, on_method_name) + on_arch_options = [:intel, :arm] + on_base_os_options = [:macos, :linux] + on_macos_version_options = MacOSVersions::SYMBOLS.keys + all_system_options = on_arch_options + on_base_os_options + on_macos_version_options - problem "Don't use '#{on_method_name}' in 'def #{formula_method_name}', " \ - "use '#{if_method_and_class}' instead." do |corrector| - block_node = offending_node.parent - next if block_node.type != :block + 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 + 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 - # 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_method_and_class) - end + all_system_options.each do |on_system_option| + on_system_method = :"on_#{on_system_option}" + if_module_name, if_method_name = 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 + on_system_method_string = on_system_method.to_s + if_statement_string = "if #{if_module_name}.#{if_method_name}" - no_on_os_block_names.each do |formula_block_name| - block_node = find_block(body_node, formula_block_name) - next unless block_node - next unless block_method_called_in_block?(block_node, on_method_name) + 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 - problem "Don't use '#{on_method_name}' in '#{formula_block_name} do', " \ - "use '#{if_method_and_class}' instead." do |corrector| - # TODO: could fix corrector to handle this but punting for now. - next if offending_node.single_line? + find_every_method_call_by_name(top_level_node, on_system_method).each do |method| + if on_macos_version_options.include?(on_system_option) + on_macos_version_method_call(method, on_method: on_system_method) do |on_method_parameters| + if on_method_parameters.empty? + if_statement_string = "#{if_statement_string} == :#{on_system_option}" + else + on_system_method_string = "#{on_system_method} :#{on_method_parameters.first}" + if_condition_operator = MACOS_VERSION_CONDITIONALS.key(on_method_parameters.first) + if_statement_string = "#{if_statement_string} #{if_condition_operator} :#{on_system_option}" + end + end + end - source_range = offending_node.send_node.source_range.join(offending_node.body.source_range.begin) - corrector.replace(source_range, "#{if_method_and_class}\n") + offending_node(method) + + problem "Don't use `#{on_system_method_string}` in `#{top_level_node_string}`, " \ + "use `#{if_statement_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, if_statement_string) + end end end @@ -422,14 +458,55 @@ module RuboCop # that case. next if formula_tap != "homebrew-core" - find_instance_method_call(body_node, "OS", if_method_name) do |method| + if_nodes_to_check = [] + + if on_arch_options.include?(on_system_option) + if_arch_node_search(body_node, arch: if_method_name) 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: if_method_name) 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 != :== && MACOS_VERSION_CONDITIONALS.key?(operator.to_s) + on_system_method_string = "#{on_system_method} :#{MACOS_VERSION_CONDITIONALS[operator.to_s]}" + end + if_statement_string = "if #{if_module_name}.#{if_method_name} #{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) - method.each_ancestor do |ancestor| + if_node.each_ancestor do |ancestor| valid_method_names = case ancestor.type when :def - no_on_os_method_names + NO_ON_SYSTEM_METHOD_NAMES when :block - no_on_os_block_names + NO_ON_SYSTEM_BLOCK_NAMES else next end @@ -440,19 +517,43 @@ module RuboCop end next if valid - offending_node(method) - problem "Don't use '#{if_method_and_class}', use '#{on_method_name} do' instead." do |corrector| - if_node = method.parent - next if if_node.type != :if + offending_node(if_node) + problem "Don't use `#{if_statement_string}`, " \ + "use `#{on_system_method_string} do` instead." do |corrector| # TODO: could fix corrector to handle this but punting for now. next if if_node.unless? - corrector.replace(if_node.source_range, "#{on_method_name} do\n#{if_node.body.source}\nend") + if else_info.present? + next unless else_info[:can_autocorrect] + + corrector.replace(if_node.source_range, + "#{on_system_method_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, + "#{on_system_method_string} do\n#{if_node.body.source}\nend") + end end end end end + + def_node_matcher :on_macos_version_method_call, <<~PATTERN + (send nil? %on_method (sym ${:or_newer :or_older})?) + 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/lines.rbi b/Library/Homebrew/rubocops/lines.rbi new file mode 100644 index 0000000000..11eb86c4c6 --- /dev/null +++ b/Library/Homebrew/rubocops/lines.rbi @@ -0,0 +1,21 @@ +# typed: strict + +module RuboCop + module Cop + module FormulaAudit + class OnSystemConditionals < FormulaCop + sig { params(node: T.any, on_method: Symbol, block: T.proc.params(parameters: T::Array[T.any]).void).void } + def on_macos_version_method_call(node, on_method:, &block); end + + sig { params(node: T.any, arch: Symbol, block: T.proc.params(node: T.any, else_node: T.any).void).void } + def if_arch_node_search(node, arch:, &block); end + + sig { params(node: T.any, base_os: Symbol, block: T.proc.params(node: T.any, else_node: T.any).void).void } + def if_base_os_node_search(node, base_os:, &block); end + + sig { params(node: T.any, os_name: Symbol, block: T.proc.params(node: T.any, operator: Symbol, else_node: T.any).void).void } + def if_macos_version_node_search(node, os_name:, &block); end + end + end + end +end diff --git a/Library/Homebrew/test/rubocops/text/on_os_conditionals_spec.rb b/Library/Homebrew/test/rubocops/text/on_os_conditionals_spec.rb deleted file mode 100644 index 28f1a41185..0000000000 --- a/Library/Homebrew/test/rubocops/text/on_os_conditionals_spec.rb +++ /dev/null @@ -1,86 +0,0 @@ -# typed: false -# frozen_string_literal: true - -require "rubocops/lines" - -describe RuboCop::Cop::FormulaAudit::OSConditionals do - subject(:cop) { described_class.new } - - context "when auditing OS conditionals" do - it "reports an offense when `OS.linux?` is used on Formula class" do - expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") - class Foo < Formula - desc "foo" - if OS.linux? - ^^^^^^^^^ Don't use 'if OS.linux?', use 'on_linux do' instead. - url 'https://brew.sh/linux-1.0.tgz' - else - url 'https://brew.sh/linux-1.0.tgz' - end - end - RUBY - end - - it "reports an offense when `OS.mac?` is used on Formula class" do - expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") - class Foo < Formula - desc "foo" - if OS.mac? - ^^^^^^^ Don't use 'if OS.mac?', use 'on_macos do' instead. - url 'https://brew.sh/mac-1.0.tgz' - else - url 'https://brew.sh/linux-1.0.tgz' - end - end - RUBY - end - - it "reports an offense when `on_macos` is used in install method" do - expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") - class Foo < Formula - desc "foo" - url 'https://brew.sh/foo-1.0.tgz' - - def install - on_macos do - ^^^^^^^^ Don't use 'on_macos' in 'def install', use 'if OS.mac?' instead. - true - end - end - end - RUBY - end - - it "reports an offense when `on_linux` is used in install method" do - expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") - class Foo < Formula - desc "foo" - url 'https://brew.sh/foo-1.0.tgz' - - def install - on_linux do - ^^^^^^^^ Don't use 'on_linux' in 'def install', use 'if OS.linux?' instead. - true - end - end - end - RUBY - end - - it "reports an offense when `on_macos` is used in test block" do - expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") - class Foo < Formula - desc "foo" - url 'https://brew.sh/foo-1.0.tgz' - - test do - on_macos do - ^^^^^^^^ Don't use 'on_macos' in 'test do', use 'if OS.mac?' instead. - true - end - end - end - RUBY - end - end -end diff --git a/Library/Homebrew/test/rubocops/text/on_system_conditionals_spec.rb b/Library/Homebrew/test/rubocops/text/on_system_conditionals_spec.rb new file mode 100644 index 0000000000..62d857a6e2 --- /dev/null +++ b/Library/Homebrew/test/rubocops/text/on_system_conditionals_spec.rb @@ -0,0 +1,488 @@ +# typed: false +# frozen_string_literal: true + +require "rubocops/lines" + +describe RuboCop::Cop::FormulaAudit::OnSystemConditionals do + subject(:cop) { described_class.new } + + context "when auditing OS conditionals" do + it "reports an offense when `OS.linux?` is used on Formula class" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + desc "foo" + if OS.linux? + ^^^^^^^^^^^^ Don't use `if OS.linux?`, use `on_linux do` instead. + url 'https://brew.sh/linux-1.0.tgz' + else + url 'https://brew.sh/linux-1.0.tgz' + end + end + RUBY + + expect_correction(<<~RUBY) + class Foo < Formula + desc "foo" + on_linux do + url 'https://brew.sh/linux-1.0.tgz' + end + on_macos do + url 'https://brew.sh/linux-1.0.tgz' + end + end + RUBY + end + + it "reports an offense when `OS.mac?` is used on Formula class" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + desc "foo" + if OS.mac? + ^^^^^^^^^^ Don't use `if OS.mac?`, use `on_macos do` instead. + url 'https://brew.sh/mac-1.0.tgz' + else + url 'https://brew.sh/linux-1.0.tgz' + end + end + RUBY + + expect_correction(<<~RUBY) + class Foo < Formula + desc "foo" + on_macos do + url 'https://brew.sh/mac-1.0.tgz' + end + on_linux do + url 'https://brew.sh/linux-1.0.tgz' + end + end + RUBY + end + + it "reports an offense when `on_macos` is used in install method" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + desc "foo" + url 'https://brew.sh/foo-1.0.tgz' + + def install + on_macos do + ^^^^^^^^ Don't use `on_macos` in `def install`, use `if OS.mac?` instead. + true + end + end + end + RUBY + + expect_correction(<<~RUBY) + class Foo < Formula + desc "foo" + url 'https://brew.sh/foo-1.0.tgz' + + def install + if OS.mac? + true + end + end + end + RUBY + end + + it "reports an offense when `on_linux` is used in install method" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + desc "foo" + url 'https://brew.sh/foo-1.0.tgz' + + def install + on_linux do + ^^^^^^^^ Don't use `on_linux` in `def install`, use `if OS.linux?` instead. + true + end + end + end + RUBY + + expect_correction(<<~RUBY) + class Foo < Formula + desc "foo" + url 'https://brew.sh/foo-1.0.tgz' + + def install + if OS.linux? + true + end + end + end + RUBY + end + + it "reports an offense when `on_macos` is used in test block" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + desc "foo" + url 'https://brew.sh/foo-1.0.tgz' + + test do + on_macos do + ^^^^^^^^ Don't use `on_macos` in `test do`, use `if OS.mac?` instead. + true + end + end + end + RUBY + + expect_correction(<<~RUBY) + class Foo < Formula + desc "foo" + url 'https://brew.sh/foo-1.0.tgz' + + test do + if OS.mac? + true + end + end + end + RUBY + end + end + + context "when auditing Hardware::CPU conditionals" do + it "reports an offense when `Hardware::CPU.arm?` is used on Formula class" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + desc "foo" + if Hardware::CPU.arm? + ^^^^^^^^^^^^^^^^^^^^^ Don't use `if Hardware::CPU.arm?`, use `on_arm do` instead. + url 'https://brew.sh/linux-1.0.tgz' + else + url 'https://brew.sh/linux-1.0.tgz' + end + end + RUBY + + expect_correction(<<~RUBY) + class Foo < Formula + desc "foo" + on_arm do + url 'https://brew.sh/linux-1.0.tgz' + end + on_intel do + url 'https://brew.sh/linux-1.0.tgz' + end + end + RUBY + end + + it "reports an offense when `Hardware::CPU.intel?` is used on Formula class" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + desc "foo" + if Hardware::CPU.intel? + ^^^^^^^^^^^^^^^^^^^^^^^ Don't use `if Hardware::CPU.intel?`, use `on_intel do` instead. + url 'https://brew.sh/mac-1.0.tgz' + else + url 'https://brew.sh/linux-1.0.tgz' + end + end + RUBY + + expect_correction(<<~RUBY) + class Foo < Formula + desc "foo" + on_intel do + url 'https://brew.sh/mac-1.0.tgz' + end + on_arm do + url 'https://brew.sh/linux-1.0.tgz' + end + end + RUBY + end + + it "reports an offense when `on_intel` is used in install method" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + desc "foo" + url 'https://brew.sh/foo-1.0.tgz' + + def install + on_intel do + ^^^^^^^^ Don't use `on_intel` in `def install`, use `if Hardware::CPU.intel?` instead. + true + end + end + end + RUBY + + expect_correction(<<~RUBY) + class Foo < Formula + desc "foo" + url 'https://brew.sh/foo-1.0.tgz' + + def install + if Hardware::CPU.intel? + true + end + end + end + RUBY + end + + it "reports an offense when `on_arm` is used in install method" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + desc "foo" + url 'https://brew.sh/foo-1.0.tgz' + + def install + on_arm do + ^^^^^^ Don't use `on_arm` in `def install`, use `if Hardware::CPU.arm?` instead. + true + end + end + end + RUBY + + expect_correction(<<~RUBY) + class Foo < Formula + desc "foo" + url 'https://brew.sh/foo-1.0.tgz' + + def install + if Hardware::CPU.arm? + true + end + end + end + RUBY + end + + it "reports an offense when `on_intel` is used in test block" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + desc "foo" + url 'https://brew.sh/foo-1.0.tgz' + + test do + on_intel do + ^^^^^^^^ Don't use `on_intel` in `test do`, use `if Hardware::CPU.intel?` instead. + true + end + end + end + RUBY + + expect_correction(<<~RUBY) + class Foo < Formula + desc "foo" + url 'https://brew.sh/foo-1.0.tgz' + + test do + if Hardware::CPU.intel? + true + end + end + end + RUBY + end + end + + context "when auditing MacOS.version conditionals" do + it "reports an offense when `MacOS.version ==` is used on Formula class" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + desc "foo" + if MacOS.version == :monterey + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Don't use `if MacOS.version == :monterey`, use `on_monterey do` instead. + url 'https://brew.sh/linux-1.0.tgz' + end + end + RUBY + + expect_correction(<<~RUBY) + class Foo < Formula + desc "foo" + on_monterey do + url 'https://brew.sh/linux-1.0.tgz' + end + end + RUBY + end + + it "reports an offense when `MacOS.version <=` is used on Formula class" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + desc "foo" + if MacOS.version <= :monterey + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Don't use `if MacOS.version <= :monterey`, use `on_monterey :or_older do` instead. + url 'https://brew.sh/mac-1.0.tgz' + end + end + RUBY + + expect_correction(<<~RUBY) + class Foo < Formula + desc "foo" + on_monterey :or_older do + url 'https://brew.sh/mac-1.0.tgz' + end + end + RUBY + end + + it "reports an offense when `MacOS.version <` is used on Formula class" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + desc "foo" + if MacOS.version < :monterey + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Don't use `if MacOS.version < :monterey`, use `on_monterey do` instead. + url 'https://brew.sh/mac-1.0.tgz' + end + end + RUBY + end + + it "reports an offense when `MacOS.version >=` is used on Formula class" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + desc "foo" + if MacOS.version >= :monterey + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Don't use `if MacOS.version >= :monterey`, use `on_monterey :or_newer do` instead. + url 'https://brew.sh/mac-1.0.tgz' + else + url 'https://brew.sh/linux-1.0.tgz' + end + end + RUBY + end + + it "reports an offense when `MacOS.version >` is used on Formula class" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + desc "foo" + if MacOS.version > :monterey + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Don't use `if MacOS.version > :monterey`, use `on_monterey do` instead. + url 'https://brew.sh/mac-1.0.tgz' + end + end + RUBY + end + + it "reports an offense when `on_monterey` is used in install method" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + desc "foo" + url 'https://brew.sh/foo-1.0.tgz' + + def install + on_monterey do + ^^^^^^^^^^^ Don't use `on_monterey` in `def install`, use `if MacOS.version == :monterey` instead. + true + end + end + end + RUBY + + expect_correction(<<~RUBY) + class Foo < Formula + desc "foo" + url 'https://brew.sh/foo-1.0.tgz' + + def install + if MacOS.version == :monterey + true + end + end + end + RUBY + end + + it "reports an offense when `on_monterey :or_older` is used in install method" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + desc "foo" + url 'https://brew.sh/foo-1.0.tgz' + + def install + on_monterey :or_older do + ^^^^^^^^^^^^^^^^^^^^^ Don't use `on_monterey :or_older` in `def install`, use `if MacOS.version <= :monterey` instead. + true + end + end + end + RUBY + + expect_correction(<<~RUBY) + class Foo < Formula + desc "foo" + url 'https://brew.sh/foo-1.0.tgz' + + def install + if MacOS.version <= :monterey + true + end + end + end + RUBY + end + + it "reports an offense when `on_monterey :or_newer` is used in install method" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + desc "foo" + url 'https://brew.sh/foo-1.0.tgz' + + def install + on_monterey :or_newer do + ^^^^^^^^^^^^^^^^^^^^^ Don't use `on_monterey :or_newer` in `def install`, use `if MacOS.version >= :monterey` instead. + true + end + end + end + RUBY + + expect_correction(<<~RUBY) + class Foo < Formula + desc "foo" + url 'https://brew.sh/foo-1.0.tgz' + + def install + if MacOS.version >= :monterey + true + end + end + end + RUBY + end + + it "reports an offense when `on_monterey` is used in test block" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + desc "foo" + url 'https://brew.sh/foo-1.0.tgz' + + test do + on_monterey do + ^^^^^^^^^^^ Don't use `on_monterey` in `test do`, use `if MacOS.version == :monterey` instead. + true + end + end + end + RUBY + + expect_correction(<<~RUBY) + class Foo < Formula + desc "foo" + url 'https://brew.sh/foo-1.0.tgz' + + test do + if MacOS.version == :monterey + true + end + end + end + RUBY + end + end +end