From f98b8f948ce20a57ae58ad5b9a0535ecee05d77b Mon Sep 17 00:00:00 2001 From: Rylan Polster Date: Wed, 22 Nov 2023 20:22:02 -0500 Subject: [PATCH] Add rubocop to remove `MacOS` references --- Library/Homebrew/rubocops/lines.rb | 15 +++ .../shared/on_system_conditionals_helper.rb | 26 +++- .../test/rubocops/text/macos_on_linux_spec.rb | 127 ++++++++++++++++++ 3 files changed, 161 insertions(+), 7 deletions(-) create mode 100644 Library/Homebrew/test/rubocops/text/macos_on_linux_spec.rb diff --git a/Library/Homebrew/rubocops/lines.rb b/Library/Homebrew/rubocops/lines.rb index e44623e459..fbb5c82081 100644 --- a/Library/Homebrew/rubocops/lines.rb +++ b/Library/Homebrew/rubocops/lines.rb @@ -441,6 +441,21 @@ module RuboCop end end + # This cop makes sure the `MacOS` module is not used in Linux-facing formula code + # + # @api private + class MacOSOnLinux < FormulaCop + include OnSystemConditionalsHelper + + ON_MACOS_BLOCKS = [:macos, *MACOS_VERSION_OPTIONS].map { |os| :"on_#{os}" }.freeze + + def audit_formula(_node, _class_node, _parent_class_node, body_node) + audit_macos_references(body_node, + allowed_methods: OnSystemConditionals::NO_ON_SYSTEM_METHOD_NAMES, + allowed_blocks: OnSystemConditionals::NO_ON_SYSTEM_BLOCK_NAMES + ON_MACOS_BLOCKS) + end + end + # This cop makes sure that the `generate_completions_from_executable` DSL is used. # # @api private diff --git a/Library/Homebrew/rubocops/shared/on_system_conditionals_helper.rb b/Library/Homebrew/rubocops/shared/on_system_conditionals_helper.rb index c6e0142afb..bee4847630 100644 --- a/Library/Homebrew/rubocops/shared/on_system_conditionals_helper.rb +++ b/Library/Homebrew/rubocops/shared/on_system_conditionals_helper.rb @@ -17,6 +17,7 @@ module RuboCop BASE_OS_OPTIONS = [:macos, :linux].freeze MACOS_VERSION_OPTIONS = MacOSVersion::SYMBOLS.keys.freeze ON_SYSTEM_OPTIONS = [*ARCH_OPTIONS, *BASE_OS_OPTIONS, *MACOS_VERSION_OPTIONS, :system].freeze + MACOS_MODULE_NAMES = ["MacOS", "OS::Mac"].freeze MACOS_VERSION_CONDITIONALS = { "==" => nil, @@ -82,7 +83,7 @@ module RuboCop 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) + next 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) @@ -93,7 +94,7 @@ module RuboCop hardware_cpu_search(body_node, method: method) do |method_node| # These should already be caught by `if_arch_node_search` next if method_node.parent.source.start_with? "if #{method_node.source}" - next if if_node_is_allowed?(method_node, allowed_methods: allowed_methods, allowed_blocks: allowed_blocks) + next if node_is_allowed?(method_node, allowed_methods: allowed_methods, allowed_blocks: allowed_blocks) offending_node(method_node) problem "Don't use `#{method_node.source}`, use `on_arm` and `on_intel` blocks instead." @@ -109,7 +110,7 @@ module RuboCop [: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) + next 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) @@ -121,7 +122,7 @@ module RuboCop 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) + next 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 == :< @@ -141,7 +142,7 @@ module RuboCop macos_version_comparison_search(body_node, os_version: macos_version_option) do |method_node| # These should already be caught by `if_macos_version_node_search` next if method_node.parent.source.start_with? "if #{method_node.source}" - next if if_node_is_allowed?(method_node, allowed_methods: allowed_methods, allowed_blocks: allowed_blocks) + next if node_is_allowed?(method_node, allowed_methods: allowed_methods, allowed_blocks: allowed_blocks) offending_node(method_node) problem "Don't use `#{method_node.source}`, use `on_{macos_version}` blocks instead." @@ -149,6 +150,17 @@ module RuboCop end end + def audit_macos_references(body_node, allowed_methods: [], allowed_blocks: []) + MACOS_MODULE_NAMES.each do |macos_module_name| + find_const(body_node, macos_module_name) do |node| + next if node_is_allowed?(node, allowed_methods: allowed_methods, allowed_blocks: allowed_blocks) + + offending_node(node) + problem "Don't use `#{macos_module_name}` where it could be called on Linux." + end + end + end + private def if_statement_problem(if_node, if_statement_string, on_system_method_string, @@ -170,10 +182,10 @@ module RuboCop end end - def if_node_is_allowed?(if_node, allowed_methods: [], allowed_blocks: []) + def node_is_allowed?(node, allowed_methods: [], allowed_blocks: []) # TODO: check to see if it's legal valid = T.let(false, T::Boolean) - if_node.each_ancestor do |ancestor| + node.each_ancestor do |ancestor| valid_method_names = case ancestor.type when :def allowed_methods diff --git a/Library/Homebrew/test/rubocops/text/macos_on_linux_spec.rb b/Library/Homebrew/test/rubocops/text/macos_on_linux_spec.rb new file mode 100644 index 0000000000..c72c2395fe --- /dev/null +++ b/Library/Homebrew/test/rubocops/text/macos_on_linux_spec.rb @@ -0,0 +1,127 @@ +# frozen_string_literal: true + +require "rubocops/lines" + +describe RuboCop::Cop::FormulaAudit::MacOSOnLinux do + subject(:cop) { described_class.new } + + it "reports an offense when `MacOS` is used in the `Formula` class" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + desc "foo" + if MacOS::Xcode.version >= "12.0" + ^^^^^ FormulaAudit/MacOSOnLinux: Don't use `MacOS` where it could be called on Linux. + url 'https://brew.sh/linux-1.0.tgz' + end + end + RUBY + end + + it "reports an offense when `MacOS` is used in a `resource` block" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + desc "foo" + url 'https://brew.sh/linux-1.0.tgz' + + resource "foo" do + url "https://brew.sh/linux-1.0.tgz" if MacOS::full_version >= "12.0" + ^^^^^ FormulaAudit/MacOSOnLinux: Don't use `MacOS` where it could be called on Linux. + end + end + RUBY + end + + it "reports an offense when `MacOS` is used in an `on_linux` block" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + desc "foo" + on_linux do + if MacOS::Xcode.version >= "12.0" + ^^^^^ FormulaAudit/MacOSOnLinux: Don't use `MacOS` where it could be called on Linux. + url 'https://brew.sh/linux-1.0.tgz' + end + end + end + RUBY + end + + it "reports an offense when `MacOS` is used in an `on_arm` block" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + desc "foo" + on_arm do + if MacOS::Xcode.version >= "12.0" + ^^^^^ FormulaAudit/MacOSOnLinux: Don't use `MacOS` where it could be called on Linux. + url 'https://brew.sh/linux-1.0.tgz' + end + end + end + RUBY + end + + it "reports an offense when `MacOS` is used in an `on_intel` block" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + desc "foo" + on_intel do + if MacOS::Xcode.version >= "12.0" + ^^^^^ FormulaAudit/MacOSOnLinux: Don't use `MacOS` where it could be called on Linux. + url 'https://brew.sh/linux-1.0.tgz' + end + end + end + RUBY + end + + it "reports no offenses when `MacOS` is used in an `on_macos` block" do + expect_no_offenses(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + desc "foo" + on_macos do + if MacOS::Xcode.version >= "12.0" + url 'https://brew.sh/linux-1.0.tgz' + end + end + end + RUBY + end + + it "reports no offenses when `MacOS` is used in an `on_ventura` block" do + expect_no_offenses(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + desc "foo" + on_ventura :or_older do + if MacOS::Xcode.version >= "12.0" + url 'https://brew.sh/linux-1.0.tgz' + end + end + end + RUBY + end + + it "reports no offenses when `MacOS` is used in the `install` method" do + expect_no_offenses(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + desc "foo" + url 'https://brew.sh/linux-1.0.tgz' + + def install + MacOS.version + end + end + RUBY + end + + it "reports no offenses when `MacOS` is used in the `test` block" do + expect_no_offenses(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + desc "foo" + url 'https://brew.sh/linux-1.0.tgz' + + test do + MacOS.version + end + end + RUBY + end +end