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): work around TypeScript bug when narrowing switch statements #52110

Closed
wants to merge 1 commit into from
Closed

fix(compiler): work around TypeScript bug when narrowing switch statements #52110

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
65 changes: 48 additions & 17 deletions packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
View file
Open in desktop
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {AST, BindingPipe, BindingType, BoundTarget, Call, DYNAMIC_TYPE, ImplicitReceiver, ParsedEventType, ParseSourceSpan, PropertyRead, PropertyWrite, SafeCall, SafePropertyRead, SchemaMetadata, ThisReceiver, TmplAstBoundAttribute, TmplAstBoundEvent, TmplAstBoundText, TmplAstDeferredBlock, TmplAstDeferredBlockTriggers, TmplAstElement, TmplAstForLoopBlock, TmplAstHoverDeferredTrigger, TmplAstIcu, TmplAstIfBlock, TmplAstIfBlockBranch, TmplAstInteractionDeferredTrigger, TmplAstNode, TmplAstReference, TmplAstSwitchBlock, TmplAstTemplate, TmplAstTextAttribute, TmplAstVariable, TmplAstViewportDeferredTrigger, TransplantedType} from '@angular/compiler';
import {AST, BindingPipe, BindingType, BoundTarget, Call, DYNAMIC_TYPE, ImplicitReceiver, ParsedEventType, ParseSourceSpan, PropertyRead, PropertyWrite, SafeCall, SafePropertyRead, SchemaMetadata, ThisReceiver, TmplAstBoundAttribute, TmplAstBoundEvent, TmplAstBoundText, TmplAstDeferredBlock, TmplAstDeferredBlockTriggers, TmplAstElement, TmplAstForLoopBlock, TmplAstHoverDeferredTrigger, TmplAstIcu, TmplAstIfBlock, TmplAstIfBlockBranch, TmplAstInteractionDeferredTrigger, TmplAstNode, TmplAstReference, TmplAstSwitchBlock, TmplAstSwitchBlockCase, TmplAstTemplate, TmplAstTextAttribute, TmplAstVariable, TmplAstViewportDeferredTrigger, TransplantedType} from '@angular/compiler';
import ts from 'typescript';

import {Reference} from '../../imports';
Expand Down Expand Up @@ -1273,27 +1273,56 @@ class TcbSwitchOp extends TcbOp {
}

override execute(): null {
const clauses: ts.CaseOrDefaultClause[] = [];
const expression = tcbExpression(this.block.expression, this.tcb, this.scope);

for (const current of this.block.cases) {
// Our template switch statements don't fall through so we always have a break at the end.
const breakStatement = ts.factory.createBreakStatement();
const clauseScope = Scope.forNodes(this.tcb, this.scope, null, current.children, 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`.
markIgnoreDiagnostics(expression);
const root = this.generateCase(0, expression, null);

if (current.expression === null) {
clauses.push(ts.factory.createDefaultClause([...clauseScope.render(), breakStatement]));
} else {
clauses.push(ts.factory.createCaseClause(
tcbExpression(current.expression, this.tcb, clauseScope),
[...clauseScope.render(), breakStatement]));
if (root !== undefined) {
this.scope.addStatement(root);
}

return null;
}

private generateCase(
index: number, switchValue: ts.Expression,
crisbeto marked this conversation as resolved.
Show resolved Hide resolved
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) {
// TODO(crisbeto): pass the guard once #52069 is merged.
const defaultScope = Scope.forNodes(this.tcb, this.scope, null, defaultCase.children, null);
return ts.factory.createBlock(defaultScope.render());
}
return undefined;
}

this.scope.addStatement(ts.factory.createSwitchStatement(
tcbExpression(this.block.expression, this.tcb, this.scope),
ts.factory.createCaseBlock(clauses)));
// 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);
}

return null;
// TODO(crisbeto): pass the guard once #52069 is merged.
const caseScope = Scope.forNodes(this.tcb, this.scope, null, current.children, null);
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));
}
}

Expand Down Expand Up @@ -1712,7 +1741,9 @@ 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 TcbSwitchOp(this.tcb, this, node));
this.opQueue.push(
new TcbExpressionOp(this.tcb, this, node.expression),
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
23 changes: 18 additions & 5 deletions packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts
View file
Open in desktop
Expand Up @@ -1444,8 +1444,20 @@ describe('type check blocks', () => {

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

it('should generate a switch block that only has a default case', () => {
const TEMPLATE = `
@switch (expr) {
@default {
{{default()}}
}
}
`;

expect(tcb(TEMPLATE)).toContain('{ "" + ((this).default()); }');
});

it('should generate a switch block inside a template', () => {
Expand All @@ -1467,9 +1479,10 @@ describe('type check blocks', () => {

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

Expand Down
99 changes: 99 additions & 0 deletions packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts
View file
Open in desktop
Expand Up @@ -3996,6 +3996,62 @@ suppress
]);
});

it('should only produce one diagnostic if the switch expression has an error', () => {
env.write('test.ts', `
import {Component} from '@angular/core';

@Component({
template: \`
@switch (does_not_exist_main) {
@case (1) {
One
}

@case (2) {
Two
}
}
\`,
standalone: true,
})
export class Main {}
`);

const diags = env.driveDiagnostics();
expect(diags.map(d => ts.flattenDiagnosticMessageText(d.messageText, ''))).toEqual([
`Property 'does_not_exist_main' does not exist on type 'Main'.`,
]);
});

it('should check a switch block that only has a default case', () => {
env.write('test.ts', `
import {Component} from '@angular/core';

@Component({
template: \`
@switch (expr) {
@default {
{{acceptsNumber(expr)}}
}
}
\`,
standalone: true,
})
export class Main {
expr: 'hello' | 1 = 'hello';

acceptsNumber(value: number) {
return value;
}
}
`);

const diags = env.driveDiagnostics();
expect(diags.map(d => ts.flattenDiagnosticMessageText(d.messageText, ''))).toEqual([
`Argument of type 'string | number' is not assignable to parameter of type 'number'. Type 'string' is not assignable to type 'number'.`
]);
});

it('should narrow the type inside switch cases', () => {
env.write('test.ts', `
import {Component} from '@angular/core';
Expand Down Expand Up @@ -4027,6 +4083,49 @@ suppress
`Argument of type 'string' is not assignable to parameter of type 'number'.`
]);
});

it('should narrow the switch type based on a field', () => {
env.write('test.ts', `
import {Component} from '@angular/core';

export interface Base {
type: 'foo' | 'bar'
}

export interface Foo extends Base {
type: 'foo';
foo: string;
}

export interface Bar extends Base {
type: 'bar';
bar: number;
}

@Component({
template: \`
@switch (value.type) {
@case ('foo') {
{{ acceptsNumber(value.foo) }}
}
}
\`,
standalone: true,
})
export class Main {
value: Foo | Bar = { type: 'foo', foo: 'foo' };

acceptsNumber(value: number) {
return value;
}
}
`);

const diags = env.driveDiagnostics();
expect(diags.map(d => ts.flattenDiagnosticMessageText(d.messageText, ''))).toEqual([
`Argument of type 'string' is not assignable to parameter of type 'number'.`
]);
});
});

describe('for loop blocks', () => {
Expand Down