eslint-plugin: Add rule no-unused-vars-before-return (#12828)

* scripts: Bump ESLint dependency to 5.10.x

* eslint-plugin: Add rule `no-unused-vars-before-return`

* eslint-plugin: Rephrase return-unused message for declarator

* Update error message in the unit test to reflect code changes

* Scripts: Include Migration Guide link for ESLint major bump CHANGELOG note

packages/eslint-plugin/CHANGELOG.md

@@ -4,6 +4,10 @@
 
 - The `esnext` and `recommended` rulesets now enforce [`object-shorthand`](https://eslint.org/docs/rules/object-shorthand)
 
+### New Features
+
+- New Rule: [`@wordpress/no-unused-vars-before-return`](https://github.com/WordPress/gutenberg/blob/master/packages/eslint-plugin/docs/rules/no-unused-vars-before-return.md)
+
 ## 1.0.0 (2018-12-12)
 
 ### New Features

packages/eslint-plugin/README.md

@@ -10,7 +10,7 @@ Install the module
 npm install @wordpress/eslint-plugin --save-dev
 ```
 
-### Usage
+## Usage
 
 To opt-in to the default configuration, extend your own project's `.eslintrc` file:
 
@@ -24,7 +24,7 @@ Refer to the [ESLint documentation on Shareable Configs](http://eslint.org/docs/
 
 The `recommended` preset will include rules governing an ES2015+ environment, and includes rules from the [`eslint-plugin-jsx-a11y`](https://github.com/evcohen/eslint-plugin-jsx-a11y) and [`eslint-plugin-react`](https://github.com/yannickcr/eslint-plugin-react) projects.
 
-#### Rulesets
+### Rulesets
 
 Alternatively, you can opt-in to only the more granular rulesets offered by the plugin. These include:
 
@@ -45,7 +45,13 @@ These rules can be used additively, so you could extend both `esnext` and `custo
 
 The granular rulesets will not define any environment globals. As such, if they are required for your project, you will need to define them yourself.
 
-#### Legacy
+### Rules
+
+Rule|Description
+---|---
+[no-unused-vars-before-return](docs/rules/no-unused-vars-before-return.md)|Disallow assigning variable values if unused before a return
+
+### Legacy
 
 If you are using WordPress' `.jshintrc` JSHint configuration and you would like to take the first step to migrate to an ESLint equivalent it is also possible to define your own project's `.eslintrc` file as:
 

packages/eslint-plugin/configs/custom.js

@@ -1,5 +1,9 @@
 module.exports = {
+	plugins: [
+		'@wordpress',
+	],
 	rules: {
+		'@wordpress/no-unused-vars-before-return': 'error',
 		'no-restricted-syntax': [
 			'error',
 			{

packages/eslint-plugin/docs/rules/no-unused-vars-before-return.md

@@ -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;
+}
+```

packages/eslint-plugin/index.js

@@ -1,3 +1,4 @@
 module.exports = {
 	configs: require( './configs' ),
+	rules: require( './rules' ),
 };

packages/eslint-plugin/rules/__tests__/no-unused-vars-before-return.js

@@ -0,0 +1,45 @@
+/**
+ * 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.' } ],
+		},
+	],
+} );

packages/eslint-plugin/rules/index.js

@@ -0,0 +1 @@
+module.exports = require( 'requireindex' )( __dirname );

packages/eslint-plugin/rules/no-unused-vars-before-return.js

@@ -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.'
+					);
+				}
+			},
+		};
+	},
+};

packages/scripts/CHANGELOG.md

@@ -1,4 +1,8 @@
-## 2.6.0 (Unreleased)
+## 3.0.0 (Unreleased)
+
+### Breaking Changes
+
+- The bundled `eslint` dependency has been updated from requiring `^4.19.1` to requiring `^5.12.1` (see [Migration Guide](https://eslint.org/docs/user-guide/migrating-to-5.0.0)).
 
 ### New Features
 

packages/scripts/package.json

@@ -38,7 +38,7 @@
 		"chalk": "^2.4.1",
 		"check-node-version": "^3.1.1",
 		"cross-spawn": "^5.1.0",
-		"eslint": "^4.19.1",
+		"eslint": "^5.12.1",
 		"jest": "^23.6.0",
 		"jest-puppeteer": "3.2.1",
 		"npm-package-json-lint": "^3.3.1",