From 26093c76903310d12f21e24e73d97c0d2ac1f359 Mon Sep 17 00:00:00 2001 From: Yosuke Ota Date: Fri, 26 Jan 2024 19:03:49 +0900 Subject: [PATCH] feat: fix false negatives in `no-this-before-super` (#17762) * fix: false negatives in `no-this-before-super` * chore: move comment * fix: traverseSegments controller.skip * chore: format * chore: add a little early judgment * fix: remove skipped.has(segment) * fix: remove unnecessary code * fix: wrong traverse * fix: rename variable --- lib/linter/code-path-analysis/code-path.js | 40 +++++--- lib/rules/no-this-before-super.js | 21 +++- .../linter/code-path-analysis/code-path.js | 52 ++++++++++ tests/lib/rules/no-this-before-super.js | 95 +++++++++++++++++++ 4 files changed, 192 insertions(+), 16 deletions(-) diff --git a/lib/linter/code-path-analysis/code-path.js b/lib/linter/code-path-analysis/code-path.js index 32b814c16ba..8c438e29a31 100644 --- a/lib/linter/code-path-analysis/code-path.js +++ b/lib/linter/code-path-analysis/code-path.js @@ -177,8 +177,8 @@ class CodePath { // tracks the traversal steps const stack = [[startSegment, 0]]; - // tracks the last skipped segment during traversal - let skippedSegment = null; + // segments that have been skipped during traversal + const skipped = new Set(); // indicates if we exited early from the traversal let broken = false; @@ -193,11 +193,7 @@ class CodePath { * @returns {void} */ skip() { - if (stack.length <= 1) { - broken = true; - } else { - skippedSegment = stack.at(-2)[0]; - } + skipped.add(segment); }, /** @@ -222,6 +218,18 @@ class CodePath { ); } + /** + * Checks if a given previous segment has been skipped. + * @param {CodePathSegment} prevSegment A previous segment to check. + * @returns {boolean} `true` if the segment has been skipped. + */ + function isSkipped(prevSegment) { + return ( + skipped.has(prevSegment) || + segment.isLoopedPrevSegment(prevSegment) + ); + } + // the traversal while (stack.length > 0) { @@ -258,17 +266,21 @@ class CodePath { continue; } - // Reset the skipping flag if all branches have been skipped. - if (skippedSegment && segment.prevSegments.includes(skippedSegment)) { - skippedSegment = null; - } visited.add(segment); + + // Skips the segment if all previous segments have been skipped. + const shouldSkip = ( + skipped.size > 0 && + segment.prevSegments.length > 0 && + segment.prevSegments.every(isSkipped) + ); + /* * If the most recent segment hasn't been skipped, then we call * the callback, passing in the segment and the controller. */ - if (!skippedSegment) { + if (!shouldSkip) { resolvedCallback.call(this, segment, controller); // exit if we're at the last segment @@ -284,6 +296,10 @@ class CodePath { if (broken) { break; } + } else { + + // If the most recent segment has been skipped, then mark it as skipped. + skipped.add(segment); } } diff --git a/lib/rules/no-this-before-super.js b/lib/rules/no-this-before-super.js index f96d8ace81d..bcd00a404ba 100644 --- a/lib/rules/no-this-before-super.js +++ b/lib/rules/no-this-before-super.js @@ -197,11 +197,26 @@ module.exports = { return; } + /** + * A collection of nodes to avoid duplicate reports. + * @type {Set} + */ + const reported = new Set(); + codePath.traverseSegments((segment, controller) => { const info = segInfoMap[segment.id]; + const invalidNodes = info.invalidNodes + .filter( + + /* + * Avoid duplicate reports. + * When there is a `finally`, invalidNodes may contain already reported node. + */ + node => !reported.has(node) + ); - for (let i = 0; i < info.invalidNodes.length; ++i) { - const invalidNode = info.invalidNodes[i]; + for (const invalidNode of invalidNodes) { + reported.add(invalidNode); context.report({ messageId: "noBeforeSuper", @@ -273,14 +288,12 @@ module.exports = { const info = segInfoMap[segment.id]; if (info.superCalled) { - info.invalidNodes = []; controller.skip(); } else if ( segment.prevSegments.length > 0 && segment.prevSegments.every(isCalled) ) { info.superCalled = true; - info.invalidNodes = []; } } ); diff --git a/tests/lib/linter/code-path-analysis/code-path.js b/tests/lib/linter/code-path-analysis/code-path.js index 0864efa822e..f7613cd99ee 100644 --- a/tests/lib/linter/code-path-analysis/code-path.js +++ b/tests/lib/linter/code-path-analysis/code-path.js @@ -356,6 +356,58 @@ describe("CodePathAnalyzer", () => { */ }); + it("should not skip the next branch when 'controller.skip()' was called.", () => { + const codePath = parseCodePaths("if (a) { if (b) { foo(); } bar(); } out1();")[0]; + const order = getOrderOfTraversing(codePath, null, (segment, controller) => { + if (segment.id === "s1_4") { + controller.skip(); // Since s1_5 is connected from s1_1, we expect it not to be skipped. + } + }); + + assert.deepStrictEqual(order, ["s1_1", "s1_2", "s1_3", "s1_4", "s1_5"]); + + /* + digraph { + node[shape=box,style="rounded,filled",fillcolor=white]; + initial[label="",shape=circle,style=filled,fillcolor=black,width=0.25,height=0.25]; + final[label="",shape=doublecircle,style=filled,fillcolor=black,width=0.25,height=0.25]; + s1_1[label="Program:enter\nIfStatement:enter\nIdentifier (a)"]; + s1_2[label="BlockStatement:enter\nIfStatement:enter\nIdentifier (b)"]; + s1_3[label="BlockStatement:enter\nExpressionStatement:enter\nCallExpression:enter\nIdentifier (foo)\nCallExpression:exit\nExpressionStatement:exit\nBlockStatement:exit"]; + s1_4[label="IfStatement:exit\nExpressionStatement:enter\nCallExpression:enter\nIdentifier (bar)\nCallExpression:exit\nExpressionStatement:exit\nBlockStatement:exit"]; + s1_5[label="IfStatement:exit\nExpressionStatement:enter\nCallExpression:enter\nIdentifier (out1)\nCallExpression:exit\nExpressionStatement:exit\nProgram:exit"]; + initial->s1_1->s1_2->s1_3->s1_4->s1_5; + s1_1->s1_5; + s1_2->s1_4; + s1_5->final; + } + */ + }); + + it("should skip the next branch when 'controller.skip()' was called at top segment.", () => { + const codePath = parseCodePaths("a; while (b) { c; }")[0]; + + const order = getOrderOfTraversing(codePath, null, (segment, controller) => { + if (segment.id === "s1_1") { + controller.skip(); + } + }); + + assert.deepStrictEqual(order, ["s1_1"]); + + /* + digraph { + node[shape=box,style="rounded,filled",fillcolor=white]; + initial[label="",shape=circle,style=filled,fillcolor=black,width=0.25,height=0.25]; + final[label="",shape=doublecircle,style=filled,fillcolor=black,width=0.25,height=0.25]; + s1_1[label="Program:enter\nExpressionStatement:enter\nIdentifier (a)\nExpressionStatement:exit\nWhileStatement:enter"]; + s1_2[label="Identifier (b)"]; + s1_3[label="BlockStatement:enter\nExpressionStatement:enter\nIdentifier (c)\nExpressionStatement:exit\nBlockStatement:exit"]; + s1_4[label="WhileStatement:exit\nProgram:exit"]; + initial->s1_1->s1_2->s1_3->s1_2->s1_4->final; + } + */ + }); /* eslint-enable internal-rules/multiline-comment-style -- Commenting out */ }); }); diff --git a/tests/lib/rules/no-this-before-super.js b/tests/lib/rules/no-this-before-super.js index c4c96b2e748..578a5bc5c14 100644 --- a/tests/lib/rules/no-this-before-super.js +++ b/tests/lib/rules/no-this-before-super.js @@ -60,6 +60,24 @@ ruleTester.run("no-this-before-super", rule, { "class A extends B { constructor() { if (a) { super(); this.a(); } else { super(); this.b(); } } }", "class A extends B { constructor() { if (a) super(); else super(); this.a(); } }", "class A extends B { constructor() { try { super(); } finally {} this.a(); } }", + `class A extends B { + constructor() { + while (foo) { + super(); + this.a(); + } + } + }`, + `class A extends B { + constructor() { + while (foo) { + if (init) { + super(); + this.a(); + } + } + } + }`, // https://github.com/eslint/eslint/issues/5261 "class A extends B { constructor(a) { super(); for (const b of a) { this.a(); } } }", @@ -186,6 +204,83 @@ ruleTester.run("no-this-before-super", rule, { code: "class A extends B { constructor() { foo ??= super().a; this.c(); } }", languageOptions: { ecmaVersion: 2021 }, errors: [{ messageId: "noBeforeSuper", data: { kind: "this" }, type: "ThisExpression" }] + }, + { + code: ` + class A extends B { + constructor() { + if (foo) { + if (bar) { } + super(); + } + this.a(); + } + }`, + errors: [{ messageId: "noBeforeSuper", data: { kind: "this" }, type: "ThisExpression" }] + }, + { + code: ` + class A extends B { + constructor() { + if (foo) { + } else { + super(); + } + this.a(); + } + }`, + errors: [{ messageId: "noBeforeSuper", data: { kind: "this" }, type: "ThisExpression" }] + }, + { + code: ` + class A extends B { + constructor() { + try { + call(); + } finally { + this.a(); + } + } + }`, + errors: [{ messageId: "noBeforeSuper", data: { kind: "this" }, type: "ThisExpression" }] + }, + { + code: ` + class A extends B { + constructor() { + while (foo) { + super(); + } + this.a(); + } + }`, + errors: [{ messageId: "noBeforeSuper", data: { kind: "this" }, type: "ThisExpression" }] + }, + { + code: ` + class A extends B { + constructor() { + while (foo) { + this.a(); + super(); + } + } + }`, + errors: [{ messageId: "noBeforeSuper", data: { kind: "this" }, type: "ThisExpression" }] + }, + { + code: ` + class A extends B { + constructor() { + while (foo) { + if (init) { + this.a(); + super(); + } + } + } + }`, + errors: [{ messageId: "noBeforeSuper", data: { kind: "this" }, type: "ThisExpression" }] } ] });