Add prefer-optional-catch-binding rule (#671)

docs/rules/prefer-optional-catch-binding.md

@@ -0,0 +1,23 @@
+# Prefer omitting the `catch` binding parameter
+
+If the `catch` binding parameter is not used, it should be omitted.
+
+This rule is fixable.
+
+## Fail
+
+```js
+try {} catch (usedError) {}
+```
+
+## Pass
+
+```js
+try {} catch {}
+```
+
+```js
+try {} catch (error) {
+	console.error(error);
+}
+```

index.js

@@ -55,6 +55,7 @@ module.exports = {
 				'unicorn/prefer-node-append': 'error',
 				'unicorn/prefer-node-remove': 'error',
 				'unicorn/prefer-number-properties': 'error',
+				'unicorn/prefer-optional-catch-binding': 'error',
 				'unicorn/prefer-query-selector': 'error',
 				'unicorn/prefer-reflect-apply': 'error',
 				// TODO: Enable this by default when it's shipping in a Node.js LTS version.

readme.md

@@ -71,6 +71,7 @@ Configure it in `package.json`.
 			"unicorn/prefer-node-append": "error",
 			"unicorn/prefer-node-remove": "error",
 			"unicorn/prefer-number-properties": "error",
+			"unicorn/prefer-optional-catch-binding": "error",
 			"unicorn/prefer-query-selector": "error",
 			"unicorn/prefer-reflect-apply": "error",
 			"unicorn/prefer-replace-all": "off",
@@ -128,6 +129,7 @@ Configure it in `package.json`.
 - [prefer-node-append](docs/rules/prefer-node-append.md) - Prefer `Node#append()` over `Node#appendChild()`. *(fixable)*
 - [prefer-node-remove](docs/rules/prefer-node-remove.md) - Prefer `childNode.remove()` over `parentNode.removeChild(childNode)`. *(fixable)*
 - [prefer-number-properties](docs/rules/prefer-number-properties.md) - Prefer `Number` static properties over global ones. *(fixable)*
+- [prefer-optional-catch-binding](docs/rules/prefer-optional-catch-binding.md) - Prefer omitting the `catch` binding parameter. *(fixable)*
 - [prefer-query-selector](docs/rules/prefer-query-selector.md) - Prefer `.querySelector()` over `.getElementById()`, `.querySelectorAll()` over `.getElementsByClassName()` and `.getElementsByTagName()`. *(partly fixable)*
 - [prefer-reflect-apply](docs/rules/prefer-reflect-apply.md) - Prefer `Reflect.apply()` over `Function#apply()`. *(fixable)*
 - [prefer-replace-all](docs/rules/prefer-replace-all.md) - Prefer `String#replaceAll()` over regex searches with the global flag. *(fixable)*

rules/better-regex.js

@@ -29,7 +29,7 @@ const create = context => {
 
 			try {
 				optimized = optimize(original, undefined, {blacklist}).toString();
-			} catch (_) {}
+			} catch {}
 
 			if (original === optimized) {
 				return;

rules/expiring-todo-comments.js

@@ -193,7 +193,7 @@ function tryToCoerceVersion(rawVersion) {
 		// Try to semver.parse a perfect match while semver.coerce tries to fix errors
 		// But coerce can't parse pre-releases.
 		return semver.parse(version) || semver.coerce(version);
-	} catch (_) {
+	} catch {
 		return false;
 	}
 }

rules/prefer-optional-catch-binding.js

@@ -0,0 +1,61 @@
+'use strict';
+const getDocumentationUrl = require('./utils/get-documentation-url');
+const {findVariable, isOpeningParenToken, isClosingParenToken} = require('eslint-utils');
+
+const ERROR_MESSAGE_ID = 'error';
+
+const selector = [
+	'CatchClause',
+	'>',
+	'Identifier.param'
+].join('');
+
+const create = context => {
+	return {
+		[selector]: node => {
+			const scope = context.getScope();
+			const variable = findVariable(scope, node);
+
+			if (variable.references.length !== 0) {
+				return;
+			}
+
+			const {name} = node;
+
+			context.report({
+				node,
+				messageId: ERROR_MESSAGE_ID,
+				data: {name},
+				fix: fixer => {
+					const tokenBefore = context.getTokenBefore(node);
+					const tokenAfter = context.getTokenAfter(node);
+
+					/* istanbul ignore next */
+					if (!isOpeningParenToken(tokenBefore) || !isClosingParenToken(tokenAfter)) {
+						throw new Error('Unexpected token.');
+					}
+
+					return [
+						tokenBefore,
+						node,
+						tokenAfter
+					].map(nodeOrToken => fixer.remove(nodeOrToken));
+				}
+			});
+		}
+	};
+};
+
+module.exports = {
+	create,
+	meta: {
+		type: 'suggestion',
+		docs: {
+			url: getDocumentationUrl(__filename)
+		},
+		fixable: 'code',
+		messages: {
+			[ERROR_MESSAGE_ID]: 'Remove unused catch binding `{{name}}`.'
+		}
+	}
+};

test/package.js

@@ -64,7 +64,7 @@ test('Every rule is defined in readme.md usage and list of rules in alphabetical
 		const usageRulesMatch = /## Usage.*?"rules": (?<rules>{.*?})/ms.exec(readme);
 		t.truthy(usageRulesMatch, 'List of rules should be defined in readme.md ## Usage');
 		usageRules = JSON.parse(usageRulesMatch.groups.rules);
-	} catch (_) {}
+	} catch {}
 
 	t.truthy(usageRules, 'List of rules should be defined in readme.md ## Usage and be valid JSON');
 

test/prefer-optional-catch-binding.js

@@ -0,0 +1,164 @@
+import test from 'ava';
+import avaRuleTester from 'eslint-ava-rule-tester';
+import {outdent} from 'outdent';
+import rule from '../rules/prefer-optional-catch-binding';
+
+const ERROR_MESSAGE_ID = 'error';
+const generateError = name => ({messageId: ERROR_MESSAGE_ID, data: {name}});
+
+const ruleTester = avaRuleTester(test, {
+	parserOptions: {
+		ecmaVersion: 2020
+	}
+});
+
+ruleTester.run('prefer-optional-catch-binding', rule, {
+	valid: [
+		'try {} catch {}',
+		outdent`
+			try {} catch {
+				error
+			}
+		`,
+		outdent`
+			try {} catch(used) {
+				console.error(used);
+			}
+		`,
+		outdent`
+			try {} catch(usedInADeeperScope) {
+				function foo() {
+					function bar() {
+						console.error(usedInADeeperScope);
+					}
+				}
+			}
+		`,
+		// We are not checking destructuring
+		'try {} catch({message}) {}',
+		'try {} catch({nonExistsProperty = thisWillExecute()}) {}'
+	],
+	invalid: [
+		{
+			code: 'try {} catch(_) {}',
+			output: 'try {} catch {}',
+			errors: [generateError('_')]
+		},
+		{
+			code: outdent`
+				try {} catch(foo) {
+					function bar(foo) {}
+				}
+			`,
+			output: outdent`
+				try {} catch {
+					function bar(foo) {}
+				}
+			`,
+			errors: [generateError('foo')]
+		},
+		// Many
+		{
+			code: outdent`
+				try {} catch(outer) {
+					try {} catch(inner) {
+					}
+				}
+				try {
+					try {} catch(inTry) {
+					}
+				} catch(another) {
+					try {} catch(inCatch) {
+					}
+				} finally {
+					try {} catch(inFinally) {
+					}
+				}
+			`,
+			output: outdent`
+				try {} catch {
+					try {} catch {
+					}
+				}
+				try {
+					try {} catch {
+					}
+				} catch {
+					try {} catch {
+					}
+				} finally {
+					try {} catch {
+					}
+				}
+			`,
+			errors: [
+				generateError('outer'),
+				generateError('inner'),
+				generateError('inTry'),
+				generateError('another'),
+				generateError('inCatch'),
+				generateError('inFinally')
+			]
+		},
+		// Actual message
+		{
+			code: 'try {} catch(theRealErrorName) {}',
+			output: 'try {} catch {}',
+			errors: [{message: 'Remove unused catch binding `theRealErrorName`.'}]
+		},
+		// TODO: this `error` should be able to remove
+		// {
+		// 	code: outdent`
+		// 		try {
+		// 		} catch(foo) {
+		// 			var foo = 1;
+		// 		}
+		// 	`,
+		// 	output: outdent`
+		// 		try {
+		// 		} catch {
+		// 			var foo = 1;
+		// 		}
+		// 	`,
+		// 	errors: [generateError('foo')]
+		// },
+		// Comments
+		{
+			code: outdent`
+				/* comment */
+				try {
+					/* comment */
+					// comment
+				} catch (
+					/* comment */
+					// comment
+					unused
+					/* comment */
+					// comment
+				) {
+					/* comment */
+					// comment
+				}
+				/* comment */
+			`,
+			output: outdent`
+				/* comment */
+				try {
+					/* comment */
+					// comment
+				} catch{SPACE}
+					/* comment */
+					// comment
+				{TAB}
+					/* comment */
+					// comment
+				 {
+					/* comment */
+					// comment
+				}
+				/* comment */
+			`.replace('{SPACE}', ' ').replace('{TAB}', '\t'),
+			errors: [generateError('unused')]
+		}
+	]
+});