Skip to content

Commit

Permalink
fix(ivy): don't infer template context types when in full mode (#33537)
Browse files Browse the repository at this point in the history
The Ivy template type-checker is capable of inferring the type of a
structural directive (such as NgForOf<T>). Previously, this was done with
fullTemplateTypeCheck: true, even if strictTemplates was false. View Engine
previously did not do this inference, and so this causes breakages if the
type of the template context is not what the user expected.

In particular, consider the template:

```html
<div *ngFor="let user of users as all">
  {{user.index}} out of {{all.length}}
</div>
```

As long as `users` is an array, this seems reasonable, because it appears
that `all` is an alias for the `users` array. However, this is misleading.

In reality, `NgForOf` is rendered with a template context that contains
both a `$implicit` value (for the loop variable `user`) as well as a
`ngForOf` value, which is the actual value assigned to `all`. The type of
`NgForOf`'s template context is `NgForContext<T>`, which declares `ngForOf`'s
type to be `NgIterable<T>`, which does not have a `length` property (due to
its incorporation of the `Iterable` type).

This commit stops the template type-checker from inferring template context
types unless strictTemplates is set (and strictInputTypes is not disabled).

Fixes #33527.

PR Close #33537
  • Loading branch information
alxhub committed Nov 20, 2019
1 parent 97fbdab commit eb6975a
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 23 deletions.
3 changes: 2 additions & 1 deletion packages/compiler-cli/src/ngtsc/program.ts
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,7 @@ export class NgtscProgram implements api.Program {
if (this.options.fullTemplateTypeCheck) {
const strictTemplates = !!this.options.strictTemplates;
typeCheckingConfig = {
applyTemplateContextGuards: true,
applyTemplateContextGuards: strictTemplates,
checkQueries: false,
checkTemplateBodies: true,
checkTypeOfInputBindings: strictTemplates,
Expand Down Expand Up @@ -468,6 +468,7 @@ export class NgtscProgram implements api.Program {
// based on "fullTemplateTypeCheck".
if (this.options.strictInputTypes !== undefined) {
typeCheckingConfig.checkTypeOfInputBindings = this.options.strictInputTypes;
typeCheckingConfig.applyTemplateContextGuards = this.options.strictInputTypes;
}
if (this.options.strictNullInputTypes !== undefined) {
typeCheckingConfig.strictNullInputBindings = this.options.strictNullInputTypes;
Expand Down
72 changes: 50 additions & 22 deletions packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,16 @@ export declare class IndexPipe {
static ɵpipe: i0.ɵPipeDefWithMeta<IndexPipe, 'index'>;
}
export declare class SlicePipe {
transform<T>(value: ReadonlyArray<T>, start: number, end?: number): Array<T>;
transform(value: string, start: number, end?: number): string;
transform(value: null, start: number, end?: number): null;
transform(value: undefined, start: number, end?: number): undefined;
transform(value: any, start: number, end?: number): any;
static ɵpipe: i0.ɵPipeDefWithMeta<SlicePipe, 'slice'>;
}
export declare class NgForOf<T> {
ngForOf: i0.NgIterable<T>;
static ngTemplateContextGuard<T>(dir: NgForOf<T>, ctx: any): ctx is NgForOfContext<T>;
Expand All @@ -56,7 +66,7 @@ export declare class NgIf {
}
export declare class CommonModule {
static ɵmod: i0.ɵɵNgModuleDefWithMeta<CommonModule, [typeof NgIf, typeof NgForOf, typeof IndexPipe], never, [typeof NgIf, typeof NgForOf, typeof IndexPipe]>;
static ɵmod: i0.ɵɵNgModuleDefWithMeta<CommonModule, [typeof NgIf, typeof NgForOf, typeof IndexPipe, typeof SlicePipe], never, [typeof NgIf, typeof NgForOf, typeof IndexPipe, typeof SlicePipe]>;
}
`);
env.write('node_modules/@angular/animations/index.d.ts', `
Expand Down Expand Up @@ -862,30 +872,48 @@ export declare class AnimationEvent {
expect(diags[0].length).toBe(19);
});

it('should property type-check a microsyntax variable with the same name as the expression',
() => {
env.write('test.ts', `
import {CommonModule} from '@angular/common';
import {Component, Input, NgModule} from '@angular/core';
describe('microsyntax variables', () => {
beforeEach(() => {
// Use the same template for both tests
env.write('test.ts', `
import {CommonModule} from '@angular/common';
import {Component, NgModule} from '@angular/core';
@Component({
selector: 'test',
template: \`<div *ngFor="let foo of foos as foos">
{{foo.name}} of {{foos.length}}
</div>
\`,
})
export class TestCmp {
foos: {name: string}[];
}
@NgModule({
declarations: [TestCmp],
imports: [CommonModule],
})
export class Module {}
`);
});

@Component({
selector: 'test',
template: '<div *ngIf="foo as foo">{{foo}}</div>',
})
export class TestCmp<T extends {name: string}> {
foo: any;
}
it('should be treated as \'any\' without strictTemplates', () => {
env.tsconfig({fullTemplateTypeCheck: true, strictTemplates: false});

@NgModule({
declarations: [TestCmp],
imports: [CommonModule],
})
export class Module {}
`);
const diags = env.driveDiagnostics();
expect(diags.length).toBe(0);
});

const diags = env.driveDiagnostics();
expect(diags.length).toBe(0);
});
it('should be correctly inferred under strictTemplates', () => {
env.tsconfig({fullTemplateTypeCheck: true, strictTemplates: true});

const diags = env.driveDiagnostics();
expect(diags.length).toBe(1);
expect((diags[0].messageText as ts.DiagnosticMessageChain).messageText)
.toBe(`Property 'length' does not exist on type 'NgIterable<{ name: string; }>'.`);
});
});

it('should properly type-check inherited directives', () => {
env.tsconfig({fullTemplateTypeCheck: true, strictInputTypes: true});
Expand Down

0 comments on commit eb6975a

Please sign in to comment.