Skip to content

Commit

Permalink
fix(compiler): template type checking not reporting diagnostics for i…
Browse files Browse the repository at this point in the history
…ncompatible type comparisons (angular#52322)

In angular#52110 the compiler was changed to produce `if` statements when type checking `@switch` in order to avoid a bug in the TypeScript compiler. In order to avoid duplicate diagnostics, the main `@switch` expression was ignored in each of the `@case` comparisons. This appears to have caused a regression where comparing incompatible types wasn't being reported anymore.

These changes resolve the issue by wrapping the expression in parentheses which allows the compiler to report comparison diagnostics while ignoring diagnostics in the expression itself.

Fixes angular#52315.

PR Close angular#52322
  • Loading branch information
crisbeto authored and tbondwilkinson committed Dec 6, 2023
1 parent 9115c3f commit a5430e1
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 10 deletions.
Expand Up @@ -1327,12 +1327,15 @@ class TcbSwitchOp extends TcbOp {
}

override execute(): null {
const expression = tcbExpression(this.block.expression, this.tcb, this.scope);

// 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 comparisonExpression = tcbExpression(this.block.expression, this.tcb, this.scope);
markIgnoreDiagnostics(comparisonExpression);

// Wrap the comparisson 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) {
Expand Down
Expand Up @@ -1468,8 +1468,8 @@ describe('type check blocks', () => {

expect(tcb(TEMPLATE))
.toContain(
'if (((this).expr) === 1) { "" + ((this).one()); } else if ' +
'(((this).expr) === 2) { "" + ((this).two()); } else { "" + ((this).default()); }');
'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', () => {
Expand Down Expand Up @@ -1501,9 +1501,10 @@ 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 @@ -1526,8 +1527,8 @@ describe('type check blocks', () => {
expect(tcb(TEMPLATE))
.toContain(
'var _t1: any = null!; { var _t2 = (_t1.exp); _t2(); ' +
'if (_t2() === "one") { "" + ((this).one()); } ' +
'else if (_t2() === "two") { "" + ((this).two()); } ' +
'if ((_t2()) === "one") { "" + ((this).one()); } ' +
'else if ((_t2()) === "two") { "" + ((this).two()); } ' +
'else { "" + ((this).default()); } }');
});
});
Expand Down
25 changes: 25 additions & 0 deletions packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts
Expand Up @@ -4294,6 +4294,31 @@ suppress
]);
});

it('should produce a diagnostic if @switch and @case have different types', () => {
env.write('test.ts', `
import {Component} from '@angular/core';
@Component({
template: \`
@switch (expr) {
@case (1) {
{{expr}}
}
}
\`,
standalone: true,
})
export class Main {
expr = true;
}
`);

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.`,
]);
});

it('should narrow the type in listener inside switch cases with expressions', () => {
env.write('test.ts', `
import {Component} from '@angular/core';
Expand Down

0 comments on commit a5430e1

Please sign in to comment.