Skip to content

Fix a false negative for Lint/ReturnInVoidContext when returning out of a block#13960

Merged
koic merged 1 commit intorubocop:masterfrom
Earlopain:return-in-void-context-blocks
Mar 6, 2025
Merged

Fix a false negative for Lint/ReturnInVoidContext when returning out of a block#13960
koic merged 1 commit intorubocop:masterfrom
Earlopain:return-in-void-context-blocks

Conversation

@Earlopain
Copy link
Contributor

Should be block happen to be called, the return will break out of the block and attribute the return value to the method.
This has been that way since the cop was added and there were no tests for blocks.

The only expection to the rule is when returning from inside a lambda.

Additionally, I simplified the implementation. The cop looked for sdef but then immediatly after required that the node be simply def.


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.


return unless context_node&.def_type?
return unless context_node&.void_context?
method_node = return_node.each_ancestor(:def).first
Copy link
Member

Choose a reason for hiding this comment

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

Can you rename from method_node to def_node?

Suggested change
method_node = return_node.each_ancestor(:def).first
def_node = return_node.each_ancestor(:def).first

expect_offense(<<~RUBY)
class A
def initialize
Proc.new do
Copy link
Member

Choose a reason for hiding this comment

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

Is the intended explanation referring to proc?

Suggested change
Proc.new do
proc do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Either works, I don't think there is any difference between the two. I've updated to proc to be more inline with the spec description (it's also what Style/Proc would want).

Copy link
Member

Choose a reason for hiding this comment

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

Yep, the behavior is the same in both cases. This is about ensuring the spec description matches the example.

…t of a block

Should be block happen to be called, the return will break out of the block and attribute the return value to the method.

This has been that way since the cop was added and there were no tests for blocks.

The only expection to the rule is when returning from inside a lambda.

Additionally, I simplified the implementation. The cop looked for `sdef` but then immediatly after
required that the node be simply `def`.
@Earlopain Earlopain force-pushed the return-in-void-context-blocks branch from 2febab9 to 0fd4829 Compare March 6, 2025 15:58
@koic koic merged commit 69af0f8 into rubocop:master Mar 6, 2025
24 checks passed
@koic
Copy link
Member

koic commented Mar 6, 2025

Thanks!

@Earlopain Earlopain deleted the return-in-void-context-blocks branch March 6, 2025 16:10
@doits
Copy link

doits commented Mar 20, 2025

I think there's a false positive for .define_method inside #initialize now where the return actually applies to the block of .define_method.

The following example is a little bit contrived but I got the errors in a more complicated case and narrowed it down to this:

class Something
  def initialize
    self.class.define_method :some_method do
      return 'abc' # rubocop: Lint/ReturnInVoidContext: Do not return a value in initialize
    end
  end
end

instance = Something.new
instance.some_method
# => 'abc'

Or am I wrong?

@doits
Copy link

doits commented Apr 1, 2025

I opened #14062 with more context and a working example for a false positive.

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.

3 participants