Add consistent-destructuring rule (#325)

Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
Co-authored-by: fisker <lionkay@gmail.com>

docs/rules/consistent-destructuring.md

@@ -0,0 +1,52 @@
+# Use destructured variables over properties
+
+Enforces the use of already destructured objects and their variables over accessing each property individually. Previous destructurings are easily missed which leads to an inconsistent code style.
+
+This rule is partly fixable. It does not fix nested destructuring.
+
+## Fail
+
+```js
+const {a} = foo;
+console.log(a, foo.b);
+```
+
+```js
+const {a} = foo;
+console.log(foo.a); 
+```
+
+```js
+const {
+	a: {
+		b
+	}
+} = foo;
+console.log(foo.a.c);
+```
+
+```js
+const {bar} = foo;
+const {a} = foo.bar;
+```
+
+## Pass
+
+```js
+const {a} = foo;
+console.log(a);
+```
+
+```js
+console.log(foo.a, foo.b);
+```
+
+```js
+const {a} = foo;
+console.log(a, foo.b());
+```
+
+```js
+const {a} = foo.bar;
+console.log(foo.bar);
+```

index.js

@@ -41,6 +41,7 @@ module.exports = {
 			rules: {
 				'unicorn/better-regex': 'error',
 				'unicorn/catch-error-name': 'error',
+				'unicorn/consistent-destructuring': 'error',
 				'unicorn/consistent-function-scoping': 'error',
 				'unicorn/custom-error-definition': 'off',
 				'unicorn/empty-brace-spaces': 'error',

readme.md

@@ -35,6 +35,7 @@ Configure it in `package.json`.
 		"rules": {
 			"unicorn/better-regex": "error",
 			"unicorn/catch-error-name": "error",
+			"unicorn/consistent-destructuring": "error",
 			"unicorn/consistent-function-scoping": "error",
 			"unicorn/custom-error-definition": "off",
 			"unicorn/empty-brace-spaces": "error",
@@ -107,6 +108,7 @@ Configure it in `package.json`.
 
 - [better-regex](docs/rules/better-regex.md) - Improve regexes by making them shorter, consistent, and safer. *(fixable)*
 - [catch-error-name](docs/rules/catch-error-name.md) - Enforce a specific parameter name in catch clauses. *(fixable)*
+- [consistent-destructuring](docs/rules/consistent-destructuring.md) - Use destructured variables over properties. *(partly fixable)*
 - [consistent-function-scoping](docs/rules/consistent-function-scoping.md) - Move function definitions to the highest possible scope.
 - [custom-error-definition](docs/rules/custom-error-definition.md) - Enforce correct `Error` subclassing. *(fixable)*
 - [empty-brace-spaces](docs/rules/empty-brace-spaces.md) - Enforce no spaces between braces. *(fixable)*

rules/consistent-destructuring.js

@@ -0,0 +1,169 @@
+'use strict';
+const avoidCapture = require('./utils/avoid-capture');
+const getDocumentationUrl = require('./utils/get-documentation-url');
+
+const MESSAGE_ID = 'consistentDestructuring';
+const MESSAGE_ID_SUGGEST = 'consistentDestructuringSuggest';
+
+const declaratorSelector = [
+	'VariableDeclarator',
+	'[id.type="ObjectPattern"]',
+	'[init]',
+	'[init.type!="Literal"]'
+].join('');
+
+const memberSelector = [
+	'MemberExpression',
+	'[computed=false]',
+	':not(',
+	'AssignmentExpression > MemberExpression.left,',
+	'CallExpression > MemberExpression.callee,',
+	'NewExpression > MemberExpression.callee,',
+	'UpdateExpression > MemberExpression.argument,',
+	'UnaryExpression[operator="delete"] > MemberExpression.argument',
+	')'
+].join('');
+
+const isSimpleExpression = expression => {
+	while (expression) {
+		if (expression.computed) {
+			return false;
+		}
+
+		if (expression.type !== 'MemberExpression') {
+			break;
+		}
+
+		expression = expression.object;
+	}
+
+	return expression.type === 'Identifier' ||
+		expression.type === 'ThisExpression';
+};
+
+const isChildInParentScope = (child, parent) => {
+	while (child) {
+		if (child === parent) {
+			return true;
+		}
+
+		child = child.upper;
+	}
+
+	return false;
+};
+
+const create = context => {
+	const {ecmaVersion} = context.parserOptions;
+	const source = context.getSourceCode();
+	const declarations = new Map();
+
+	return {
+		[declaratorSelector]: node => {
+			// Ignore any complex expressions (e.g. arrays, functions)
+			if (!isSimpleExpression(node.init)) {
+				return;
+			}
+
+			declarations.set(source.getText(node.init), {
+				scope: context.getScope(),
+				variables: context.getDeclaredVariables(node),
+				objectPattern: node.id
+			});
+		},
+		[memberSelector]: node => {
+			const declaration = declarations.get(source.getText(node.object));
+
+			if (!declaration) {
+				return;
+			}
+
+			const {scope, objectPattern} = declaration;
+			const memberScope = context.getScope();
+
+			// Property is destructured outside the current scope
+			if (!isChildInParentScope(memberScope, scope)) {
+				return;
+			}
+
+			const destructurings = objectPattern.properties.filter(property =>
+				property.type === 'Property' &&
+				property.key.type === 'Identifier' &&
+				property.value.type === 'Identifier'
+			);
+			const lastProperty = objectPattern.properties[objectPattern.properties.length - 1];
+			const hasRest = lastProperty && lastProperty.type === 'RestElement';
+
+			const expression = source.getText(node);
+			const member = source.getText(node.property);
+
+			// Member might already be destructured
+			const destructuredMember = destructurings.find(property =>
+				property.key.name === member
+			);
+
+			if (!destructuredMember) {
+				// Don't destructure additional members when rest is used
+				if (hasRest) {
+					return;
+				}
+
+				// Destructured member collides with an existing identifier
+				if (avoidCapture(member, [memberScope], ecmaVersion) !== member) {
+					return;
+				}
+			}
+
+			// Don't try to fix nested member expressions
+			if (node.parent.type === 'MemberExpression') {
+				context.report({
+					node,
+					messageId: MESSAGE_ID
+				});
+
+				return;
+			}
+
+			const newMember = destructuredMember ? destructuredMember.value.name : member;
+
+			context.report({
+				node,
+				messageId: MESSAGE_ID,
+				suggest: [{
+					messageId: MESSAGE_ID_SUGGEST,
+					data: {
+						expression,
+						property: newMember
+					},
+					* fix(fixer) {
+						const {properties} = objectPattern;
+						const lastProperty = properties[properties.length - 1];
+
+						yield fixer.replaceText(node, newMember);
+
+						if (!destructuredMember) {
+							yield lastProperty ?
+								fixer.insertTextAfter(lastProperty, `, ${newMember}`) :
+								fixer.replaceText(objectPattern, `{${newMember}}`);
+						}
+					}
+				}]
+			});
+		}
+	};
+};
+
+module.exports = {
+	create,
+	meta: {
+		type: 'suggestion',
+		docs: {
+			url: getDocumentationUrl(__filename)
+		},
+		fixable: 'code',
+		messages: {
+			[MESSAGE_ID]: 'Use destructured variables over properties.',
+			[MESSAGE_ID_SUGGEST]: 'Replace `{{expression}}` with destructured property `{{property}}`.'
+		}
+	}
+};

rules/custom-error-definition.js

@@ -23,10 +23,10 @@ const hasValidSuperClass = node => {
 		return false;
 	}
 
-	let {name} = node.superClass;
+	let {name, type, property} = node.superClass;
 
-	if (node.superClass.type === 'MemberExpression') {
-		({name} = node.superClass.property);
+	if (type === 'MemberExpression') {
+		({name} = property);
 	}
 
 	return nameRegexp.test(name);
@@ -87,16 +87,16 @@ const customErrorDefinition = (context, node) => {
 		});
 	}
 
-	const {body} = node.body;
+	const {body, range} = node.body;
 	const constructor = body.find(x => x.kind === 'constructor');
 
 	if (!constructor) {
 		context.report({
 			node,
 			message: 'Add a constructor to your error.',
 			fix: fixer => fixer.insertTextAfterRange([
-				node.body.range[0],
-				node.body.range[0] + 1
+				range[0],
+				range[0] + 1
 			], getConstructorMethod(name))
 		});
 		return;
@@ -144,7 +144,7 @@ const customErrorDefinition = (context, node) => {
 
 	const nameExpression = constructorBody.find(x => isAssignmentExpression(x, 'name'));
 	if (!nameExpression) {
-		const nameProperty = node.body.body.find(node => isClassProperty(node, 'name'));
+		const nameProperty = body.find(node => isClassProperty(node, 'name'));
 
 		if (!nameProperty || !nameProperty.value || nameProperty.value.value !== name) {
 			context.report({

rules/new-for-builtins.js

@@ -14,15 +14,15 @@ const disallowNew = new Set(builtins.disallowNew);
 const create = context => {
 	return {
 		CallExpression: node => {
-			const {callee} = node;
+			const {callee, parent} = node;
 			const {name} = callee;
 
 			if (
 				name === 'Object' &&
-				node.parent &&
-				node.parent.type === 'BinaryExpression' &&
-				(node.parent.operator === '===' || node.parent.operator === '!==') &&
-				(node.parent.left === node || node.parent.right === node)
+				parent &&
+				parent.type === 'BinaryExpression' &&
+				(parent.operator === '===' || parent.operator === '!==') &&
+				(parent.left === node || parent.right === node)
 			) {
 				return;
 			}
@@ -37,8 +37,8 @@ const create = context => {
 			}
 		},
 		NewExpression: node => {
-			const {callee} = node;
-			const {name} = callee;
+			const {callee, range} = node;
+			const {name, range: calleeRange} = callee;
 
 			if (disallowNew.has(name) && !isShadowed(context.getScope(), callee)) {
 				const problem = {
@@ -49,8 +49,8 @@ const create = context => {
 
 				if (name !== 'String' && name !== 'Boolean' && name !== 'Number') {
 					problem.fix = fixer => fixer.removeRange([
-						node.range[0],
-						node.callee.range[0]
+						range[0],
+						calleeRange[0]
 					]);
 				}
 

rules/no-for-loop.js

@@ -283,7 +283,7 @@ const getSingularName = originalName => {
 
 const create = context => {
 	const sourceCode = context.getSourceCode();
-	const {scopeManager} = sourceCode;
+	const {scopeManager, text: sourceCodeText} = sourceCode;
 
 	return {
 		ForStatement(node) {
@@ -378,7 +378,7 @@ const create = context => {
 						if (removeDeclaration) {
 							declarationType = element.type === 'VariableDeclarator' ? elementNode.kind : elementNode.parent.kind;
 							if (elementNode.id.typeAnnotation && shouldGenerateIndex) {
-								declarationElement = sourceCode.text.slice(elementNode.id.range[0], elementNode.id.typeAnnotation.range[0]).trim();
+								declarationElement = sourceCodeText.slice(elementNode.id.range[0], elementNode.id.typeAnnotation.range[0]).trim();
 								typeAnnotation = sourceCode.getText(
 									elementNode.id.typeAnnotation,
 									-1 // Skip leading `:`