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

[Fix #7849] Add new Lint/AmbiguousOperatorPrecedence cop #10047

Merged
merged 2 commits into from Aug 29, 2021
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
1 change: 1 addition & 0 deletions changelog/new_add_new_lintambiguousoperatorprecedence.md
@@ -0,0 +1 @@
* [#7849](https://github.com/rubocop/rubocop/issues/7849): Add new `Lint/AmbiguousOperatorPrecedence` cop. ([@dvandersluis][])
7 changes: 7 additions & 0 deletions config/default.yml
Expand Up @@ -1448,6 +1448,13 @@ Lint/AmbiguousOperator:
VersionAdded: '0.17'
VersionChanged: '0.83'

Lint/AmbiguousOperatorPrecedence:
Description: >-
Checks for expressions containing multiple binary operations with
ambiguous precedence.
Enabled: pending
VersionAdded: '<<next>>'

Lint/AmbiguousRange:
Description: Checks for ranges with ambiguous boundaries.
Enabled: pending
Expand Down
1 change: 1 addition & 0 deletions lib/rubocop.rb
Expand Up @@ -263,6 +263,7 @@
require_relative 'rubocop/cop/lint/ambiguous_assignment'
require_relative 'rubocop/cop/lint/ambiguous_block_association'
require_relative 'rubocop/cop/lint/ambiguous_operator'
require_relative 'rubocop/cop/lint/ambiguous_operator_precedence'
require_relative 'rubocop/cop/lint/ambiguous_range'
require_relative 'rubocop/cop/lint/ambiguous_regexp_literal'
require_relative 'rubocop/cop/lint/assignment_in_condition'
Expand Down
4 changes: 2 additions & 2 deletions lib/rubocop/cop/base.rb
Expand Up @@ -222,8 +222,8 @@ def target_rails_version

def relevant_file?(file)
file == RuboCop::AST::ProcessedSource::STRING_SOURCE_NAME ||
file_name_matches_any?(file, 'Include', true) &&
!file_name_matches_any?(file, 'Exclude', false)
(file_name_matches_any?(file, 'Include', true) &&
!file_name_matches_any?(file, 'Exclude', false))
end

def excluded_file?(file)
Expand Down
Expand Up @@ -87,8 +87,8 @@ def lambda_arg_string
end

def needs_separating_space?
block_begin.begin_pos == arguments_end_pos &&
selector_end.end_pos == arguments_begin_pos ||
(block_begin.begin_pos == arguments_end_pos &&
selector_end.end_pos == arguments_begin_pos) ||
block_begin.begin_pos == selector_end.end_pos
end

Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/correctors/line_break_corrector.rb
Expand Up @@ -28,7 +28,7 @@ def break_line_before(range:, node:, corrector:, configured_width:,
indent_steps: 1)
corrector.insert_before(
range,
"\n#{' ' * (node.loc.keyword.column + indent_steps * configured_width)}"
"\n#{' ' * (node.loc.keyword.column + (indent_steps * configured_width))}"
)
end

Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/layout/argument_alignment.rb
Expand Up @@ -54,7 +54,7 @@ class ArgumentAlignment < Base

def on_send(node)
first_arg = node.first_argument
return if !multiple_arguments?(node, first_arg) || node.send_type? && node.method?(:[]=)
return if !multiple_arguments?(node, first_arg) || (node.send_type? && node.method?(:[]=))

if first_arg.hash_type? && !first_arg.braces?
pairs = first_arg.pairs
Expand Down
3 changes: 2 additions & 1 deletion lib/rubocop/cop/layout/class_structure.rb
Expand Up @@ -264,7 +264,8 @@ def humanize_node(node)

def source_range_with_comment(node)
begin_pos, end_pos =
if node.def_type? && !node.method?(:initialize) || node.send_type? && node.def_modifier?
if (node.def_type? && !node.method?(:initialize)) ||
(node.send_type? && node.def_modifier?)
start_node = find_visibility_start(node) || node
end_node = find_visibility_end(node) || node
[begin_pos_with_comment(start_node),
Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/layout/end_alignment.rb
Expand Up @@ -182,7 +182,7 @@ def alignment_node_for_variable_style(node)

def assignment_or_operator_method(node)
node.ancestors.find do |ancestor|
ancestor.assignment_or_similar? || ancestor.send_type? && ancestor.operator_method?
ancestor.assignment_or_similar? || (ancestor.send_type? && ancestor.operator_method?)
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/layout/leading_comment_space.rb
Expand Up @@ -77,7 +77,7 @@ def hash_mark(expr)
end

def allowed_on_first_line?(comment)
shebang?(comment) || rackup_config_file? && rackup_options?(comment)
shebang?(comment) || (rackup_config_file? && rackup_options?(comment))
end

def shebang?(comment)
Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/layout/line_length.rb
Expand Up @@ -176,7 +176,7 @@ def check_line(line, line_index)
def ignored_line?(line, line_index)
matches_ignored_pattern?(line) ||
shebang?(line, line_index) ||
heredocs && line_in_permitted_heredoc?(line_index.succ)
(heredocs && line_in_permitted_heredoc?(line_index.succ))
end

def shebang?(line, line_index)
Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/layout/multiline_block_layout.rb
Expand Up @@ -89,7 +89,7 @@ def characters_needed_for_space_and_pipes(node)
if node.source.lines.first.end_with?("|\n")
PIPE_SIZE
else
1 + PIPE_SIZE * 2
1 + (PIPE_SIZE * 2)
end
end

Expand Down
Expand Up @@ -47,7 +47,8 @@ def check_optarg(arg, equals, value)
space_on_both_sides = space_on_both_sides?(arg, equals)
no_surrounding_space = no_surrounding_space?(arg, equals)

if style == :space && space_on_both_sides || style == :no_space && no_surrounding_space
if (style == :space && space_on_both_sides) ||
(style == :no_space && no_surrounding_space)
correct_style_detected
else
incorrect_style_detected(arg, value)
Expand Down
4 changes: 2 additions & 2 deletions lib/rubocop/cop/layout/space_around_keyword.rb
Expand Up @@ -228,8 +228,8 @@ def space_after_missing?(range)
def accepted_opening_delimiter?(range, char)
return true unless char

accept_left_square_bracket?(range) && char == '[' ||
accept_left_parenthesis?(range) && char == '('
(accept_left_square_bracket?(range) && char == '[') ||
(accept_left_parenthesis?(range) && char == '(')
end

def accept_left_parenthesis?(range)
Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/layout/space_inside_reference_brackets.rb
Expand Up @@ -107,7 +107,7 @@ def left_ref_bracket(node, tokens)
current_token = tokens.reverse.find(&:left_ref_bracket?)
previous_token = previous_token(current_token)

if node.method?(:[]=) || previous_token && !previous_token.right_bracket?
if node.method?(:[]=) || (previous_token && !previous_token.right_bracket?)
tokens.find(&:left_ref_bracket?)
else
current_token
Expand Down
107 changes: 107 additions & 0 deletions lib/rubocop/cop/lint/ambiguous_operator_precedence.rb
@@ -0,0 +1,107 @@
# frozen_string_literal: true

module RuboCop
module Cop
module Lint
# This cop looks for expressions containing multiple binary operators
# where precedence is ambiguous due to lack of parentheses. For example,
# in `1 + 2 * 3`, the multiplication will happen before the addition, but
# lexically it appears that the addition will happen first.
#
# The cop does not consider unary operators (ie. `!a` or `-b`) or comparison
# operators (ie. `a =~ b`) because those are not ambiguous.
#
# NOTE: Ranges are handled by `Lint/AmbiguousRange`.
#
# @example
# # bad
# a + b * c
# a || b && c
# a ** b + c
#
# # good (different precedence)
# a + (b * c)
# a || (b && c)
# (a ** b) + c
#
# # good (same precedence)
# a + b + c
# a * b / c % d
class AmbiguousOperatorPrecedence < Base
Copy link
Member

Choose a reason for hiding this comment

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

Ambiguous looked weird because operator precedence is clear in the Ruby specification. How about name like Style/OperatorPrecedenceParentheses?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think Ambiguous makes sense because it's aligned with the other Ambiguous cops -- flagging syntax that, while clearly defined for ruby itself, is not necessarily clear for a user reading it, especially one who is not well-versed in Ruby.

However, if it's not desired here, ExplicitOperatorPrecedence could be a better name.

Copy link
Member

Choose a reason for hiding this comment

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

ExplicitOperatorPrecedence could be a better name.

I also first came up with the same name :-) However, I've withheld the name because I'm a little confused as to whether it's Explicit or Implicit. But I think it's a better name than Ambiguous.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, probably we can do a bit better here, although the point is more that the precedence might be ambiguous for the people, not for Ruby. :-) It's clear that as far as Ruby's concerned there's no ambiguity, yet still people make a ton of mistakes with logical expressions, based on wrong assumptions about the priorities there. A more neutral name can be just LogicalOperatorPrecedence with styles like "Explicit" and "Implicit" if we want to appease everyone.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would be strongly against making it configurable in a way that makes these parens an offense, and otherwise "Implicit" style would probably just be choosing to disable the cop 😅

extend AutoCorrector

# See https://ruby-doc.org/core-3.0.2/doc/syntax/precedence_rdoc.html
PRECEDENCE = [
%i[**],
%i[* / %],
%i[+ -],
%i[<< >>],
%i[&],
%i[| ^],
%i[&&],
%i[||]
].freeze
RESTRICT_ON_SEND = PRECEDENCE.flatten.freeze
MSG = 'Wrap expressions with varying precedence with parentheses to avoid ambiguity.'

def on_new_investigation
# Cache the precedence of each node being investigated
# so that we only need to calculate it once
@node_precedences = {}
super
end

def on_and(node)
return unless (parent = node.parent)

return if parent.begin_type? # if the `and` is in a `begin`, it's parenthesized already
return unless parent.or_type?

add_offense(node) do |corrector|
autocorrect(corrector, node)
end
end

def on_send(node)
return if node.parenthesized?

return unless (parent = node.parent)
return unless operator?(parent)
return unless greater_precedence?(node, parent)

add_offense(node) do |corrector|
autocorrect(corrector, node)
end
end

private

def precedence(node)
@node_precedences.fetch(node) do
PRECEDENCE.index { |operators| operators.include?(operator_name(node)) }
end
end

def operator?(node)
(node.send_type? && RESTRICT_ON_SEND.include?(node.method_name)) || node.operator_keyword?
end

def greater_precedence?(node1, node2)
precedence(node2) > precedence(node1)
end

def operator_name(node)
if node.send_type?
node.method_name
else
node.operator.to_sym
end
end

def autocorrect(corrector, node)
corrector.wrap(node, '(', ')')
end
end
end
end
end
2 changes: 1 addition & 1 deletion lib/rubocop/cop/lint/ambiguous_range.rb
Expand Up @@ -83,7 +83,7 @@ def acceptable?(node)
node.begin_type? ||
node.basic_literal? ||
node.variable? || node.const_type? ||
node.call_type? && acceptable_call?(node)
(node.call_type? && acceptable_call?(node))
end

def acceptable_call?(node)
Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/lint/empty_in_pattern.rb
Expand Up @@ -51,7 +51,7 @@ class EmptyInPattern < Base

def on_case_match(node)
node.in_pattern_branches.each do |branch|
next if branch.body || cop_config['AllowComments'] && comment_lines?(node)
next if branch.body || (cop_config['AllowComments'] && comment_lines?(node))

add_offense(branch)
end
Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/lint/erb_new_arguments.rb
Expand Up @@ -118,7 +118,7 @@ def autocorrect(corrector, node)
end

def correct_arguments?(arguments)
arguments.size == 1 || arguments.size == 2 && arguments[1].hash_type?
arguments.size == 1 || (arguments.size == 2 && arguments[1].hash_type?)
end

def build_kwargs(node)
Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/lint/float_out_of_range.rb
Expand Up @@ -24,7 +24,7 @@ class FloatOutOfRange < Base
def on_float(node)
value, = *node

return unless value.infinite? || value.zero? && /[1-9]/.match?(node.source)
return unless value.infinite? || (value.zero? && /[1-9]/.match?(node.source))

add_offense(node)
end
Expand Down
5 changes: 2 additions & 3 deletions lib/rubocop/cop/lint/unused_method_argument.rb
Expand Up @@ -87,9 +87,8 @@ def check_argument(variable)
end

def ignored_method?(body)
cop_config['IgnoreEmptyMethods'] && body.nil? ||
cop_config['IgnoreNotImplementedMethods'] &&
not_implemented?(body)
(cop_config['IgnoreEmptyMethods'] && body.nil?) ||
(cop_config['IgnoreNotImplementedMethods'] && not_implemented?(body))
end

def message(variable)
Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/lint/useless_times.rb
Expand Up @@ -65,7 +65,7 @@ def on_send(node)
private

def never_process?(count, node)
count < 1 || node.block_type? && node.body.nil?
count < 1 || (node.block_type? && node.body.nil?)
end

def remove_node(corrector, node)
Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/metrics/perceived_complexity.rb
Expand Up @@ -45,7 +45,7 @@ def complexity_score_for(node)
else
# Otherwise, the case node gets 0.8 complexity points and each
# when gets 0.2.
(0.8 + 0.2 * nb_branches).round
(0.8 + (0.2 * nb_branches)).round
end
when :if
node.else? && !node.elsif? ? 2 : 1
Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/metrics/utils/abc_size_calculator.rb
Expand Up @@ -46,7 +46,7 @@ def calculate
visit_depth_last(@node) { |child| calculate_node(child) }

[
Math.sqrt(@assignment**2 + @branch**2 + @condition**2).round(2),
Math.sqrt((@assignment**2) + (@branch**2) + (@condition**2)).round(2),
"<#{@assignment}, #{@branch}, #{@condition}>"
]
end
Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/metrics/utils/code_length_calculator.rb
Expand Up @@ -147,7 +147,7 @@ def extract_body(node)

# Returns true for lines that shall not be included in the count.
def irrelevant_line?(source_line)
source_line.blank? || !count_comments? && comment_line?(source_line)
source_line.blank? || (!count_comments? && comment_line?(source_line))
end

def count_comments?
Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/mixin/code_length.rb
Expand Up @@ -43,7 +43,7 @@ def check_code_length(node)

# Returns true for lines that shall not be included in the count.
def irrelevant_line(source_line)
source_line.blank? || !count_comments? && comment_line?(source_line)
source_line.blank? || (!count_comments? && comment_line?(source_line))
end

def build_code_length_calculator(node)
Expand Down
4 changes: 2 additions & 2 deletions lib/rubocop/cop/mixin/multiline_expression_indentation.rb
Expand Up @@ -134,7 +134,7 @@ def argument_in_method_call(node, kind) # rubocop:todo Metrics/CyclomaticComplex

next if a.setter_method?
next unless kind == :with_or_without_parentheses ||
kind == :with_parentheses && parentheses?(a)
(kind == :with_parentheses && parentheses?(a))

a.arguments.any? { |arg| within_node?(node, arg) }
end
Expand All @@ -156,7 +156,7 @@ def part_of_assignment_rhs(node, candidate)

def disqualified_rhs?(candidate, ancestor)
UNALIGNED_RHS_TYPES.include?(ancestor.type) ||
ancestor.block_type? && part_of_block_body?(candidate, ancestor)
(ancestor.block_type? && part_of_block_body?(candidate, ancestor))
end

def valid_rhs?(candidate, ancestor)
Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/naming/constant_name.rb
Expand Up @@ -54,7 +54,7 @@ def on_casgn(node)
private

def allowed_assignment?(value)
value && %i[block const casgn].include?(value.type) ||
(value && %i[block const casgn].include?(value.type)) ||
allowed_method_call_on_rhs?(value) ||
class_or_struct_return_method?(value) ||
allowed_conditional_expression_on_rhs?(value)
Expand Down
4 changes: 2 additions & 2 deletions lib/rubocop/cop/style/accessor_grouping.rb
Expand Up @@ -59,8 +59,8 @@ def on_class(node)

def check(send_node)
return if previous_line_comment?(send_node)
return unless grouped_style? && sibling_accessors(send_node).size > 1 ||
separated_style? && send_node.arguments.size > 1
return unless (grouped_style? && sibling_accessors(send_node).size > 1) ||
(separated_style? && send_node.arguments.size > 1)

message = message(send_node)
add_offense(send_node, message: message) do |corrector|
Expand Down
3 changes: 2 additions & 1 deletion lib/rubocop/cop/style/collection_methods.rb
Expand Up @@ -68,7 +68,8 @@ def implicit_block?(node)
return false unless node.arguments.any?

node.last_argument.block_pass_type? ||
node.last_argument.sym_type? && methods_accepting_symbol.include?(node.method_name.to_s)
(node.last_argument.sym_type? &&
methods_accepting_symbol.include?(node.method_name.to_s))
end

def message(node)
Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/style/document_dynamic_eval_definition.rb
Expand Up @@ -86,7 +86,7 @@ def on_send(node)

return unless arg_node&.dstr_type? && interpolated?(arg_node)
return if inline_comment_docs?(arg_node) ||
arg_node.heredoc? && comment_block_docs?(arg_node)
(arg_node.heredoc? && comment_block_docs?(arg_node))

add_offense(node.loc.selector)
end
Expand Down