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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .eslintrc.js
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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."
});
}
}
}
};
}
};