New: `no-useless-return` rule (fixes #7309) #7441

mysticatea · 2016-10-24T12:25:37Z

What is the purpose of this pull request? (put an "X" next to item)

[X] New rule (template)

See #7309 for the template.

What changes did you make? (Give an overview)

This PR implement no-useless-return rule.
The logic is:

At ReturnStatement nodes which may be useless, it stores the node into both the current code path segment and the current code path.
Every new code path segment inherits the ReturnStatement nodes from the previous segments.
- If a previous segment is unreachable and the previous segment was terminated by a ReturnStatement, the new segment inherits ReturnStatement nodes from the unreachable segment. If the unreachable segment was terminated by other kinds of statements, the new segment ignores the unreachable segment. This is the emulation for the case that ReturnStatement nodes don't exist.
At every statement nodes, it removes ReturnStatement nodes which are on the current segments from the code path.
At the end of a code path, it reports remaining ReturnStatement nodes of the code path.

In addition, this PR enable no-useless-return rule for dogfooding.

Is there anything you'd like reviewers to focus on?

Other examples for tests which should cover.
no-useless-return rule seems to conflict to callback-return rule. Please check 52f8391. Thought about that?

mention-bot · 2016-10-24T12:25:40Z

@mysticatea, thanks for your PR! By analyzing the history of the files in this pull request, we identified @IanVS, @vitorbal and @kaicataldo to be potential reviewers.

eslintbot · 2016-10-24T12:25:40Z

LGTM

not-an-aardvark

Thanks! This mostly looks good, but I have a few questions.

not-an-aardvark · 2016-10-24T18:27:01Z

docs/rules/no-useless-return.md

@@ -0,0 +1,83 @@
+# Disallow redundant return statements (no-useless-return)
+
+A `return;` statement with nothing after it is redundant, and has no effect on the runtime behavior of a function. This can be confusing, so it's better to disallow these redundant statements


Oops, I forgot a . at the end of the sentence: "to disallow these redundant statements**.**"

not-an-aardvark · 2016-10-24T18:30:08Z

lib/rules/no-useless-return.js

+ * @param {ASTNode} node - The return statement node to check.
+ * @returns {boolean} `true` if the node is removeable.
+ */
+function isRemoveable(node) {


Nit: I think the word is "removable" (no "e" in the middle).

not-an-aardvark · 2016-10-25T16:31:55Z

lib/rules/no-useless-return.js

+            ForOfStatement: markReturnStatementsOnCurrentSegmentsAsUsed,
+            ForStatement: markReturnStatementsOnCurrentSegmentsAsUsed,
+            IfStatement: markReturnStatementsOnCurrentSegmentsAsUsed,
+            ImportDeclaration: markReturnStatementsOnCurrentSegmentsAsUsed,


Do ImportDeclaration nodes need to be on this list? I think import declarations are hoisted, so the following would be redundant:

import {foo} from 'bar'; return;

However, I'm not sure about the correct semantics, because I don't know of any environments with globalReturn: true that support import.

I also don't know such environments 😃 .
In this case, I guessed ImportDeclarations behaves as similar to VariableDeclarations. On the other hand, even if globalReturn: true does not support import, this will not trigger false positive/negative.

eslintbot · 2016-10-27T03:42:10Z

LGTM

eslintbot · 2016-10-27T03:50:19Z

LGTM

not-an-aardvark

LGTM, thanks!

eslintbot · 2016-10-27T08:59:56Z

LGTM

mysticatea · 2016-10-27T09:02:52Z

lib/rules/indent.js

@@ -651,7 +651,7 @@ module.exports = {
            if (elements.length > 0 && node.type === "ArrayExpression") {
                elements.forEach(el => {
                    if (el.loc.start.line === node.loc.start.line) {
-                        return;
+                        return; // eslint-disable-line no-useless-return


Sorry, I'm not sure what the correct fix is.
I tried to change the forEach to a for-of statement, but several tests got failing.

I think we can just remove the entire elements.forEach block (see #7237 (comment))

eslintbot · 2016-10-27T22:08:11Z

LGTM

mysticatea · 2016-10-27T22:44:43Z

OK, ready to merge.
Thank you, �@not-an-aardvark .

not-an-aardvark and others added 4 commits October 23, 2016 14:23

New: (WIP) no-useless-return rule (fixes #7309)

6ac98c4

New: no-useless-return rule (fixes #7309)

14fcc7d

Chore: dogfooding no-useless-return rule

52f8391

Fix: remove useless upswash

daa7168

mysticatea added rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion feature This change adds a new feature to ESLint labels Oct 24, 2016

jquerybot added the CLA: Valid label Oct 24, 2016

not-an-aardvark suggested changes Oct 25, 2016

View reviewed changes

mysticatea added 2 commits October 27, 2016 12:28

add period.

f1ea642

isRemoveable → isRemovable

79e2dba

fix TypeError at return statements in unreachable code.

b079c82

not-an-aardvark approved these changes Oct 27, 2016

View reviewed changes

Merge branch 'master' into no-useless-return-7309

c2db41f

mysticatea mentioned this pull request Oct 27, 2016

fix: indent.js to support multiline array statements. #7237

Merged

temporary fix

bff4159

mysticatea commented Oct 27, 2016

View reviewed changes

remove useless block from indent.js

50b3d55

ilyavolodin approved these changes Oct 28, 2016

View reviewed changes

ilyavolodin merged commit d933516 into master Oct 28, 2016

mysticatea deleted the no-useless-return-7309 branch October 29, 2016 10:58

not-an-aardvark mentioned this pull request Nov 21, 2016

no-useless-return clashes with callback-return when return is not referencing the callback #7626

Closed

eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New: `no-useless-return` rule (fixes #7309) #7441

New: `no-useless-return` rule (fixes #7309) #7441

mysticatea commented Oct 24, 2016

mention-bot commented Oct 24, 2016

eslintbot commented Oct 24, 2016

not-an-aardvark left a comment

not-an-aardvark Oct 24, 2016

not-an-aardvark Oct 24, 2016

not-an-aardvark Oct 25, 2016

mysticatea Oct 27, 2016

eslintbot commented Oct 27, 2016

eslintbot commented Oct 27, 2016

not-an-aardvark left a comment

eslintbot commented Oct 27, 2016

mysticatea Oct 27, 2016

not-an-aardvark Oct 27, 2016

eslintbot commented Oct 27, 2016

mysticatea commented Oct 27, 2016

		@@ -0,0 +1,83 @@
		# Disallow redundant return statements (no-useless-return)

		A `return;` statement with nothing after it is redundant, and has no effect on the runtime behavior of a function. This can be confusing, so it's better to disallow these redundant statements

New: no-useless-return rule (fixes #7309) #7441

New: no-useless-return rule (fixes #7309) #7441

Conversation

mysticatea commented Oct 24, 2016

mention-bot commented Oct 24, 2016

eslintbot commented Oct 24, 2016

not-an-aardvark left a comment

Choose a reason for hiding this comment

not-an-aardvark Oct 24, 2016

Choose a reason for hiding this comment

not-an-aardvark Oct 24, 2016

Choose a reason for hiding this comment

not-an-aardvark Oct 25, 2016

Choose a reason for hiding this comment

mysticatea Oct 27, 2016

Choose a reason for hiding this comment

eslintbot commented Oct 27, 2016

eslintbot commented Oct 27, 2016

not-an-aardvark left a comment

Choose a reason for hiding this comment

eslintbot commented Oct 27, 2016

mysticatea Oct 27, 2016

Choose a reason for hiding this comment

not-an-aardvark Oct 27, 2016

Choose a reason for hiding this comment

eslintbot commented Oct 27, 2016

mysticatea commented Oct 27, 2016

New: `no-useless-return` rule (fixes #7309) #7441

New: `no-useless-return` rule (fixes #7309) #7441