Skip to content

Commit

Permalink
fix: Ensure correct code path for && followed by ?? (#17618)
Browse files Browse the repository at this point in the history 
* fix: Ensure correct code path for && followed by ??

fixes #13614

* Remove .only

* Update lib/linter/code-path-analysis/code-path-state.js

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

* Update tests/lib/linter/code-path-analysis/code-path-analyzer.js

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

* Update variable names in comments

---------

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
  • Loading branch information
@nzakas @mdjermanovic
nzakas and mdjermanovic committed Oct 4, 2023
1 parent 2665552 commit d2f6801
Show file tree
Hide file tree
Showing 4 changed files with 130 additions and 59 deletions.
147 changes: 96 additions & 51 deletions lib/linter/code-path-analysis/code-path-state.js
View file
Expand Up @@ -121,7 +121,7 @@ class ChainContext {
* true go one way and if false go the other (tracked by `trueForkContext` and
* `falseForkContext`). The `??` operator doesn't operate on true/false because
* the left expression is evaluated to be nullish or not, so only if nullish do
* we fork to the right expression (tracked by `qqForkcontext`).
* we fork to the right expression (tracked by `nullishForkContext`).
*/
class ChoiceContext {

Expand Down Expand Up @@ -170,14 +170,14 @@ class ChoiceContext {
this.falseForkContext = ForkContext.newEmpty(forkContext);

/**
* The fork context for the right side of the `??` path of the choice.
* The fork context for when the choice result is `null` or `undefined`.
* @type {ForkContext}
*/
this.qqForkContext = ForkContext.newEmpty(forkContext);
this.nullishForkContext = ForkContext.newEmpty(forkContext);

/**
* Indicates if any of `trueForkContext`, `falseForkContext`, or
* `qqForkContext` have been updated with segments from a child context.
* `nullishForkContext` have been updated with segments from a child context.
* @type {boolean}
*/
this.processed = false;
Expand Down Expand Up @@ -849,7 +849,7 @@ function finalizeTestSegmentsOfFor(context, choiceContext, head) {
if (!choiceContext.processed) {
choiceContext.trueForkContext.add(head);
choiceContext.falseForkContext.add(head);
choiceContext.qqForkContext.add(head);
choiceContext.nullishForkContext.add(head);
}

/*
Expand Down Expand Up @@ -1095,31 +1095,31 @@ class CodePathState {

/**
* Pops the last choice context and finalizes it.
* This is called upon leaving a node that represents a choice.
* @throws {Error} (Unreachable.)
* @returns {ChoiceContext} The popped context.
*/
popChoiceContext() {
const context = this.choiceContext;

this.choiceContext = context.upper;

const poppedChoiceContext = this.choiceContext;
const forkContext = this.forkContext;
const headSegments = forkContext.head;
const head = forkContext.head;

switch (context.kind) {
this.choiceContext = poppedChoiceContext.upper;

switch (poppedChoiceContext.kind) {
case "&&":
case "||":
case "??":

/*
* The `headSegments` are the path of the right-hand operand.
* The `head` are the path of the right-hand operand.
* If we haven't previously added segments from child contexts,
* then we add these segments to all possible forks.
*/
if (!context.processed) {
context.trueForkContext.add(headSegments);
context.falseForkContext.add(headSegments);
context.qqForkContext.add(headSegments);
if (!poppedChoiceContext.processed) {
poppedChoiceContext.trueForkContext.add(head);
poppedChoiceContext.falseForkContext.add(head);
poppedChoiceContext.nullishForkContext.add(head);
}

/*
Expand All @@ -1128,38 +1128,38 @@ class CodePathState {
* then we take the segments for this context and move them up
* to the parent context.
*/
if (context.isForkingAsResult) {
if (poppedChoiceContext.isForkingAsResult) {
const parentContext = this.choiceContext;

parentContext.trueForkContext.addAll(context.trueForkContext);
parentContext.falseForkContext.addAll(context.falseForkContext);
parentContext.qqForkContext.addAll(context.qqForkContext);
parentContext.trueForkContext.addAll(poppedChoiceContext.trueForkContext);
parentContext.falseForkContext.addAll(poppedChoiceContext.falseForkContext);
parentContext.nullishForkContext.addAll(poppedChoiceContext.nullishForkContext);
parentContext.processed = true;

// Exit early so we don't collapse all paths into one.
return context;
return poppedChoiceContext;
}

break;

case "test":
if (!context.processed) {
if (!poppedChoiceContext.processed) {

/*
* The head segments are the path of the `if` block here.
* Updates the `true` path with the end of the `if` block.
*/
context.trueForkContext.clear();
context.trueForkContext.add(headSegments);
poppedChoiceContext.trueForkContext.clear();
poppedChoiceContext.trueForkContext.add(head);
} else {

/*
* The head segments are the path of the `else` block here.
* Updates the `false` path with the end of the `else`
* block.
*/
context.falseForkContext.clear();
context.falseForkContext.add(headSegments);
poppedChoiceContext.falseForkContext.clear();
poppedChoiceContext.falseForkContext.add(head);
}

break;
Expand All @@ -1170,7 +1170,7 @@ class CodePathState {
* Loops are addressed in `popLoopContext()` so just return
* the context without modification.
*/
return context;
return poppedChoiceContext;

/* c8 ignore next */
default:
Expand All @@ -1180,71 +1180,116 @@ class CodePathState {
/*
* Merge the true path with the false path to create a single path.
*/
const combinedForkContext = context.trueForkContext;
const combinedForkContext = poppedChoiceContext.trueForkContext;

combinedForkContext.addAll(context.falseForkContext);
combinedForkContext.addAll(poppedChoiceContext.falseForkContext);
forkContext.replaceHead(combinedForkContext.makeNext(0, -1));

return context;
return poppedChoiceContext;
}

/**
* Makes a code path segment of the right-hand operand of a logical
* Creates a code path segment to represent right-hand operand of a logical
* expression.
* This is called in the preprocessing phase when entering a node.
* @throws {Error} (Unreachable.)
* @returns {void}
*/
makeLogicalRight() {
const context = this.choiceContext;
const currentChoiceContext = this.choiceContext;
const forkContext = this.forkContext;

if (context.processed) {
if (currentChoiceContext.processed) {

/*
* This got segments already from the child choice context.
* Creates the next path from own true/false fork context.
* This context was already assigned segments from a child
* choice context. In this case, we are concerned only about
* the path that does not short-circuit and so ends up on the
* right-hand operand of the logical expression.
*/
let prevForkContext;

switch (context.kind) {
switch (currentChoiceContext.kind) {
case "&&": // if true then go to the right-hand side.
prevForkContext = context.trueForkContext;
prevForkContext = currentChoiceContext.trueForkContext;
break;
case "||": // if false then go to the right-hand side.
prevForkContext = context.falseForkContext;
prevForkContext = currentChoiceContext.falseForkContext;
break;
case "??": // Both true/false can short-circuit, so needs the third path to go to the right-hand side. That's qqForkContext.
prevForkContext = context.qqForkContext;
case "??": // Both true/false can short-circuit, so needs the third path to go to the right-hand side. That's nullishForkContext.
prevForkContext = currentChoiceContext.nullishForkContext;
break;
default:
throw new Error("unreachable");
}

/*
* Create the segment for the right-hand operand of the logical expression
* and adjust the fork context pointer to point there. The right-hand segment
* is added at the end of all segments in `prevForkContext`.
*/
forkContext.replaceHead(prevForkContext.makeNext(0, -1));

/*
* We no longer need this list of segments.
*
* Reset `processed` because we've removed the segments from the child
* choice context. This allows `popChoiceContext()` to continue adding
* segments later.
*/
prevForkContext.clear();
context.processed = false;
currentChoiceContext.processed = false;

} else {

/*
* This did not get segments from the child choice context.
* So addresses the head segments.
* The head segments are the path of the left-hand operand.
* This choice context was not assigned segments from a child
* choice context, which means that it's a terminal logical
* expression.
*
* `head` is the segments for the left-hand operand of the
* logical expression.
*
* Each of the fork contexts below are empty at this point. We choose
* the path(s) that will short-circuit and add the segment for the
* left-hand operand to it. Ultimately, this will be the only segment
* in that path due to the short-circuting, so we are just seeding
* these paths to start.
*/
switch (context.kind) {
case "&&": // the false path can short-circuit.
context.falseForkContext.add(forkContext.head);
switch (currentChoiceContext.kind) {
case "&&":

/*
* In most contexts, when a && expression evaluates to false,
* it short circuits, so we need to account for that by setting
* the `falseForkContext` to the left operand.
*
* When a && expression is the left-hand operand for a ??
* expression, such as `(a && b) ?? c`, a nullish value will
* also short-circuit in a different way than a false value,
* so we also set the `nullishForkContext` to the left operand.
* This path is only used with a ?? expression and is thrown
* away for any other type of logical expression, so it's safe
* to always add.
*/
currentChoiceContext.falseForkContext.add(forkContext.head);
currentChoiceContext.nullishForkContext.add(forkContext.head);
break;
case "||": // the true path can short-circuit.
context.trueForkContext.add(forkContext.head);
currentChoiceContext.trueForkContext.add(forkContext.head);
break;
case "??": // both can short-circuit.
context.trueForkContext.add(forkContext.head);
context.falseForkContext.add(forkContext.head);
currentChoiceContext.trueForkContext.add(forkContext.head);
currentChoiceContext.falseForkContext.add(forkContext.head);
break;
default:
throw new Error("unreachable");
}

/*
* Create the segment for the right-hand operand of the logical expression
* and adjust the fork context pointer to point there.
*/
forkContext.replaceHead(forkContext.makeNext(-1, -1));
}
}
Expand All @@ -1265,7 +1310,7 @@ class CodePathState {
if (!context.processed) {
context.trueForkContext.add(forkContext.head);
context.falseForkContext.add(forkContext.head);
context.qqForkContext.add(forkContext.head);
context.nullishForkContext.add(forkContext.head);
}

context.processed = false;
Expand Down
10 changes: 6 additions & 4 deletions tests/fixtures/code-path-analysis/assignment--nested-and-3.js
View file
@@ -1,7 +1,8 @@
/*expected
initial->s1_1->s1_2->s1_3->s1_4;
s1_1->s1_4;
s1_2->s1_4->final;
s1_1->s1_3;
s1_2->s1_4;
s1_1->s1_4->final;
*/
(a &&= b) ?? c;

Expand All @@ -15,7 +16,8 @@ digraph {
s1_3[label="Identifier (c)"];
s1_4[label="LogicalExpression:exit\nExpressionStatement:exit\nProgram:exit"];
initial->s1_1->s1_2->s1_3->s1_4;
s1_1->s1_4;
s1_2->s1_4->final;
s1_1->s1_3;
s1_2->s1_4;
s1_1->s1_4->final;
}
*/
22 changes: 22 additions & 0 deletions tests/fixtures/code-path-analysis/logical--and-qq.js
View file
@@ -0,0 +1,22 @@
/*expected
initial->s1_1->s1_2->s1_3->s1_4;
s1_1->s1_3;
s1_2->s1_4;
s1_1->s1_4->final;
*/
(a && b) ?? c;

/*DOT
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\nLogicalExpression:enter\nLogicalExpression:enter\nIdentifier (a)"];
s1_2[label="Identifier (b)\nLogicalExpression:exit"];
s1_3[label="Identifier (c)"];
s1_4[label="LogicalExpression:exit\nExpressionStatement:exit\nProgram:exit"];
initial->s1_1->s1_2->s1_3->s1_4;
s1_1->s1_3;
s1_2->s1_4;
s1_1->s1_4->final;
}*/
10 changes: 6 additions & 4 deletions tests/fixtures/code-path-analysis/logical--if-mix-and-qq-1.js
View file
@@ -1,8 +1,9 @@
/*expected
initial->s1_1->s1_2->s1_3->s1_4->s1_6;
s1_1->s1_5->s1_6;
s1_1->s1_3;
s1_2->s1_4;
s1_3->s1_5;
s1_3->s1_5->s1_6;
s1_1->s1_5;
s1_2->s1_5;
s1_6->final;
*/
Expand All @@ -24,9 +25,10 @@ digraph {
s1_6[label="IfStatement:exit\nProgram:exit"];
s1_5[label="BlockStatement\nExpressionStatement\nCallExpression\nIdentifier (bar)\nIdentifier:exit (bar)\nCallExpression:exit\nExpressionStatement:exit\nBlockStatement:exit"];
initial->s1_1->s1_2->s1_3->s1_4->s1_6;
s1_1->s1_5->s1_6;
s1_1->s1_3;
s1_2->s1_4;
s1_3->s1_5;
s1_3->s1_5->s1_6;
s1_1->s1_5;
s1_2->s1_5;
s1_6->final;
}
Expand Down

0 comments on commit d2f6801

Please sign in to comment.