From 96400e01e1fbb82147f84d94164088a6c89bfe3e Mon Sep 17 00:00:00 2001 From: Issy Long Date: Mon, 22 Jul 2024 23:06:53 +0100 Subject: [PATCH 1/4] rubocops/text: Enforce `bin/"formula"` instead of `"#{bin}/formula"` --- Library/Homebrew/rubocops/text.rb | 10 ++++++++++ .../dsl/rubo_cop/cop/formula_audit_strict/text.rbi | 10 ++++++++++ Library/Homebrew/test/rubocops/text/strict_spec.rb | 11 +++++++++++ 3 files changed, 31 insertions(+) diff --git a/Library/Homebrew/rubocops/text.rb b/Library/Homebrew/rubocops/text.rb index 3932c42757..b3e06f050e 100644 --- a/Library/Homebrew/rubocops/text.rb +++ b/Library/Homebrew/rubocops/text.rb @@ -137,6 +137,11 @@ module RuboCop problem "Use `\#{pkgshare}` instead of `\#{share}/#{@formula_name}`" end + interpolated_bin_path_starts_with(body_node, "/#{@formula_name}") do |bin_node| + offending_node(bin_node) + problem "Use `bin/\"#{@formula_name}\"` instead of `\"\#{bin}/#{@formula_name}\"`" + end + return if formula_tap != "homebrew-core" find_method_with_args(body_node, :env, :std) do @@ -154,6 +159,11 @@ module RuboCop $(dstr (begin (send nil? :share)) (str #path_starts_with?(%1))) EOS + # Find "#{bin}/foo" + def_node_search :interpolated_bin_path_starts_with, <<~EOS + $(dstr (begin (send nil? :bin)) (str #path_starts_with?(%1))) + EOS + # Find share/"foo" def_node_search :share_path_starts_with, <<~EOS $(send (send nil? :share) :/ (str #path_starts_with?(%1))) diff --git a/Library/Homebrew/sorbet/rbi/dsl/rubo_cop/cop/formula_audit_strict/text.rbi b/Library/Homebrew/sorbet/rbi/dsl/rubo_cop/cop/formula_audit_strict/text.rbi index 79a6ccab5b..7a0b12502c 100644 --- a/Library/Homebrew/sorbet/rbi/dsl/rubo_cop/cop/formula_audit_strict/text.rbi +++ b/Library/Homebrew/sorbet/rbi/dsl/rubo_cop/cop/formula_audit_strict/text.rbi @@ -6,6 +6,16 @@ class RuboCop::Cop::FormulaAuditStrict::Text + sig do + params( + node: RuboCop::AST::Node, + pattern: T.any(String, Symbol), + kwargs: T.untyped, + block: T.untyped + ).returns(T.untyped) + end + def interpolated_bin_path_starts_with(node, *pattern, **kwargs, &block); end + sig do params( node: RuboCop::AST::Node, diff --git a/Library/Homebrew/test/rubocops/text/strict_spec.rb b/Library/Homebrew/test/rubocops/text/strict_spec.rb index 339bb77f24..e39288dcdc 100644 --- a/Library/Homebrew/test/rubocops/text/strict_spec.rb +++ b/Library/Homebrew/test/rubocops/text/strict_spec.rb @@ -131,5 +131,16 @@ RSpec.describe RuboCop::Cop::FormulaAuditStrict::Text do end RUBY end + + it 'reports an offense if "\#{bin}/" is present' do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + test do + ohai "\#{bin}/foo", "-v" + ^^^^^^^^^^^^ FormulaAuditStrict/Text: Use `bin/"foo"` instead of `"\#{bin}/foo"` + end + end + RUBY + end end end From 3713939e0dfe85fa8d00974fdf619f3b2c92528c Mon Sep 17 00:00:00 2001 From: Issy Long Date: Wed, 24 Jul 2024 21:57:45 +0100 Subject: [PATCH 2/4] rubocops/text: Include dashed binaries in `bin/` interpolation check - Previously this only included the formula name. - But, for example in tests, we have "#{bin}/ansible-test", not just "#{bin}/ansible". So handle that too. - I decided to make the error message better by extracting the binary name from the interpolation, but I'm not sure it was worth it. ``` $ brew audit --strict ansible ansible * line 580, col 29: Use `bin/"ansible-test"` instead of `"#{bin}/ansible-test"` Error: 1 problem in 1 formula detected. ``` --- Library/Homebrew/rubocops/text.rb | 15 +++++++++------ .../Homebrew/test/rubocops/text/strict_spec.rb | 4 +++- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/Library/Homebrew/rubocops/text.rb b/Library/Homebrew/rubocops/text.rb index b3e06f050e..9f3454c033 100644 --- a/Library/Homebrew/rubocops/text.rb +++ b/Library/Homebrew/rubocops/text.rb @@ -137,9 +137,10 @@ module RuboCop problem "Use `\#{pkgshare}` instead of `\#{share}/#{@formula_name}`" end - interpolated_bin_path_starts_with(body_node, "/#{@formula_name}") do |bin_node| + interpolated_bin_path_starts_with(body_node, "/#{@formula_name}", true) do |bin_node| offending_node(bin_node) - problem "Use `bin/\"#{@formula_name}\"` instead of `\"\#{bin}/#{@formula_name}\"`" + cmd = bin_node.source.match(%r{\#{bin}/(\S+)})[1]&.delete_suffix('"') || @formula_name + problem "Use `bin/\"#{cmd}\"` instead of `\"\#{bin}/#{cmd}\"`" end return if formula_tap != "homebrew-core" @@ -150,8 +151,10 @@ module RuboCop end # Check whether value starts with the formula name and then a "/", " " or EOS. - def path_starts_with?(path, starts_with) - path.match?(%r{^#{Regexp.escape(starts_with)}(/| |$)}) + # If we're checking for "#{bin}", we also check for "-" since similar binaries also don't need interpolation. + def path_starts_with?(path, starts_with, bin = false) + ending = bin ? "/| |-|$" : "/| |$" + path.match?(/^#{Regexp.escape(starts_with)}(#{ending})/) end # Find "#{share}/foo" @@ -159,9 +162,9 @@ module RuboCop $(dstr (begin (send nil? :share)) (str #path_starts_with?(%1))) EOS - # Find "#{bin}/foo" + # Find "#{bin}/foo" and "#{bin}/foo-bar" def_node_search :interpolated_bin_path_starts_with, <<~EOS - $(dstr (begin (send nil? :bin)) (str #path_starts_with?(%1))) + $(dstr (begin (send nil? :bin)) (str #path_starts_with?(%1, %2))) EOS # Find share/"foo" diff --git a/Library/Homebrew/test/rubocops/text/strict_spec.rb b/Library/Homebrew/test/rubocops/text/strict_spec.rb index e39288dcdc..f802cb376d 100644 --- a/Library/Homebrew/test/rubocops/text/strict_spec.rb +++ b/Library/Homebrew/test/rubocops/text/strict_spec.rb @@ -132,12 +132,14 @@ RSpec.describe RuboCop::Cop::FormulaAuditStrict::Text do RUBY end - it 'reports an offense if "\#{bin}/" is present' do + it 'reports an offense if "\#{bin}/" or other dashed binaries too are present' do expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") class Foo < Formula test do ohai "\#{bin}/foo", "-v" ^^^^^^^^^^^^ FormulaAuditStrict/Text: Use `bin/"foo"` instead of `"\#{bin}/foo"` + ohai "\#{bin}/foo-bar", "-v" + ^^^^^^^^^^^^^^^^ FormulaAuditStrict/Text: Use `bin/"foo-bar"` instead of `"\#{bin}/foo-bar"` end end RUBY From ace23ce735875e09a94fff18ff347a2365b4d3c2 Mon Sep 17 00:00:00 2001 From: Issy Long Date: Wed, 24 Jul 2024 22:40:01 +0100 Subject: [PATCH 3/4] Make the bin `starts_with` method its own thing as it needs more args - I couldn't get https://docs.rubocop.org/rubocop-ast/node_pattern.html#param_name-for-named-parameters to work like it said it should (bad syntax in the node_matcher, apart from with `bin = false` which RuboCop complained about boolean args not being named), so here's a workaround. --- Library/Homebrew/rubocops/text.rb | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/Library/Homebrew/rubocops/text.rb b/Library/Homebrew/rubocops/text.rb index 9f3454c033..b3bb324d74 100644 --- a/Library/Homebrew/rubocops/text.rb +++ b/Library/Homebrew/rubocops/text.rb @@ -137,7 +137,7 @@ module RuboCop problem "Use `\#{pkgshare}` instead of `\#{share}/#{@formula_name}`" end - interpolated_bin_path_starts_with(body_node, "/#{@formula_name}", true) do |bin_node| + interpolated_bin_path_starts_with(body_node, "/#{@formula_name}") do |bin_node| offending_node(bin_node) cmd = bin_node.source.match(%r{\#{bin}/(\S+)})[1]&.delete_suffix('"') || @formula_name problem "Use `bin/\"#{cmd}\"` instead of `\"\#{bin}/#{cmd}\"`" @@ -152,11 +152,15 @@ module RuboCop # Check whether value starts with the formula name and then a "/", " " or EOS. # If we're checking for "#{bin}", we also check for "-" since similar binaries also don't need interpolation. - def path_starts_with?(path, starts_with, bin = false) + def path_starts_with?(path, starts_with, bin: false) ending = bin ? "/| |-|$" : "/| |$" path.match?(/^#{Regexp.escape(starts_with)}(#{ending})/) end + def path_starts_with_bin?(path, starts_with) + path_starts_with?(path, starts_with, bin: true) + end + # Find "#{share}/foo" def_node_search :interpolated_share_path_starts_with, <<~EOS $(dstr (begin (send nil? :share)) (str #path_starts_with?(%1))) @@ -164,7 +168,7 @@ module RuboCop # Find "#{bin}/foo" and "#{bin}/foo-bar" def_node_search :interpolated_bin_path_starts_with, <<~EOS - $(dstr (begin (send nil? :bin)) (str #path_starts_with?(%1, %2))) + $(dstr (begin (send nil? :bin)) (str #path_starts_with_bin?(%1))) EOS # Find share/"foo" From a6596c969f86eb0f1e95b175d90092f4bca40835 Mon Sep 17 00:00:00 2001 From: Issy Long Date: Thu, 25 Jul 2024 10:41:13 +0100 Subject: [PATCH 4/4] Test the `shell_output` single string edge case --- Library/Homebrew/test/rubocops/text/strict_spec.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/Library/Homebrew/test/rubocops/text/strict_spec.rb b/Library/Homebrew/test/rubocops/text/strict_spec.rb index f802cb376d..8ccd8ef678 100644 --- a/Library/Homebrew/test/rubocops/text/strict_spec.rb +++ b/Library/Homebrew/test/rubocops/text/strict_spec.rb @@ -144,5 +144,16 @@ RSpec.describe RuboCop::Cop::FormulaAuditStrict::Text do end RUBY end + + it 'reports an offense if "\#{bin}" is in a `shell_output` string' do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + test do + shell_output("\#{bin}/foo --version") + ^^^^^^^^^^^^^^^^^^^^^^ FormulaAuditStrict/Text: Use `bin/"foo"` instead of `"\#{bin}/foo"` + end + end + RUBY + end end end