diff --git a/Library/.rubocop.yml b/Library/.rubocop.yml index 1fc717c0b5..1b448cca23 100644 --- a/Library/.rubocop.yml +++ b/Library/.rubocop.yml @@ -51,6 +51,11 @@ FormulaAuditStrict: Homebrew: Enabled: true +Homebrew/Blank: + Exclude: + # Core extensions are not available here: + - "Homebrew/rubocops/**/*" + # only used internally Homebrew/MoveToExtendOS: Enabled: false @@ -210,19 +215,10 @@ Rails: - "Homebrew/rubocops/**/*" # These relate to ActiveSupport and not other parts of Rails. -Rails/Blank: - Enabled: true Rails/CompactBlank: Enabled: true -Rails/NegateInclude: - Enabled: true Rails/Presence: Enabled: true -Rails/Present: - Enabled: true - Exclude: - # `present?` is defined as `!blank?` wihin this file - - "Homebrew/extend/blank.rb" Rails/SafeNavigationWithBlank: Enabled: true @@ -355,6 +351,15 @@ Style/HashAsLastArrayItem: - "/**/Formula/**/*.rb" - "**/Formula/**/*.rb" +Style/InverseMethods: + Exclude: + # Core extensions are not available here: + - "Homebrew/standalone/init.rb" + - "Homebrew/rubocops/**/*" + InverseMethods: + :blank?: :present? + :exclude?: :include? + Style/InvertibleUnlessCondition: Enabled: true InverseMethods: @@ -364,6 +369,7 @@ Style/InvertibleUnlessCondition: :zero?: # Don't require non-standard `exclude?` (for now at least) - it's not available in every file :include?: + :blank?: :present? Style/MutableConstant: # would rather freeze too much than too little diff --git a/Library/Homebrew/rubocops/all.rb b/Library/Homebrew/rubocops/all.rb index 5986732a9b..4fee6b2e24 100644 --- a/Library/Homebrew/rubocops/all.rb +++ b/Library/Homebrew/rubocops/all.rb @@ -2,8 +2,10 @@ # frozen_string_literal: true require_relative "../extend/array" +require_relative "blank" require_relative "io_read" require_relative "move_to_extend_os" +require_relative "present" require_relative "shell_commands" # formula audit cops diff --git a/Library/Homebrew/rubocops/blank.rb b/Library/Homebrew/rubocops/blank.rb new file mode 100644 index 0000000000..4affb4e59d --- /dev/null +++ b/Library/Homebrew/rubocops/blank.rb @@ -0,0 +1,73 @@ +# typed: true +# frozen_string_literal: true + +module RuboCop + module Cop + module Homebrew + # Checks for code that can be written with simpler conditionals + # using `Object#blank?`. + # + # @note + # This cop is unsafe autocorrection, because `' '.empty?` returns false, + # but `' '.blank?` returns true. Therefore, autocorrection is not compatible + # if the receiver is a non-empty blank string, tab, or newline meta characters. + # + # @example + # # Converts usages of `nil? || empty?` to `blank?` + # + # # bad + # foo.nil? || foo.empty? + # foo == nil || foo.empty? + # + # # good + # foo.blank? + class Blank < Base + extend AutoCorrector + + MSG_NIL_OR_EMPTY = "Use `%s` instead of `%s`." + + # `(send nil $_)` is not actually a valid match for an offense. Nodes + # that have a single method call on the left hand side + # (`bar || foo.empty?`) will blow up when checking + # `(send (:nil) :== $_)`. + def_node_matcher :nil_or_empty?, <<~PATTERN + (or + { + (send $_ :!) + (send $_ :nil?) + (send $_ :== nil) + (send nil :== $_) + } + { + (send $_ :empty?) + (send (send (send $_ :empty?) :!) :!) + } + ) + PATTERN + + def on_or(node) + nil_or_empty?(node) do |var1, var2| + return if var1 != var2 + + message = format(MSG_NIL_OR_EMPTY, prefer: replacement(var1), current: node.source) + add_offense(node, message: message) do |corrector| + autocorrect(corrector, node) + end + end + end + + private + + def autocorrect(corrector, node) + variable1, _variable2 = nil_or_empty?(node) + range = node.source_range + corrector.replace(range, replacement(variable1)) + end + + def replacement(node) + node.respond_to?(:source) ? "#{node.source}.blank?" : "blank?" + end + end + end + end +end diff --git a/Library/Homebrew/rubocops/present.rb b/Library/Homebrew/rubocops/present.rb new file mode 100644 index 0000000000..99e8fe2898 --- /dev/null +++ b/Library/Homebrew/rubocops/present.rb @@ -0,0 +1,76 @@ +# typed: true +# frozen_string_literal: true + +module RuboCop + module Cop + module Homebrew + # Checks for code that can be written with simpler conditionals + # using `Object#present?`. + # + # @example + # # Converts usages of `!nil? && !empty?` to `present?` + # + # # bad + # !foo.nil? && !foo.empty? + # + # # bad + # foo != nil && !foo.empty? + # + # # good + # foo.present? + class Present < Base + extend AutoCorrector + + MSG_EXISTS_AND_NOT_EMPTY = "Use `%s` instead of `%s`." + + def_node_matcher :exists_and_not_empty?, <<~PATTERN + (and + { + (send (send $_ :nil?) :!) + (send (send $_ :!) :!) + (send $_ :!= nil) + $_ + } + { + (send (send $_ :empty?) :!) + } + ) + PATTERN + + def on_and(node) + exists_and_not_empty?(node) do |var1, var2| + return if var1 != var2 + + message = format(MSG_EXISTS_AND_NOT_EMPTY, prefer: replacement(var1), current: node.source) + + add_offense(node, message: message) do |corrector| + autocorrect(corrector, node) + end + end + end + + def on_or(node) + exists_and_not_empty?(node) do |var1, var2| + return if var1 != var2 + + add_offense(node, message: MSG_EXISTS_AND_NOT_EMPTY) do |corrector| + autocorrect(corrector, node) + end + end + end + + def autocorrect(corrector, node) + variable1, _variable2 = exists_and_not_empty?(node) + range = node.source_range + corrector.replace(range, replacement(variable1)) + end + + private + + def replacement(node) + node.respond_to?(:source) ? "#{node.source}.present?" : "present?" + end + end + end + end +end diff --git a/Library/Homebrew/startup/bootsnap.rb b/Library/Homebrew/startup/bootsnap.rb index b78fb8d317..d8879e3c1c 100644 --- a/Library/Homebrew/startup/bootsnap.rb +++ b/Library/Homebrew/startup/bootsnap.rb @@ -2,7 +2,7 @@ # frozen_string_literal: true # Disable Rails cops, as we haven't required active_support yet. -# rubocop:disable Rails +# rubocop:disable Rails, Homebrew/Blank homebrew_bootsnap_enabled = ENV["HOMEBREW_NO_BOOTSNAP"].nil? && !ENV["HOMEBREW_BOOTSNAP"].nil? # we need some development tools to build bootsnap native code @@ -49,4 +49,4 @@ if homebrew_bootsnap_enabled $stderr.puts "Error: HOMEBREW_BOOTSNAP could not `require \"bootsnap\"`!\n\n" end end -# rubocop:enable Rails +# rubocop:enable Rails, Homebrew/Blank diff --git a/Library/Homebrew/test/rubocops/blank_spec.rb b/Library/Homebrew/test/rubocops/blank_spec.rb new file mode 100644 index 0000000000..c151dd523e --- /dev/null +++ b/Library/Homebrew/test/rubocops/blank_spec.rb @@ -0,0 +1,108 @@ +# frozen_string_literal: true + +require "rubocops/blank" + +describe RuboCop::Cop::Homebrew::Blank do + subject(:cop) { described_class.new } + + shared_examples "offense" do |source, correction, message| + it "registers an offense and corrects" do + expect_offense(<<~RUBY, source: source, message: message) + #{source} + ^{source} #{message} + RUBY + + expect_correction(<<~RUBY) + #{correction} + RUBY + end + end + + it "accepts checking nil?" do + expect_no_offenses("foo.nil?") + end + + it "accepts checking empty?" do + expect_no_offenses("foo.empty?") + end + + it "accepts checking nil? || empty? on different objects" do + expect_no_offenses("foo.nil? || bar.empty?") + end + + # Bug: https://github.com/rubocop/rubocop/issues/4171 + it "does not break when RHS of `or` is a naked falsiness check" do + expect_no_offenses("foo.empty? || bar") + end + + it "does not break when LHS of `or` is a naked falsiness check" do + expect_no_offenses("bar || foo.empty?") + end + + # Bug: https://github.com/rubocop/rubocop/issues/4814 + it "does not break when LHS of `or` is a send node with an argument" do + expect_no_offenses("x(1) || something") + end + + context "when nil or empty" do + it_behaves_like "offense", "foo.nil? || foo.empty?", + "foo.blank?", + "Homebrew/Blank: Use `foo.blank?` instead of `foo.nil? || foo.empty?`." + it_behaves_like "offense", "nil? || empty?", "blank?", "Homebrew/Blank: Use `blank?` instead of `nil? || empty?`." + it_behaves_like "offense", "foo == nil || foo.empty?", + "foo.blank?", + "Homebrew/Blank: Use `foo.blank?` instead of `foo == nil || foo.empty?`." + it_behaves_like "offense", "nil == foo || foo.empty?", + "foo.blank?", + "Homebrew/Blank: Use `foo.blank?` instead of `nil == foo || foo.empty?`." + it_behaves_like "offense", "!foo || foo.empty?", "foo.blank?", + "Homebrew/Blank: Use `foo.blank?` instead of `!foo || foo.empty?`." + + it_behaves_like "offense", "foo.nil? || !!foo.empty?", + "foo.blank?", + "Homebrew/Blank: Use `foo.blank?` instead of `foo.nil? || !!foo.empty?`." + it_behaves_like "offense", "foo == nil || !!foo.empty?", + "foo.blank?", + "Homebrew/Blank: Use `foo.blank?` instead of " \ + "`foo == nil || !!foo.empty?`." + it_behaves_like "offense", "nil == foo || !!foo.empty?", + "foo.blank?", + "Homebrew/Blank: Use `foo.blank?` instead of " \ + "`nil == foo || !!foo.empty?`." + end + + context "when checking all variable types" do + it_behaves_like "offense", "foo.bar.nil? || foo.bar.empty?", + "foo.bar.blank?", + "Homebrew/Blank: Use `foo.bar.blank?` instead of " \ + "`foo.bar.nil? || foo.bar.empty?`." + it_behaves_like "offense", "FOO.nil? || FOO.empty?", + "FOO.blank?", + "Homebrew/Blank: Use `FOO.blank?` instead of `FOO.nil? || FOO.empty?`." + it_behaves_like "offense", "Foo.nil? || Foo.empty?", + "Foo.blank?", + "Homebrew/Blank: Use `Foo.blank?` instead of `Foo.nil? || Foo.empty?`." + it_behaves_like "offense", "Foo::Bar.nil? || Foo::Bar.empty?", + "Foo::Bar.blank?", + "Homebrew/Blank: Use `Foo::Bar.blank?` instead of " \ + "`Foo::Bar.nil? || Foo::Bar.empty?`." + it_behaves_like "offense", "@foo.nil? || @foo.empty?", + "@foo.blank?", + "Homebrew/Blank: Use `@foo.blank?` instead of `@foo.nil? || @foo.empty?`." + it_behaves_like "offense", "$foo.nil? || $foo.empty?", + "$foo.blank?", + "Homebrew/Blank: Use `$foo.blank?` instead of `$foo.nil? || $foo.empty?`." + it_behaves_like "offense", "@@foo.nil? || @@foo.empty?", + "@@foo.blank?", + "Homebrew/Blank: Use `@@foo.blank?` instead of " \ + "`@@foo.nil? || @@foo.empty?`." + it_behaves_like "offense", "foo[bar].nil? || foo[bar].empty?", + "foo[bar].blank?", + "Homebrew/Blank: Use `foo[bar].blank?` instead of " \ + "`foo[bar].nil? || foo[bar].empty?`." + it_behaves_like "offense", "foo(bar).nil? || foo(bar).empty?", + "foo(bar).blank?", + "Homebrew/Blank: Use `foo(bar).blank?` instead of " \ + "`foo(bar).nil? || foo(bar).empty?`." + end +end diff --git a/Library/Homebrew/test/rubocops/present_spec.rb b/Library/Homebrew/test/rubocops/present_spec.rb new file mode 100644 index 0000000000..ec9280a935 --- /dev/null +++ b/Library/Homebrew/test/rubocops/present_spec.rb @@ -0,0 +1,94 @@ +# frozen_string_literal: true + +require "rubocops/present" + +describe RuboCop::Cop::Homebrew::Present do + subject(:cop) { described_class.new } + + shared_examples "offense" do |source, correction, message| + it "registers an offense and corrects" do + expect_offense(<<~RUBY, source: source, message: message) + #{source} + ^{source} #{message} + RUBY + + expect_correction(<<~RUBY) + #{correction} + RUBY + end + end + + it "accepts checking nil?" do + expect_no_offenses("foo.nil?") + end + + it "accepts checking empty?" do + expect_no_offenses("foo.empty?") + end + + it "accepts checking nil? || empty? on different objects" do + expect_no_offenses("foo.nil? || bar.empty?") + end + + it "accepts checking existence && not empty? on different objects" do + expect_no_offenses("foo && !bar.empty?") + end + + it_behaves_like "offense", "foo && !foo.empty?", + "foo.present?", + "Homebrew/Present: Use `foo.present?` instead of `foo && !foo.empty?`." + it_behaves_like "offense", "!foo.nil? && !foo.empty?", + "foo.present?", + "Homebrew/Present: Use `foo.present?` instead of `!foo.nil? && !foo.empty?`." + it_behaves_like "offense", "!nil? && !empty?", "present?", + "Homebrew/Present: Use `present?` instead of `!nil? && !empty?`." + it_behaves_like "offense", "foo != nil && !foo.empty?", + "foo.present?", + "Homebrew/Present: Use `foo.present?` instead of `foo != nil && !foo.empty?`." + it_behaves_like "offense", "!!foo && !foo.empty?", + "foo.present?", + "Homebrew/Present: Use `foo.present?` instead of `!!foo && !foo.empty?`." + + context "when checking all variable types" do + it_behaves_like "offense", "!foo.nil? && !foo.empty?", + "foo.present?", + "Homebrew/Present: Use `foo.present?` instead of " \ + "`!foo.nil? && !foo.empty?`." + it_behaves_like "offense", "!foo.bar.nil? && !foo.bar.empty?", + "foo.bar.present?", + "Homebrew/Present: Use `foo.bar.present?` instead of " \ + "`!foo.bar.nil? && !foo.bar.empty?`." + it_behaves_like "offense", "!FOO.nil? && !FOO.empty?", + "FOO.present?", + "Homebrew/Present: Use `FOO.present?` instead of " \ + "`!FOO.nil? && !FOO.empty?`." + it_behaves_like "offense", "!Foo.nil? && !Foo.empty?", + "Foo.present?", + "Homebrew/Present: Use `Foo.present?` instead of " \ + "`!Foo.nil? && !Foo.empty?`." + it_behaves_like "offense", "!@foo.nil? && !@foo.empty?", + "@foo.present?", + "Homebrew/Present: Use `@foo.present?` instead of " \ + "`!@foo.nil? && !@foo.empty?`." + it_behaves_like "offense", "!$foo.nil? && !$foo.empty?", + "$foo.present?", + "Homebrew/Present: Use `$foo.present?` instead of " \ + "`!$foo.nil? && !$foo.empty?`." + it_behaves_like "offense", "!@@foo.nil? && !@@foo.empty?", + "@@foo.present?", + "Homebrew/Present: Use `@@foo.present?` instead of " \ + "`!@@foo.nil? && !@@foo.empty?`." + it_behaves_like "offense", "!foo[bar].nil? && !foo[bar].empty?", + "foo[bar].present?", + "Homebrew/Present: Use `foo[bar].present?` instead of " \ + "`!foo[bar].nil? && !foo[bar].empty?`." + it_behaves_like "offense", "!Foo::Bar.nil? && !Foo::Bar.empty?", + "Foo::Bar.present?", + "Homebrew/Present: Use `Foo::Bar.present?` instead of " \ + "`!Foo::Bar.nil? && !Foo::Bar.empty?`." + it_behaves_like "offense", "!foo(bar).nil? && !foo(bar).empty?", + "foo(bar).present?", + "Homebrew/Present: Use `foo(bar).present?` instead of " \ + "`!foo(bar).nil? && !foo(bar).empty?`." + end +end