-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
eslint-plugin: Add rule no-unused-vars-before-return
#12828
Changes from all commits
cb3da2a
cc4642f
bdab122
31a46bf
034c075
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
# Avoid assigning variable values if unused before a return (no-unused-vars-before-return) | ||
|
||
To avoid wastefully computing the result of a function call when assigning a variable value, the variable should be declared as late as possible. In practice, if a function includes an early return path, any variable declared prior to the `return` must be referenced at least once. Otherwise, in the condition which satisfies that return path, the declared variable is effectively considered dead code. This can have a performance impact when the logic performed in assigning the value is non-trivial. | ||
|
||
## Rule details | ||
|
||
The following patterns are considered warnings: | ||
|
||
```js | ||
function example( number ) { | ||
const foo = doSomeCostlyOperation(); | ||
if ( number > 10 ) { | ||
return number + 1; | ||
} | ||
|
||
return number + foo; | ||
} | ||
``` | ||
|
||
The following patterns are not considered warnings: | ||
|
||
```js | ||
function example( number ) { | ||
if ( number > 10 ) { | ||
return number + 1; | ||
} | ||
|
||
const foo = doSomeCostlyOperation(); | ||
return number + foo; | ||
} | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
module.exports = { | ||
configs: require( './configs' ), | ||
rules: require( './rules' ), | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
/** | ||
gziolo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* External dependencies | ||
*/ | ||
import { RuleTester } from 'eslint'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import rule from '../no-unused-vars-before-return'; | ||
|
||
const ruleTester = new RuleTester( { | ||
parserOptions: { | ||
ecmaVersion: 6, | ||
}, | ||
} ); | ||
|
||
ruleTester.run( 'no-unused-vars-before-return', rule, { | ||
valid: [ | ||
{ | ||
code: ` | ||
function example( number ) { | ||
if ( number > 10 ) { | ||
return number + 1; | ||
} | ||
|
||
const foo = doSomeCostlyOperation(); | ||
return number + foo; | ||
}`, | ||
}, | ||
], | ||
invalid: [ | ||
{ | ||
code: ` | ||
function example( number ) { | ||
const foo = doSomeCostlyOperation(); | ||
if ( number > 10 ) { | ||
return number + 1; | ||
} | ||
|
||
return number + foo; | ||
}`, | ||
errors: [ { message: 'Variables should not be assigned until just prior its first reference. An early return statement may leave this variable unused.' } ], | ||
}, | ||
], | ||
} ); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
module.exports = require( 'requireindex' )( __dirname ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it equivalent to re-exporting all files in the folder? just being curious :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It’s equivalent to something like: module.exports = {
foo: require( './foo' ),
bar: require( './bar' ),
baz: require( './baz' ),
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
module.exports = { | ||
meta: { | ||
type: 'problem', | ||
schema: [], | ||
}, | ||
create( context ) { | ||
return { | ||
ReturnStatement( node ) { | ||
let functionScope = context.getScope(); | ||
while ( functionScope.type !== 'function' && functionScope.upper ) { | ||
functionScope = functionScope.upper; | ||
} | ||
|
||
if ( ! functionScope ) { | ||
return; | ||
} | ||
|
||
for ( const variable of functionScope.variables ) { | ||
const declaratorCandidate = variable.defs.find( ( def ) => { | ||
return ( | ||
def.node.type === 'VariableDeclarator' && | ||
// Allow declarations which are not initialized. | ||
def.node.init && | ||
// Target function calls as "expensive". | ||
def.node.init.type === 'CallExpression' && | ||
// Allow unused if part of an object destructuring. | ||
def.node.id.type !== 'ObjectPattern' && | ||
// Only target assignments preceding `return`. | ||
def.node.end < node.end | ||
); | ||
} ); | ||
|
||
if ( ! declaratorCandidate ) { | ||
continue; | ||
} | ||
|
||
// The first entry in `references` is the declaration | ||
// itself, which can be ignored. | ||
const isUsedBeforeReturn = variable.references.slice( 1 ).some( ( reference ) => { | ||
return reference.identifier.end < node.end; | ||
} ); | ||
|
||
if ( isUsedBeforeReturn ) { | ||
continue; | ||
} | ||
|
||
context.report( | ||
declaratorCandidate.node, | ||
'Variables should not be assigned until just prior its first reference. ' + | ||
'An early return statement may leave this variable unused.' | ||
); | ||
} | ||
}, | ||
}; | ||
}, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reads nicely 👍