Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(compiler-cli): use switch statements to narrow Angular switch blocks #55168

Closed
wants to merge 1 commit into from
Closed

fix(compiler-cli): use switch statements to narrow Angular switch blocks #55168

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 16 additions & 55 deletions packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1480,62 +1480,25 @@ class TcbSwitchOp extends TcbOp {
}

override execute(): null {
// Since we'll use the expression in multiple comparisons, we don't want to
// log the same diagnostic more than once. Ignore this expression since we already
// produced a `TcbExpressionOp`.
const comparisonExpression = tcbExpression(this.block.expression, this.tcb, this.scope);
markIgnoreDiagnostics(comparisonExpression);

// Wrap the comparison expression in parentheses so we don't ignore
// diagnostics when comparing incompatible types (see #52315).
const expression = ts.factory.createParenthesizedExpression(comparisonExpression);
const root = this.generateCase(0, expression, null);

if (root !== undefined) {
this.scope.addStatement(root);
}
const switchExpression = tcbExpression(this.block.expression, this.tcb, this.scope);
const clauses = this.block.cases.map(current => {
const clauseScope = Scope.forNodes(
this.tcb, this.scope, null, current.children,
this.generateGuard(current, switchExpression));
const statements = [...clauseScope.render(), ts.factory.createBreakStatement()];

return current.expression === null ?
ts.factory.createDefaultClause(statements) :
ts.factory.createCaseClause(
tcbExpression(current.expression, this.tcb, clauseScope), statements);
});

this.scope.addStatement(
ts.factory.createSwitchStatement(switchExpression, ts.factory.createCaseBlock(clauses)));

return null;
}

private generateCase(
index: number, switchValue: ts.Expression,
defaultCase: TmplAstSwitchBlockCase|null): ts.Statement|undefined {
// If we've reached the end, output the default case as the final `else`.
if (index >= this.block.cases.length) {
if (defaultCase !== null) {
const defaultScope = Scope.forNodes(
this.tcb, this.scope, null, defaultCase.children,
this.generateGuard(defaultCase, switchValue));
return ts.factory.createBlock(defaultScope.render());
}
return undefined;
}

// If the current case is the default (could be placed arbitrarily),
// skip it since it needs to be generated as the final `else` at the end.
const current = this.block.cases[index];
if (current.expression === null) {
return this.generateCase(index + 1, switchValue, current);
}

const caseScope = Scope.forNodes(
this.tcb, this.scope, null, current.children, this.generateGuard(current, switchValue));
const caseValue = tcbExpression(current.expression, this.tcb, caseScope);

// TODO(crisbeto): change back to a switch statement when the TS bug is resolved.
// Note that it would be simpler to generate a `switch` statement to represent the user's
// code, but the current TypeScript version (at the time of writing 5.2.2) has a bug where
// parenthesized `switch` expressions aren't narrowed correctly:
// https://github.com/microsoft/TypeScript/issues/56030. We work around it by generating
// `if`/`else if`/`else` statements to represent the switch block instead.
return ts.factory.createIfStatement(
ts.factory.createBinaryExpression(
switchValue, ts.SyntaxKind.EqualsEqualsEqualsToken, caseValue),
ts.factory.createBlock(caseScope.render()),
this.generateCase(index + 1, switchValue, defaultCase));
}

private generateGuard(node: TmplAstSwitchBlockCase, switchValue: ts.Expression): ts.Expression
|null {
// For non-default cases, the guard needs to compare against the case value, e.g.
Expand Down Expand Up @@ -2024,9 +1987,7 @@ class Scope {
} else if (node instanceof TmplAstIfBlock) {
this.opQueue.push(new TcbIfOp(this.tcb, this, node));
} else if (node instanceof TmplAstSwitchBlock) {
this.opQueue.push(
new TcbExpressionOp(this.tcb, this, node.expression),
new TcbSwitchOp(this.tcb, this, node));
this.opQueue.push(new TcbSwitchOp(this.tcb, this, node));
} else if (node instanceof TmplAstForLoopBlock) {
this.opQueue.push(new TcbForOfOp(this.tcb, this, node));
node.empty && this.appendChildren(node.empty);
Expand Down
26 changes: 14 additions & 12 deletions packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1589,8 +1589,10 @@ describe('type check blocks', () => {

expect(tcb(TEMPLATE))
.toContain(
'if ((((this).expr)) === 1) { "" + ((this).one()); } else if ' +
'((((this).expr)) === 2) { "" + ((this).two()); } else { "" + ((this).default()); }');
'switch (((this).expr)) { ' +
'case 1: "" + ((this).one()); break; ' +
'case 2: "" + ((this).two()); break; ' +
'default: "" + ((this).default()); break; }');
});

it('should generate a switch block that only has a default case', () => {
Expand All @@ -1602,7 +1604,8 @@ describe('type check blocks', () => {
}
`;

expect(tcb(TEMPLATE)).toContain('{ "" + ((this).default()); }');
expect(tcb(TEMPLATE))
.toContain('switch (((this).expr)) { default: "" + ((this).default()); break; }');
});

it('should generate a guard expression for a listener inside a switch case', () => {
Expand All @@ -1622,10 +1625,9 @@ describe('type check blocks', () => {

const result = tcb(TEMPLATE);

expect(result).toContain(`if ((((this).expr)) === 1) (this).one();`);
expect(result).toContain(`if ((((this).expr)) === 2) (this).two();`);
expect(result).toContain(
`if ((((this).expr)) !== 1 && (((this).expr)) !== 2) (this).default();`);
expect(result).toContain(`if (((this).expr) === 1) (this).one();`);
expect(result).toContain(`if (((this).expr) === 2) (this).two();`);
expect(result).toContain(`if (((this).expr) !== 1 && ((this).expr) !== 2) (this).default();`);
});

it('should generate a switch block inside a template', () => {
Expand All @@ -1647,14 +1649,14 @@ describe('type check blocks', () => {

expect(tcb(TEMPLATE))
.toContain(
'var _t1 = null! as any; { var _t2 = (_t1.exp); _t2(); ' +
'if ((_t2()) === "one") { "" + ((this).one()); } ' +
'else if ((_t2()) === "two") { "" + ((this).two()); } ' +
'else { "" + ((this).default()); } }');
'var _t1 = null! as any; { var _t2 = (_t1.exp); switch (_t2()) { ' +
'case "one": "" + ((this).one()); break; ' +
'case "two": "" + ((this).two()); break; ' +
'default: "" + ((this).default()); break; } }');
});

it('should handle an empty switch block', () => {
expect(tcb('@switch (expr) {}')).toContain('if (true) { ((this).expr); }');
expect(tcb('@switch (expr) {}')).toContain('if (true) { switch (((this).expr)) { } }');
});
});

Expand Down
2 changes: 1 addition & 1 deletion packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4643,7 +4643,7 @@ suppress

const diags = env.driveDiagnostics();
expect(diags.map(d => ts.flattenDiagnosticMessageText(d.messageText, ''))).toEqual([
`This comparison appears to be unintentional because the types 'boolean' and 'number' have no overlap.`,
`Type 'number' is not comparable to type 'boolean'.`,
]);
});

Expand Down