From 3aa5fbfa41e687d45ed595d4daa70f71458d9805 Mon Sep 17 00:00:00 2001 From: Pig Fang Date: Tue, 5 Nov 2019 17:51:43 +0800 Subject: [PATCH 01/13] New: new rule no-constructor-return (fixes #12481) --- docs/rules/no-constructor-return.md | 49 ++++++++++++++++++++++ lib/rules/index.js | 1 + lib/rules/no-constructor-return.js | 52 ++++++++++++++++++++++++ tests/lib/rules/no-constructor-return.js | 46 +++++++++++++++++++++ tools/rule-types.json | 1 + 5 files changed, 149 insertions(+) create mode 100644 docs/rules/no-constructor-return.md create mode 100644 lib/rules/no-constructor-return.js create mode 100644 tests/lib/rules/no-constructor-return.js diff --git a/docs/rules/no-constructor-return.md b/docs/rules/no-constructor-return.md new file mode 100644 index 00000000000..327ab0cf404 --- /dev/null +++ b/docs/rules/no-constructor-return.md @@ -0,0 +1,49 @@ +# Disallow returning value in constructor (no-constructor-return) + +In JavaScript returning value in constructor is allowed, however this looks meaningless. Forbidding this pattern prevents mistake resulting from unfamiliarity with the language or copy-paste error. + +Note that returning nothing with flow control is allowed. + +## Rule Details + +This rule disallows return statement in constructor of a class. + +Examples of **incorrect** code for this rule: + +```js +/*eslint no-constructor-return: "error"*/ + +class A { + constructor() { + return 'a'; + } +} + +class B { + constructor(f) { + if (!f) { + return 'falsy'; + } + } +} +``` + +Examples of **correct** code for this rule: + +```js +/*eslint no-constructor-return: "error"*/ + +class C { + constructor() { } +} + +class D { + constructor(f) { + if (!f) { + return; // Flow control. + } + + f(); + } +} +``` diff --git a/lib/rules/index.js b/lib/rules/index.js index dbda93fb325..047a1e73f26 100644 --- a/lib/rules/index.js +++ b/lib/rules/index.js @@ -101,6 +101,7 @@ module.exports = new LazyLoadingRuleMap(Object.entries({ "no-console": () => require("./no-console"), "no-const-assign": () => require("./no-const-assign"), "no-constant-condition": () => require("./no-constant-condition"), + "no-constructor-return": () => require("./no-constructor-return"), "no-continue": () => require("./no-continue"), "no-control-regex": () => require("./no-control-regex"), "no-debugger": () => require("./no-debugger"), diff --git a/lib/rules/no-constructor-return.js b/lib/rules/no-constructor-return.js new file mode 100644 index 00000000000..ee6ead40b5f --- /dev/null +++ b/lib/rules/no-constructor-return.js @@ -0,0 +1,52 @@ +/** + * @fileoverview Rule to disallow returning value from constructor. + * @author Pig Fang + */ + +"use strict"; + +//------------------------------------------------------------------------------ +// Rule Definition +//------------------------------------------------------------------------------ + +module.exports = { + meta: { + type: "suggestion", + + docs: { + description: "disallow returning value from constructor", + category: "Best Practices", + recommended: false, + url: "https://eslint.org/docs/rules/no-constructor-return" + }, + + schema: {}, + + fixable: "code", + + messages: { + unexpected: "Unexpected return statement in constructor." + } + }, + + create(context) { + return { + "MethodDefinition[kind='constructor'] ReturnStatement"(node) { + const ancestors = context.getAncestors(); + const ancestor = ancestors[ancestors.length - 3]; + + if ( + ancestor && + ancestor.type === "MethodDefinition" && + ancestor.kind === "constructor" || + node.argument + ) { + context.report({ + node, + messageId: "unexpected" + }); + } + } + }; + } +}; diff --git a/tests/lib/rules/no-constructor-return.js b/tests/lib/rules/no-constructor-return.js new file mode 100644 index 00000000000..9e5202ac839 --- /dev/null +++ b/tests/lib/rules/no-constructor-return.js @@ -0,0 +1,46 @@ +/** + * @fileoverview Tests for no-constructor-return rule. + * @author Pig Fang + */ + +"use strict"; + +//------------------------------------------------------------------------------ +// Requirements +//------------------------------------------------------------------------------ + +const rule = require("../../../lib/rules/no-constructor-return"), + { RuleTester } = require("../../../lib/rule-tester"); + +//------------------------------------------------------------------------------ +// Tests +//------------------------------------------------------------------------------ + +const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 2015 } }); + +const errors = [{ type: "ReturnStatement", messageId: "unexpected" }]; + +ruleTester.run("no-constructor-return", rule, { + valid: [ + "class C { }", + "class C { constructor() {} }", + "class C { constructor() { let v } }", + "class C { method() { return '' } }", + "class C { get value() { return '' } }", + "class C { constructor(a) { if (!a) { return } else { a() } } }" + ], + invalid: [ + { + code: "class C { constructor() { return } }", + errors + }, + { + code: "class C { constructor() { return '' } }", + errors + }, + { + code: "class C { constructor(a) { if (!a) { return '' } else { a() } } }", + errors + } + ] +}); diff --git a/tools/rule-types.json b/tools/rule-types.json index 4bc07d4e43e..49b66469ea8 100644 --- a/tools/rule-types.json +++ b/tools/rule-types.json @@ -88,6 +88,7 @@ "no-console": "suggestion", "no-const-assign": "problem", "no-constant-condition": "problem", + "no-constructor-return": "suggestion", "no-continue": "suggestion", "no-control-regex": "problem", "no-debugger": "problem", From b3d13e454f45d645e5ec08ad3ed3c850bd5d35f6 Mon Sep 17 00:00:00 2001 From: Pig Fang Date: Wed, 6 Nov 2019 14:31:51 +0800 Subject: [PATCH 02/13] Docs: polish example --- docs/rules/no-constructor-return.md | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/docs/rules/no-constructor-return.md b/docs/rules/no-constructor-return.md index 327ab0cf404..e1296e2da6d 100644 --- a/docs/rules/no-constructor-return.md +++ b/docs/rules/no-constructor-return.md @@ -14,8 +14,9 @@ Examples of **incorrect** code for this rule: /*eslint no-constructor-return: "error"*/ class A { - constructor() { - return 'a'; + constructor(a) { + this.a = a; + return a; } } @@ -34,7 +35,9 @@ Examples of **correct** code for this rule: /*eslint no-constructor-return: "error"*/ class C { - constructor() { } + constructor(c) { + this.c = c; + } } class D { From e7da62b47589ef028f2ca799cc0304f43b4050ed Mon Sep 17 00:00:00 2001 From: Pig Fang Date: Wed, 13 Nov 2019 18:52:11 +0800 Subject: [PATCH 03/13] Docs: polish --- docs/rules/no-constructor-return.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rules/no-constructor-return.md b/docs/rules/no-constructor-return.md index e1296e2da6d..5ceb82ba512 100644 --- a/docs/rules/no-constructor-return.md +++ b/docs/rules/no-constructor-return.md @@ -1,6 +1,6 @@ # Disallow returning value in constructor (no-constructor-return) -In JavaScript returning value in constructor is allowed, however this looks meaningless. Forbidding this pattern prevents mistake resulting from unfamiliarity with the language or copy-paste error. +In JavaScript returning value in constructor is allowed, however this may be a mistake. Forbidding this pattern prevents mistake resulting from unfamiliarity with the language or copy-paste error. Note that returning nothing with flow control is allowed. From 0421c39e2709c1f761e0d8adc06ef54a1a0bacae Mon Sep 17 00:00:00 2001 From: Pig Fang Date: Wed, 13 Nov 2019 18:53:01 +0800 Subject: [PATCH 04/13] Fix: fix `fixable` property --- lib/rules/no-constructor-return.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rules/no-constructor-return.js b/lib/rules/no-constructor-return.js index ee6ead40b5f..cc74a7a9a12 100644 --- a/lib/rules/no-constructor-return.js +++ b/lib/rules/no-constructor-return.js @@ -22,7 +22,7 @@ module.exports = { schema: {}, - fixable: "code", + fixable: null, messages: { unexpected: "Unexpected return statement in constructor." From 5d91acecf9896bd83541dc0deb4e9a19c04027c0 Mon Sep 17 00:00:00 2001 From: Pig Fang Date: Wed, 13 Nov 2019 19:05:50 +0800 Subject: [PATCH 05/13] Update: update rule type --- tools/rule-types.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/rule-types.json b/tools/rule-types.json index 49b66469ea8..dc8d9fbfb01 100644 --- a/tools/rule-types.json +++ b/tools/rule-types.json @@ -88,7 +88,7 @@ "no-console": "suggestion", "no-const-assign": "problem", "no-constant-condition": "problem", - "no-constructor-return": "suggestion", + "no-constructor-return": "problem", "no-continue": "suggestion", "no-control-regex": "problem", "no-debugger": "problem", From f903e4cb4a0adf076d57ccfb006bce86bd79115f Mon Sep 17 00:00:00 2001 From: Pig Fang Date: Wed, 13 Nov 2019 19:27:02 +0800 Subject: [PATCH 06/13] Fix: inner functions --- lib/rules/no-constructor-return.js | 19 ++++++++++++++----- tests/lib/rules/no-constructor-return.js | 5 ++++- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/lib/rules/no-constructor-return.js b/lib/rules/no-constructor-return.js index cc74a7a9a12..32bfa2083c8 100644 --- a/lib/rules/no-constructor-return.js +++ b/lib/rules/no-constructor-return.js @@ -5,6 +5,8 @@ "use strict"; +const astUtils = require("./utils/ast-utils"); + //------------------------------------------------------------------------------ // Rule Definition //------------------------------------------------------------------------------ @@ -32,13 +34,20 @@ module.exports = { create(context) { return { "MethodDefinition[kind='constructor'] ReturnStatement"(node) { - const ancestors = context.getAncestors(); - const ancestor = ancestors[ancestors.length - 3]; + const ancestors = context.getAncestors().reverse(); + const ctorIndex = ancestors.findIndex( + ancestor => ancestor.type === "MethodDefinition" && ancestor.kind === "constructor" + ); + const upperFnIndex = ancestors.findIndex(astUtils.isFunction); + + if (~upperFnIndex && upperFnIndex < ctorIndex - 1) { + return; + } if ( - ancestor && - ancestor.type === "MethodDefinition" && - ancestor.kind === "constructor" || + ancestors[2] && + ancestors[2].type === "MethodDefinition" && + ancestors[2].kind === "constructor" || node.argument ) { context.report({ diff --git a/tests/lib/rules/no-constructor-return.js b/tests/lib/rules/no-constructor-return.js index 9e5202ac839..836304ece16 100644 --- a/tests/lib/rules/no-constructor-return.js +++ b/tests/lib/rules/no-constructor-return.js @@ -27,7 +27,10 @@ ruleTester.run("no-constructor-return", rule, { "class C { constructor() { let v } }", "class C { method() { return '' } }", "class C { get value() { return '' } }", - "class C { constructor(a) { if (!a) { return } else { a() } } }" + "class C { constructor(a) { if (!a) { return } else { a() } } }", + "class C { constructor() { function fn() { return true } } }", + "class C { constructor() { this.fn = function () { return true } } }", + "class C { constructor() { this.fn = () => { return true } } }" ], invalid: [ { From 0d0443e1bf12106792894e7f1e6731470bf818dd Mon Sep 17 00:00:00 2001 From: Pig Fang Date: Tue, 19 Nov 2019 14:40:53 +0800 Subject: [PATCH 07/13] Chore: refactor --- lib/rules/no-constructor-return.js | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/lib/rules/no-constructor-return.js b/lib/rules/no-constructor-return.js index 32bfa2083c8..af7713e8712 100644 --- a/lib/rules/no-constructor-return.js +++ b/lib/rules/no-constructor-return.js @@ -5,8 +5,6 @@ "use strict"; -const astUtils = require("./utils/ast-utils"); - //------------------------------------------------------------------------------ // Rule Definition //------------------------------------------------------------------------------ @@ -32,23 +30,22 @@ module.exports = { }, create(context) { + const stack = []; + return { - "MethodDefinition[kind='constructor'] ReturnStatement"(node) { - const ancestors = context.getAncestors().reverse(); - const ctorIndex = ancestors.findIndex( - ancestor => ancestor.type === "MethodDefinition" && ancestor.kind === "constructor" - ); - const upperFnIndex = ancestors.findIndex(astUtils.isFunction); - - if (~upperFnIndex && upperFnIndex < ctorIndex - 1) { - return; - } + onCodePathStart(_, node) { + stack.push(node); + }, + onCodePathEnd() { + stack.pop(); + }, + ReturnStatement(node) { + const last = stack[stack.length - 1]; if ( - ancestors[2] && - ancestors[2].type === "MethodDefinition" && - ancestors[2].kind === "constructor" || - node.argument + last.parent.type === "MethodDefinition" && + last.parent.kind === "constructor" && + (node.parent.parent === last || node.argument) ) { context.report({ node, From af50c31d8fe2f81e6d9b5c54dc46f7cc68fb2d4a Mon Sep 17 00:00:00 2001 From: Pig Fang Date: Tue, 19 Nov 2019 14:49:17 +0800 Subject: [PATCH 08/13] Docs: polish --- docs/rules/no-constructor-return.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rules/no-constructor-return.md b/docs/rules/no-constructor-return.md index 5ceb82ba512..de5941b1ff8 100644 --- a/docs/rules/no-constructor-return.md +++ b/docs/rules/no-constructor-return.md @@ -1,6 +1,6 @@ # Disallow returning value in constructor (no-constructor-return) -In JavaScript returning value in constructor is allowed, however this may be a mistake. Forbidding this pattern prevents mistake resulting from unfamiliarity with the language or copy-paste error. +In JavaScript, returning value in the constructor of a class may be a mistake. Forbidding this pattern prevents mistake resulting from unfamiliarity with the language or copy-paste error. Note that returning nothing with flow control is allowed. From f87b7a85a573d01e7b2a2262ce5d95e90e1654ac Mon Sep 17 00:00:00 2001 From: Pig Fang Date: Tue, 19 Nov 2019 14:50:06 +0800 Subject: [PATCH 09/13] Docs: polish --- docs/rules/no-constructor-return.md | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/docs/rules/no-constructor-return.md b/docs/rules/no-constructor-return.md index de5941b1ff8..0e0b9136791 100644 --- a/docs/rules/no-constructor-return.md +++ b/docs/rules/no-constructor-return.md @@ -2,11 +2,9 @@ In JavaScript, returning value in the constructor of a class may be a mistake. Forbidding this pattern prevents mistake resulting from unfamiliarity with the language or copy-paste error. -Note that returning nothing with flow control is allowed. - ## Rule Details -This rule disallows return statement in constructor of a class. +This rule disallows return statement in constructor of a class. Note that returning nothing with flow control is allowed. Examples of **incorrect** code for this rule: From bc5c51a1770a96bd5f7814c6b9b288c15e2aa2e1 Mon Sep 17 00:00:00 2001 From: Pig Fang Date: Tue, 19 Nov 2019 14:53:50 +0800 Subject: [PATCH 10/13] Docs: polish --- docs/rules/no-constructor-return.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/rules/no-constructor-return.md b/docs/rules/no-constructor-return.md index 0e0b9136791..65f9cefc591 100644 --- a/docs/rules/no-constructor-return.md +++ b/docs/rules/no-constructor-return.md @@ -1,10 +1,10 @@ # Disallow returning value in constructor (no-constructor-return) -In JavaScript, returning value in the constructor of a class may be a mistake. Forbidding this pattern prevents mistake resulting from unfamiliarity with the language or copy-paste error. +In JavaScript, returning a value in the constructor of a class may be a mistake. Forbidding this pattern prevents mistake resulting from unfamiliarity with the language or copy-paste error. ## Rule Details -This rule disallows return statement in constructor of a class. Note that returning nothing with flow control is allowed. +This rule disallows return statements in the constructor of a class. Note that returning nothing with flow control is allowed. Examples of **incorrect** code for this rule: From 34e44415202305174cfe2f937b503c93d0b33044 Mon Sep 17 00:00:00 2001 From: Pig Fang Date: Wed, 20 Nov 2019 12:49:17 +0800 Subject: [PATCH 11/13] Docs: polish --- docs/rules/no-constructor-return.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rules/no-constructor-return.md b/docs/rules/no-constructor-return.md index 65f9cefc591..284b757488e 100644 --- a/docs/rules/no-constructor-return.md +++ b/docs/rules/no-constructor-return.md @@ -1,6 +1,6 @@ # Disallow returning value in constructor (no-constructor-return) -In JavaScript, returning a value in the constructor of a class may be a mistake. Forbidding this pattern prevents mistake resulting from unfamiliarity with the language or copy-paste error. +In JavaScript, returning a value in the constructor of a class may be a mistake. Forbidding this pattern prevents mistakes resulting from unfamiliarity with the language or a copy-paste error. ## Rule Details From c20a0d70da8e1bea8e818f7c877a8abaa416f066 Mon Sep 17 00:00:00 2001 From: Pig Fang Date: Wed, 20 Nov 2019 15:08:55 +0800 Subject: [PATCH 12/13] Fix: rule type --- lib/rules/no-constructor-return.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rules/no-constructor-return.js b/lib/rules/no-constructor-return.js index af7713e8712..1f1c9a367c7 100644 --- a/lib/rules/no-constructor-return.js +++ b/lib/rules/no-constructor-return.js @@ -11,7 +11,7 @@ module.exports = { meta: { - type: "suggestion", + type: "problem", docs: { description: "disallow returning value from constructor", From 885157c54db07627a83960d6ef2c31654caaee95 Mon Sep 17 00:00:00 2001 From: Pig Fang Date: Wed, 20 Nov 2019 15:16:54 +0800 Subject: [PATCH 13/13] Chore: add more cases --- lib/rules/no-constructor-return.js | 4 ++++ tests/lib/rules/no-constructor-return.js | 11 +++++++++++ 2 files changed, 15 insertions(+) diff --git a/lib/rules/no-constructor-return.js b/lib/rules/no-constructor-return.js index 1f1c9a367c7..4757770b7cc 100644 --- a/lib/rules/no-constructor-return.js +++ b/lib/rules/no-constructor-return.js @@ -42,6 +42,10 @@ module.exports = { ReturnStatement(node) { const last = stack[stack.length - 1]; + if (!last.parent) { + return; + } + if ( last.parent.type === "MethodDefinition" && last.parent.kind === "constructor" && diff --git a/tests/lib/rules/no-constructor-return.js b/tests/lib/rules/no-constructor-return.js index 836304ece16..0a8d3b0aeb3 100644 --- a/tests/lib/rules/no-constructor-return.js +++ b/tests/lib/rules/no-constructor-return.js @@ -22,6 +22,17 @@ const errors = [{ type: "ReturnStatement", messageId: "unexpected" }]; ruleTester.run("no-constructor-return", rule, { valid: [ + "function fn() { return }", + "function fn(kumiko) { if (kumiko) { return kumiko } }", + "const fn = function () { return }", + "const fn = function () { if (kumiko) { return kumiko } }", + "const fn = () => { return }", + "const fn = () => { if (kumiko) { return kumiko } }", + { + code: "return 'Kumiko Oumae'", + parserOptions: { ecmaFeatures: { globalReturn: true } } + }, + "class C { }", "class C { constructor() {} }", "class C { constructor() { let v } }",