Skip to content

Commit

Permalink
fix(ivy): avoid implicit any errors in event handlers (#33550)
Browse files Browse the repository at this point in the history
When template type checking is configured with `strictDomEventTypes` or
`strictOutputEventTypes` disabled, in compilation units that have
`noImplicitAny` enabled but `strictNullChecks` disabled, a template type
checking error could be produced for certain event handlers.

The error is avoided by letting an event handler in the generated TCB
always have an explicit `any` return type.

Fixes #33528

PR Close #33550
  • Loading branch information
JoostK authored and atscott committed Nov 6, 2019
1 parent d749dd3 commit e2d7b25
Show file tree
Hide file tree
Showing 4 changed files with 40 additions and 21 deletions.
Expand Up @@ -1276,7 +1276,7 @@ function tcbCreateEventHandler(
/* modifier */ undefined,
/* typeParameters */ undefined,
/* parameters */[eventParam],
/* type */ undefined,
/* type */ ts.createKeywordTypeNode(ts.SyntaxKind.AnyKeyword),
/* equalsGreaterThanToken*/ undefined,
/* body */ handler);
}
Expand Down
24 changes: 21 additions & 3 deletions packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts
Expand Up @@ -6,9 +6,11 @@
* found in the LICENSE file at https://angular.io/license
*/

import {TestFile, runInEachFileSystem} from '@angular/compiler-cli/src/ngtsc/file_system/testing';
import * as ts from 'typescript';

import {TestFile, runInEachFileSystem} from '../../file_system/testing';
import {TypeCheckingConfig} from '../src/api';

import {TestDeclaration, ngForDeclaration, ngForDts, typecheck} from './test_utils';

runInEachFileSystem(() => {
Expand Down Expand Up @@ -258,6 +260,21 @@ runInEachFileSystem(() => {

expect(messages).toEqual([]);
});

// https://github.com/angular/angular/issues/33528
it('should not produce a diagnostic for implicit any return types', () => {
const messages = diagnose(
`<div (click)="state = null"></div>`, `
class TestComponent {
state: any;
}`,
// Disable strict DOM event checking and strict null checks, to infer an any return type
// that would be implicit if the handler function would not have an explicit return
// type.
[], [], {checkTypeOfDomEvents: false}, {strictNullChecks: false});

expect(messages).toEqual([]);
});
});

describe('strict null checks', () => {
Expand Down Expand Up @@ -314,8 +331,9 @@ class TestComponent {

function diagnose(
template: string, source: string, declarations?: TestDeclaration[],
additionalSources: TestFile[] = []): string[] {
const diagnostics = typecheck(template, source, declarations, additionalSources);
additionalSources: TestFile[] = [], config?: Partial<TypeCheckingConfig>,
options?: ts.CompilerOptions): string[] {
const diagnostics = typecheck(template, source, declarations, additionalSources, config, options);
return diagnostics.map(diag => {
const text =
typeof diag.messageText === 'string' ? diag.messageText : diag.messageText.messageText;
Expand Down
7 changes: 4 additions & 3 deletions packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts
Expand Up @@ -230,7 +230,8 @@ export function tcb(

export function typecheck(
template: string, source: string, declarations: TestDeclaration[] = [],
additionalSources: {name: AbsoluteFsPath; contents: string}[] = []): ts.Diagnostic[] {
additionalSources: {name: AbsoluteFsPath; contents: string}[] = [],
config: Partial<TypeCheckingConfig> = {}, opts: ts.CompilerOptions = {}): ts.Diagnostic[] {
const typeCheckFilePath = absoluteFrom('/_typecheck_.ts');
const files = [
typescriptLibDts(),
Expand All @@ -243,7 +244,7 @@ export function typecheck(
...additionalSources,
];
const {program, host, options} =
makeProgram(files, {strictNullChecks: true, noImplicitAny: true}, undefined, false);
makeProgram(files, {strictNullChecks: true, noImplicitAny: true, ...opts}, undefined, false);
const sf = program.getSourceFile(absoluteFrom('/main.ts')) !;
const checker = program.getTypeChecker();
const logicalFs = new LogicalFileSystem(getRootDirs(host, options));
Expand All @@ -254,7 +255,7 @@ export function typecheck(
program, checker, options, host, new TypeScriptReflectionHost(checker)),
new LogicalProjectStrategy(reflectionHost, logicalFs),
]);
const ctx = new TypeCheckContext(ALL_ENABLED_CONFIG, emitter, typeCheckFilePath);
const ctx = new TypeCheckContext({...ALL_ENABLED_CONFIG, ...config}, emitter, typeCheckFilePath);

const templateUrl = 'synthetic.html';
const templateFile = new ParseSourceFile(template, templateUrl);
Expand Down
Expand Up @@ -253,26 +253,26 @@ describe('type check blocks', () => {
const TEMPLATE = `<div dir (dirOutput)="foo($event)"></div>`;
const block = tcb(TEMPLATE, DIRECTIVES);
expect(block).toContain(
'_outputHelper(_t2.outputField).subscribe($event => (ctx).foo($event));');
'_outputHelper(_t2.outputField).subscribe(($event): any => (ctx).foo($event));');
});

it('should emit a listener function with AnimationEvent for animation events', () => {
const TEMPLATE = `<div (@animation.done)="foo($event)"></div>`;
const block = tcb(TEMPLATE);
expect(block).toContain('($event: animations.AnimationEvent) => (ctx).foo($event);');
expect(block).toContain('($event: animations.AnimationEvent): any => (ctx).foo($event);');
});

it('should emit addEventListener calls for unclaimed outputs', () => {
const TEMPLATE = `<div (event)="foo($event)"></div>`;
const block = tcb(TEMPLATE);
expect(block).toContain('_t1.addEventListener("event", $event => (ctx).foo($event));');
expect(block).toContain('_t1.addEventListener("event", ($event): any => (ctx).foo($event));');
});

it('should allow to cast $event using $any', () => {
const TEMPLATE = `<div (event)="foo($any($event))"></div>`;
const block = tcb(TEMPLATE);
expect(block).toContain(
'_t1.addEventListener("event", $event => (ctx).foo(($event as any)));');
'_t1.addEventListener("event", ($event): any => (ctx).foo(($event as any)));');
});

});
Expand Down Expand Up @@ -374,18 +374,18 @@ describe('type check blocks', () => {
it('should check types of directive outputs when enabled', () => {
const block = tcb(TEMPLATE, DIRECTIVES);
expect(block).toContain(
'_outputHelper(_t2.outputField).subscribe($event => (ctx).foo($event));');
'_outputHelper(_t2.outputField).subscribe(($event): any => (ctx).foo($event));');
expect(block).toContain(
'_t1.addEventListener("nonDirOutput", $event => (ctx).foo($event));');
'_t1.addEventListener("nonDirOutput", ($event): any => (ctx).foo($event));');
});
it('should not check types of directive outputs when disabled', () => {
const DISABLED_CONFIG:
TypeCheckingConfig = {...BASE_CONFIG, checkTypeOfOutputEvents: false};
const block = tcb(TEMPLATE, DIRECTIVES, DISABLED_CONFIG);
expect(block).toContain('($event: any) => (ctx).foo($event);');
expect(block).toContain('($event: any): any => (ctx).foo($event);');
// Note that DOM events are still checked, that is controlled by `checkTypeOfDomEvents`
expect(block).toContain(
'_t1.addEventListener("nonDirOutput", $event => (ctx).foo($event));');
'_t1.addEventListener("nonDirOutput", ($event): any => (ctx).foo($event));');
});
});

Expand All @@ -394,13 +394,13 @@ describe('type check blocks', () => {

it('should check types of animation events when enabled', () => {
const block = tcb(TEMPLATE, DIRECTIVES);
expect(block).toContain('($event: animations.AnimationEvent) => (ctx).foo($event);');
expect(block).toContain('($event: animations.AnimationEvent): any => (ctx).foo($event);');
});
it('should not check types of animation events when disabled', () => {
const DISABLED_CONFIG:
TypeCheckingConfig = {...BASE_CONFIG, checkTypeOfAnimationEvents: false};
const block = tcb(TEMPLATE, DIRECTIVES, DISABLED_CONFIG);
expect(block).toContain('($event: any) => (ctx).foo($event);');
expect(block).toContain('($event: any): any => (ctx).foo($event);');
});
});

Expand All @@ -410,18 +410,18 @@ describe('type check blocks', () => {
it('should check types of DOM events when enabled', () => {
const block = tcb(TEMPLATE, DIRECTIVES);
expect(block).toContain(
'_outputHelper(_t2.outputField).subscribe($event => (ctx).foo($event));');
'_outputHelper(_t2.outputField).subscribe(($event): any => (ctx).foo($event));');
expect(block).toContain(
'_t1.addEventListener("nonDirOutput", $event => (ctx).foo($event));');
'_t1.addEventListener("nonDirOutput", ($event): any => (ctx).foo($event));');
});
it('should not check types of DOM events when disabled', () => {
const DISABLED_CONFIG: TypeCheckingConfig = {...BASE_CONFIG, checkTypeOfDomEvents: false};
const block = tcb(TEMPLATE, DIRECTIVES, DISABLED_CONFIG);
// Note that directive outputs are still checked, that is controlled by
// `checkTypeOfOutputEvents`
expect(block).toContain(
'_outputHelper(_t2.outputField).subscribe($event => (ctx).foo($event));');
expect(block).toContain('($event: any) => (ctx).foo($event);');
'_outputHelper(_t2.outputField).subscribe(($event): any => (ctx).foo($event));');
expect(block).toContain('($event: any): any => (ctx).foo($event);');
});
});

Expand Down

0 comments on commit e2d7b25

Please sign in to comment.