Skip to content
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

Chore: avoid useless catch clauses that just rethrow errors #10010

Merged
merged 1 commit into from Feb 24, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion .eslintrc.js
Expand Up @@ -25,6 +25,7 @@ module.exports = {
"eslint-plugin/report-message-format": ["error", "[^a-z].*\\.$"],
"eslint-plugin/test-case-property-ordering": "error",
"eslint-plugin/test-case-shorthand-strings": "error",
"rulesdir/multiline-comment-style": "error"
"rulesdir/multiline-comment-style": "error",
"rulesdir/no-useless-catch": "error"
}
};
2 changes: 0 additions & 2 deletions tests/lib/testers/no-test-runners.js
Expand Up @@ -27,8 +27,6 @@ try {
]
});
}, /Output is incorrect\. \(' foo = bar;' == 'invalid output'\)$/);
} catch (e) {
throw e;
} finally {
it = tmpIt;
describe = tmpDescribe;
Expand Down
62 changes: 62 additions & 0 deletions tests/tools/internal-rules/no-useless-catch.js
@@ -0,0 +1,62 @@
/**
* @fileoverview Tests for no-useless-throw rule
* @author Teddy Katz
*/

"use strict";

//------------------------------------------------------------------------------
// Requirements
//------------------------------------------------------------------------------

const rule = require("../../../tools/internal-rules/no-useless-catch");
const RuleTester = require("../../../lib/testers/rule-tester");

//------------------------------------------------------------------------------
// Tests
//------------------------------------------------------------------------------

new RuleTester({ parserOptions: { ecmaVersion: 6 } }).run("rulesdir/no-useless-catch", rule, {
valid: [
`
try {
foo();
} catch (err) {
console.error(err);
} finally {
foo;
}
`,
`
try {
foo();
} catch ({ err }) {
throw err;
}
`
],
invalid: [
{
code: `
try {
foo();
} catch (e) {
throw e;
}
`,
errors: [{ message: "Unnecessary try/catch wrapper." }]
},
{
code: `
try {
foo();
} catch (e) {
throw e;
} finally {
foo();
}
`,
errors: [{ message: "Unnecessary catch clause." }]
}
]
});
44 changes: 44 additions & 0 deletions tools/internal-rules/no-useless-catch.js
@@ -0,0 +1,44 @@
/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like it shouldn't be an internal rule. It's useful in general, and not specific to ESLint. Granted, I'm not sure if it's important enough to be in the core, but maybe a separate plugin?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I would be fine with adding it as a core rule. I just made it an internal rule for now so that it doesn't need to wait for approval.

* @fileoverview Reports useless `catch` clauses that just rethrow their error.
* @author Teddy Katz
*/

"use strict";

module.exports = {
meta: {
docs: {
description: "disallow unnecessary `catch` clauses",
category: "Internal",
recommended: false
},

schema: []
},

create(context) {
return {
CatchClause(node) {
if (
node.param.type === "Identifier" &&
node.body.body.length &&
node.body.body[0].type === "ThrowStatement" &&
node.body.body[0].argument.type === "Identifier" &&
node.body.body[0].argument.name === node.param.name
) {
if (node.parent.finalizer) {
context.report({
node,
message: "Unnecessary catch clause."
});
} else {
context.report({
node,
message: "Unnecessary try/catch wrapper."
});
}
}
}
};
}
};