From d504f7536210f1b81991cac98ba4d3a9bd203da3 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Fri, 22 Apr 2016 11:09:40 +0300 Subject: [PATCH] New: Rule - 'no-unsafe-finally' (fixes #5808) --- conf/eslint.json | 1 + docs/rules/README.md | 1 + docs/rules/no-unsafe-finally.md | 134 +++++++++++++++++++++++++++ lib/rules/no-unsafe-finally.js | 78 ++++++++++++++++ tests/lib/rules/no-unsafe-finally.js | 87 +++++++++++++++++ 5 files changed, 301 insertions(+) create mode 100644 docs/rules/no-unsafe-finally.md create mode 100644 lib/rules/no-unsafe-finally.js create mode 100644 tests/lib/rules/no-unsafe-finally.js diff --git a/conf/eslint.json b/conf/eslint.json index 957101cc846a..33336a504f66 100755 --- a/conf/eslint.json +++ b/conf/eslint.json @@ -109,6 +109,7 @@ "no-unmodified-loop-condition": "off", "no-unneeded-ternary": "off", "no-unreachable": "error", + "no-unsafe-finally": "off", "no-unused-expressions": "off", "no-unused-labels": "error", "no-unused-vars": "error", diff --git a/docs/rules/README.md b/docs/rules/README.md index 9431a91ce638..ccd0282d3464 100644 --- a/docs/rules/README.md +++ b/docs/rules/README.md @@ -35,6 +35,7 @@ These rules relate to possible syntax or logic errors in JavaScript code: * [no-sparse-arrays](no-sparse-arrays.md): disallow sparse arrays (recommended) * [no-unexpected-multiline](no-unexpected-multiline.md): disallow confusing multiline expressions (recommended) * [no-unreachable](no-unreachable.md): disallow unreachable code after `return`, `throw`, `continue`, and `break` statements (recommended) +* [no-unsafe-finally](no-unsafe-finally.md): disallow control flow statements in `finally` blocks * [use-isnan](use-isnan.md): require calls to `isNaN()` when checking for `NaN` (recommended) * [valid-jsdoc](valid-jsdoc.md): enforce valid JSDoc comments * [valid-typeof](valid-typeof.md): enforce comparing `typeof` expressions against valid strings (recommended) diff --git a/docs/rules/no-unsafe-finally.md b/docs/rules/no-unsafe-finally.md new file mode 100644 index 000000000000..259e480a3a63 --- /dev/null +++ b/docs/rules/no-unsafe-finally.md @@ -0,0 +1,134 @@ +# disallow control flow statements in `finally` blocks (no-unsafe-finally) + +Javascript suspends the control flow statements of `try` and `catch` blocks until the execution of `finally` block finishes. So, when a control flow statement is used in a `finally` block, it overwrites the previous statements, which is considered as unexpected behavior. Examples of this situation: + +When `return` is used: + +```js +(() => { + try { + return 1; + } catch(err) { + return 2; + } finally { + return 3; + } +})(); + +// > 3 +``` + +When `throw` is used: + +```js +(() => { + try { + throw new Error("Try"); + } catch(err) { + throw err; + } finally { + throw new Error("Finally"); + } +})(); + +// > Uncaught Error: Finally(...) +``` + +When `break` is used: + +```js +(() => { + label: try { + return 0; + } finally { + break label; + } + return 1; +})(); + +// > 1 +``` + +## Rule Details + +This rule disallows `return`, `throw`, `break` and `continue` statements inside `finally` blocks. It allows indirect usages such as in `function` or `class` declarations / expressions etc. + +Examples of **incorrect** code for this rule: + +```js +/*eslint no-unsafe-finally: "error"*/ +let foo = function() { + try { + return 1; + } catch(err) { + return 2; + } finally { + return 3; + } +}; +``` + +```js +/*eslint no-unsafe-finally: "error"*/ +let foo = function() { + try { + return 1; + } catch(err) { + return 2; + } finally { + throw new Error; + } +}; +``` + +Examples of **correct** code for this rule: + +```js +/*eslint no-unsafe-finally: "error"*/ +let foo = function() { + try { + return 1; + } catch(err) { + return 2; + } finally { + console.log("hola!"); + } +}; +``` + +```js +/*eslint no-unsafe-finally: "error"*/ +let foo = function() { + try { + return 1; + } catch(err) { + return 2; + } finally { + let a = function() { + return "hola!"; + } + } +}; +``` + +```js +/*eslint no-unsafe-finally: "error"*/ +let foo = function(a) { + try { + return 1; + } catch(err) { + return 2; + } finally { + switch(a) { + case 1: { + console.log("hola!") + break; + } + } + } +}; +``` + +## When Not To Use It + +If you want to allow control flow operations in finally blocks, you can turn this rule off. diff --git a/lib/rules/no-unsafe-finally.js b/lib/rules/no-unsafe-finally.js new file mode 100644 index 000000000000..55ea2971f88e --- /dev/null +++ b/lib/rules/no-unsafe-finally.js @@ -0,0 +1,78 @@ +/** + * @fileoverview Rule to flag unsafe statements in finally block + * @author Onur Temizkan + */ + +"use strict"; + +//------------------------------------------------------------------------------ +// Helpers +//------------------------------------------------------------------------------ + +var SENTINEL_NODE_TYPE = /^(?:Program|(?:Function|Class)(?:Declaration|Expression)|ArrowFunctionExpression)$/; + +//------------------------------------------------------------------------------ +// Rule Definition +//------------------------------------------------------------------------------ + +module.exports = { + meta: { + docs: { + description: "disallow control flow statements in finally blocks", + category: "Possible Errors", + recommended: false + } + }, + create: function(context) { + + /** + * Checks if the node is the finalizer of a TryStatement + * + * @param {ASTNode} node - node to check. + * @returns {Boolean} - true if the node is the finalizer of a TryStatement + */ + function isFinallyBlock(node) { + return node.parent.type === "TryStatement" && node.parent.finalizer === node; + } + + /** + * Climbs up the tree if the node is not a sentinel node + * + * @param {ASTNode} node - node to check. + * @returns {Boolean} - return whether the node is a finally block or a sentinel node + */ + function isInFinallyBlock(node) { + while (node && !SENTINEL_NODE_TYPE.test(node.type)) { + if (isFinallyBlock(node)) { + return true; + } + node = node.parent; + } + return false; + } + + /** + * Checks whether the possibly-unsafe statement is inside a finally block. + * + * @param {ASTNode} node - node to check. + * @returns {void} + */ + function check(node) { + if (isInFinallyBlock(node)) { + context.report({ + message: "Unsafe usage of " + node.type, + node: node, + line: node.loc.line, + column: node.loc.column + }); + } + } + + return { + ReturnStatement: check, + ThrowStatement: check, + BreakStatement: check, + ContinueStatement: check + }; + } +}; diff --git a/tests/lib/rules/no-unsafe-finally.js b/tests/lib/rules/no-unsafe-finally.js new file mode 100644 index 000000000000..4a4a705c0d28 --- /dev/null +++ b/tests/lib/rules/no-unsafe-finally.js @@ -0,0 +1,87 @@ +/** + * @fileoverview Tests for no-unpredictable-finally + * @author Onur Temizkan + */ + +"use strict"; + +//------------------------------------------------------------------------------ +// Requirements +//------------------------------------------------------------------------------ + +var rule = require("../../../lib/rules/no-unsafe-finally"), + RuleTester = require("../../../lib/testers/rule-tester"); + +//------------------------------------------------------------------------------ +// Tests +//------------------------------------------------------------------------------ + +var ruleTester = new RuleTester(); + +ruleTester.run("no-unsafe-finally", rule, { + valid: [ + "var foo = function() {\n try { \n return 1; \n } catch(err) { \n return 2; \n } finally { \n console.log('hola!') \n } \n }", + "var foo = function() { try { return 1 } catch(err) { return 2 } finally { console.log('hola!') } }", + "var foo = function() { try { return 1 } catch(err) { return 2 } finally { function a(x) { return x } } }", + "var foo = function() { try { return 1 } catch(err) { return 2 } finally { var a = function(x) { if(!x) { throw new Error() } } } }", + "var foo = function() { try { return 1 } catch(err) { return 2 } finally { var a = function(x) { while(true) { if(x) { break } else { continue } } } } }", + { + code: "var foo = function() { try { return 1; } catch(err) { return 2; } finally { var bar = () => { throw new Error(); }; } };", + parserOptions: { ecmaVersion: 6 } + }, + { + code: "var foo = function() { try { return 1; } catch(err) { return 2 } finally { (x) => x } }", + parserOptions: { ecmaVersion: 6 } + }, + + { + code: "var foo = function() { try { return 1; } finally { class bar { constructor() {} static ehm() { return 'Hola!'; } } } };", + parserOptions: { ecmaVersion: 6 } + } + + ], + invalid: [ + { + code: "var foo = function() { \n try { \n return 1; \n } catch(err) { \n return 2; \n } finally { \n return 3; \n } \n }", + errors: [{ message: "Unsafe usage of ReturnStatement", type: "ReturnStatement", line: 7, column: 2 }] + }, + { + code: "var foo = function() { try { return 1 } catch(err) { return 2 } finally { if(true) { return 3 } else { return 2 } } }", + errors: [ + { message: "Unsafe usage of ReturnStatement", type: "ReturnStatement", line: 1, column: 86 }, + { message: "Unsafe usage of ReturnStatement", type: "ReturnStatement", line: 1, column: 104 } + ] + }, + { + code: "var foo = function() { try { return 1 } catch(err) { return 2 } finally { return 3 } }", + errors: [{ message: "Unsafe usage of ReturnStatement", type: "ReturnStatement", line: 1, column: 75 }] + }, + { + code: "var foo = function() { try { return 1 } catch(err) { return 2 } finally { return function(x) { return y } } }", + errors: [{ message: "Unsafe usage of ReturnStatement", type: "ReturnStatement", line: 1, column: 75 }] + }, + { + code: "var foo = function() { try { return 1 } catch(err) { return 2 } finally { return { x: function(c) { return c } } } }", + errors: [{ message: "Unsafe usage of ReturnStatement", type: "ReturnStatement", line: 1, column: 75 }] + }, + { + code: "var foo = function() { try { return 1 } catch(err) { return 2 } finally { throw new Error() } }", + errors: [{ message: "Unsafe usage of ThrowStatement", type: "ThrowStatement", line: 1, column: 75 }] + }, + { + code: "var foo = function() { try { return 1 } catch(err) { return 2 } finally { while(true) { if(x) { break } else { continue } } } }", + errors: [ + { message: "Unsafe usage of BreakStatement", type: "BreakStatement", line: 1, column: 97 }, + { message: "Unsafe usage of ContinueStatement", type: "ContinueStatement", line: 1, column: 112 } + ] + }, + { + code: "var foo = function() { try { foo(); } finally { try { bar(); } finally { return; } } };", + errors: [{ message: "Unsafe usage of ReturnStatement", type: "ReturnStatement", line: 1, column: 74 }] + }, + { + code: "var foo = function() { label: try { return 0; } finally { break label; } return 1; }", + errors: [{ message: "Unsafe usage of BreakStatement", type: "BreakStatement", line: 1, column: 59}] + } + ] +});