diff --git a/Library/Homebrew/formula.rb b/Library/Homebrew/formula.rb index b1c4121cde641..72abd4543364c 100644 --- a/Library/Homebrew/formula.rb +++ b/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 diff --git a/Library/Homebrew/formula_support.rb b/Library/Homebrew/formula_support.rb index ccbfe63f59065..1f8e69a00ad67 100644 --- a/Library/Homebrew/formula_support.rb +++ b/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 diff --git a/Library/Homebrew/rubocops/lines.rb b/Library/Homebrew/rubocops/lines.rb index 83423d59c888e..01d53088f79f8 100644 --- a/Library/Homebrew/rubocops/lines.rb +++ b/Library/Homebrew/rubocops/lines.rb @@ -347,6 +347,82 @@ def audit_formula(_node, _class_node, _parent_class_node, body_node) 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 diff --git a/Library/Homebrew/rubocops/shared/helper_functions.rb b/Library/Homebrew/rubocops/shared/helper_functions.rb index 40f522a7bfc07..cd4e29c9d501f 100644 --- a/Library/Homebrew/rubocops/shared/helper_functions.rb +++ b/Library/Homebrew/rubocops/shared/helper_functions.rb @@ -113,8 +113,10 @@ def find_node_method_by_name(node, method_name) nil end - # Sets the given node as the offending node when required in custom cops. - def offending_node(node) + # Gets/sets the given node as the offending node when required in custom cops. + def offending_node(node = nil) + return @offensive_node if node.nil? + @offensive_node = node end diff --git a/Library/Homebrew/test/rubocops/text/on_os_conditionals_spec.rb b/Library/Homebrew/test/rubocops/text/on_os_conditionals_spec.rb new file mode 100644 index 0000000000000..28f1a41185a88 --- /dev/null +++ b/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