Skip to content

Commit

Permalink
fix: handle conditional breaks in nested switch statement cases (#4937)
Browse files Browse the repository at this point in the history 
* fix: add a new InclusionContext prop existedBroken

* test: tweak test

* Rework using labels

* Use flags over labels for simplicity and performance

---------

Co-authored-by: Lukas Taegert-Atkinson <lukas.taegert-atkinson@tngtech.com>
  • Loading branch information
@TrickyPi @lukastaegert
TrickyPi and lukastaegert committed Apr 17, 2023
1 parent ab33645 commit baa0311
Show file tree
Hide file tree
Showing 16 changed files with 124 additions and 121 deletions.
18 changes: 10 additions & 8 deletions src/ast/ExecutionContext.ts
View file
Expand Up @@ -11,12 +11,10 @@ interface ExecutionContextIgnore {
this: boolean;
}

export const BROKEN_FLOW_NONE = 0;
export const BROKEN_FLOW_BREAK_CONTINUE = 1;
export const BROKEN_FLOW_ERROR_RETURN_LABEL = 2;

interface ControlFlowContext {
brokenFlow: number;
brokenFlow: boolean;
hasBreak: boolean;
hasContinue: boolean;
includedLabels: Set<string>;
}

Expand All @@ -27,7 +25,7 @@ export interface InclusionContext extends ControlFlowContext {
export interface HasEffectsContext extends ControlFlowContext {
accessed: PathTracker;
assigned: PathTracker;
brokenFlow: number;
brokenFlow: boolean;
called: DiscriminatedPathTracker;
ignore: ExecutionContextIgnore;
instantiated: DiscriminatedPathTracker;
Expand All @@ -36,7 +34,9 @@ export interface HasEffectsContext extends ControlFlowContext {

export function createInclusionContext(): InclusionContext {
return {
brokenFlow: BROKEN_FLOW_NONE,
brokenFlow: false,
hasBreak: false,
hasContinue: false,
includedCallArguments: new Set(),
includedLabels: new Set()
};
Expand All @@ -46,8 +46,10 @@ export function createHasEffectsContext(): HasEffectsContext {
return {
accessed: new PathTracker(),
assigned: new PathTracker(),
brokenFlow: BROKEN_FLOW_NONE,
brokenFlow: false,
called: new DiscriminatedPathTracker(),
hasBreak: false,
hasContinue: false,
ignore: {
breaks: false,
continues: false,
Expand Down
15 changes: 6 additions & 9 deletions src/ast/nodes/BreakStatement.ts
View file
@@ -1,9 +1,4 @@
import {
BROKEN_FLOW_BREAK_CONTINUE,
BROKEN_FLOW_ERROR_RETURN_LABEL,
type HasEffectsContext,
type InclusionContext
} from '../ExecutionContext';
import { type HasEffectsContext, type InclusionContext } from '../ExecutionContext';
import type Identifier from './Identifier';
import type * as NodeType from './NodeType';
import { StatementBase } from './shared/Node';
Expand All @@ -16,11 +11,11 @@ export default class BreakStatement extends StatementBase {
if (this.label) {
if (!context.ignore.labels.has(this.label.name)) return true;
context.includedLabels.add(this.label.name);
context.brokenFlow = BROKEN_FLOW_ERROR_RETURN_LABEL;
} else {
if (!context.ignore.breaks) return true;
context.brokenFlow = BROKEN_FLOW_BREAK_CONTINUE;
context.hasBreak = true;
}
context.brokenFlow = true;
return false;
}

Expand All @@ -29,7 +24,9 @@ export default class BreakStatement extends StatementBase {
if (this.label) {
this.label.include();
context.includedLabels.add(this.label.name);
} else {
context.hasBreak = true;
}
context.brokenFlow = this.label ? BROKEN_FLOW_ERROR_RETURN_LABEL : BROKEN_FLOW_BREAK_CONTINUE;
context.brokenFlow = true;
}
}
15 changes: 6 additions & 9 deletions src/ast/nodes/ContinueStatement.ts
View file
@@ -1,9 +1,4 @@
import {
BROKEN_FLOW_BREAK_CONTINUE,
BROKEN_FLOW_ERROR_RETURN_LABEL,
type HasEffectsContext,
type InclusionContext
} from '../ExecutionContext';
import { type HasEffectsContext, type InclusionContext } from '../ExecutionContext';
import type Identifier from './Identifier';
import type * as NodeType from './NodeType';
import { StatementBase } from './shared/Node';
Expand All @@ -16,11 +11,11 @@ export default class ContinueStatement extends StatementBase {
if (this.label) {
if (!context.ignore.labels.has(this.label.name)) return true;
context.includedLabels.add(this.label.name);
context.brokenFlow = BROKEN_FLOW_ERROR_RETURN_LABEL;
} else {
if (!context.ignore.continues) return true;
context.brokenFlow = BROKEN_FLOW_BREAK_CONTINUE;
context.hasContinue = true;
}
context.brokenFlow = true;
return false;
}

Expand All @@ -29,7 +24,9 @@ export default class ContinueStatement extends StatementBase {
if (this.label) {
this.label.include();
context.includedLabels.add(this.label.name);
} else {
context.hasContinue = true;
}
context.brokenFlow = this.label ? BROKEN_FLOW_ERROR_RETURN_LABEL : BROKEN_FLOW_BREAK_CONTINUE;
context.brokenFlow = true;
}
}
15 changes: 3 additions & 12 deletions src/ast/nodes/DoWhileStatement.ts
View file
Expand Up @@ -6,6 +6,7 @@ import {
StatementBase,
type StatementNode
} from './shared/Node';
import { hasLoopBodyEffects, includeLoopBody } from './shared/loops';

export default class DoWhileStatement extends StatementBase {
declare body: StatementNode;
Expand All @@ -14,22 +15,12 @@ export default class DoWhileStatement extends StatementBase {

hasEffects(context: HasEffectsContext): boolean {
if (this.test.hasEffects(context)) return true;
const { brokenFlow, ignore } = context;
const { breaks, continues } = ignore;
ignore.breaks = true;
ignore.continues = true;
if (this.body.hasEffects(context)) return true;
ignore.breaks = breaks;
ignore.continues = continues;
context.brokenFlow = brokenFlow;
return false;
return hasLoopBodyEffects(context, this.body);
}

include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void {
this.included = true;
this.test.include(context, includeChildrenRecursively);
const { brokenFlow } = context;
this.body.include(context, includeChildrenRecursively, { asSingleStatement: true });
context.brokenFlow = brokenFlow;
includeLoopBody(context, this.body, includeChildrenRecursively);
}
}
15 changes: 3 additions & 12 deletions src/ast/nodes/ForInStatement.ts
View file
Expand Up @@ -15,6 +15,7 @@ import {
type StatementNode
} from './shared/Node';
import type { PatternNode } from './shared/Pattern';
import { hasLoopBodyEffects, includeLoopBody } from './shared/loops';

export default class ForInStatement extends StatementBase {
declare body: StatementNode;
Expand All @@ -30,15 +31,7 @@ export default class ForInStatement extends StatementBase {
const { body, deoptimized, left, right } = this;
if (!deoptimized) this.applyDeoptimizations();
if (left.hasEffectsAsAssignmentTarget(context, false) || right.hasEffects(context)) return true;
const { brokenFlow, ignore } = context;
const { breaks, continues } = ignore;
ignore.breaks = true;
ignore.continues = true;
if (body.hasEffects(context)) return true;
ignore.breaks = breaks;
ignore.continues = continues;
context.brokenFlow = brokenFlow;
return false;
return hasLoopBodyEffects(context, body);
}

include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void {
Expand All @@ -47,9 +40,7 @@ export default class ForInStatement extends StatementBase {
this.included = true;
left.includeAsAssignmentTarget(context, includeChildrenRecursively || true, false);
right.include(context, includeChildrenRecursively);
const { brokenFlow } = context;
body.include(context, includeChildrenRecursively, { asSingleStatement: true });
context.brokenFlow = brokenFlow;
includeLoopBody(context, body, includeChildrenRecursively);
}

initialise() {
Expand Down
5 changes: 2 additions & 3 deletions src/ast/nodes/ForOfStatement.ts
View file
Expand Up @@ -15,6 +15,7 @@ import {
type StatementNode
} from './shared/Node';
import type { PatternNode } from './shared/Pattern';
import { includeLoopBody } from './shared/loops';

export default class ForOfStatement extends StatementBase {
declare await: boolean;
Expand All @@ -39,9 +40,7 @@ export default class ForOfStatement extends StatementBase {
this.included = true;
left.includeAsAssignmentTarget(context, includeChildrenRecursively || true, false);
right.include(context, includeChildrenRecursively);
const { brokenFlow } = context;
body.include(context, includeChildrenRecursively, { asSingleStatement: true });
context.brokenFlow = brokenFlow;
includeLoopBody(context, body, includeChildrenRecursively);
}

initialise() {
Expand Down
18 changes: 5 additions & 13 deletions src/ast/nodes/ForStatement.ts
View file
Expand Up @@ -11,6 +11,7 @@ import {
StatementBase,
type StatementNode
} from './shared/Node';
import { hasLoopBodyEffects, includeLoopBody } from './shared/loops';

export default class ForStatement extends StatementBase {
declare body: StatementNode;
Expand All @@ -28,27 +29,18 @@ export default class ForStatement extends StatementBase {
this.init?.hasEffects(context) ||
this.test?.hasEffects(context) ||
this.update?.hasEffects(context)
)
) {
return true;
const { brokenFlow, ignore } = context;
const { breaks, continues } = ignore;
ignore.breaks = true;
ignore.continues = true;
if (this.body.hasEffects(context)) return true;
ignore.breaks = breaks;
ignore.continues = continues;
context.brokenFlow = brokenFlow;
return false;
}
return hasLoopBodyEffects(context, this.body);
}

include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void {
this.included = true;
this.init?.include(context, includeChildrenRecursively, { asSingleStatement: true });
this.test?.include(context, includeChildrenRecursively);
const { brokenFlow } = context;
this.update?.include(context, includeChildrenRecursively);
this.body.include(context, includeChildrenRecursively, { asSingleStatement: true });
context.brokenFlow = brokenFlow;
includeLoopBody(context, this.body, includeChildrenRecursively);
}

render(code: MagicString, options: RenderOptions): void {
Expand Down
18 changes: 6 additions & 12 deletions src/ast/nodes/IfStatement.ts
View file
@@ -1,11 +1,7 @@
import type MagicString from 'magic-string';
import type { RenderOptions } from '../../utils/renderHelpers';
import type { DeoptimizableEntity } from '../DeoptimizableEntity';
import {
BROKEN_FLOW_NONE,
type HasEffectsContext,
type InclusionContext
} from '../ExecutionContext';
import { type HasEffectsContext, type InclusionContext } from '../ExecutionContext';
import TrackingScope from '../scopes/TrackingScope';
import { EMPTY_PATH, SHARED_RECURSION_TRACKER } from '../utils/PathTracker';
import BlockStatement from './BlockStatement';
Expand Down Expand Up @@ -49,9 +45,8 @@ export default class IfStatement extends StatementBase implements DeoptimizableE
context.brokenFlow = brokenFlow;
if (this.alternate === null) return false;
if (this.alternate.hasEffects(context)) return true;
context.brokenFlow =
// eslint-disable-next-line unicorn/consistent-destructuring
context.brokenFlow < consequentBrokenFlow ? context.brokenFlow : consequentBrokenFlow;
// eslint-disable-next-line unicorn/consistent-destructuring
context.brokenFlow = context.brokenFlow && consequentBrokenFlow;
return false;
}
return testValue ? this.consequent.hasEffects(context) : !!this.alternate?.hasEffects(context);
Expand Down Expand Up @@ -168,7 +163,7 @@ export default class IfStatement extends StatementBase implements DeoptimizableE
private includeUnknownTest(context: InclusionContext) {
this.test.include(context, false);
const { brokenFlow } = context;
let consequentBrokenFlow = BROKEN_FLOW_NONE;
let consequentBrokenFlow = false;
if (this.consequent.shouldBeIncluded(context)) {
this.consequent.include(context, false, { asSingleStatement: true });
// eslint-disable-next-line unicorn/consistent-destructuring
Expand All @@ -177,9 +172,8 @@ export default class IfStatement extends StatementBase implements DeoptimizableE
}
if (this.alternate?.shouldBeIncluded(context)) {
this.alternate.include(context, false, { asSingleStatement: true });
context.brokenFlow =
// eslint-disable-next-line unicorn/consistent-destructuring
context.brokenFlow < consequentBrokenFlow ? context.brokenFlow : consequentBrokenFlow;
// eslint-disable-next-line unicorn/consistent-destructuring
context.brokenFlow = context.brokenFlow && consequentBrokenFlow;
}
}

Expand Down
10 changes: 3 additions & 7 deletions src/ast/nodes/ReturnStatement.ts
View file
@@ -1,10 +1,6 @@
import type MagicString from 'magic-string';
import type { RenderOptions } from '../../utils/renderHelpers';
import {
BROKEN_FLOW_ERROR_RETURN_LABEL,
type HasEffectsContext,
type InclusionContext
} from '../ExecutionContext';
import { type HasEffectsContext, type InclusionContext } from '../ExecutionContext';
import type * as NodeType from './NodeType';
import { UNKNOWN_EXPRESSION } from './shared/Expression';
import { type ExpressionNode, type IncludeChildren, StatementBase } from './shared/Node';
Expand All @@ -15,14 +11,14 @@ export default class ReturnStatement extends StatementBase {

hasEffects(context: HasEffectsContext): boolean {
if (!context.ignore.returnYield || this.argument?.hasEffects(context)) return true;
context.brokenFlow = BROKEN_FLOW_ERROR_RETURN_LABEL;
context.brokenFlow = true;
return false;
}

include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void {
this.included = true;
this.argument?.include(context, includeChildrenRecursively);
context.brokenFlow = BROKEN_FLOW_ERROR_RETURN_LABEL;
context.brokenFlow = true;
}

initialise(): void {
Expand Down
33 changes: 17 additions & 16 deletions src/ast/nodes/SwitchStatement.ts
View file
@@ -1,7 +1,6 @@
import type MagicString from 'magic-string';
import { type RenderOptions, renderStatementList } from '../../utils/renderHelpers';
import {
BROKEN_FLOW_BREAK_CONTINUE,
createHasEffectsContext,
type HasEffectsContext,
type InclusionContext
Expand All @@ -25,28 +24,32 @@ export default class SwitchStatement extends StatementBase {

hasEffects(context: HasEffectsContext): boolean {
if (this.discriminant.hasEffects(context)) return true;
const { brokenFlow, ignore } = context;
const { brokenFlow, hasBreak, ignore } = context;
const { breaks } = ignore;
let minBrokenFlow = Infinity;
ignore.breaks = true;
context.hasBreak = false;
let onlyHasBrokenFlow = true;
for (const switchCase of this.cases) {
if (switchCase.hasEffects(context)) return true;
// eslint-disable-next-line unicorn/consistent-destructuring
minBrokenFlow = context.brokenFlow < minBrokenFlow ? context.brokenFlow : minBrokenFlow;
onlyHasBrokenFlow &&= context.brokenFlow && !context.hasBreak;
context.hasBreak = false;
context.brokenFlow = brokenFlow;
}
if (this.defaultCase !== null && !(minBrokenFlow === BROKEN_FLOW_BREAK_CONTINUE)) {
context.brokenFlow = minBrokenFlow;
if (this.defaultCase !== null) {
context.brokenFlow = onlyHasBrokenFlow;
}
ignore.breaks = breaks;
context.hasBreak = hasBreak;
return false;
}

include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void {
this.included = true;
this.discriminant.include(context, includeChildrenRecursively);
const { brokenFlow } = context;
let minBrokenFlow = Infinity;
const { brokenFlow, hasBreak } = context;
context.hasBreak = false;
let onlyHasBrokenFlow = true;
let isCaseIncluded =
includeChildrenRecursively ||
(this.defaultCase !== null && this.defaultCase < this.cases.length - 1);
Expand All @@ -63,19 +66,17 @@ export default class SwitchStatement extends StatementBase {
if (isCaseIncluded) {
switchCase.include(context, includeChildrenRecursively);
// eslint-disable-next-line unicorn/consistent-destructuring
minBrokenFlow = minBrokenFlow < context.brokenFlow ? minBrokenFlow : context.brokenFlow;
onlyHasBrokenFlow &&= context.brokenFlow && !context.hasBreak;
context.hasBreak = false;
context.brokenFlow = brokenFlow;
} else {
minBrokenFlow = brokenFlow;
onlyHasBrokenFlow = brokenFlow;
}
}
if (
isCaseIncluded &&
this.defaultCase !== null &&
!(minBrokenFlow === BROKEN_FLOW_BREAK_CONTINUE)
) {
context.brokenFlow = minBrokenFlow;
if (isCaseIncluded && this.defaultCase !== null) {
context.brokenFlow = onlyHasBrokenFlow;
}
context.hasBreak = hasBreak;
}

initialise(): void {
Expand Down

0 comments on commit baa0311

Please sign in to comment.