From ea559dd1c8a3a159cf23aa45b80f2184ba6693ba Mon Sep 17 00:00:00 2001 From: Ryan Rosenblum Date: Thu, 9 Aug 2018 11:24:22 -0400 Subject: [PATCH 1/4] Add an expect_correction helper to allow testing a correction after testing an offense --- lib/rubocop/rspec/cop_helper.rb | 2 + lib/rubocop/rspec/expect_offense.rb | 45 ++++++++++++++++++- .../node_type_predicate_spec.rb | 12 +++-- 3 files changed, 50 insertions(+), 9 deletions(-) diff --git a/lib/rubocop/rspec/cop_helper.rb b/lib/rubocop/rspec/cop_helper.rb index a69bee5f179..4656f34bc2a 100644 --- a/lib/rubocop/rspec/cop_helper.rb +++ b/lib/rubocop/rspec/cop_helper.rb @@ -38,6 +38,8 @@ def autocorrect_source_file(source) end def autocorrect_source(source, file = nil) + RuboCop::Formatter::DisabledConfigFormatter.config_to_allow_offenses = {} + RuboCop::Formatter::DisabledConfigFormatter.detected_styles = {} cop.instance_variable_get(:@options)[:auto_correct] = true processed_source = parse_source(source, file) _investigate(cop, processed_source) diff --git a/lib/rubocop/rspec/expect_offense.rb b/lib/rubocop/rspec/expect_offense.rb index 00864cac351..1b55f29db87 100644 --- a/lib/rubocop/rspec/expect_offense.rb +++ b/lib/rubocop/rspec/expect_offense.rb @@ -38,24 +38,65 @@ module RSpec # 'Avoid chaining a method call on a do...end block.' # ) # + # Auto-correction can be tested using `expect_correction` after + # `expect_offense`. + # + # @example `expect_offense` and `expect_correction` + # + # expect_offense(<<-RUBY.strip_indent) + # x % 2 == 0 + # ^^^^^^^^^^ Replace with `Integer#even?`. + # RUBY + # + # expect_correction(<<-RUBY.strip_indent) + # x.even? + # RUBY + # # If you do not want to specify an offense then use the # companion method `expect_no_offenses`. This method is a much # simpler assertion since it just inspects the source and checks # that there were no offenses. The `expect_offense` method has # to do more work by parsing out lines that contain carets. module ExpectOffense + # rubocop:disable Metrics/AbcSize, Metrics/MethodLength def expect_offense(source, file = nil) + RuboCop::Formatter::DisabledConfigFormatter + .config_to_allow_offenses = {} + RuboCop::Formatter::DisabledConfigFormatter.detected_styles = {} + cop.instance_variable_get(:@options)[:auto_correct] = true + expected_annotations = AnnotatedSource.parse(source) if expected_annotations.plain_source == source - raise 'Use expect_no_offenses to assert that no offenses are found' + raise 'Use `expect_no_offenses` to assert that no offenses are found' + end + + @processed_source = parse_source(expected_annotations.plain_source, + file) + + unless @processed_source.valid_syntax? + raise 'Error parsing example code' end - inspect_source(expected_annotations.plain_source, file) + _investigate(cop, @processed_source) actual_annotations = expected_annotations.with_offense_annotations(cop.offenses) + expect(actual_annotations.to_s).to eq(expected_annotations.to_s) end + # rubocop:enable Metrics/AbcSize, Metrics/MethodLength + + def expect_correction(correction) + unless @processed_source + raise '`expect_correction` must follow `expect_offense`' + end + + corrector = + RuboCop::Cop::Corrector.new(@processed_source.buffer, cop.corrections) + new_source = corrector.rewrite + + expect(new_source).to eq(correction) + end def expect_no_offenses(source, file = nil) inspect_source(source, file) diff --git a/spec/rubocop/cop/internal_affairs/node_type_predicate_spec.rb b/spec/rubocop/cop/internal_affairs/node_type_predicate_spec.rb index 3c12eff7bca..1d0eb3ee6a3 100644 --- a/spec/rubocop/cop/internal_affairs/node_type_predicate_spec.rb +++ b/spec/rubocop/cop/internal_affairs/node_type_predicate_spec.rb @@ -4,17 +4,15 @@ subject(:cop) { described_class.new } context 'comparison node type check' do - it 'registers an offense' do - expect_offense(<<-RUBY.strip_indent, 'example_cop.rb') + it 'registers an offense and auto-corrects' do + expect_offense(<<-RUBY.strip_indent) node.type == :send ^^^^^^^^^^^^^^^^^^ Use `#send_type?` to check node type. RUBY - end - - it 'auto-corrects' do - new_source = autocorrect_source('node.type == :send') - expect(new_source).to eq('node.send_type?') + expect_correction(<<-RUBY.strip_indent) + node.send_type? + RUBY end end From de98699f093701579f37c7a211bf39110ba1753a Mon Sep 17 00:00:00 2001 From: Ryan Rosenblum Date: Tue, 30 Oct 2018 11:30:07 -0400 Subject: [PATCH 2/4] Fix exception in RelativeDateConstant when auto-correcting anything other than direct constant assignment --- CHANGELOG.md | 1 + .../cop/rails/relative_date_constant.rb | 76 ++++---- .../cop/rails/relative_date_constant_spec.rb | 168 ++++++++++-------- 3 files changed, 138 insertions(+), 107 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1eeb31baea5..92363cc28a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ * [#6625](https://github.com/rubocop-hq/rubocop/issues/6625): Revert the "auto-exclusion of files ignored by git" feature. ([@bbatsov][]) * [#4460](https://github.com/rubocop-hq/rubocop/issues/4460): Fix the determination of unsafe auto-correct in `Style/TernaryParentheses`. ([@jonas054][]) * [#6651](https://github.com/rubocop-hq/rubocop/issues/6651): Fix auto-correct issue in `Style/RegexpLiteral` cop when there is string interpolation. ([@roooodcastro][]) +* Do not attempt to auto-correct mass assignment or optional assignment in `Rails/RelativeDateConstant`. ([@rrosenblum][]) ### Changes diff --git a/lib/rubocop/cop/rails/relative_date_constant.rb b/lib/rubocop/cop/rails/relative_date_constant.rb index 5f9ea193da4..6ed22aa24af 100644 --- a/lib/rubocop/cop/rails/relative_date_constant.rb +++ b/lib/rubocop/cop/rails/relative_date_constant.rb @@ -19,18 +19,15 @@ module Rails # end # end class RelativeDateConstant < Cop + include RangeHelp + MSG = 'Do not assign %s to constants as it ' \ 'will be evaluated only once.'.freeze - RELATIVE_DATE_METHODS = %i[ago from_now since until].freeze - def on_casgn(node) - _scope, _constant, rhs = *node - - # rhs would be nil in a or_asgn node - return unless rhs - - check_node(rhs) + relative_date_assignment?(node) do |method_name| + add_offense(node, message: format(MSG, method_name: method_name)) + end end def on_masgn(node) @@ -39,20 +36,29 @@ def on_masgn(node) return unless rhs && rhs.array_type? lhs.children.zip(rhs.children).each do |(name, value)| - check_node(value) if name.casgn_type? + next unless name.casgn_type? + + relative_date?(value) do |method_name| + add_offense(node, + location: range_between(name.loc.expression.begin_pos, + value.loc.expression.end_pos), + message: format(MSG, method_name: method_name)) + end end end def on_or_asgn(node) - lhs, rhs = *node - - return unless lhs.casgn_type? - - check_node(rhs) + relative_date_or_assignment?(node) do |method_name| + add_offense(node, message: format(MSG, method_name: method_name)) + end end def autocorrect(node) - _scope, const_name, value = *node + return unless node.casgn_type? + + scope, const_name, value = *node + return unless scope.nil? + indent = ' ' * node.loc.column new_code = ["def self.#{const_name.downcase}", "#{indent}#{value.source}", @@ -62,27 +68,25 @@ def autocorrect(node) private - def check_node(node) - return unless node.irange_type? || - node.erange_type? || - node.send_type? - - # for range nodes we need to check both their boundaries - nodes = node.send_type? ? [node] : node.children - - nodes.each do |n| - if relative_date_method?(n) - add_offense(node.parent, - message: format(MSG, method_name: n.method_name)) - end - end - end - - def relative_date_method?(node) - node.send_type? && - RELATIVE_DATE_METHODS.include?(node.method_name) && - !node.arguments? - end + def_node_matcher :relative_date_assignment?, <<-PATTERN + { + (casgn _ _ (send _ ${:since :from_now :after :ago :until :before})) + (casgn _ _ ({erange irange} _ (send _ ${:since :from_now :after :ago :until :before}))) + (casgn _ _ ({erange irange} (send _ ${:since :from_now :after :ago :until :before}) _)) + } + PATTERN + + def_node_matcher :relative_date_or_assignment?, <<-PATTERN + (:or_asgn (casgn _ _) (send _ ${:since :from_now :after :ago :until :before})) + PATTERN + + def_node_matcher :relative_date?, <<-PATTERN + { + ({erange irange} _ (send _ ${:since :from_now :after :ago :until :before})) + ({erange irange} (send _ ${:since :from_now :after :ago :until :before}) _) + (send _ ${:since :from_now :after :ago :until :before}) + } + PATTERN end end end diff --git a/spec/rubocop/cop/rails/relative_date_constant_spec.rb b/spec/rubocop/cop/rails/relative_date_constant_spec.rb index ebb2f43ea51..3aa5598d2e0 100644 --- a/spec/rubocop/cop/rails/relative_date_constant_spec.rb +++ b/spec/rubocop/cop/rails/relative_date_constant_spec.rb @@ -3,87 +3,113 @@ RSpec.describe RuboCop::Cop::Rails::RelativeDateConstant do subject(:cop) { described_class.new } - it 'registers an offense for ActiveSupport::Duration.since' do - expect_offense(<<-RUBY.strip_indent) - class SomeClass - EXPIRED_AT = 1.week.since - ^^^^^^^^^^^^^^^^^^^^^^^^^ Do not assign since to constants as it will be evaluated only once. - end - RUBY - end + context 'direct assignment' do + it 'accepts a method with arguments' do + expect_no_offenses(<<-RUBY.strip_indent) + class SomeClass + EXPIRED_AT = 1.week.since(base) + end + RUBY + end - it 'accepts a method with arguments' do - expect_no_offenses(<<-RUBY.strip_indent) - class SomeClass - EXPIRED_AT = 1.week.since(base) - end - RUBY - end + it 'accepts a lambda' do + expect_no_offenses(<<-RUBY.strip_indent) + class SomeClass + EXPIRED_AT = -> { 1.year.ago } + end + RUBY + end - it 'accepts a lambda' do - expect_no_offenses(<<-RUBY.strip_indent) - class SomeClass - EXPIRED_AT = -> { 1.year.ago } - end - RUBY - end + it 'accepts a proc' do + expect_no_offenses(<<-RUBY.strip_indent) + class SomeClass + EXPIRED_AT = Proc.new { 1.year.ago } + end + RUBY + end - it 'accepts a proc' do - expect_no_offenses(<<-RUBY.strip_indent) - class SomeClass - EXPIRED_AT = Proc.new { 1.year.ago } - end - RUBY - end + it 'registers an offense for ActiveSupport::Duration.since' do + expect_offense(<<-RUBY.strip_indent) + class SomeClass + EXPIRED_AT = 1.week.since + ^^^^^^^^^^^^^^^^^^^^^^^^^ Do not assign since to constants as it will be evaluated only once. + end + RUBY - it 'registers an offense for relative date in ||=' do - expect_offense(<<-RUBY.strip_indent) - class SomeClass - EXPIRED_AT ||= 1.week.since - ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not assign since to constants as it will be evaluated only once. - end - RUBY - end + expect_correction(<<-RUBY.strip_indent) + class SomeClass + def self.expired_at + 1.week.since + end + end + RUBY + end - it 'registers an offense for relative date in multiple assignment' do - expect_offense(<<-RUBY.strip_indent) - class SomeClass - START, A, x = 2.weeks.ago, 1.week.since, 5 - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not assign ago to constants as it will be evaluated only once. - end - RUBY - end + it 'registers an offense for exclusive end range' do + expect_offense(<<-RUBY.strip_indent) + class SomeClass + TRIAL_PERIOD = DateTime.current..1.day.since + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not assign since to constants as it will be evaluated only once. + end + RUBY + + expect_correction(<<-RUBY.strip_indent) + class SomeClass + def self.trial_period + DateTime.current..1.day.since + end + end + RUBY + end + + it 'registers an offense for inclusive end range' do + expect_offense(<<-RUBY.strip_indent) + class SomeClass + TRIAL_PERIOD = DateTime.current...1.day.since + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not assign since to constants as it will be evaluated only once. + end + RUBY + end + + it 'registers an offense for exclusive begin range' do + expect_offense(<<-RUBY.strip_indent) + class SomeClass + TRIAL_PERIOD = 1.day.ago..DateTime.current + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not assign ago to constants as it will be evaluated only once. + end + RUBY + end - it 'registers an offense for exclusive end range' do - expect_offense(<<-RUBY.strip_indent) - class SomeClass - TRIAL_PERIOD = DateTime.current..1.day.since - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not assign since to constants as it will be evaluated only once. - end - RUBY + it 'registers an offense for inclusive begin range' do + expect_offense(<<-RUBY.strip_indent) + class SomeClass + TRIAL_PERIOD = 1.day.ago...DateTime.current + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not assign ago to constants as it will be evaluated only once. + end + RUBY + end end - it 'registers an offense for inclusive end range' do - expect_offense(<<-RUBY.strip_indent) - class SomeClass - TRIAL_PERIOD = DateTime.current...1.day.since - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not assign since to constants as it will be evaluated only once. - end - RUBY + context 'or assignment' do + it 'registers an offense for relative date in ||=' do + expect_offense(<<-RUBY.strip_indent) + class SomeClass + EXPIRED_AT ||= 1.week.since + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not assign since to constants as it will be evaluated only once. + end + RUBY + end end - it 'autocorrects' do - new_source = autocorrect_source(<<-RUBY.strip_indent) - class SomeClass - EXPIRED_AT = 1.week.since - end - RUBY - expect(new_source).to eq(<<-RUBY.strip_indent) - class SomeClass - def self.expired_at - 1.week.since + context 'mass assignment' do + it 'registers an offense for relative date in multiple assignment' do + expect_offense(<<-RUBY.strip_indent) + class SomeClass + START, A, x = 2.weeks.ago, 1.week.since, 5 + ^^^^^^^^^^^^^^^^^^^^^^^^^ Do not assign ago to constants as it will be evaluated only once. + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not assign since to constants as it will be evaluated only once. end - end - RUBY + RUBY + end end end From df077fbaa7a9f88e870978c4e400aedda1888ae8 Mon Sep 17 00:00:00 2001 From: Ryan Rosenblum Date: Tue, 30 Oct 2018 19:20:03 -0400 Subject: [PATCH 3/4] Fix bug in percent literal autocorrection on multiline statement ending in a comment --- CHANGELOG.md | 1 + lib/rubocop/cop/correctors/percent_literal_corrector.rb | 2 +- spec/rubocop/cop/style/word_array_spec.rb | 8 ++++++++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 92363cc28a7..a0c6c21d0b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ * [#4460](https://github.com/rubocop-hq/rubocop/issues/4460): Fix the determination of unsafe auto-correct in `Style/TernaryParentheses`. ([@jonas054][]) * [#6651](https://github.com/rubocop-hq/rubocop/issues/6651): Fix auto-correct issue in `Style/RegexpLiteral` cop when there is string interpolation. ([@roooodcastro][]) * Do not attempt to auto-correct mass assignment or optional assignment in `Rails/RelativeDateConstant`. ([@rrosenblum][]) +* Fix auto-correction of `Style/WordArray` and `Style/SymbolArray` when all elements are on separate lines and there is a trailing comment after the closing bracket. ([@rrosenblum][]) ### Changes diff --git a/lib/rubocop/cop/correctors/percent_literal_corrector.rb b/lib/rubocop/cop/correctors/percent_literal_corrector.rb index 4efb9f4eb59..eb62ea894ae 100644 --- a/lib/rubocop/cop/correctors/percent_literal_corrector.rb +++ b/lib/rubocop/cop/correctors/percent_literal_corrector.rb @@ -94,7 +94,7 @@ def process_lines(node, previous_line_num, base_line_num, source_in_lines) begin_line_num = previous_line_num - base_line_num + 1 end_line_num = node.first_line - base_line_num + 1 lines = source_in_lines[begin_line_num...end_line_num] - "\n" + lines.join("\n").split(node.source).first + "\n#{(lines.join("\n").split(node.source).first || '')}" end def fix_escaped_content(word_node, escape, delimiters) diff --git a/spec/rubocop/cop/style/word_array_spec.rb b/spec/rubocop/cop/style/word_array_spec.rb index a2062f4bff9..715a6f234d1 100644 --- a/spec/rubocop/cop/style/word_array_spec.rb +++ b/spec/rubocop/cop/style/word_array_spec.rb @@ -122,6 +122,14 @@ "baz" ] # test RUBY + + expect_correction(<<-RUBY.strip_indent) + %w( + foo + bar + baz + ) # test + RUBY end it 'auto-corrects an array of words' do From 39cf4421e973487f3bafbb094fe9b336c5ad56fe Mon Sep 17 00:00:00 2001 From: Ryan Rosenblum Date: Wed, 31 Oct 2018 14:24:27 -0400 Subject: [PATCH 4/4] Fix exception in auto-correcting ClosingParenthesisIndentation when there are no parameters --- CHANGELOG.md | 1 + lib/rubocop/cop/layout/closing_parenthesis_indentation.rb | 1 + .../cop/layout/closing_parenthesis_indentation_spec.rb | 5 +++++ 3 files changed, 7 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a0c6c21d0b4..cadecf42b7a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ * [#6651](https://github.com/rubocop-hq/rubocop/issues/6651): Fix auto-correct issue in `Style/RegexpLiteral` cop when there is string interpolation. ([@roooodcastro][]) * Do not attempt to auto-correct mass assignment or optional assignment in `Rails/RelativeDateConstant`. ([@rrosenblum][]) * Fix auto-correction of `Style/WordArray` and `Style/SymbolArray` when all elements are on separate lines and there is a trailing comment after the closing bracket. ([@rrosenblum][]) +* Fix an exception that occurs when auto-correcting `Layout/ClosingParenthesesIndentation` when there are no arguments. ([@rrosenblum][]) ### Changes diff --git a/lib/rubocop/cop/layout/closing_parenthesis_indentation.rb b/lib/rubocop/cop/layout/closing_parenthesis_indentation.rb index e7e2442c83e..07952fe5200 100644 --- a/lib/rubocop/cop/layout/closing_parenthesis_indentation.rb +++ b/lib/rubocop/cop/layout/closing_parenthesis_indentation.rb @@ -133,6 +133,7 @@ def check_for_no_elements(node) # Although there are multiple choices for a correct column, # select the first one of candidates to determine a specification. correct_column = candidates.first + @column_delta = correct_column - right_paren.column add_offense(right_paren, location: right_paren, message: message(correct_column, diff --git a/spec/rubocop/cop/layout/closing_parenthesis_indentation_spec.rb b/spec/rubocop/cop/layout/closing_parenthesis_indentation_spec.rb index 3bcc563ab35..8bdb4023b14 100644 --- a/spec/rubocop/cop/layout/closing_parenthesis_indentation_spec.rb +++ b/spec/rubocop/cop/layout/closing_parenthesis_indentation_spec.rb @@ -279,6 +279,11 @@ ) ^ Indent `)` to column 0 (not 2) RUBY + + expect_correction(<<-RUBY.strip_indent) + foo = some_method( + ) + RUBY end end end