Merge pull request #21698 from Homebrew/audit-os-checks-in-extend-os

rubocops/move_to_extend_os: audit OS checks in `extend/os`

Author: MikeMcQuaid

Library/Homebrew/.rubocop.yml

@@ -7,7 +7,7 @@ Bundler/GemFilename:
 Homebrew/MoveToExtendOS:
   Enabled: true
   Exclude:
-    - "{extend,test,requirements}/**/*"
+    - "{test,requirements}/**/*"
     - "os.rb"
 
 # We don't use Sorbet for RSpec tests so let's disable this there.

Library/Homebrew/rubocops/move_to_extend_os.rb

@@ -4,19 +4,37 @@
 module RuboCop
   module Cop
     module Homebrew
-      # This cop ensures that platform specific code ends up in `extend/os`.
+      # This cop ensures that platform specific code ends up in `extend/os`, and
+      # that `extend/os` doesn't contain incorrect or redundant OS checks.
       class MoveToExtendOS < Base
-        MSG = "Move `OS.linux?` and `OS.mac?` calls to `extend/os`."
+        NON_EXTEND_OS_MSG = "Move `OS.linux?` and `OS.mac?` calls to `extend/os`."
 
-        def_node_matcher :os_check?, <<~PATTERN
-          (send (const nil? :OS) {:mac? | :linux?})
+        def_node_matcher :os_mac?, <<~PATTERN
+          (send (const nil? :OS) :mac?)
         PATTERN
 
+        def_node_matcher :os_linux?, <<~PATTERN
+          (send (const nil? :OS) :linux?)
+        PATTERN
+
+        sig { params(extend_os: String, os_method: String).returns(String) }
+        def extend_offense_message(extend_os, os_method)
+          "Don't use `OS.#{os_method}?` in `extend/os/#{extend_os}`, it is " \
+            "always `#{(extend_os == os_method) ? "true" : "false"}`."
+        end
+
         sig { params(node: RuboCop::AST::Node).void }
         def on_send(node)
-          return unless os_check?(node)
-
-          add_offense(node)
+          file_path = processed_source.file_path
+          if file_path.include?("extend/os/mac/")
+            add_offense(node, message: extend_offense_message("mac", "mac")) if os_mac?(node)
+            add_offense(node, message: extend_offense_message("mac", "linux")) if os_linux?(node)
+          elsif file_path.include?("extend/os/linux/")
+            add_offense(node, message: extend_offense_message("linux", "mac")) if os_mac?(node)
+            add_offense(node, message: extend_offense_message("linux", "linux")) if os_linux?(node)
+          elsif !file_path.include?("extend/os/") && (os_mac?(node) || os_linux?(node))
+            add_offense(node, message: NON_EXTEND_OS_MSG)
+          end
         end
       end
     end

Library/Homebrew/sorbet/rbi/dsl/rubo_cop/cop/homebrew/move_to_extend_os.rbi

@@ -18,4 +18,36 @@
       ^^^^^^^ Homebrew/MoveToExtendOS: Move `OS.linux?` and `OS.mac?` calls to `extend/os`.
     RUBY
   end
+
+  context "when in extend/os/mac" do
+    it "registers an offense when using `OS.linux?`" do
+      expect_offense(<<~RUBY, "Library/Homebrew/extend/os/mac/foo.rb")
+        OS.linux?
+        ^^^^^^^^^ Homebrew/MoveToExtendOS: Don't use `OS.linux?` in `extend/os/mac`, it is always `false`.
+      RUBY
+    end
+
+    it "registers an offense when using `OS.mac?`" do
+      expect_offense(<<~RUBY, "Library/Homebrew/extend/os/mac/foo.rb")
+        OS.mac?
+        ^^^^^^^ Homebrew/MoveToExtendOS: Don't use `OS.mac?` in `extend/os/mac`, it is always `true`.
+      RUBY
+    end
+  end
+
+  context "when in extend/os/linux" do
+    it "registers an offense when using `OS.mac?`" do
+      expect_offense(<<~RUBY, "Library/Homebrew/extend/os/linux/foo.rb")
+        OS.mac?
+        ^^^^^^^ Homebrew/MoveToExtendOS: Don't use `OS.mac?` in `extend/os/linux`, it is always `false`.
+      RUBY
+    end
+
+    it "registers an offense when using `OS.linux?`" do
+      expect_offense(<<~RUBY, "Library/Homebrew/extend/os/linux/foo.rb")
+        OS.linux?
+        ^^^^^^^^^ Homebrew/MoveToExtendOS: Don't use `OS.linux?` in `extend/os/linux`, it is always `true`.
+      RUBY
+    end
+  end
 end