Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Small Lint cops perf tweaks #8358

Merged
merged 1 commit into from Jul 18, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions lib/rubocop/cop/lint/ambiguous_block_association.rb
Expand Up @@ -30,10 +30,11 @@ class AmbiguousBlockAssociation < Cop
'call.'

def on_send(node)
return if !node.arguments? || node.parenthesized? ||
node.last_argument.lambda? || allowed_method?(node)
return unless node.arguments?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was surprised to see that this line was still taking non-negligible time, so I investigated a bit and this led to a bunch of fixes and a bunch of optimizations in rubocop-ast 🎉


return unless ambiguous_block_association?(node)
return if node.parenthesized? ||
node.last_argument.lambda? || allowed_method?(node)

add_offense(node)
end
Expand Down
4 changes: 3 additions & 1 deletion lib/rubocop/cop/lint/big_decimal_new.rb
Expand Up @@ -24,7 +24,9 @@ class BigDecimalNew < Cop
PATTERN

def on_send(node)
return unless big_decimal_new(node) do |captured_value|
return unless node.method?(:new)

big_decimal_new(node) do |captured_value|
double_colon = captured_value ? '::' : ''
message = format(MSG, double_colon: double_colon)

Expand Down
8 changes: 8 additions & 0 deletions lib/rubocop/cop/lint/debugger.rb
@@ -1,5 +1,7 @@
# frozen_string_literal: true

require 'set'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to require 'set' here, it's required by parser and (just to be explicit) by rubocop-ast too.


module RuboCop
module Cop
module Lint
Expand Down Expand Up @@ -35,6 +37,11 @@ module Lint
class Debugger < Cop
MSG = 'Remove debugger entry point `%<source>s`.'

DEBUGGER_METHODS = %i[
debugger byebug remote_byebug pry remote_pry pry_remote console rescue
save_and_open_page save_and_open_screenshot save_screenshot irb
].to_set.freeze

def_node_matcher :kernel?, <<~PATTERN
Copy link
Contributor

@marcandre marcandre Jul 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to make a release of rubocop-ast soon so we can use #global_const? :-)

Should we also add #kernel_call? == {#global_const?(:Kernel) nil?} or similar to rubocop-ast?

{
(const nil? :Kernel)
Expand All @@ -57,6 +64,7 @@ class Debugger < Cop
PATTERN

def on_send(node)
return unless DEBUGGER_METHODS.include?(node.method_name)
return unless debugger_call?(node) || binding_irb?(node)

add_offense(node)
Expand Down
4 changes: 4 additions & 0 deletions lib/rubocop/cop/lint/deprecated_class_methods.rb
Expand Up @@ -60,7 +60,11 @@ def class_nodes
replacement: :block_given?)
].freeze

DEPRECATED_METHODS = DEPRECATED_METHODS_OBJECT.map(&:deprecated_method).freeze

def on_send(node)
return unless DEPRECATED_METHODS.include?(node.method_name)

check(node) do |data|
message = format(MSG, current: deprecated_method(data),
prefer: replacement_method(data))
Expand Down
9 changes: 9 additions & 0 deletions lib/rubocop/cop/lint/duplicate_methods.rb
@@ -1,5 +1,7 @@
# frozen_string_literal: true

require 'set'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need


module RuboCop
module Cop
module Lint
Expand Down Expand Up @@ -53,6 +55,9 @@ class DuplicateMethods < Cop
MSG = 'Method `%<method>s` is defined at both %<defined>s and ' \
'%<current>s.'

METHOD_DEF_METHODS = %i[alias_method attr_reader attr_writer
attr_accessor attr].to_set.freeze

def initialize(config = nil, options = nil)
super
@definitions = {}
Expand Down Expand Up @@ -97,7 +102,10 @@ def on_alias(node)

def_node_matcher :sym_name, '(sym $_name)'

# rubocop:disable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
def on_send(node)
return unless METHOD_DEF_METHODS.include?(node.method_name)

if (name = alias_method?(node))
return unless name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is redundant. Apparently we don't have a cop for this 😅

return if node.ancestors.any?(&:if_type?)
Expand All @@ -108,6 +116,7 @@ def on_send(node)
on_attr(node, *attr)
end
end
# rubocop:enable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity

private

Expand Down
3 changes: 2 additions & 1 deletion lib/rubocop/cop/lint/format_parameter_mismatch.rb
Expand Up @@ -44,9 +44,10 @@ class FormatParameterMismatch < Cop
KERNEL = 'Kernel'
SHOVEL = '<<'
STRING_TYPES = %i[str dstr].freeze
FORMAT_METHODS = %i[format sprintf %].freeze

def on_send(node)
return unless format_string?(node)
return unless FORMAT_METHODS.include?(node.method_name) && format_string?(node)

if invalid_format_string?(node)
add_offense(node, location: :selector, message: MSG_INVALID)
Expand Down
12 changes: 8 additions & 4 deletions lib/rubocop/cop/lint/ineffective_access_modifier.rb
Expand Up @@ -70,9 +70,7 @@ def on_module(node)
def check_node(node)
return unless node&.begin_type?

ignored_methods = private_class_method_names(node)

ineffective_modifier(node, ignored_methods) do |defs_node, modifier|
ineffective_modifier(node) do |defs_node, modifier|
add_offense(defs_node,
location: :keyword,
message: format_message(modifier))
Expand All @@ -97,20 +95,26 @@ def format_message(modifier)
alternative: alternative)
end

def ineffective_modifier(node, ignored_methods, modifier = nil, &block)
# rubocop:disable Metrics/CyclomaticComplexity
def ineffective_modifier(node, modifier = nil, &block)
ignored_methods = nil

node.each_child_node do |child|
case child.type
when :send
modifier = child if access_modifier?(child)
when :defs
ignored_methods ||= private_class_method_names(node)
next if correct_visibility?(child, modifier, ignored_methods)

yield child, modifier
when :kwbegin
ignored_methods ||= private_class_method_names(node)
ineffective_modifier(child, ignored_methods, modifier, &block)
end
end
end
# rubocop:enable Metrics/CyclomaticComplexity

def access_modifier?(node)
node.bare_access_modifier? && !node.method?(:module_function)
Expand Down
2 changes: 2 additions & 0 deletions lib/rubocop/cop/lint/inherit_exception.rb
Expand Up @@ -69,6 +69,8 @@ def on_class(node)
end

def on_send(node)
return unless node.method?(:new)

constant = class_new_call?(node)
return unless constant && illegal_class_name?(constant)

Expand Down
3 changes: 1 addition & 2 deletions lib/rubocop/cop/lint/interpolation_check.rb
Expand Up @@ -21,11 +21,10 @@ class InterpolationCheck < Cop
'Use double quoted strings if you need interpolation.'

def on_str(node)
return if heredoc?(node)

parent = node.parent
return if parent && (parent.dstr_type? || parent.regexp_type?)
return unless /(?<!\\)#\{.*\}/.match?(node.source.scrub)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, parser only handles valid utf-8 sequences, so scrub should not be needed at all here, or am I missing something?

return if heredoc?(node)

add_offense(node)
end
Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/lint/rand_one.rb
Expand Up @@ -29,7 +29,7 @@ class RandOne < Cop
PATTERN

def on_send(node)
return unless rand_one?(node)
return unless node.method?(:rand) && rand_one?(node)

add_offense(node)
end
Expand Down
2 changes: 2 additions & 0 deletions lib/rubocop/cop/lint/uri_regexp.rb
Expand Up @@ -30,6 +30,8 @@ class UriRegexp < Cop
PATTERN

def on_send(node)
return unless node.method?(:regexp)

uri_regexp_with_argument?(node) do |double_colon, arg|
register_offense(
node, top_level: double_colon ? '::' : '', arg: "(#{arg.source})"
Expand Down