Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

False positive in Style/TrivialAccessors with inline private #5641

Closed
fujimura opened this Issue Mar 7, 2018 · 4 comments

Comments

Projects
None yet
4 participants
@fujimura
Copy link
Contributor

fujimura commented Mar 7, 2018

Expected behavior

Not an offense. In this case, private / protected clause needs symbol returned from def but attr_reader returns nil, which causes TypeError.

Actual behavior

# a.rb
class A
  private def foo
    @foo
  end
end
$ rubocop -a a.rb
Inspecting 1 file
C

Offenses:

a.rb:2:11: C: [Corrected] Style/TrivialAccessors: Use attr_reader to define trivial reader methods.
  private def foo
          ^^^

1 file inspected, 1 offense detected, 1 offense corrected
# Corrected a.rb
class A
  private attr_reader :foo
end
$ ruby a.rb
a.rb:2:in `private': nil is not a symbol nor a string (TypeError)
        from a.rb:2:in `<class:A>'
        from a.rb:1:in `<main>'

Steps to reproduce the problem

Described in actual behavour.

RuboCop version

$ rubocop -V
0.53.0 (using Parser 2.5.0.2, running on ruby 2.4.2 x86_64-darwin16)
@Drenmi

This comment has been minimized.

Copy link
Collaborator

Drenmi commented Mar 12, 2018

You can still define private readers using attr_reader, which is what RuboCop is suggesting:

private

attr_reader :foo

although not using the new inline syntax.

@pocke

This comment has been minimized.

Copy link
Member

pocke commented Mar 14, 2018

I don't think it is a false positive for the same reason as the @Drenmi comment.
But the auto-correction behaviour is a bug. So we should disable the auto-correction on private.

BTW, maybe we can add an option to ignore this case, i.e. this cop ignores private def foo ... with this option. But I'm not sure the option is necessary as I think the private; attr_reader :foo is better style.

@pocke pocke added the bug label Mar 14, 2018

pocke added a commit to pocke/rubocop that referenced this issue Mar 15, 2018

[rubocop-hq#5641] Disable `Style/TrivialAccessors` auto-correction fo…
…r `def` with `private`

See rubocop-hq#5641

This cop should not auto-correct `private def foo() @foo end`. Because `def foo() @foo end` returns `:foo`, but `attr_reader :foo` returns `nil`.

The `autocorrect` method checks only the parent is a `send` type. It does not check method name. Because the cop also should handle methods other than `private`, `public`, `protected`. For example, [finalist](https://github.com/joker1007/finalist) gem defines `final` method (`final def foo; end`).

Note
===

This pull-request does not close the related issue since the issue still has an problem if the auto-correction is fixed.
I think we should discuss the cop should have or not an option to ignore `private def foo() @foo end` on the issue.

bbatsov added a commit that referenced this issue Mar 16, 2018

[#5641] Disable `Style/TrivialAccessors` auto-correction for `def` wi…
…th `private` (#5691)

See #5641

This cop should not auto-correct `private def foo() @foo end`. Because `def foo() @foo end` returns `:foo`, but `attr_reader :foo` returns `nil`.

The `autocorrect` method checks only the parent is a `send` type. It does not check method name. Because the cop also should handle methods other than `private`, `public`, `protected`. For example, [finalist](https://github.com/joker1007/finalist) gem defines `final` method (`final def foo; end`).

Note
===

This pull-request does not close the related issue since the issue still has an problem if the auto-correction is fixed.
I think we should discuss the cop should have or not an option to ignore `private def foo() @foo end` on the issue.
@fujimura

This comment has been minimized.

Copy link
Contributor Author

fujimura commented Mar 19, 2018

I don't think it is a false positive for the same reason as the @Drenmi comment.
But the auto-correction behaviour is a bug. So we should disable the auto-correction on private.

Agreed. Thanks for fixing this!

This was referenced Mar 21, 2018

This was referenced Mar 21, 2018

@Darhazer

This comment has been minimized.

Copy link
Member

Darhazer commented Apr 14, 2018

Fixed in 0.54. Can we close this?

@Drenmi Drenmi closed this Apr 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.