Merge pull request #10256 from SeekingMeaning/unless_multiple_conditions

rubocops: add `unless_multiple_conditions`

Library/Homebrew/build.rb

@@ -39,7 +39,7 @@ class Build
   def post_superenv_hacks
     # Only allow Homebrew-approved directories into the PATH, unless
     # a formula opts-in to allowing the user's path.
-    return unless formula.env.userpaths? || reqs.any? { |rq| rq.env.userpaths? }
+    return if !formula.env.userpaths? && reqs.none? { |rq| rq.env.userpaths? }
 
     ENV.userpaths!
   end

Library/Homebrew/cask/audit.rb

@@ -154,7 +154,7 @@ module Cask
       return if tap.nil?
       return if tap.user != "Homebrew"
 
-      return unless cask.artifacts.any? { |k| k.is_a?(Artifact::Pkg) && k.stanza_options.key?(:allow_untrusted) }
+      return if cask.artifacts.none? { |k| k.is_a?(Artifact::Pkg) && k.stanza_options.key?(:allow_untrusted) }
 
       add_error "allow_untrusted is not permitted in official Homebrew Cask taps"
     end
@@ -472,7 +472,7 @@ module Cask
         add_error "cask token should only contain lowercase alphanumeric characters and hyphens"
       end
 
-      return unless cask.token.start_with?("-") || cask.token.end_with?("-")
+      return if !cask.token.start_with?("-") && !cask.token.end_with?("-")
 
       add_error "cask token should not have leading or trailing hyphens"
     end
@@ -498,7 +498,8 @@ module Cask
 
       add_warning "cask token mentions architecture" if token.end_with? "x86", "32_bit", "x86_64", "64_bit"
 
-      return unless token.end_with?("cocoa", "qt", "gtk", "wx", "java") && %w[cocoa qt gtk wx java].exclude?(token)
+      frameworks = %w[cocoa qt gtk wx java]
+      return if frameworks.include?(token) || !token.end_with?(*frameworks)
 
       add_warning "cask token mentions framework"
     end
@@ -517,7 +518,7 @@ module Cask
     end
 
     def check_download
-      return unless download && cask.url
+      return if download.blank? || cask.url.blank?
 
       odebug "Auditing download"
       download.fetch

Library/Homebrew/cask/cmd/upgrade.rb

@@ -92,7 +92,7 @@ module Cask
           end
         else
           casks.select do |cask|
-            raise CaskNotInstalledError, cask unless cask.installed? || force
+            raise CaskNotInstalledError, cask if !cask.installed? && !force
 
             cask.outdated?(greedy: true)
           end

Library/Homebrew/cask/installer.rb

@@ -414,7 +414,7 @@ module Cask
     def uninstall
       oh1 "Uninstalling Cask #{Formatter.identifier(@cask)}"
       uninstall_artifacts(clear: true)
-      remove_config_file unless reinstall? || upgrade?
+      remove_config_file if !reinstall? && !upgrade?
       purge_versioned_files
       purge_caskroom_path if force?
     end
@@ -435,7 +435,7 @@ module Cask
     end
 
     def restore_backup
-      return unless backup_path.directory? && backup_metadata_path.directory?
+      return if !backup_path.directory? || !backup_metadata_path.directory?
 
       Pathname.new(@cask.staged_path).rmtree if @cask.staged_path.exist?
       Pathname.new(@cask.metadata_versioned_path).rmtree if @cask.metadata_versioned_path.exist?

Library/Homebrew/cask/metadata.rb

@@ -41,10 +41,7 @@ module Cask
 
     def metadata_subdir(leaf, version: self.version, timestamp: :latest, create: false)
       raise CaskError, "Cannot create metadata subdir when timestamp is :latest." if create && timestamp == :latest
-
+      raise CaskError, "Cannot create metadata subdir for empty leaf." if !leaf.respond_to?(:empty?) || leaf.empty?
-      unless leaf.respond_to?(:empty?) && !leaf.empty?
-        raise CaskError, "Cannot create metadata subdir for empty leaf."
-      end
 
       parent = metadata_timestamped_path(version: version, timestamp: timestamp, create: create)
 

Library/Homebrew/caveats.rb

@@ -113,7 +113,7 @@ class Caveats
 
     completion_installed = keg.completion_installed?(shell)
     functions_installed = keg.functions_installed?(shell)
-    return unless completion_installed || functions_installed
+    return if !completion_installed && !functions_installed
 
     installed = []
     installed << "completions" if completion_installed

Library/Homebrew/cmd/migrate.rb

@@ -31,9 +31,8 @@ module Homebrew
 
     args.named.to_resolved_formulae.each do |f|
       if f.oldname
-        unless (rack = HOMEBREW_CELLAR/f.oldname).exist? && !rack.subdirs.empty?
+        rack = HOMEBREW_CELLAR/f.oldname
-          raise NoSuchKegError, f.oldname
+        raise NoSuchKegError, f.oldname if !rack.exist? || rack.subdirs.empty?
-        end
         raise "#{rack} is a symlink" if rack.symlink?
       end
 

Library/Homebrew/debrew.rb

@@ -112,9 +112,7 @@ module Debrew
   end
 
   def self.debug(e)
-    original_raise(e) unless active? &&
+    original_raise(e) if !active? || !debugged_exceptions.add?(e) || !try_lock
-                             debugged_exceptions.add?(e) &&
-                             try_lock
 
     begin
       puts e.backtrace.first.to_s

Library/Homebrew/deprecate_disable.rb

@@ -19,14 +19,14 @@ module DeprecateDisable
   }.freeze
 
   def deprecate_disable_info(formula)
-    return unless formula.deprecated? || formula.disabled?
-
     if formula.deprecated?
       type = :deprecated
       reason = formula.deprecation_reason
-    else
+    elsif formula.disabled?
       type = :disabled
       reason = formula.disable_reason
+    else
+      return
     end
 
     reason = DEPRECATE_DISABLE_REASONS[reason] if DEPRECATE_DISABLE_REASONS.key? reason

Library/Homebrew/dev-cmd/bottle.rb

@@ -172,7 +172,7 @@ module Homebrew
         end
       end
 
-      next unless args.verbose? && !text_matches.empty?
+      next if !args.verbose? || text_matches.empty?
 
       print_filename.call(string, file)
       text_matches.first(MAXIMUM_STRING_MATCHES).each do |match, offset|
@@ -190,7 +190,7 @@ module Homebrew
   def keg_contain_absolute_symlink_starting_with?(string, keg, args:)
     absolute_symlinks_start_with_string = []
     keg.find do |pn|
-      next unless pn.symlink? && (link = pn.readlink).absolute?
+      next if !pn.symlink? || !(link = pn.readlink).absolute?
 
       absolute_symlinks_start_with_string << pn if link.to_s.start_with?(string)
     end

Library/Homebrew/dev-cmd/bump-unversioned-casks.rb

@@ -93,7 +93,7 @@ module Homebrew
 
     unversioned_cask_checker = UnversionedCaskChecker.new(cask)
 
-    unless unversioned_cask_checker.single_app_cask? || unversioned_cask_checker.single_pkg_cask?
+    if !unversioned_cask_checker.single_app_cask? && !unversioned_cask_checker.single_pkg_cask?
       opoo "Skipping, not a single-app or PKG cask."
       return
     end

Library/Homebrew/diagnostic.rb

@@ -893,7 +893,7 @@ module Homebrew
 
         add_info "Homebrew Cask Staging Location", user_tilde(path.to_s)
 
-        return unless path.exist? && !path.writable?
+        return if !path.exist? || path.writable?
 
         <<~EOS
           The staging path #{user_tilde(path.to_s)} is not writable by the current user.

Library/Homebrew/download_strategy.rb

@@ -182,9 +182,7 @@ class VCSDownloadStrategy < AbstractDownloadStrategy
 
     version.update_commit(last_commit) if head?
 
-    return unless @ref_type == :tag
+    return if @ref_type != :tag || @revision.blank? || current_revision.blank? || current_revision == @revision
-    return unless @revision && current_revision
-    return if current_revision == @revision
 
     raise <<~EOS
       #{@ref} tag should be #{@revision}
@@ -827,7 +825,7 @@ class GitDownloadStrategy < VCSDownloadStrategy
   end
 
   def update_repo
-    return unless @ref_type == :branch || !ref?
+    return if @ref_type != :branch && ref?
 
     if !shallow_clone? && shallow_dir?
       command! "git",

Library/Homebrew/extend/os/mac/keg_relocate.rb

@@ -162,7 +162,7 @@ class Keg
     mach_o_files = []
     path.find do |pn|
       next if pn.symlink? || pn.directory?
-      next unless pn.dylib? || pn.mach_o_bundle? || pn.mach_o_executable?
+      next if !pn.dylib? && !pn.mach_o_bundle? && !pn.mach_o_executable?
       # if we've already processed a file, ignore its hardlinks (which have the same dev ID and inode)
       # this prevents relocations from being performed on a binary more than once
       next unless hardlinks.add? [pn.stat.dev, pn.stat.ino]

Library/Homebrew/extend/pathname.rb

@@ -118,9 +118,9 @@ class Pathname
 
   sig { params(src: T.any(String, Pathname), new_basename: String).void }
   def install_p(src, new_basename)
-    raise Errno::ENOENT, src.to_s unless File.symlink?(src) || File.exist?(src)
-
     src = Pathname(src)
+    raise Errno::ENOENT, src.to_s if !src.symlink? && !src.exist?
+
     dst = join(new_basename)
     dst = yield(src, dst) if block_given?
     return unless dst

Library/Homebrew/fetch.rb

@@ -10,12 +10,12 @@ module Homebrew
     def fetch_bottle?(f, args:)
       bottle = f.bottle
 
-      return true if args.force_bottle? && bottle
+      return true if args.force_bottle? && bottle.present?
-      return false unless bottle && f.pour_bottle?
-      return false if args.build_from_source_formulae.include?(f.full_name)
-      return false unless bottle.compatible_locations?
 
-      true
+      bottle.present? &&
+        f.pour_bottle? &&
+        args.build_from_source_formulae.exclude?(f.full_name) &&
+        bottle.compatible_locations?
     end
   end
 end

Library/Homebrew/formula.rb

@@ -262,13 +262,13 @@ class Formula
   end
 
   def validate_attributes!
-    raise FormulaValidationError.new(full_name, :name, name) if name.blank? || name =~ /\s/
+    raise FormulaValidationError.new(full_name, :name, name) if name.blank? || name.match?(/\s/)
 
     url = active_spec.url
-    raise FormulaValidationError.new(full_name, :url, url) if url.blank? || url =~ /\s/
+    raise FormulaValidationError.new(full_name, :url, url) if url.blank? || url.match?(/\s/)
 
     val = version.respond_to?(:to_str) ? version.to_str : version
-    return unless val.blank? || val =~ /\s/
+    return if val.present? && !val.match?(/\s/)
 
     raise FormulaValidationError.new(full_name, :version, val)
   end

Library/Homebrew/formula_installer.rb

@@ -193,12 +193,12 @@ class FormulaInstaller
   sig { params(dep: Formula, build: BuildOptions).returns(T::Boolean) }
   def install_bottle_for?(dep, build)
     return pour_bottle? if dep == formula
-    return false if @build_from_source_formulae.include?(dep.full_name)
-    return false unless dep.bottle && dep.pour_bottle?
-    return false unless build.used_options.empty?
-    return false unless dep.bottle&.compatible_locations?
 
-    true
+    @build_from_source_formulae.exclude?(dep.full_name) &&
+      dep.bottle.present? &&
+      dep.pour_bottle? &&
+      build.used_options.empty? &&
+      dep.bottle&.compatible_locations?
   end
 
   sig { void }
@@ -803,7 +803,7 @@ class FormulaInstaller
     keg = Keg.new(formula.prefix)
     link(keg)
 
-    fix_dynamic_linkage(keg) unless @poured_bottle && formula.bottle_specification.skip_relocation?
+    fix_dynamic_linkage(keg) if !@poured_bottle || !formula.bottle_specification.skip_relocation?
 
     if build_bottle?
       ohai "Not running post_install as we're building a bottle"

Library/Homebrew/formula_pin.rb

@@ -18,7 +18,7 @@ class FormulaPin
   def pin_at(version)
     HOMEBREW_PINNED_KEGS.mkpath
     version_path = @f.rack/version
-    path.make_relative_symlink(version_path) unless pinned? || !version_path.exist?
+    path.make_relative_symlink(version_path) if !pinned? && version_path.exist?
   end
 
   def pin

Library/Homebrew/keg_relocate.rb

@@ -146,7 +146,7 @@ class Keg
 
   def text_files
     text_files = []
-    return text_files unless which("file") && which("xargs")
+    return text_files if !which("file") || !which("xargs")
 
     # file has known issues with reading files on other locales. Has
     # been fixed upstream for some time, but a sufficiently new enough

Library/Homebrew/linkage_checker.rb

@@ -212,9 +212,8 @@ class LinkageChecker
   def check_formula_deps
     filter_out = proc do |dep|
       next true if dep.build?
-      next false unless dep.optional? || dep.recommended?
 
-      formula.build.without?(dep)
+      (dep.optional? || dep.recommended?) && formula.build.without?(dep)
     end
 
     declared_deps_full_names = formula.deps

Library/Homebrew/livecheck/strategy.rb

@@ -64,7 +64,7 @@ module Homebrew
           if strategy == PageMatch
             # Only treat the `PageMatch` strategy as usable if a regex is
             # present in the `livecheck` block
-            next unless regex_provided || block_provided
+            next if !regex_provided && !block_provided
           elsif strategy.const_defined?(:PRIORITY) &&
                 !strategy::PRIORITY.positive? &&
                 from_symbol(livecheck_strategy) != strategy

Library/Homebrew/lock_file.rb

@@ -40,7 +40,7 @@ class LockFile
   private
 
   def create_lockfile
-    return unless @lockfile.nil? || @lockfile.closed?
+    return if @lockfile.present? && !@lockfile.closed?
 
     @lockfile = @path.open(File::RDWR | File::CREAT)
     @lockfile.fcntl(Fcntl::F_SETFD, Fcntl::FD_CLOEXEC)

Library/Homebrew/rubocops.rb

@@ -32,5 +32,6 @@ require "rubocops/files"
 require "rubocops/keg_only"
 require "rubocops/version"
 require "rubocops/deprecate_disable"
+require "rubocops/unless_multiple_conditions"
 
 require "rubocops/rubocop-cask"

Library/Homebrew/rubocops/extend/formula.rb

@@ -296,7 +296,7 @@ module RuboCop
 
         node.each_child_node(:def) do |def_node|
           def_method_name = method_name(def_node)
-          next unless method_name == def_method_name || method_name.nil?
+          next if method_name != def_method_name && method_name.present?
 
           @offensive_node = def_node
           @offense_source_range = def_node.source_range

Library/Homebrew/rubocops/patches.rb

@@ -113,7 +113,7 @@ module RuboCop
         end
 
         def inline_patch_problems(patch)
-          return unless patch_data?(patch) && !patch_end?
+          return if !patch_data?(patch) || patch_end?
 
           offending_node(patch)
           problem "patch is missing '__END__'"

Library/Homebrew/rubocops/unless_multiple_conditions.rb

@@ -0,0 +1,39 @@
+# typed: strict
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Style
+      # This cop checks that `unless` is not used with multiple conditions.
+      #
+      # @api private
+      class UnlessMultipleConditions < Cop
+        extend T::Sig
+
+        MSG = "Avoid using `unless` with multiple conditions."
+
+        sig { params(node: RuboCop::AST::IfNode).void }
+        def on_if(node)
+          return if !node.unless? || (!node.condition.and_type? && !node.condition.or_type?)
+
+          add_offense(node, location: node.condition.source_range.with(begin_pos: node.loc.keyword.begin_pos))
+        end
+
+        sig { params(node: RuboCop::AST::IfNode).returns(T.proc.params(arg0: RuboCop::Cop::Corrector).void) }
+        def autocorrect(node)
+          lambda do |corrector|
+            corrector.replace(node.loc.keyword, "if")
+            corrector.replace(node.condition.loc.operator, node.condition.inverse_operator)
+            [node.condition.lhs, node.condition.rhs].each do |subcondition|
+              if !subcondition.source.start_with?("(") || !subcondition.source.end_with?(")")
+                corrector.insert_before(subcondition, "(")
+                corrector.insert_after(subcondition, ")")
+              end
+              corrector.insert_before(subcondition, "!")
+            end
+          end
+        end
+      end
+    end
+  end
+end

Library/Homebrew/tap.rb

@@ -193,7 +193,7 @@ class Tap
   # e.g. `https://github.com/user/homebrew-repo/issues`
   sig { returns(T.nilable(String)) }
   def issues_url
-    return unless official? || !custom_remote?
+    return if !official? && custom_remote?
 
     "#{default_remote}/issues"
   end

Library/Homebrew/test/rubocops/unless_multiple_conditions_spec.rb

@@ -0,0 +1,128 @@
+# typed: false
+# frozen_string_literal: true
+
+require "rubocops/unless_multiple_conditions"
+
+describe RuboCop::Cop::Style::UnlessMultipleConditions do
+  subject(:cop) { described_class.new }
+
+  it "reports an offense when using `unless` with multiple `and` conditions" do
+    expect_offense <<~RUBY
+      unless foo && bar
+      ^^^^^^^^^^^^^^^^^ Avoid using `unless` with multiple conditions.
+        something
+      end
+    RUBY
+
+    expect_offense <<~RUBY
+      something unless foo && bar
+                ^^^^^^^^^^^^^^^^^ Avoid using `unless` with multiple conditions.
+    RUBY
+  end
+
+  it "reports an offense when using `unless` with multiple `or` conditions" do
+    expect_offense <<~RUBY
+      unless foo || bar
+      ^^^^^^^^^^^^^^^^^ Avoid using `unless` with multiple conditions.
+        something
+      end
+    RUBY
+
+    expect_offense <<~RUBY
+      something unless foo || bar
+                ^^^^^^^^^^^^^^^^^ Avoid using `unless` with multiple conditions.
+    RUBY
+  end
+
+  it "reports no offenses when using `if` with multiple `and` conditions" do
+    expect_no_offenses <<~RUBY
+      if !foo && !bar
+        something
+      end
+    RUBY
+
+    expect_no_offenses <<~RUBY
+      something if !foo && !bar
+    RUBY
+  end
+
+  it "reports no offenses when using `if` with multiple `or` conditions" do
+    expect_no_offenses <<~RUBY
+      if !foo || !bar
+        something
+      end
+    RUBY
+
+    expect_no_offenses <<~RUBY
+      something if !foo || !bar
+    RUBY
+  end
+
+  it "reports no offenses when using `unless` with single condition" do
+    expect_no_offenses <<~RUBY
+      unless foo
+        something
+      end
+    RUBY
+
+    expect_no_offenses <<~RUBY
+      something unless foo
+    RUBY
+  end
+
+  it "auto-corrects `unless` with multiple `and` conditions" do
+    source = <<~RUBY
+      unless foo && (bar || baz)
+        something
+      end
+    RUBY
+
+    corrected_source = <<~RUBY
+      if !(foo) || !(bar || baz)
+        something
+      end
+    RUBY
+
+    new_source = autocorrect_source(source)
+    expect(new_source).to eq(corrected_source)
+
+    source = <<~RUBY
+      something unless foo && bar
+    RUBY
+
+    corrected_source = <<~RUBY
+      something if !(foo) || !(bar)
+    RUBY
+
+    new_source = autocorrect_source(source)
+    expect(new_source).to eq(corrected_source)
+  end
+
+  it "auto-corrects `unless` with multiple `or` conditions" do
+    source = <<~RUBY
+      unless foo || (bar && baz)
+        something
+      end
+    RUBY
+
+    corrected_source = <<~RUBY
+      if !(foo) && !(bar && baz)
+        something
+      end
+    RUBY
+
+    new_source = autocorrect_source(source)
+    expect(new_source).to eq(corrected_source)
+
+    source = <<~RUBY
+      something unless foo || bar
+    RUBY
+
+    corrected_source = <<~RUBY
+      something if !(foo) && !(bar)
+    RUBY
+
+    new_source = autocorrect_source(source)
+    expect(new_source).to eq(corrected_source)
+  end
+end

Library/Homebrew/utils.rb

@@ -119,7 +119,7 @@ module Kernel
       Context.current.debug?
     end
 
-    return unless debug || always_display
+    return if !debug && !always_display
 
     puts Formatter.headline(title, color: :magenta)
     puts sput unless sput.empty?

Library/Homebrew/utils/bottles.rb

@@ -24,7 +24,7 @@ module Utils
 
       def file_outdated?(f, file)
         filename = file.basename.to_s
-        return unless f.bottle && filename.match(Pathname::BOTTLE_EXTNAME_RX)
+        return if f.bottle.blank? || !filename.match?(Pathname::BOTTLE_EXTNAME_RX)
 
         bottle_ext = filename[native_regex, 1]
         bottle_url_ext = f.bottle.url[native_regex, 1]

Library/Homebrew/utils/pypi.rb

@@ -29,10 +29,10 @@ module PyPI
       @pypi_info = nil
 
       if is_url
-        unless package_string.start_with?(PYTHONHOSTED_URL_PREFIX) &&
+        match = if package_string.start_with?(PYTHONHOSTED_URL_PREFIX)
-               match = File.basename(package_string).match(/^(.+)-([a-z\d.]+?)(?:.tar.gz|.zip)$/)
+          File.basename(package_string).match(/^(.+)-([a-z\d.]+?)(?:.tar.gz|.zip)$/)
-          raise ArgumentError, "package should be a valid PyPI url"
         end
+        raise ArgumentError, "package should be a valid PyPI url" if match.blank?
 
         @name = match[1]
         @version = match[2]