Clarify dangers of Rails/LexicallyScopedActionFilter

[urbanautomaton]

This cop insists that users define action methods in the same controller class as any before_, after_ or around_actions that refer to the action.

However, the examples given don't account for the fact that behaviour might be defined in the superclass. Indeed, this is one of the main use cases for action decorators: customisations can be applied to superclass behaviour without redefining the whole action in the subclass.

If this is the case, then the empty action definitions shown in the docs will override the desired behaviour, and lead users to introduce bugs.

class ContentController < ApplicationController
  def update
    @content.update(content_attributes)
  end
end

class ArticlesController < ContentController
  before_action :load_article, only: [:update]

  # the cop says add this, but it breaks the update
  def update
  end

  private

  def load_article
    @content = Article.find(params[:article_id])
  end
end

In this PR I've tried to emphasise this in the docs, and have added an example to encourage users to think about the inheritance chain. The example is slightly awkward because the superclass in this case is ApplicationController, which is pretty unlikely to define actions in most apps; I wasn't sure about this, but also felt that introducing a different named superclass might also be confusing. An alternative might be to add the above snippet as a third, more realistic example case. What do people think?

I'd also be interested in people's thoughts about whether this cop should be disabled by default. My view is that it encourages semantically meaningful changes, and is therefore quite risky no matter how clear the documentation is, so I'd be tempted to disable it.

---

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.

[urbanautomaton]

Oh - should I have opened this on rubocop-rails instead? Happy to reopen there if so.

[wata727]

Interesting. I wasn't considering the possibility of this cop would such mislead.

An alternative might be to add the above snippet as a third, more realistic example case. What do people think?

I agree with this idea 👍

I'd also be interested in people's thoughts about whether this cop should be disabled by default. My view is that it encourages semantically meaningful changes, and is therefore quite risky no matter how clear the documentation is, so I'd be tempted to disable it.

Hmm... This is a very difficult problem. I believe that this Cop will help a lot, but we should avoid introducing such bugs. An alternative is to mark this cop as unsafe.

[urbanautomaton]

@wata727 Thanks for the feedback - I've added the inheritance example as a separate case.

What does marking the cop unsafe involve?

[wata727]

Cops marked as unsafe will not run when executed with the --safe flag. Originally, it seems to indicate a Cop which can yield false positives, but it may be suitable for the case. See #5978 (comment)

[urbanautomaton]

Ah okay, interesting - thanks very much for the extra context. 👍

As you say, we'd be changing the definition of "safety" somewhat if we used that here, and while I certainly feel that the suggestions this cop makes are "unsafe" in some general sense, I'd be interested in whether other people think this flag is an appropriate way to indicate that. @bbatsov, what do you think about using the "Safe: false" flag for cops that suggest semantically meaningful edits, as well as those that emit false positives?

(On a side note: the expanded default.yml metadata and elimination of the enabled / disabled configs is a great improvement, thank you to those who worked on it. 😄)

[bbatsov]

@bbatsov, what do you think about using the "Safe: false" flag for cops that suggest semantically meaningful edits, as well as those that emit false positives?

That's what we usually do. I guess we should mark this one as unsafe indeed.

[urbanautomaton]

Okay, thanks very much both - I've marked the cop as unsafe and updated the changelog. From my perspective this is ready to go, but please let me know if you'd like anything changed.

[bbatsov]

Thanks!