Merge pull request #11955 from Homebrew/on_os_if_rubocops

rubocops/lines: recommend on_os/OS.os? based on context.

Library/Homebrew/formula.rb

@@ -64,7 +64,7 @@ class Formula
   include Utils::Shebang
   include Utils::Shell
   include Context
-  include OnOS
+  include OnOS # TODO: 3.3.0: deprecate OnOS usage in instance methods.
   extend Enumerable
   extend Forwardable
   extend Cachable

Library/Homebrew/formula_support.rb

@@ -69,7 +69,7 @@ class BottleDisableReason
   def initialize(type, reason)
     @type = type
     @reason = reason
-    # TODO: 3.3.0 should deprecate this behaviour as it was only needed for
+    # TODO: 3.3.0: deprecate this behaviour as it was only needed for
     # Homebrew/core (where we don't want unneeded or disabled bottles any more)
     # odeprecated "bottle :#{@type}" if valid?
   end

Library/Homebrew/rubocops/lines.rb

@@ -347,6 +347,82 @@ module RuboCop
         end
       end
 
+      # This cop makes sure that OS conditionals are consistent.
+      #
+      # @api private
+      class OSConditionals < FormulaCop
+        extend AutoCorrector
+
+        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 = [: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)
+
+              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
+
+                source_range = offending_node.source_range.join(offending_node.parent.loc.begin)
+                corrector.replace(source_range, if_method_and_class)
+              end
+            end
+
+            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 method_called_in_block?(block_node, on_method_name)
+
+              problem "Don't use '#{on_method_name}' in '#{formula_block_name} do', " \
+                      "use '#{if_method_and_class}' instead." do |corrector|
+                block_node = offending_node.parent
+                next if block_node.type != :block
+
+                source_range = offending_node.source_range.join(offending_node.parent.loc.begin)
+                corrector.replace(source_range, if_method_and_class)
+              end
+            end
+
+            find_instance_method_call(body_node, "OS", if_method_name) do |method|
+              valid = T.let(false, T::Boolean)
+              method.each_ancestor do |ancestor|
+                valid_method_names = case ancestor.type
+                when :def
+                  no_on_os_method_names
+                when :block
+                  no_on_os_block_names
+                else
+                  next
+                end
+                next unless valid_method_names.include?(ancestor.method_name)
+
+                valid = true
+                break
+              end
+              next if valid
+
+              # TODO: temporarily allow this for linuxbrew-core merge.
+              next if method&.parent&.parent&.source&.start_with?("revision OS.mac?")
+              next if method&.parent&.source&.match?(/revision \d unless OS\.mac\?/)
+
+              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
+                next if if_node.unless?
+
+                corrector.replace(if_node.source_range, "#{on_method_name} do\n#{if_node.body.source}\nend")
+              end
+            end
+          end
+        end
+      end
+
       # This cop checks for other miscellaneous style violations.
       #
       # @api private

Library/Homebrew/rubocops/shared/helper_functions.rb

@@ -113,8 +113,10 @@ module RuboCop
         nil
       end
 
-      # Sets the given node as the offending node when required in custom cops.
+      # Gets/sets the given node as the offending node when required in custom cops.
-      def offending_node(node)
+      def offending_node(node = nil)
+        return @offensive_node if node.nil?
+
         @offensive_node = node
       end
 

Library/Homebrew/test/rubocops/text/on_os_conditionals_spec.rb

@@ -0,0 +1,86 @@
+# 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