Skip to content

Commit

Permalink
feat: fix false negatives in no-this-before-super (#17762)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@ota-meshi
ota-meshi committed Jan 26, 2024
1 parent 57089cb commit 26093c7
Show file tree
Hide file tree
Showing 4 changed files with 192 additions and 16 deletions.
40 changes: 28 additions & 12 deletions lib/linter/code-path-analysis/code-path.js
View file
Expand Up @@ -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;
Expand All @@ -193,11 +193,7 @@ class CodePath {
* @returns {void}
*/
skip() {
if (stack.length <= 1) {
broken = true;
} else {
skippedSegment = stack.at(-2)[0];
}
skipped.add(segment);
},

/**
Expand All @@ -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) {

Expand Down Expand Up @@ -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
Expand All @@ -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);
}
}

Expand Down
21 changes: 17 additions & 4 deletions lib/rules/no-this-before-super.js
View file
Expand Up @@ -197,11 +197,26 @@ module.exports = {
return;
}

/**
* A collection of nodes to avoid duplicate reports.
* @type {Set<ASTNode>}
*/
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",
Expand Down Expand Up @@ -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 = [];
}
}
);
Expand Down
52 changes: 52 additions & 0 deletions tests/lib/linter/code-path-analysis/code-path.js
View file
Expand Up @@ -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 */
});
});
95 changes: 95 additions & 0 deletions tests/lib/rules/no-this-before-super.js
View file
Expand Up @@ -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(); } } }",
Expand Down Expand Up @@ -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" }]
}
]
});

0 comments on commit 26093c7

Please sign in to comment.