From cec5f31e4072fe7eb4e72bead4b62a0319401daf Mon Sep 17 00:00:00 2001 From: Andrew Bromwich Date: Thu, 24 Jan 2019 00:54:30 +1000 Subject: [PATCH] [Fix #6701] Trailing comma detection regression with HEREDOC The TrailingCommaInArguments and TrailingCommaInArrayLiteral cops have a false positive when the parent node contains HEREDOC with commas in them. The issue appears to be due to lax comma detection. Instead of only matching the last item for heredoc, match all items and search for trailing commas, excluding newlines in match when heredoc is present --- CHANGELOG.md | 1 + lib/rubocop/cop/mixin/trailing_comma.rb | 12 +++- .../style/trailing_comma_in_arguments_spec.rb | 57 +++++++++++++++++-- .../trailing_comma_in_array_literal_spec.rb | 27 +++++++++ .../trailing_comma_in_hash_literal_spec.rb | 12 ++-- 5 files changed, 93 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7264bd8f7a8..c0082f2a5af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ * [#6382](https://github.com/rubocop-hq/rubocop/issues/6382): Fix `Layout/IndentationWidth` with `Layout/EndAlignment` set to start_of_line. ([@dischorde][], [@siegfault][], [@mhelmetag][]) * [#6710](https://github.com/rubocop-hq/rubocop/issues/6710): Fix `Naming/MemoizedInstanceVariableName` on method starts with underscore. ([@pocke][]) * [#6722](https://github.com/rubocop-hq/rubocop/issues/6722): Fix an error for `Style/OneLineConditional` when `then` branch has no body. ([@koic][]) +* [#6702](https://github.com/rubocop-hq/rubocop/pull/6702): Fix `TrailingComma` regression where heredoc with commas caused false positives. ([@abrom][]) ### Changes diff --git a/lib/rubocop/cop/mixin/trailing_comma.rb b/lib/rubocop/cop/mixin/trailing_comma.rb index 956d2bd9bb1..4f8819dc983 100644 --- a/lib/rubocop/cop/mixin/trailing_comma.rb +++ b/lib/rubocop/cop/mixin/trailing_comma.rb @@ -17,11 +17,13 @@ def style_parameter_name end def check(node, items, kind, begin_pos, end_pos) - return if heredoc?(items.last) - after_last_item = range_between(begin_pos, end_pos) - comma_offset = after_last_item.source =~ /,/ + # If there is any heredoc in items, then match the comma succeeding + # any whitespace (except newlines), otherwise allow for newlines + comma_regex = any_heredoc?(items) ? /\A[^\S\n]*,/ : /\A\s*,/ + comma_offset = after_last_item.source =~ comma_regex && + after_last_item.source.index(',') if comma_offset && !inside_comment?(after_last_item, comma_offset) check_comma(node, kind, after_last_item.begin_pos + comma_offset) @@ -167,6 +169,10 @@ def avoid_autocorrect?(_nodes) false end + def any_heredoc?(items) + items.any? { |item| heredoc?(item) } + end + def heredoc?(node) return false unless node.is_a?(RuboCop::AST::Node) return true if node.loc.respond_to?(:heredoc_body) diff --git a/spec/rubocop/cop/style/trailing_comma_in_arguments_spec.rb b/spec/rubocop/cop/style/trailing_comma_in_arguments_spec.rb index c6019f05a40..4c1050cf0a0 100644 --- a/spec/rubocop/cop/style/trailing_comma_in_arguments_spec.rb +++ b/spec/rubocop/cop/style/trailing_comma_in_arguments_spec.rb @@ -11,6 +11,14 @@ RUBY end + it 'registers an offense for trailing comma preceded by whitespace' \ + ' in a method call' do + expect_offense(<<-RUBY.strip_indent) + some_method(a, b, c , ) + ^ Avoid comma after the last parameter of a method call#{extra_info}. + RUBY + end + it 'registers an offense for trailing comma in a method call with hash' \ ' parameters at the end' do expect_offense(<<-RUBY.strip_indent) @@ -57,6 +65,14 @@ new_source = autocorrect_source('some_method(a, b, c: 0, d: 1, )') expect(new_source).to eq('some_method(a, b, c: 0, d: 1 )') end + + it 'accepts heredoc without trailing comma' do + expect_no_offenses(<<-RUBY.strip_indent) + route(1, <<-HELP.chomp) + ... + HELP + RUBY + end end context 'with single line list of values' do @@ -167,6 +183,33 @@ RUBY end + it 'accepts comma inside a modified heredoc parameter' do + expect_no_offenses(<<-RUBY.strip_indent) + some_method( + <<-LOREM.delete("\\n") + Something with a , in it + LOREM + ) + RUBY + end + + it 'auto-corrects unwanted comma after modified heredoc parameter' do + new_source = autocorrect_source(<<-RUBY.strip_indent) + some_method( + <<-LOREM.delete("\\n"), + Something with a , in it + LOREM + ) + RUBY + expect(new_source).to eq(<<-RUBY.strip_indent) + some_method( + <<-LOREM.delete("\\n") + Something with a , in it + LOREM + ) + RUBY + end + context 'when there is string interpolation inside heredoc parameter' do it 'accepts comma inside a heredoc parameter' do expect_no_offenses(<<-RUBY.strip_indent) @@ -280,7 +323,7 @@ it 'accepts missing comma after heredoc with comments' do expect_no_offenses(<<-RUBY.strip_indent) route( - <<-HELP.chomp + a, <<-HELP.chomp , # some comment HELP @@ -437,15 +480,19 @@ RUBY end - it 'accepts missing comma after a heredoc' do - # A heredoc that's the last item in a literal or parameter list can not - # have a trailing comma. It's a syntax error. - expect_no_offenses(<<-RUBY.strip_indent) + it 'auto-corrects missing comma after a heredoc' do + new_source = autocorrect_source(<<-RUBY.strip_indent) route(1, <<-HELP.chomp ... HELP ) RUBY + expect(new_source).to eq(<<-RUBY.strip_indent) + route(1, <<-HELP.chomp, + ... + HELP + ) + RUBY end it 'auto-corrects missing comma in a method call with hash parameters' \ diff --git a/spec/rubocop/cop/style/trailing_comma_in_array_literal_spec.rb b/spec/rubocop/cop/style/trailing_comma_in_array_literal_spec.rb index 143fac27dde..5ea7908048b 100644 --- a/spec/rubocop/cop/style/trailing_comma_in_array_literal_spec.rb +++ b/spec/rubocop/cop/style/trailing_comma_in_array_literal_spec.rb @@ -100,6 +100,33 @@ ] RUBY end + + it 'accepts HEREDOC with commas' do + expect_no_offenses(<<-RUBY.strip_indent) + [ + <<-TEXT, 123 + Something with a , in it + TEXT + ] + RUBY + end + + it 'auto-corrects unwanted comma where HEREDOC has commas' do + new_source = autocorrect_source(<<-RUBY.strip_indent) + [ + <<-TEXT, 123, + Something with a , in it + TEXT + ] + RUBY + expect(new_source).to eq(<<-RUBY.strip_indent) + [ + <<-TEXT, 123 + Something with a , in it + TEXT + ] + RUBY + end end context 'when EnforcedStyleForMultiline is comma' do diff --git a/spec/rubocop/cop/style/trailing_comma_in_hash_literal_spec.rb b/spec/rubocop/cop/style/trailing_comma_in_hash_literal_spec.rb index f250f0979a5..13d6c6f544c 100644 --- a/spec/rubocop/cop/style/trailing_comma_in_hash_literal_spec.rb +++ b/spec/rubocop/cop/style/trailing_comma_in_hash_literal_spec.rb @@ -154,12 +154,10 @@ RUBY end - it 'accepts missing comma after a heredoc' do - # A heredoc that's the last item in a literal or parameter list can not - # have a trailing comma. It's a syntax error. + it 'accepts trailing comma after a heredoc' do expect_no_offenses(<<-RUBY.strip_indent) route(help: { - 'auth' => <<-HELP.chomp + 'auth' => <<-HELP.chomp, ... HELP }) @@ -238,12 +236,10 @@ RUBY end - it 'accepts missing comma after a heredoc' do - # A heredoc that's the last item in a literal or parameter list can not - # have a trailing comma. It's a syntax error. + it 'accepts trailing comma after a heredoc' do expect_no_offenses(<<-RUBY.strip_indent) route(help: { - 'auth' => <<-HELP.chomp + 'auth' => <<-HELP.chomp, ... HELP })