[Fix #6701] Trailing comma detection regression with HEREDOC

[abrom]

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 optional whitespace followed by trailing commas, excluding newlines in the match when heredoc is present.

---

Before submitting the PR make sure the following are checked:

• Wrote good commit messages.
• Commit message starts with [Fix #issue-number] (if the related issue exists).
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
• The PR relates to only one subject with a clear title
  and description in grammatically correct, complete sentences.
• Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

[abrom]

I've tried to solve this problem a different way. Instead of skipping the trailing comma check if the last item is heredoc, we look to see if any item in the node is heredoc and if so we check for a trailing comma directly preceding the last item source (with only horizontal whitespace). If there is no heredoc then we allow horizontal and vertical whitespace before the comma.

This did raise some potential issues with the existing specs, specifically those titled

accepts missing comma after a heredoc

The comment at the top of these specs is:

# A heredoc that's the last item in a literal or parameter list can not
# have a trailing comma. It's a syntax error.

Although this isn't true??

some_method(
  <<-FOO,
    ...
  FOO
)

is valid syntax?!

As is

[
  <<-FOO,
    ...
  FOO
]

[abrom]

...as such, my belief is that the specs failing on this branch are actually incorrect. If others agree, I'll modify those specs as needed

[Drenmi]

Thank you, @abrom! 🙇

[Drenmi]

@abrom Since there have been no objections to this, I'm ready to merge it in. Please rebase on latest master, and make sure the CHANGELOG is merged correctly, then I will accept the PR. 🙇

[abrom]

Done 👍

[wilg]

It seems like after this PR if I have:

Style/TrailingCommaInArguments:
  Enabled: true
  EnforcedStyleForMultiline: comma

(or the other trailing comma cops), it forces a trailing comma in the heredoc, like in the test cases:

[
  <<-FOO,
    ...
  FOO
]

This ends up looking really confusing and bizarre to me. It's particularly bizarre when a one-argument function that takes a multiline heredoc is required to have a comma. Is this intended behavior? Would it make sense to have an option to disable it for heredocs?

[abrom]

But that's the expected result with EnforcedStyleForMultiline: comma and your array being defined across multiple lines?!

[wilg]

I think my example was poor, it makes more sense for array/hash literals, but less for:

my_func(
 <<-FOO,
    ...
  FOO
)

[abrom]

But that's exactly what EnforcedStyleForMultiline: comma is expected to do there too. If you wanted to have the cop configured as such, but not have the trailing comma you'd have something like:

my_func(<<-FOO)  # <-- the method definition is now on one line
  ...
FOO

[wilg]

Yep, I think you're right. The codebase I'm looking at just uses heredocs and multiline comma enforcement in a way that makes things look and feel weird after this change. I'll open an issue if I can figure out a way to have my cake and eat it too. Thanks!