From 5b97a8f2e163f09d2ba7d7ef6424b3394cc71d44 Mon Sep 17 00:00:00 2001 From: Gautham Goli Date: Wed, 24 May 2017 00:07:06 +0530 Subject: Add methods in FormulaCop to find method nodes, dependency nodes --- Library/Homebrew/rubocops/extend/formula_cop.rb | 94 ++++++++++++++++++++++--- 1 file changed, 86 insertions(+), 8 deletions(-) (limited to 'Library') diff --git a/Library/Homebrew/rubocops/extend/formula_cop.rb b/Library/Homebrew/rubocops/extend/formula_cop.rb index 3f70086b3..6215479b9 100644 --- a/Library/Homebrew/rubocops/extend/formula_cop.rb +++ b/Library/Homebrew/rubocops/extend/formula_cop.rb @@ -7,9 +7,9 @@ module RuboCop def on_class(node) file_path = processed_source.buffer.name return unless file_path_allowed?(file_path) - class_node, parent_class_node, body = *node - return unless formula_class?(parent_class_node) + return unless formula_class?(node) return unless respond_to?(:audit_formula) + class_node, parent_class_node, body = *node @formula_name = class_name(class_node) audit_formula(node, class_node, parent_class_node, body) end @@ -46,12 +46,61 @@ module RuboCop nil end - # Returns an array of method call nodes matching method_name inside node + # Set the given node as the offending node when required in custom cops + def offending_node(node) + @offensive_node = node + @offense_source_range = node.source_range + end + + # Returns an array of method call nodes matching method_name inside node with depth first order (Children nodes) def find_method_calls_by_name(node, method_name) return if node.nil? node.each_child_node(:send).select { |method_node| method_name == method_node.method_name } end + # Returns an array of method call nodes matching method_name in every descendant of node + def find_every_method_call_by_name(node, method_name) + return if node.nil? + node.each_descendant(:send).select { |method_node| method_name == method_node.method_name } + end + + # Returns nil if does not depend on dependency_name + # args: node - formula class' body node + # dependency_name - dependency's name + def depends_on?(node, dependency_name) + dependency_nodes = find_every_method_call_by_name(node, :depends_on) + idx = dependency_nodes.index do |n| + depends_on_name_type?(n, dependency_name, :required) || + depends_on_name_type?(n, dependency_name, :build) || + depends_on_name_type?(n, dependency_name, :optional) || + depends_on_name_type?(n, dependency_name, :recommended) || + depends_on_name_type?(n, dependency_name, :run) + end + return nil if idx.nil? + @offense_source_range = dependency_nodes[idx].source_range + @offensive_node = dependency_nodes[idx] + end + + # Returns true if given dependency name and dependency type exist in given dependency method call node + def depends_on_name_type?(node, dependency_name = nil, dependency_type = :required) + dependency_name_match = true if dependency_name.nil? # Match only by type + case dependency_type + when :required + dependency_type_match = !node.method_args.nil? && node.method_args.first.str_type? + dependency_name_match = (string_content(node.method_args.first) == dependency_name) if dependency_type_match + when :build || :optional || :recommended || :run + dependency_type_match = !node.method_args.nil? && + node.method_args.first.hash_type? && + node.method_args.first.values[0].children.first == dependency_type + dependency_name_match = (node.method_args.first.keys[0].children.first == dependency_name) if dependency_type_match + end + if dependency_type_match || dependency_name_match + @offensive_node = node + @offense_source_range = node.source_range + end + dependency_type_match && dependency_name_match + end + # Returns a block named block_name inside node def find_block(node, block_name) return if node.nil? @@ -112,6 +161,17 @@ module RuboCop false end + # Check if method_name is called among every descendant node of given node + def method_called_ever?(node, method_name) + node.each_descendant(:send) do |call_node| + next unless call_node.method_name == method_name + @offensive_node = call_node + @offense_source_range = call_node.source_range + return true + end + false + end + # Checks for precedence, returns the first pair of precedence violating nodes def check_precedence(first_nodes, next_nodes) next_nodes.each do |each_next_node| @@ -138,6 +198,17 @@ module RuboCop method_node.method_args end + # Returns true if the given parameters are present in method call + # and sets the method call as the offending node + def parameters_passed?(method_node, *params) + method_params = parameters(method_node) + @offensive_node = method_node + @offense_source_range = method_node.source_range + params.all? do |given_param| + method_params.any? { |method_param| given_param == string_content(method_param) } + end + end + # Returns the begin position of the node's line in source code def line_start_column(node) node.source_range.source_buffer.line_range(node.loc.line).begin_pos @@ -180,10 +251,16 @@ module RuboCop node.source_range.source_buffer end - # Returns the string representation if node is of type str(plain) or dstr(interpolated) + # Returns the string representation if node is of type str(plain) or dstr(interpolated) or const def string_content(node) - return node.str_content if node.type == :str - node.each_child_node(:str).map(&:str_content).join("") if node.type == :dstr + case node.type + when :str + return node.str_content if node.type == :str + when :dstr + return node.each_child_node(:str).map(&:str_content).join("") if node.type == :dstr + when :const + return node.const_name if node.type == :const + end end # Returns printable component name @@ -198,8 +275,9 @@ module RuboCop private - def formula_class?(parent_class_node) - parent_class_node && parent_class_node.const_name == "Formula" + def formula_class?(node) + _, class_node, = *node + class_node && string_content(class_node) == "Formula" end def file_path_allowed?(file_path) -- cgit v1.2.3 From 51f2338dd57eed5a7f18e147ccd4f4b9da19fb52 Mon Sep 17 00:00:00 2001 From: Gautham Goli Date: Wed, 24 May 2017 00:08:31 +0530 Subject: audit: Port audit_text method to rubocop and add tests --- Library/.rubocop.yml | 3 + Library/Homebrew/dev-cmd/audit.rb | 36 ---- Library/Homebrew/rubocops.rb | 1 + Library/Homebrew/rubocops/text_cop.rb | 73 +++++++ Library/Homebrew/test/dev-cmd/audit_spec.rb | 51 ----- Library/Homebrew/test/rubocops/text_cop_spec.rb | 261 ++++++++++++++++++++++++ 6 files changed, 338 insertions(+), 87 deletions(-) create mode 100644 Library/Homebrew/rubocops/text_cop.rb create mode 100644 Library/Homebrew/test/rubocops/text_cop_spec.rb (limited to 'Library') diff --git a/Library/.rubocop.yml b/Library/.rubocop.yml index 66e71027e..8fd64cb1a 100644 --- a/Library/.rubocop.yml +++ b/Library/.rubocop.yml @@ -8,6 +8,9 @@ AllCops: require: ./Homebrew/rubocops.rb +FormulaAudit/Text: + Enabled: true + FormulaAuditStrict/BottleBlock: Enabled: true diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index d1665ea6f..4ceff0f3d 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -850,14 +850,6 @@ class FormulaAuditor end def audit_text - if text =~ /system\s+['"]scons/ - problem "use \"scons *args\" instead of \"system 'scons', *args\"" - end - - if text =~ /system\s+['"]xcodebuild/ - problem %q(use "xcodebuild *args" instead of "system 'xcodebuild', *args") - end - bin_names = Set.new bin_names << formula.name bin_names += formula.aliases @@ -872,34 +864,6 @@ class FormulaAuditor end end end - - if text =~ /xcodebuild[ (]*["'*]*/ && !text.include?("SYMROOT=") - problem 'xcodebuild should be passed an explicit "SYMROOT"' - end - - if text.include? "Formula.factory(" - problem "\"Formula.factory(name)\" is deprecated in favor of \"Formula[name]\"" - end - - if text.include?("def plist") && !text.include?("plist_options") - problem "Please set plist_options when using a formula-defined plist." - end - - if text =~ /depends_on\s+['"]openssl['"]/ && text =~ /depends_on\s+['"]libressl['"]/ - problem "Formulae should not depend on both OpenSSL and LibreSSL (even optionally)." - end - - if text =~ /virtualenv_(create|install_with_resources)/ && - text =~ /resource\s+['"]setuptools['"]\s+do/ - problem "Formulae using virtualenvs do not need a `setuptools` resource." - end - - if text =~ /system\s+['"]go['"],\s+['"]get['"]/ - problem "Formulae should not use `go get`. If non-vendored resources are required use `go_resource`s." - end - - return unless text.include?('require "language/go"') && !text.include?("go_resource") - problem "require \"language/go\" is unnecessary unless using `go_resource`s" end def audit_lines diff --git a/Library/Homebrew/rubocops.rb b/Library/Homebrew/rubocops.rb index 587bf4b20..8627d2c04 100644 --- a/Library/Homebrew/rubocops.rb +++ b/Library/Homebrew/rubocops.rb @@ -3,3 +3,4 @@ require_relative "./rubocops/formula_desc_cop" require_relative "./rubocops/components_order_cop" require_relative "./rubocops/components_redundancy_cop" require_relative "./rubocops/homepage_cop" +require_relative "./rubocops/text_cop" diff --git a/Library/Homebrew/rubocops/text_cop.rb b/Library/Homebrew/rubocops/text_cop.rb new file mode 100644 index 000000000..f657dde66 --- /dev/null +++ b/Library/Homebrew/rubocops/text_cop.rb @@ -0,0 +1,73 @@ +require_relative "./extend/formula_cop" + +module RuboCop + module Cop + module FormulaAudit + class Text < FormulaCop + def audit_formula(_node, _class_node, _parent_class_node, body_node) + if !find_node_method_by_name(body_node, :plist_options) && + find_method_def(body_node, :plist) + problem "Please set plist_options when using a formula-defined plist." + end + + if depends_on?(body_node, "openssl") && depends_on?(body_node, "libressl") + problem "Formulae should not depend on both OpenSSL and LibreSSL (even optionally)." + end + + if method_called_ever?(body_node, :virtualenv_create) || + method_called_ever?(body_node, :virtualenv_install_with_resources) + resource_calls = find_every_method_call_by_name(body_node, :resource) + resource_calls.each do |m| + if parameters_passed?(m, "setuptools") + problem "Formulae using virtualenvs do not need a `setuptools` resource." + end + end + end + + unless method_called_ever?(body_node, :go_resource) + # processed_source.ast is passed instead of body_node because `require` would be outside body_node + require_calls = find_every_method_call_by_name(processed_source.ast, :require) + require_calls.each do |m| + if parameters_passed?(m, "language/go") + problem "require \"language/go\" is unnecessary unless using `go_resource`s" + end + end + end + + factory_calls = find_every_method_call_by_name(body_node, :factory) + unless factory_calls.nil? + factory_calls.each do |m| + if !m.children.empty? && m.children[0] && string_content(m.children[0]) == "Formula" + offending_node(m) + problem "\"Formula.factory(name)\" is deprecated in favor of \"Formula[name]\"" + end + end + end + + xcodebuild_calls = find_every_method_call_by_name(body_node, :xcodebuild) + unless xcodebuild_calls.nil? + xcodebuild_calls.each do |m| + params = parameters(m).map { |param| string_content(param) } + if params.none? { |param| param.include?("SYMROOT=") } || params.empty? + offending_node(m) + problem 'xcodebuild should be passed an explicit "SYMROOT"' + end + end + end + + system_calls = find_every_method_call_by_name(body_node, :system) + return if system_calls.nil? + system_calls.each do |m| + if parameters_passed?(m, "go", "get") + problem "Formulae should not use `go get`. If non-vendored resources are required use `go_resource`s." + elsif parameters_passed?(m, "xcodebuild") + problem %q(use "xcodebuild *args" instead of "system 'xcodebuild', *args") + elsif parameters_passed?(m, "scons") + problem "use \"scons *args\" instead of \"system 'scons', *args\"" + end + end + end + end + end + end +end diff --git a/Library/Homebrew/test/dev-cmd/audit_spec.rb b/Library/Homebrew/test/dev-cmd/audit_spec.rb index 97cc0f152..b90a21b55 100644 --- a/Library/Homebrew/test/dev-cmd/audit_spec.rb +++ b/Library/Homebrew/test/dev-cmd/audit_spec.rb @@ -385,57 +385,6 @@ describe FormulaAuditor do end end - describe "#audit_text" do - specify "xcodebuild suggests symroot" do - fa = formula_auditor "foo", <<-EOS.undent - class Foo < Formula - url "http://example.com/foo-1.0.tgz" - homepage "http://example.com" - - def install - xcodebuild "-project", "meow.xcodeproject" - end - end - EOS - - fa.audit_text - expect(fa.problems.first) - .to match('xcodebuild should be passed an explicit "SYMROOT"') - end - - specify "bare xcodebuild also suggests symroot" do - fa = formula_auditor "foo", <<-EOS.undent - class Foo < Formula - url "http://example.com/foo-1.0.tgz" - homepage "http://example.com" - - def install - xcodebuild - end - end - EOS - - fa.audit_text - expect(fa.problems.first) - .to match('xcodebuild should be passed an explicit "SYMROOT"') - end - - specify "disallow go get usage" do - fa = formula_auditor "foo", <<-EOS.undent - class Foo + + + + Label + org.nrpe.agent + + + \EOS + end + end + EOS + + expected_offenses = [{ message: "Please set plist_options when using a formula-defined plist.", + severity: :convention, + line: 9, + column: 2, + source: source }] + + inspect_source(cop, source) + + expected_offenses.zip(cop.offenses).each do |expected, actual| + expect_offense(expected, actual) + end + end + + it "When language/go is require'd" do + source = <<-EOS.undent + require "language/go" + + class Foo < Formula + url "http://example.com/foo-1.0.tgz" + homepage "http://example.com" + + def install + system "go", "get", "bar" + end + end + EOS + + expected_offenses = [{ message: "require \"language/go\" is unnecessary unless using `go_resource`s", + severity: :convention, + line: 1, + column: 0, + source: source }] + + inspect_source(cop, source) + + expected_offenses.zip(cop.offenses).each do |expected, actual| + expect_offense(expected, actual) + end + end + + it "When formula uses virtualenv and also `setuptools` resource" do + source = <<-EOS.undent + class Foo < Formula + url "http://example.com/foo-1.0.tgz" + homepage "http://example.com" + + resource "setuptools" do + url "https://foo.com/foo.tar.gz" + sha256 "db0904a28253cfe53e7dedc765c71596f3c53bb8a866ae50123320ec1a7b73fd" + end + + def install + virtualenv_create(libexec) + end + end + EOS + + expected_offenses = [{ message: "Formulae using virtualenvs do not need a `setuptools` resource.", + severity: :convention, + line: 5, + column: 2, + source: source }] + + inspect_source(cop, source) + + expected_offenses.zip(cop.offenses).each do |expected, actual| + expect_offense(expected, actual) + end + end + + it "When Formula.factory(name) is used" do + source = <<-EOS.undent + class Foo < Formula + url "http://example.com/foo-1.0.tgz" + homepage "http://example.com" + + def install + Formula.factory(name) + end + end + EOS + + expected_offenses = [{ message: "\"Formula.factory(name)\" is deprecated in favor of \"Formula[name]\"", + severity: :convention, + line: 6, + column: 4, + source: source }] + + inspect_source(cop, source) + + expected_offenses.zip(cop.offenses).each do |expected, actual| + expect_offense(expected, actual) + end + end + + def expect_offense(expected, actual) + expect(actual.message).to eq(expected[:message]) + expect(actual.severity).to eq(expected[:severity]) + expect(actual.line).to eq(expected[:line]) + expect(actual.column).to eq(expected[:column]) + end + end +end -- cgit v1.2.3 From 88f21b84f46fe94a42544255bc4ffe633d67fbe7 Mon Sep 17 00:00:00 2001 From: Gautham Goli Date: Tue, 30 May 2017 01:22:47 +0530 Subject: Refactor and add more methods in formula_cop.rb --- Library/Homebrew/rubocops/extend/formula_cop.rb | 88 +++++++++++++++++++------ 1 file changed, 68 insertions(+), 20 deletions(-) (limited to 'Library') diff --git a/Library/Homebrew/rubocops/extend/formula_cop.rb b/Library/Homebrew/rubocops/extend/formula_cop.rb index 6215479b9..d9940a037 100644 --- a/Library/Homebrew/rubocops/extend/formula_cop.rb +++ b/Library/Homebrew/rubocops/extend/formula_cop.rb @@ -1,3 +1,5 @@ +require "parser/current" + module RuboCop module Cop class FormulaCop < Cop @@ -9,9 +11,9 @@ module RuboCop return unless file_path_allowed?(file_path) return unless formula_class?(node) return unless respond_to?(:audit_formula) - class_node, parent_class_node, body = *node + class_node, parent_class_node, @body = *node @formula_name = class_name(class_node) - audit_formula(node, class_node, parent_class_node, body) + audit_formula(node, class_node, parent_class_node, @body) end # Checks for regex match of pattern in the node and @@ -64,11 +66,34 @@ module RuboCop node.each_descendant(:send).select { |method_node| method_name == method_node.method_name } end + # Given a method_name and arguments, yields to a block with + # matching method passed as a parameter to the block + def find_method_with_args(node, method_name, *args) + methods = find_every_method_call_by_name(node, method_name) + methods.each do |method| + next unless parameters_passed?(method, *args) + yield method + end + end + + # Matches a method with a receiver, + # EX: to match `Formula.factory(name)` + # call `find_instance_method_call(node, "Formula", :factory)` + # yields to a block with matching method node + def find_instance_method_call(node, instance, method_name) + methods = find_every_method_call_by_name(node, method_name) + methods.each do |method| + next unless method.receiver.const_name == instance + @offense_source_range = method.source_range + @offensive_node = method + yield method + end + end + # Returns nil if does not depend on dependency_name - # args: node - formula class' body node - # dependency_name - dependency's name - def depends_on?(node, dependency_name) - dependency_nodes = find_every_method_call_by_name(node, :depends_on) + # args: node - dependency_name - dependency's name + def depends_on?(dependency_name) + dependency_nodes = find_every_method_call_by_name(@body, :depends_on) idx = dependency_nodes.index do |n| depends_on_name_type?(n, dependency_name, :required) || depends_on_name_type?(n, dependency_name, :build) || @@ -76,29 +101,45 @@ module RuboCop depends_on_name_type?(n, dependency_name, :recommended) || depends_on_name_type?(n, dependency_name, :run) end - return nil if idx.nil? + return if idx.nil? @offense_source_range = dependency_nodes[idx].source_range @offensive_node = dependency_nodes[idx] end # Returns true if given dependency name and dependency type exist in given dependency method call node - def depends_on_name_type?(node, dependency_name = nil, dependency_type = :required) - dependency_name_match = true if dependency_name.nil? # Match only by type - case dependency_type + # TODO: Add case where key of hash is an array + def depends_on_name_type?(node, name = nil, type = :required) + if name + name_match = false + else + name_match = true # Match only by type when name is nil + end + + case type when :required - dependency_type_match = !node.method_args.nil? && node.method_args.first.str_type? - dependency_name_match = (string_content(node.method_args.first) == dependency_name) if dependency_type_match - when :build || :optional || :recommended || :run - dependency_type_match = !node.method_args.nil? && - node.method_args.first.hash_type? && - node.method_args.first.values[0].children.first == dependency_type - dependency_name_match = (node.method_args.first.keys[0].children.first == dependency_name) if dependency_type_match + type_match = !node.method_args.nil? && node.method_args.first.str_type? + if type_match && !name_match + name_match = node_equals?(node.method_args.first, name) + end + when :build, :optional, :recommended, :run + type_match = !node.method_args.nil? && + node.method_args.first.hash_type? && + node.method_args.first.values.first.children.first == type + if type_match && !name_match + name_match = node_equals?(node.method_args.first.keys.first.children.first, name) + end end - if dependency_type_match || dependency_name_match + + if type_match || name_match @offensive_node = node @offense_source_range = node.source_range end - dependency_type_match && dependency_name_match + type_match && name_match + end + + # To compare node with appropriate Ruby variable + def node_equals?(node, var) + node == Parser::CurrentRuby.parse(var.inspect) end # Returns a block named block_name inside node @@ -200,12 +241,19 @@ module RuboCop # Returns true if the given parameters are present in method call # and sets the method call as the offending node + # params can be string, symbol, array, hash, matching regex def parameters_passed?(method_node, *params) method_params = parameters(method_node) @offensive_node = method_node @offense_source_range = method_node.source_range params.all? do |given_param| - method_params.any? { |method_param| given_param == string_content(method_param) } + method_params.any? do |method_param| + if given_param.class == Regexp + regex_match_group(method_param, given_param) + else + node_equals?(method_param, given_param) + end + end end end -- cgit v1.2.3 From cfbdc17cb7a86bca8efcea082f8cabb459b9b260 Mon Sep 17 00:00:00 2001 From: Gautham Goli Date: Tue, 30 May 2017 01:24:17 +0530 Subject: Use relevant methods to consolidate logic in text_cop.rb --- Library/Homebrew/rubocops/text_cop.rb | 58 ++++++++++++----------------------- 1 file changed, 20 insertions(+), 38 deletions(-) (limited to 'Library') diff --git a/Library/Homebrew/rubocops/text_cop.rb b/Library/Homebrew/rubocops/text_cop.rb index f657dde66..d56c9bf46 100644 --- a/Library/Homebrew/rubocops/text_cop.rb +++ b/Library/Homebrew/rubocops/text_cop.rb @@ -10,61 +10,43 @@ module RuboCop problem "Please set plist_options when using a formula-defined plist." end - if depends_on?(body_node, "openssl") && depends_on?(body_node, "libressl") + if depends_on?("openssl") && depends_on?("libressl") problem "Formulae should not depend on both OpenSSL and LibreSSL (even optionally)." end if method_called_ever?(body_node, :virtualenv_create) || method_called_ever?(body_node, :virtualenv_install_with_resources) - resource_calls = find_every_method_call_by_name(body_node, :resource) - resource_calls.each do |m| - if parameters_passed?(m, "setuptools") - problem "Formulae using virtualenvs do not need a `setuptools` resource." - end + find_method_with_args(body_node, :resource, "setuptools") do + problem "Formulae using virtualenvs do not need a `setuptools` resource." end end unless method_called_ever?(body_node, :go_resource) # processed_source.ast is passed instead of body_node because `require` would be outside body_node - require_calls = find_every_method_call_by_name(processed_source.ast, :require) - require_calls.each do |m| - if parameters_passed?(m, "language/go") - problem "require \"language/go\" is unnecessary unless using `go_resource`s" - end + find_method_with_args(processed_source.ast, :require, "language/go") do + problem "require \"language/go\" is unnecessary unless using `go_resource`s" end end - factory_calls = find_every_method_call_by_name(body_node, :factory) - unless factory_calls.nil? - factory_calls.each do |m| - if !m.children.empty? && m.children[0] && string_content(m.children[0]) == "Formula" - offending_node(m) - problem "\"Formula.factory(name)\" is deprecated in favor of \"Formula[name]\"" - end - end + find_instance_method_call(body_node, "Formula", :factory) do + problem "\"Formula.factory(name)\" is deprecated in favor of \"Formula[name]\"" end - xcodebuild_calls = find_every_method_call_by_name(body_node, :xcodebuild) - unless xcodebuild_calls.nil? - xcodebuild_calls.each do |m| - params = parameters(m).map { |param| string_content(param) } - if params.none? { |param| param.include?("SYMROOT=") } || params.empty? - offending_node(m) - problem 'xcodebuild should be passed an explicit "SYMROOT"' - end - end + find_every_method_call_by_name(body_node, :xcodebuild).each do |m| + next if parameters_passed?(m, /SYMROOT=/) + problem 'xcodebuild should be passed an explicit "SYMROOT"' end - system_calls = find_every_method_call_by_name(body_node, :system) - return if system_calls.nil? - system_calls.each do |m| - if parameters_passed?(m, "go", "get") - problem "Formulae should not use `go get`. If non-vendored resources are required use `go_resource`s." - elsif parameters_passed?(m, "xcodebuild") - problem %q(use "xcodebuild *args" instead of "system 'xcodebuild', *args") - elsif parameters_passed?(m, "scons") - problem "use \"scons *args\" instead of \"system 'scons', *args\"" - end + find_method_with_args(body_node, :system, "xcodebuild") do + problem %q(use "xcodebuild *args" instead of "system 'xcodebuild', *args") + end + + find_method_with_args(body_node, :system, "scons") do + problem "use \"scons *args\" instead of \"system 'scons', *args\"" + end + + find_method_with_args(body_node, :system, "go", "get") do + problem "Formulae should not use `go get`. If non-vendored resources are required use `go_resource`s." end end end -- cgit v1.2.3