Add new Lint/TrailingCommaInAttributeDeclaration cop

[Drenmi]

Using a trailing comma in an attribute declaration, such as attr_reader will lead to the next method definition being shadowed by a (probably no-op) reader method. This happens because def returns a symbol, which will be interpreted as the last argument to the attribute declaration.

This led to a particularly hard to track down bug for one of my colleagues.

---

Leaving a trailing comma without any method definition following is a syntax error:

class Foo
  attr_reader :bar,
end
#=> SyntaxError ((irb):3: syntax error, unexpected `end')

but with a method definition following, Ruby gets by, with unexpected results:

class Foo
  attr_reader :bar,

  def baz
    puts "This will be shadowed by a reader of the same name."
  end
end

The generated AST shows what is happening:

(def :Foo
  (args)
  (send nil :attr_reader
    (sym :bar)
    (def :baz
      (args)
      (send nil :puts
        (str "This will be shadowed by a reader of the same name.")))))

As we can see, the whole method definition has become part of the attribute declaration.

---

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.

[bbatsov]

I think Avoid should be "Do not" in this case.

[bbatsov]

I guess that's a more general problem for all sort of macro-like methods that operate on symbols, but I'm fine with having it with only attribute methods as its scope.

[Drenmi]

I guess that's a more general problem for all sort of macro-like methods that operate on symbols, but I'm fine with having it with only attribute methods as its scope.

Yes. I think all class body methods that consume an arbitrary number of arguments will see (probably different) problems, but I think this is the most realistic case. I had never heard about any issue like this until it happened to my colleague a couple of days ago. 🙂

[KNejad]

I was just about to open an issue for a similar problem:

x = 12,
y = 34

Will give a Layout/ArrayAlignment cop, and when auto-linting will just align y to the 1.

In the above case, similar to the attr_reader issue in this PR, x will equal [12, 34]. I would argue that is rarely the intended outcome and it is more likely just a typo. In the event it's not a typo you can be more explicit by writing x = [12, y = 34].

What do you think of extending this cop to catch all commas at the end of an assignment, or method call line where the next line is not a simple variable. (e.g. the next line is a method as is your case, or an assignment in my case)