Skip to content

Commit

Permalink
feat(ivy): input type coercion for template type-checking
Browse files Browse the repository at this point in the history
Often the types of an `@Input`'s field don't fully reflect the types of
assignable values. This can happen when an input has a getter/setter pair
where the getter always returns a narrow type, and the setter coerces a
wider value down to the narrow type.

For example, you could imagine an input of the form:

```typescript
@input() get value(): string {
  return this._value;
}

set value(v: {toString(): string}) {
  this._value = v.toString();
}
```

Here, the getter always returns a `string`, but the setter accepts any value
that can be `toString()`'d, and coerces it to a string.

Unfortunately TypeScript does not actually support this syntax, and so
Angular users are forced to type their setters as narrowly as the getters,
even though at runtime the coercion works just fine.

To support these kinds of patterns (e.g. as used by Material), this commit
adds a compiler feature called "input type coercion". When a binding is made
to the 'value' input of a directive like MatInput, the compiler will look
for a static function with the name ngInputAcceptType_value. If such a function
is found, the type-checking expression for the input will be wrapped in a call
to the function, allowing for the expression of a type conversion between
the binding expression and the value being written to the input's field.

To solve the case above, for example, MatInput might write:

```typescript
class MatInput {
  // rest of the directive...

  static ngInputAcceptType_value: {toString(): string};
}
```

FW-1475 #resolve
  • Loading branch information
alxhub authored and IgorMinar committed Oct 23, 2019
1 parent 398ff1e commit 678b2f0
Show file tree
Hide file tree
Showing 10 changed files with 187 additions and 15 deletions.
1 change: 1 addition & 0 deletions packages/compiler-cli/src/ngtsc/metadata/src/api.ts
Expand Up @@ -34,6 +34,7 @@ export interface DirectiveMeta extends T2DirectiveMeta {
queries: string[];
ngTemplateGuards: TemplateGuardMeta[];
hasNgTemplateContextGuard: boolean;
coercedInputFields: Set<string>;

/**
* A `Reference` to the base class for the directive, if one was detected.
Expand Down
24 changes: 22 additions & 2 deletions packages/compiler-cli/src/ngtsc/metadata/src/util.ts
Expand Up @@ -82,20 +82,25 @@ export function readStringArrayType(type: ts.TypeNode): string[] {
export function extractDirectiveGuards(node: ClassDeclaration, reflector: ReflectionHost): {
ngTemplateGuards: TemplateGuardMeta[],
hasNgTemplateContextGuard: boolean,
coercedInputFields: Set<string>,
} {
const staticMembers = reflector.getMembersOfClass(node).filter(member => member.isStatic);
const ngTemplateGuards = staticMembers.map(extractTemplateGuard)
.filter((guard): guard is TemplateGuardMeta => guard !== null);
const hasNgTemplateContextGuard = staticMembers.some(
member => member.kind === ClassMemberKind.Method && member.name === 'ngTemplateContextGuard');
return {hasNgTemplateContextGuard, ngTemplateGuards};

const coercedInputFields =
new Set(staticMembers.map(extractCoercedInput)
.filter((inputName): inputName is string => inputName !== null));
return {hasNgTemplateContextGuard, ngTemplateGuards, coercedInputFields};
}

function extractTemplateGuard(member: ClassMember): TemplateGuardMeta|null {
if (!member.name.startsWith('ngTemplateGuard_')) {
return null;
}
const inputName = member.name.split('_', 2)[1];
const inputName = afterUnderscore(member.name);
if (member.kind === ClassMemberKind.Property) {
let type: string|null = null;
if (member.type !== null && ts.isLiteralTypeNode(member.type) &&
Expand All @@ -115,6 +120,13 @@ function extractTemplateGuard(member: ClassMember): TemplateGuardMeta|null {
}
}

function extractCoercedInput(member: ClassMember): string|null {
if (member.kind !== ClassMemberKind.Property || !member.name.startsWith('ngAcceptInputType_')) {
return null !;
}
return afterUnderscore(member.name);
}

/**
* A `MetadataReader` that reads from an ordered set of child readers until it obtains the requested
* metadata.
Expand Down Expand Up @@ -158,3 +170,11 @@ export class CompoundMetadataReader implements MetadataReader {
return null;
}
}

function afterUnderscore(str: string): string {
const pos = str.indexOf('_');
if (pos === -1) {
throw new Error(`Expected '${str}' to contain '_'`);
}
return str.substr(pos + 1);
}
1 change: 1 addition & 0 deletions packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts
Expand Up @@ -228,6 +228,7 @@ function fakeDirective(ref: Reference<ClassDeclaration>): DirectiveMeta {
queries: [],
hasNgTemplateContextGuard: false,
ngTemplateGuards: [],
coercedInputFields: new Set<string>(),
baseClass: null,
};
}
Expand Down
6 changes: 6 additions & 0 deletions packages/compiler-cli/src/ngtsc/typecheck/src/api.ts
Expand Up @@ -21,6 +21,7 @@ export interface TypeCheckableDirectiveMeta extends DirectiveMeta {
ref: Reference<ClassDeclaration>;
queries: string[];
ngTemplateGuards: TemplateGuardMeta[];
coercedInputFields: Set<string>;
hasNgTemplateContextGuard: boolean;
}

Expand Down Expand Up @@ -67,6 +68,11 @@ export interface TypeCtorMetadata {
* Input, output, and query field names in the type which should be included as constructor input.
*/
fields: {inputs: string[]; outputs: string[]; queries: string[];};

/**
* `Set` of field names which have type coercion enabled.
*/
coercedInputFields: Set<string>;
}

export interface TypeCheckingConfig {
Expand Down
1 change: 1 addition & 0 deletions packages/compiler-cli/src/ngtsc/typecheck/src/context.ts
Expand Up @@ -90,6 +90,7 @@ export class TypeCheckContext {
// TODO(alxhub): support queries
queries: dir.queries,
},
coercedInputFields: dir.coercedInputFields,
});
}
}
Expand Down
3 changes: 2 additions & 1 deletion packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts
Expand Up @@ -81,7 +81,8 @@ export class Environment {
outputs: Object.keys(dir.outputs),
// TODO: support queries
queries: dir.queries,
}
},
coercedInputFields: dir.coercedInputFields,
};
const typeCtor = generateTypeCtorDeclarationFn(node, meta, nodeTypeRef.typeName, this.config);
this.typeCtorStatements.push(typeCtor);
Expand Down
42 changes: 32 additions & 10 deletions packages/compiler-cli/src/ngtsc/typecheck/src/type_constructor.ts
Expand Up @@ -22,7 +22,7 @@ export function generateTypeCtorDeclarationFn(

const rawTypeArgs =
node.typeParameters !== undefined ? generateGenericArgs(node.typeParameters) : undefined;
const rawType: ts.TypeNode = ts.createTypeReferenceNode(nodeTypeRef, rawTypeArgs);
const rawType = ts.createTypeReferenceNode(nodeTypeRef, rawTypeArgs);

const initParam = constructTypeCtorParameter(node, meta, rawType);

Expand Down Expand Up @@ -97,7 +97,7 @@ export function generateInlineTypeCtor(
// `FooDirective<T extends Bar>`, its rawType would be `FooDirective<T>`.
const rawTypeArgs =
node.typeParameters !== undefined ? generateGenericArgs(node.typeParameters) : undefined;
const rawType: ts.TypeNode = ts.createTypeReferenceNode(node.name, rawTypeArgs);
const rawType = ts.createTypeReferenceNode(node.name, rawTypeArgs);

const initParam = constructTypeCtorParameter(node, meta, rawType);

Expand Down Expand Up @@ -126,7 +126,7 @@ export function generateInlineTypeCtor(

function constructTypeCtorParameter(
node: ClassDeclaration<ts.ClassDeclaration>, meta: TypeCtorMetadata,
rawType: ts.TypeNode): ts.ParameterDeclaration {
rawType: ts.TypeReferenceNode): ts.ParameterDeclaration {
// initType is the type of 'init', the single argument to the type constructor method.
// If the Directive has any inputs, its initType will be:
//
Expand All @@ -136,20 +136,42 @@ function constructTypeCtorParameter(
// directive will be inferred.
//
// In the special case there are no inputs, initType is set to {}.
let initType: ts.TypeNode;
let initType: ts.TypeNode|null = null;

const keys: string[] = meta.fields.inputs;
if (keys.length === 0) {
// Special case - no inputs, outputs, or other fields which could influence the result type.
initType = ts.createTypeLiteralNode([]);
} else {
const plainKeys: ts.LiteralTypeNode[] = [];
const coercedKeys: ts.PropertySignature[] = [];
for (const key of keys) {
if (!meta.coercedInputFields.has(key)) {
plainKeys.push(ts.createLiteralTypeNode(ts.createStringLiteral(key)));
} else {
coercedKeys.push(ts.createPropertySignature(
/* modifiers */ undefined,
/* name */ key,
/* questionToken */ undefined,
/* type */ ts.createTypeQueryNode(
ts.createQualifiedName(rawType.typeName, `ngAcceptInputType_${key}`)),
/* initializer */ undefined));
}
}
if (plainKeys.length > 0) {
// Construct a union of all the field names.
const keyTypeUnion = ts.createUnionTypeNode(
keys.map(key => ts.createLiteralTypeNode(ts.createStringLiteral(key))));
const keyTypeUnion = ts.createUnionTypeNode(plainKeys);

// Construct the Pick<rawType, keyTypeUnion>.
initType = ts.createTypeReferenceNode('Pick', [rawType, keyTypeUnion]);
}
if (coercedKeys.length > 0) {
const coercedLiteral = ts.createTypeLiteralNode(coercedKeys);

initType =
initType !== null ? ts.createUnionTypeNode([initType, coercedLiteral]) : coercedLiteral;
}

if (initType === null) {
// Special case - no inputs, outputs, or other fields which could influence the result type.
initType = ts.createTypeLiteralNode([]);
}

// Create the 'init' parameter itself.
return ts.createParameter(
Expand Down
11 changes: 9 additions & 2 deletions packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts
Expand Up @@ -161,8 +161,14 @@ export const ALL_ENABLED_CONFIG: TypeCheckingConfig = {

// Remove 'ref' from TypeCheckableDirectiveMeta and add a 'selector' instead.
export type TestDirective =
Partial<Pick<TypeCheckableDirectiveMeta, Exclude<keyof TypeCheckableDirectiveMeta, 'ref'>>>&
{selector: string, name: string, file?: AbsoluteFsPath, type: 'directive'};
Partial<Pick<
TypeCheckableDirectiveMeta,
Exclude<keyof TypeCheckableDirectiveMeta, 'ref'|'coercedInputFields'>>>&
{
selector: string,
name: string, file?: AbsoluteFsPath,
type: 'directive', coercedInputFields?: string[],
};
export type TestPipe = {
name: string,
file?: AbsoluteFsPath,
Expand Down Expand Up @@ -295,6 +301,7 @@ function prepareDeclarations(
inputs: decl.inputs || {},
isComponent: decl.isComponent || false,
ngTemplateGuards: decl.ngTemplateGuards || [],
coercedInputFields: new Set<string>(decl.coercedInputFields || []),
outputs: decl.outputs || {},
queries: decl.queries || [],
};
Expand Down
Expand Up @@ -81,6 +81,7 @@ TestClass.ngTypeCtor({value: 'test'});
outputs: [],
queries: [],
},
coercedInputFields: new Set(),
});
ctx.calculateTemplateDiagnostics(program, host, options);
});
Expand Down Expand Up @@ -113,6 +114,7 @@ TestClass.ngTypeCtor({value: 'test'});
outputs: [],
queries: ['queryField'],
},
coercedInputFields: new Set(),
});
const res = ctx.calculateTemplateDiagnostics(program, host, options);
const TestClassWithCtor =
Expand All @@ -121,6 +123,47 @@ TestClass.ngTypeCtor({value: 'test'});
expect(typeCtor.getText()).not.toContain('queryField');
});
});

describe('input type coercion', () => {
it('should coerce input types', () => {
const files: TestFile[] = [
LIB_D_TS, TYPE_CHECK_TS, {
name: _('/main.ts'),
contents: `class TestClass { value: any; }`,
}
];
const {program, host, options} = makeProgram(files, undefined, undefined, false);
const checker = program.getTypeChecker();
const reflectionHost = new TypeScriptReflectionHost(checker);
const logicalFs = new LogicalFileSystem(getRootDirs(host, options));
const emitter = new ReferenceEmitter([
new LocalIdentifierStrategy(),
new AbsoluteModuleStrategy(program, checker, options, host, reflectionHost),
new LogicalProjectStrategy(reflectionHost, logicalFs),
]);
const ctx = new TypeCheckContext(ALL_ENABLED_CONFIG, emitter, _('/_typecheck_.ts'));
const TestClass =
getDeclaration(program, _('/main.ts'), 'TestClass', isNamedClassDeclaration);
ctx.addInlineTypeCtor(
getSourceFileOrError(program, _('/main.ts')), new Reference(TestClass), {
fnName: 'ngTypeCtor',
body: true,
fields: {
inputs: ['foo', 'bar'],
outputs: [],
queries: [],
},
coercedInputFields: new Set(['bar']),
});
const res = ctx.calculateTemplateDiagnostics(program, host, options);
const TestClassWithCtor =
getDeclaration(res.program, _('/main.ts'), 'TestClass', isNamedClassDeclaration);
const typeCtor = TestClassWithCtor.members.find(isTypeCtor) !;
const ctorText = typeCtor.getText().replace(/[ \n]+/g, ' ');
expect(ctorText).toContain(
'init: Pick<TestClass, "foo"> | { bar: typeof TestClass.ngAcceptInputType_bar; }');
});
});
});

function isTypeCtor(el: ts.ClassElement): el is ts.MethodDeclaration {
Expand Down
70 changes: 70 additions & 0 deletions packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts
Expand Up @@ -406,6 +406,76 @@ export declare class CommonModule {
expect(diags[1].length).toEqual(15);
});

describe('input coercion', () => {
beforeEach(() => {
env.tsconfig({
'fullTemplateTypeCheck': true,
});
env.write('node_modules/@angular/material/index.d.ts', `
import * as i0 from '@angular/core';
export declare class MatInput {
value: string;
static ɵdir: i0.ɵɵDirectiveDefWithMeta<MatInput, '[matInput]', never, {'value': 'value'}, {}, never>;
static ngAcceptInputType_value: string|number;
}
export declare class MatInputModule {
static ɵmod: i0.ɵɵNgModuleDefWithMeta<MatInputModule, [typeof MatInput], never, [typeof MatInput]>;
}
`);
});

it('should coerce an input using a coercion function if provided', () => {
env.write('test.ts', `
import {Component, NgModule} from '@angular/core';
import {MatInputModule} from '@angular/material';
@Component({
selector: 'blah',
template: '<input matInput [value]="someNumber">',
})
export class FooCmp {
someNumber = 3;
}
@NgModule({
declarations: [FooCmp],
imports: [MatInputModule],
})
export class FooModule {}
`);
const diags = env.driveDiagnostics();
expect(diags.length).toBe(0);
});

it('should give an error if the binding expression type is not accepted by the coercion function',
() => {
env.write('test.ts', `
import {Component, NgModule} from '@angular/core';
import {MatInputModule} from '@angular/material';
@Component({
selector: 'blah',
template: '<input matInput [value]="invalidType">',
})
export class FooCmp {
invalidType = true;
}
@NgModule({
declarations: [FooCmp],
imports: [MatInputModule],
})
export class FooModule {}
`);
const diags = env.driveDiagnostics();
expect(diags.length).toBe(1);
expect(diags[0].messageText)
.toBe(`Type 'boolean' is not assignable to type 'string | number'.`);
});
});

describe('legacy schema checking with the DOM schema', () => {
beforeEach(
() => { env.tsconfig({ivyTemplateTypeCheck: true, fullTemplateTypeCheck: false}); });
Expand Down

0 comments on commit 678b2f0

Please sign in to comment.