Added new `Lint/TopLevelReturnWithArgument` cop #8377

IamRaviTejaG · 2020-07-20T21:33:10Z

Fixes #7425

This cop detects top level return statements with argument(s).

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.

marcandre

Good PR!
A few comments. Biggest issue is that return 42 if condition, or module X; return 42 won't be caught

marcandre · 2020-07-20T22:14:58Z

lib/rubocop/cop/lint/top_level_return_with_argument.rb

+        MSG = 'Top level return with argument detected.'
+
+        def on_return(return_node)
+          add_offense(return_node) if ancestors_valid?(return_node) && !return_node.arguments.empty?


return_node.arguments.empty? can also be written return_node.arguments?

marcandre · 2020-07-20T22:16:03Z

lib/rubocop/cop/lint/top_level_return_with_argument.rb

+        def ancestors_valid?(return_node)
+          return true if return_node.parent.nil?
+
+          if return_node.parent.instance_of?(AST::Node) && return_node.parent.parent.nil?


Won't return_node.parent.instance_of?(AST::Node) always be true?

return_node.parent.is_a?(AST::Node) will always return true, but return_node.parent.instance_of?(AST::Node) would be true only if that object is strictly of that exact class, and not the subclass.

Oh, right, my bad. Then I would say you need to tweak this test; you should not rely on the class of Node, only on their type. Here anyways I believe it is not the best anyways; you're excluding if for example.

marcandre · 2020-07-20T22:16:51Z

lib/rubocop/cop/lint/top_level_return_with_argument.rb

+            return true
+          end
+
+          false


I'm surprised there's no cop against if condition; return true; end; return false => return condition

I did write it in one line, but then it exceeded the 100 characters limit, so had to change it to this.

@marcandre There actually is a cop that suggests this, one suggests for single line if and the other suggests to remove redundant false.

In this case here, you can remove the if, the return true\n end\n false completely...

The if condition checks for the types and returns things appropriately, if we remove that, the cop will report false positives.

Updated this! Thanks for the idea. 😄

IamRaviTejaG · 2020-07-21T06:12:15Z

@marcandre Updated the PR. Now return 42 if a == b are being caught.

marcandre · 2020-07-21T14:03:34Z

@marcandre Updated the PR. Now return 42 if a == b are being caught.

RIght, good, but it won't catch if a == b; warn 'hey'; return 42; end...

I was trying to hint at a more general logic. Unless I'm missing something, you can check all ancestors for either a block, def or defs; if you don't find any then you are still at the top level scope.

IamRaviTejaG · 2020-07-21T15:53:52Z

@marcandre Updated the PR. Now return 42 if a == b are being caught.

RIght, good, but it won't catch if a == b; warn 'hey'; return 42; end...

I was trying to hint at a more general logic. Unless I'm missing something, you can check all ancestors for either a block, def or defs; if you don't find any then you are still at the top level scope.

Now this checks for the ancestors to not belong to the AST::BlockNode or AST::DefNode class.

marcandre · 2020-07-21T19:35:05Z

Now this checks for the ancestors to not belong to the AST::BlockNode or AST::DefNode class.

Much better. Please do not rely on classes ever, as this is subject to change. You should rely on the result of Node#type or Node#block_type? / def_type? / defs_type? In this case, each_ancestor already accepts a list of types...

IamRaviTejaG · 2020-07-21T21:20:10Z

Now this checks for the ancestors to not belong to the AST::BlockNode or AST::DefNode class.

Much better. Please do not rely on classes ever, as this is subject to change. You should rely on the result of Node#type or Node#block_type? / def_type? / defs_type? In this case, each_ancestor already accepts a list of types...

Aah, this suggestion and the resulting changes made the code so compact! Can't thank you enough for this.

marcandre · 2020-07-22T00:10:06Z

lib/rubocop/cop/lint/top_level_return_with_argument.rb

+
+        def ancestors_valid?(return_node)
+          prohibited_ancestors = return_node.each_ancestor(:block, :def, :defs)
+          return false if prohibited_ancestors.any?


Personal preference, I'd simply write prohibited_ancestors.none?, no return, no if...

marcandre · 2020-07-22T00:10:54Z

lib/rubocop/cop/lint/top_level_return_with_argument.rb

+        MSG = 'Top level return with argument detected.'
+
+        def on_return(return_node)
+          add_offense(return_node) if ancestors_valid?(return_node) && return_node.arguments?


I'd switch the conditions' order, as arguments? is very quick while ancestors_valid? might be slower

Yes, this also makes a lot of sense given Ruby's short circuit evaluation.

marcandre · 2020-07-22T00:12:03Z

Aah, this suggestion and the resulting changes made the code so compact! Can't thank you enough for this.

Cool, I'm happy you're happy about the resulting code. I have two last remarks for styling/performance...

Also you'll have to rebase your branch

IamRaviTejaG · 2020-07-22T04:23:33Z

Cool, I'm happy you're happy about the resulting code. I have two last remarks for styling/performance...

Also you'll have to rebase your branch

Made performance & styling related changes, and also rebased my branch. 😄

marcandre · 2020-07-22T04:56:18Z

docs/modules/ROOT/pages/cops_lint.adoc

+This cop works by validating the ancestors of the return node. A
+top-level return node's ancestors will never belong to `AST::BlockNode`
+or `AST::DefNode` class.


I'd remove this comment; implementation detail is too technical and not useful to cop users.

Makes sense to me, removed this. Added this inside the class instead.

marcandre · 2020-07-22T04:57:18Z

spec/rubocop/cop/lint/top_level_return_with_argument_spec.rb

+
+        [1, 2, 3, 4, 5].each { |n| return n }
+
+        return # Should raise a `top level return with argument detected` offense


Change comment or remove...

marcandre · 2020-07-22T04:58:14Z

spec/rubocop/cop/lint/top_level_return_with_argument_spec.rb

+    it 'Expects no offense from the method-level return statement' do
+      expect_no_offenses(<<~RUBY)
+        def method
+          return 'Hello World'
+        end
+      RUBY
+    end
+


Remove this spec, it is redundant with the next spec.

marcandre · 2020-07-22T04:59:04Z

spec/rubocop/cop/lint/top_level_return_with_argument_spec.rb

+    it 'Expects an offense from the return with arguments' do
+      expect_offense(<<~RUBY)
+        if a == b; warn 'hey'; return 42; end
+                               ^^^^^^^^^ Top level return with argument detected.
+      RUBY
+    end


Seems redundant with previous spec

Yes, removed this spec.

marcandre · 2020-07-22T04:59:24Z

spec/rubocop/cop/lint/top_level_return_with_argument_spec.rb

+    it 'Expects no offense from the return without arguments' do
+      expect_no_offenses(<<~RUBY)
+        if a == b; warn 'hey'; return; end
+      RUBY
+    end
+


Seems redundant with next spec

Yes, removed this.

marcandre · 2020-07-22T05:00:19Z

Implementation is perfect now 🎉. I think some specs can be removed and some doc can be modified.

marcandre · 2020-07-22T05:01:16Z

lib/rubocop/cop/lint/top_level_return_with_argument.rb

+      # This cop works by validating the ancestors of the return node. A
+      # top-level return node's ancestors will never belong to `AST::BlockNode`
+      # or `AST::DefNode` class.
+      #


Please update and put inside the class so it doesn't show in the doc

Updated this and added it inside class.

marcandre

LGTM, I'll let @bbatsov check naming & al.
(Will need to be squash-merged)

IamRaviTejaG · 2020-07-24T18:00:06Z

Hey @bbatsov, can you please review this PR. 😄

marcandre · 2020-07-24T18:05:27Z

It's ok, I probably should have simply merged it, looks really good. Looks like there are conflicts again, could you rebase and I'll merge it?

IamRaviTejaG · 2020-07-24T18:40:52Z

@marcandre One test failed erroneously, and I cannot seem to re-run it, can you please re-run it? Once that passes this should be good to merge.

marcandre · 2020-07-24T18:46:39Z

Thanks @IamRaviTejaG 🎉

Follow rubocop#7868, rubocop#8377, and rubocop#8395 This PR uses `Cop::Base` API for `Lint/TopLevelReturnWithArgument`.

marcandre reviewed Jul 20, 2020

View reviewed changes

IamRaviTejaG requested a review from marcandre July 21, 2020 08:45

marcandre reviewed Jul 22, 2020

View reviewed changes

marcandre approved these changes Jul 22, 2020

View reviewed changes

IamRaviTejaG added 9 commits July 24, 2020 23:55

Added new cop for detecting top-level return with argument

ffc7cb8

Updated docs

5f34208

Updated CHANGELOG

3c560ae

Updated cop functionality

7d7beef

Run bundle exec rake default

41d08db

Updated CHANGELOG

130fce8

Fixed rubocop issues

7746959

Fix for failing CI tests

0409396

Removed redundant if, return, end & false keywords

a2b6a2c

IamRaviTejaG added 9 commits July 24, 2020 23:55

Removed unnecessary file

0735048

Updated logic to support top-level return with inline if

e8ef118

Removed on_if method for inline if

956e7db

Updated logic to check ancestors to not belong to certain class

896ee09

Filter ancestors by types and not classes

ac906ae

Fix performance issues

2b5730e

Removed redundant specs & updated docs

11f53dd

Rebased & updated CHANGELOG to resolve conflicts

027bd57

Rebased & updated CHANGELOG

93fdcd0

marcandre merged commit b8eaa55 into rubocop:master Jul 24, 2020

koic added a commit to koic/rubocop that referenced this pull request Aug 28, 2020

Use Cop::Base API for Lint/TopLevelReturnWithArgument

d22b72b

Follow rubocop#7868, rubocop#8377, and rubocop#8395 This PR uses `Cop::Base` API for `Lint/TopLevelReturnWithArgument`.

koic mentioned this pull request Aug 28, 2020

Use Cop::Base API for Lint/TopLevelReturnWithArgument #8614

Merged

8 tasks

IamRaviTejaG deleted the lint-top-level-return-with-arguments branch January 8, 2021 02:10


		[1, 2, 3, 4, 5].each { \|n\| return n }

		return # Should raise a `top level return with argument detected` offense

Added new Lint/TopLevelReturnWithArgument cop #8377

Added new Lint/TopLevelReturnWithArgument cop #8377

Conversation

IamRaviTejaG commented Jul 20, 2020

marcandre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IamRaviTejaG commented Jul 21, 2020

marcandre commented Jul 21, 2020

IamRaviTejaG commented Jul 21, 2020

marcandre commented Jul 21, 2020

IamRaviTejaG commented Jul 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcandre commented Jul 22, 2020 • edited

IamRaviTejaG commented Jul 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcandre Jul 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcandre commented Jul 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcandre left a comment • edited

Choose a reason for hiding this comment

IamRaviTejaG commented Jul 24, 2020

marcandre commented Jul 24, 2020

IamRaviTejaG commented Jul 24, 2020

marcandre commented Jul 24, 2020

Added new `Lint/TopLevelReturnWithArgument` cop #8377

Added new `Lint/TopLevelReturnWithArgument` cop #8377

marcandre commented Jul 22, 2020 •

edited

marcandre Jul 22, 2020 •

edited

marcandre commented Jul 22, 2020 •

edited

marcandre left a comment •

edited