Skip to content

Commit

Permalink
[Fix #6701] Trailing comma detection regression with HEREDOC
Browse files Browse the repository at this point in the history
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
  • Loading branch information
abrom committed Feb 7, 2019
1 parent 648085e commit cec5f31
Show file tree
Hide file tree
Showing 5 changed files with 93 additions and 16 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -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

Expand Down
12 changes: 9 additions & 3 deletions lib/rubocop/cop/mixin/trailing_comma.rb
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
57 changes: 52 additions & 5 deletions spec/rubocop/cop/style/trailing_comma_in_arguments_spec.rb
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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' \
Expand Down
27 changes: 27 additions & 0 deletions spec/rubocop/cop/style/trailing_comma_in_array_literal_spec.rb
Expand Up @@ -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
Expand Down
12 changes: 4 additions & 8 deletions spec/rubocop/cop/style/trailing_comma_in_hash_literal_spec.rb
Expand Up @@ -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
})
Expand Down Expand Up @@ -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
})
Expand Down

0 comments on commit cec5f31

Please sign in to comment.