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

Implement NoReturnFromBlock rule #12971

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

toddnestor
Copy link

@toddnestor toddnestor commented Jun 6, 2024

Fixes #4064.

This PR adds a NoReturnFromBlock rule that warns when return is called from inside a block. This rule is turned off by default so as not to cause any breaking changes. It can be enabled by setting AllowReturnFromBlock to false in the config.

This rule is important as a return in a block can often cause bugs.


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • 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.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

@toddnestor toddnestor marked this pull request as ready for review June 6, 2024 20:13
@toddnestor toddnestor changed the title Implement NoReturnFromBlock rule Implement NoReturnFromBlock rule Jun 6, 2024
@bbatsov
Copy link
Collaborator

bbatsov commented Jun 7, 2024

One issue that I see which such a rule is that it's more or less the same as forbidding the use of procs, as the behavior of return there is the only difference from a lambda.

@rubocop/rubocop-core What do you think this PR?

module Cop
module Lint
# Checks for blocks that have a return statement.
# Return statements in blocks are typically an oversight that can lead to bugs.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be good to clarify about the nature of the problems that can be caused.

private

def allow_return_from_block?
cop_config['AllowReturnFromBlock']
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the point of this config, as it's essentially the same as disabling the cop. Also - what about procs? They are essentially blocks for all practical purposes.

@koic
Copy link
Member

koic commented Jun 8, 2024

The bad case described in the example of this PR can already be detected by Lint/NonLocalExitFromIterator, so I have concerns about the necessity of providing this new cop.

      # @example
      #   # bad
      #   items.each do |item|
      #     return if item.nil?
      #     puts item.some_attribute
      #   end

https://docs.rubocop.org/rubocop/1.64/cops_lint.html#lintnonlocalexitfromiterator

@bbatsov
Copy link
Collaborator

bbatsov commented Jun 8, 2024

Ah, great point! I had forgotten about it.

@toddnestor
Copy link
Author

The bad case described in the example of this PR can already be detected by Lint/NonLocalExitFromIterator, so I have concerns about the necessity of providing this new cop.

      # @example
      #   # bad
      #   items.each do |item|
      #     return if item.nil?
      #     puts item.some_attribute
      #   end

https://docs.rubocop.org/rubocop/1.64/cops_lint.html#lintnonlocalexitfromiterator

Ah I wasn't aware of this one, let me test that rule out and see if why it didn't catch the bugs we had recently, I'll close this PR if it is indeed redundant.

@dimakura
Copy link

dimakura commented Jul 5, 2024

The bad case described in the example of this PR can already be detected by Lint/NonLocalExitFromIterator, so I have concerns about the necessity of providing this new cop.

      # @example
      #   # bad
      #   items.each do |item|
      #     return if item.nil?
      #     puts item.some_attribute
      #   end

https://docs.rubocop.org/rubocop/1.64/cops_lint.html#lintnonlocalexitfromiterator

@koic @bbatsov how about return with an expression, which is allowed by Lint/NonLocalExitFromIterator?

items.each do |item|
  return nil if item.nil? # Lint/NonLocalExitFromIterator doesn't complain about this
  # or
  return 'something' if item.nil? # this is also okay

  # ...
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

please add a cop that warns if there is a return from within a block
4 participants