From efdb03af14999b3d99202fe1d611415d64669e38 Mon Sep 17 00:00:00 2001 From: fisker Cheung Date: Fri, 1 May 2020 15:39:54 +0800 Subject: [PATCH] Add `prefer-optional-catch-binding` rule (#671) --- docs/rules/prefer-optional-catch-binding.md | 23 +++ index.js | 1 + readme.md | 2 + rules/better-regex.js | 2 +- rules/expiring-todo-comments.js | 2 +- rules/prefer-optional-catch-binding.js | 61 ++++++++ test/package.js | 2 +- test/prefer-optional-catch-binding.js | 164 ++++++++++++++++++++ 8 files changed, 254 insertions(+), 3 deletions(-) create mode 100644 docs/rules/prefer-optional-catch-binding.md create mode 100644 rules/prefer-optional-catch-binding.js create mode 100644 test/prefer-optional-catch-binding.js diff --git a/docs/rules/prefer-optional-catch-binding.md b/docs/rules/prefer-optional-catch-binding.md new file mode 100644 index 0000000000..9cb0ee1467 --- /dev/null +++ b/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); +} +``` diff --git a/index.js b/index.js index af012ef99b..30e82b5f05 100644 --- a/index.js +++ b/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. diff --git a/readme.md b/readme.md index d4ee63c7b9..511be35d99 100644 --- a/readme.md +++ b/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)* diff --git a/rules/better-regex.js b/rules/better-regex.js index 22b635ce6b..1d0241074f 100644 --- a/rules/better-regex.js +++ b/rules/better-regex.js @@ -29,7 +29,7 @@ const create = context => { try { optimized = optimize(original, undefined, {blacklist}).toString(); - } catch (_) {} + } catch {} if (original === optimized) { return; diff --git a/rules/expiring-todo-comments.js b/rules/expiring-todo-comments.js index aa893fef75..2c3c1f6106 100644 --- a/rules/expiring-todo-comments.js +++ b/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; } } diff --git a/rules/prefer-optional-catch-binding.js b/rules/prefer-optional-catch-binding.js new file mode 100644 index 0000000000..84900f7933 --- /dev/null +++ b/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}}`.' + } + } +}; diff --git a/test/package.js b/test/package.js index 0bb511bb68..4020fcd59e 100644 --- a/test/package.js +++ b/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": (?{.*?})/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'); diff --git a/test/prefer-optional-catch-binding.js b/test/prefer-optional-catch-binding.js new file mode 100644 index 0000000000..f4211803a0 --- /dev/null +++ b/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')] + } + ] +});