From 4007422cc63ff17eb080c78dc2a9a05d75369c60 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Wed, 2 Sep 2020 15:18:29 -0400 Subject: [PATCH] fix(compiler): correct confusion between field and property names (#38685) The `R3TargetBinder` accepts an interface for directive metadata which declares types for `input` and `output` objects. These types convey the mapping between the property names for an input or output and the corresponding property name on the component class. Due to `R3TargetBinder`'s requirements, this mapping was specified with property names as keys and field names as values. However, because of duck typing, this interface was accidentally satisifed by the opposite mapping, of field names to property names, that was produced in other parts of the compiler. This form more naturally represents the data model for inputs. Rather than accept the field -> property mapping and invert it, this commit introduces a new abstraction for such mappings which is bidirectional, eliminating the ambiguous plain object type. This mapping uses new, unambiguous terminology ("class property name" and "binding property name") and can be used to satisfy both the needs of the binder as well as those of the template type-checker (field -> property). A new test ensures that the input/output metadata produced by the compiler during analysis is directly compatible with the binder via this unambiguous new interface. PR Close #38685 --- .../src/ngtsc/annotations/src/component.ts | 15 +- .../src/ngtsc/annotations/src/directive.ts | 40 ++-- .../ngtsc/annotations/test/directive_spec.ts | 44 ++++ .../src/ngtsc/indexer/test/util.ts | 5 +- .../compiler-cli/src/ngtsc/metadata/index.ts | 1 + .../src/ngtsc/metadata/src/api.ts | 20 +- .../src/ngtsc/metadata/src/dts.ts | 8 +- .../src/ngtsc/metadata/src/inheritance.ts | 21 +- .../ngtsc/metadata/src/property_mapping.ts | 200 ++++++++++++++++++ .../src/ngtsc/metadata/src/util.ts | 21 +- .../src/ngtsc/scope/test/local_spec.ts | 6 +- .../src/ngtsc/typecheck/api/api.ts | 4 +- .../src/ngtsc/typecheck/src/context.ts | 4 +- .../src/ngtsc/typecheck/src/environment.ts | 4 +- .../ngtsc/typecheck/src/type_check_block.ts | 54 ++--- .../src/ngtsc/typecheck/test/BUILD.bazel | 1 + .../src/ngtsc/typecheck/test/test_utils.ts | 8 +- packages/compiler/src/render3/view/t2_api.ts | 14 +- .../compiler/src/render3/view/t2_binder.ts | 2 +- .../test/render3/view/binding_spec.ts | 37 +++- 20 files changed, 399 insertions(+), 110 deletions(-) create mode 100644 packages/compiler-cli/src/ngtsc/metadata/src/property_mapping.ts diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index 964aaebf87..3a616ea5fe 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -15,7 +15,7 @@ import {absoluteFrom, relative} from '../../file_system'; import {DefaultImportRecorder, ModuleResolver, Reference, ReferenceEmitter} from '../../imports'; import {DependencyTracker} from '../../incremental/api'; import {IndexingContext} from '../../indexer'; -import {DirectiveMeta, DirectiveTypeCheckMeta, extractDirectiveTypeCheckMeta, InjectableClassRegistry, MetadataReader, MetadataRegistry} from '../../metadata'; +import {ClassPropertyMapping, DirectiveMeta, DirectiveTypeCheckMeta, extractDirectiveTypeCheckMeta, InjectableClassRegistry, MetadataReader, MetadataRegistry} from '../../metadata'; import {flattenInheritedDirectiveMetadata} from '../../metadata/src/inheritance'; import {EnumValue, PartialEvaluator} from '../../partial_evaluator'; import {ClassDeclaration, Decorator, ReflectionHost, reflectObjectLiteral} from '../../reflection'; @@ -56,6 +56,9 @@ export interface ComponentAnalysisData { template: ParsedTemplateWithSource; metadataStmt: Statement|null; + inputs: ClassPropertyMapping; + outputs: ClassPropertyMapping; + /** * Providers extracted from the `providers` field of the component annotation which will require * an Angular factory definition at runtime. @@ -191,7 +194,7 @@ export class ComponentDecoratorHandler implements } // Next, read the `@Component`-specific fields. - const {decorator: component, metadata} = directiveResult; + const {decorator: component, metadata, inputs, outputs} = directiveResult; // Go through the root directories for this project, and select the one with the smallest // relative path representation. @@ -328,6 +331,8 @@ export class ComponentDecoratorHandler implements const output: AnalysisOutput = { analysis: { baseClass: readBaseClass(node, this.reflector, this.evaluator), + inputs, + outputs, meta: { ...metadata, template: { @@ -345,7 +350,7 @@ export class ComponentDecoratorHandler implements i18nUseExternalIds: this.i18nUseExternalIds, relativeContextFilePath, }, - typeCheckMeta: extractDirectiveTypeCheckMeta(node, metadata.inputs, this.reflector), + typeCheckMeta: extractDirectiveTypeCheckMeta(node, inputs, this.reflector), metadataStmt: generateSetClassMetadataCall( node, this.reflector, this.defaultImportRecorder, this.isCore, this.annotateForClosureCompiler), @@ -370,8 +375,8 @@ export class ComponentDecoratorHandler implements name: node.name.text, selector: analysis.meta.selector, exportAs: analysis.meta.exportAs, - inputs: analysis.meta.inputs, - outputs: analysis.meta.outputs, + inputs: analysis.inputs, + outputs: analysis.outputs, queries: analysis.meta.queries.map(query => query.propertyName), isComponent: true, baseClass: analysis.baseClass, diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts index 6bdd3398a7..e85af13159 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts @@ -11,7 +11,7 @@ import * as ts from 'typescript'; import {ErrorCode, FatalDiagnosticError} from '../../diagnostics'; import {DefaultImportRecorder, Reference} from '../../imports'; -import {DirectiveTypeCheckMeta, InjectableClassRegistry, MetadataReader, MetadataRegistry} from '../../metadata'; +import {ClassPropertyMapping, DirectiveTypeCheckMeta, InjectableClassRegistry, MetadataReader, MetadataRegistry} from '../../metadata'; import {extractDirectiveTypeCheckMeta} from '../../metadata/src/util'; import {DynamicValue, EnumValue, PartialEvaluator} from '../../partial_evaluator'; import {ClassDeclaration, ClassMember, ClassMemberKind, Decorator, filterToMembersWithDecorator, ReflectionHost, reflectObjectLiteral} from '../../reflection'; @@ -39,6 +39,8 @@ export interface DirectiveHandlerData { meta: R3DirectiveMetadata; metadataStmt: Statement|null; providersRequiringFactory: Set>|null; + inputs: ClassPropertyMapping; + outputs: ClassPropertyMapping; } export class DirectiveDecoratorHandler implements @@ -83,11 +85,10 @@ export class DirectiveDecoratorHandler implements const directiveResult = extractDirectiveMetadata( node, decorator, this.reflector, this.evaluator, this.defaultImportRecorder, this.isCore, flags, this.annotateForClosureCompiler); - const analysis = directiveResult && directiveResult.metadata; - - if (analysis === undefined) { + if (directiveResult === undefined) { return {}; } + const analysis = directiveResult.metadata; let providersRequiringFactory: Set>|null = null; if (directiveResult !== undefined && directiveResult.decorator.has('providers')) { @@ -97,12 +98,14 @@ export class DirectiveDecoratorHandler implements return { analysis: { + inputs: directiveResult.inputs, + outputs: directiveResult.outputs, meta: analysis, metadataStmt: generateSetClassMetadataCall( node, this.reflector, this.defaultImportRecorder, this.isCore, this.annotateForClosureCompiler), baseClass: readBaseClass(node, this.reflector, this.evaluator), - typeCheckMeta: extractDirectiveTypeCheckMeta(node, analysis.inputs, this.reflector), + typeCheckMeta: extractDirectiveTypeCheckMeta(node, directiveResult.inputs, this.reflector), providersRequiringFactory } }; @@ -117,8 +120,8 @@ export class DirectiveDecoratorHandler implements name: node.name.text, selector: analysis.meta.selector, exportAs: analysis.meta.exportAs, - inputs: analysis.meta.inputs, - outputs: analysis.meta.outputs, + inputs: analysis.inputs, + outputs: analysis.outputs, queries: analysis.meta.queries.map(query => query.propertyName), isComponent: false, baseClass: analysis.baseClass, @@ -199,8 +202,13 @@ export class DirectiveDecoratorHandler implements export function extractDirectiveMetadata( clazz: ClassDeclaration, decorator: Readonly, reflector: ReflectionHost, evaluator: PartialEvaluator, defaultImportRecorder: DefaultImportRecorder, isCore: boolean, - flags: HandlerFlags, annotateForClosureCompiler: boolean, defaultSelector: string|null = null): - {decorator: Map, metadata: R3DirectiveMetadata}|undefined { + flags: HandlerFlags, annotateForClosureCompiler: boolean, + defaultSelector: string|null = null): { + decorator: Map, + metadata: R3DirectiveMetadata, + inputs: ClassPropertyMapping, + outputs: ClassPropertyMapping, +}|undefined { let directive: Map; if (decorator === null || decorator.args === null || decorator.args.length === 0) { directive = new Map(); @@ -331,6 +339,9 @@ export function extractDirectiveMetadata( const type = wrapTypeReference(reflector, clazz); const internalType = new WrappedNodeExpr(reflector.getInternalNameOfClass(clazz)); + const inputs = ClassPropertyMapping.fromMappedObject({...inputsFromMeta, ...inputsFromFields}); + const outputs = ClassPropertyMapping.fromMappedObject({...outputsFromMeta, ...outputsFromFields}); + const metadata: R3DirectiveMetadata = { name: clazz.name.text, deps: ctorDeps, @@ -338,8 +349,8 @@ export function extractDirectiveMetadata( lifecycle: { usesOnChanges, }, - inputs: {...inputsFromMeta, ...inputsFromFields}, - outputs: {...outputsFromMeta, ...outputsFromFields}, + inputs: inputs.toJointMappedObject(), + outputs: outputs.toDirectMappedObject(), queries, viewQueries, selector, @@ -352,7 +363,12 @@ export function extractDirectiveMetadata( exportAs, providers }; - return {decorator: directive, metadata}; + return { + decorator: directive, + metadata, + inputs, + outputs, + }; } export function extractQueryMetadata( diff --git a/packages/compiler-cli/src/ngtsc/annotations/test/directive_spec.ts b/packages/compiler-cli/src/ngtsc/annotations/test/directive_spec.ts index bfdee6b4b1..1eed4de010 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/test/directive_spec.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/test/directive_spec.ts @@ -5,6 +5,7 @@ * Use of this source code is governed by an MIT-style license that can be * found in the LICENSE file at https://angular.io/license */ +import {CssSelector, DirectiveMeta as T2DirectiveMeta, parseTemplate, R3TargetBinder, SelectorMatcher, TmplAstElement} from '@angular/compiler'; import * as ts from 'typescript'; import {absoluteFrom} from '../../file_system'; @@ -73,6 +74,49 @@ runInEachFileSystem(() => { expect(span.start.toString()).toContain('/entry.ts@5:22'); expect(span.end.toString()).toContain('/entry.ts@5:29'); }); + + it('should produce metadata compatible with template binding', () => { + const src = ` + import {Directive, Input} from '@angular/core'; + + @Directive({selector: '[dir]'}) + export class TestDir { + @Input('propName') + fieldName: string; + } + `; + const {program} = makeProgram([ + { + name: _('/node_modules/@angular/core/index.d.ts'), + contents: 'export const Directive: any; export const Input: any;', + }, + { + name: _('/entry.ts'), + contents: src, + }, + ]); + + const analysis = analyzeDirective(program, 'TestDir'); + const matcher = new SelectorMatcher(); + const dirMeta: T2DirectiveMeta = { + exportAs: null, + inputs: analysis.inputs, + outputs: analysis.outputs, + isComponent: false, + name: 'Dir', + }; + matcher.addSelectables(CssSelector.parse('[dir]'), dirMeta); + + const {nodes} = parseTemplate('
', 'unimportant.html'); + const binder = new R3TargetBinder(matcher).bind({template: nodes}); + const propBinding = (nodes[0] as TmplAstElement).inputs[0]; + const propBindingConsumer = binder.getConsumerOfBinding(propBinding); + + // Assert that the consumer of the binding is the directive, which means that the metadata + // fed into the SelectorMatcher was compatible with the binder, and did not confuse property + // and field names. + expect(propBindingConsumer).toBe(dirMeta); + }); }); // Helpers diff --git a/packages/compiler-cli/src/ngtsc/indexer/test/util.ts b/packages/compiler-cli/src/ngtsc/indexer/test/util.ts index 4e16da6e11..5013f4652c 100644 --- a/packages/compiler-cli/src/ngtsc/indexer/test/util.ts +++ b/packages/compiler-cli/src/ngtsc/indexer/test/util.ts @@ -11,6 +11,7 @@ import * as ts from 'typescript'; import {absoluteFrom, AbsoluteFsPath} from '../../file_system'; import {Reference} from '../../imports'; +import {ClassPropertyMapping} from '../../metadata'; import {ClassDeclaration} from '../../reflection'; import {getDeclaration, makeProgram} from '../../testing'; import {ComponentMeta} from '../src/context'; @@ -50,8 +51,8 @@ export function getBoundTemplate( selector, name: declaration.name.getText(), isComponent: true, - inputs: {}, - outputs: {}, + inputs: ClassPropertyMapping.fromMappedObject({}), + outputs: ClassPropertyMapping.fromMappedObject({}), exportAs: null, }); }); diff --git a/packages/compiler-cli/src/ngtsc/metadata/index.ts b/packages/compiler-cli/src/ngtsc/metadata/index.ts index ee3f910b1b..2f857eee0b 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/index.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/index.ts @@ -10,3 +10,4 @@ export * from './src/api'; export {DtsMetadataReader} from './src/dts'; export {CompoundMetadataRegistry, LocalMetadataRegistry, InjectableClassRegistry} from './src/registry'; export {extractDirectiveTypeCheckMeta, CompoundMetadataReader} from './src/util'; +export {BindingPropertyName, ClassPropertyMapping, ClassPropertyName, InputOrOutput} from './src/property_mapping'; diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/api.ts b/packages/compiler-cli/src/ngtsc/metadata/src/api.ts index 8009207f50..3ec001db95 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/api.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/api.ts @@ -12,6 +12,8 @@ import * as ts from 'typescript'; import {Reference} from '../../imports'; import {ClassDeclaration} from '../../reflection'; +import {ClassPropertyMapping, ClassPropertyName} from './property_mapping'; + /** * Metadata collected for an `NgModule`. @@ -52,25 +54,25 @@ export interface DirectiveTypeCheckMeta { * Directive's class. This allows inputs to accept a wider range of types and coerce the input to * a narrower type with a getter/setter. See https://angular.io/guide/template-typecheck. */ - coercedInputFields: Set; + coercedInputFields: Set; /** * The set of input fields which map to `readonly`, `private`, or `protected` members in the * Directive's class. */ - restrictedInputFields: Set; + restrictedInputFields: Set; /** * The set of input fields which are declared as string literal members in the Directive's class. * We need to track these separately because these fields may not be valid JS identifiers so * we cannot use them with property access expressions when assigning inputs. */ - stringLiteralInputFields: Set; + stringLiteralInputFields: Set; /** * The set of input fields which do not have corresponding members in the Directive's class. */ - undeclaredInputFields: Set; + undeclaredInputFields: Set; /** * Whether the Directive's class is generic, i.e. `class MyDir {...}`. @@ -89,6 +91,16 @@ export interface DirectiveMeta extends T2DirectiveMeta, DirectiveTypeCheckMeta { selector: string|null; queries: string[]; + /** + * A mapping of input field names to the property names. + */ + inputs: ClassPropertyMapping; + + /** + * A mapping of output field names to the property names. + */ + outputs: ClassPropertyMapping; + /** * A `Reference` to the base class for the directive, if one was detected. * diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/dts.ts b/packages/compiler-cli/src/ngtsc/metadata/src/dts.ts index faf588ab67..4bbdc14fd8 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/dts.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/dts.ts @@ -12,6 +12,7 @@ import {Reference} from '../../imports'; import {ClassDeclaration, isNamedClassDeclaration, ReflectionHost} from '../../reflection'; import {DirectiveMeta, MetadataReader, NgModuleMeta, PipeMeta} from './api'; +import {ClassPropertyMapping} from './property_mapping'; import {extractDirectiveTypeCheckMeta, extractReferencesFromType, readStringArrayType, readStringMapType, readStringType} from './util'; /** @@ -76,7 +77,10 @@ export class DtsMetadataReader implements MetadataReader { return null; } - const inputs = readStringMapType(def.type.typeArguments[3]); + const inputs = + ClassPropertyMapping.fromMappedObject(readStringMapType(def.type.typeArguments[3])); + const outputs = + ClassPropertyMapping.fromMappedObject(readStringMapType(def.type.typeArguments[4])); return { ref, name: clazz.name.text, @@ -84,7 +88,7 @@ export class DtsMetadataReader implements MetadataReader { selector: readStringType(def.type.typeArguments[1]), exportAs: readStringArrayType(def.type.typeArguments[2]), inputs, - outputs: readStringMapType(def.type.typeArguments[4]), + outputs, queries: readStringArrayType(def.type.typeArguments[5]), ...extractDirectiveTypeCheckMeta(clazz, inputs, this.reflector), baseClass: readBaseClass(clazz, this.checker, this.reflector), diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/inheritance.ts b/packages/compiler-cli/src/ngtsc/metadata/src/inheritance.ts index 455d103d7d..f5271f4351 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/inheritance.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/inheritance.ts @@ -7,9 +7,11 @@ */ import {Reference} from '../../imports'; -import {DirectiveMeta, MetadataReader} from '../../metadata'; import {ClassDeclaration} from '../../reflection'; +import {DirectiveMeta, MetadataReader} from './api'; +import {ClassPropertyMapping, ClassPropertyName} from './property_mapping'; + /** * Given a reference to a directive, return a flattened version of its `DirectiveMeta` metadata * which includes metadata from its entire inheritance chain. @@ -25,13 +27,13 @@ export function flattenInheritedDirectiveMetadata( throw new Error(`Metadata not found for directive: ${dir.debugName}`); } - let inputs: {[key: string]: string|[string, string]} = {}; - let outputs: {[key: string]: string} = {}; - const coercedInputFields = new Set(); - const undeclaredInputFields = new Set(); - const restrictedInputFields = new Set(); - const stringLiteralInputFields = new Set(); + const coercedInputFields = new Set(); + const undeclaredInputFields = new Set(); + const restrictedInputFields = new Set(); + const stringLiteralInputFields = new Set(); let isDynamic = false; + let inputs = ClassPropertyMapping.empty(); + let outputs = ClassPropertyMapping.empty(); const addMetadata = (meta: DirectiveMeta): void => { if (meta.baseClass === 'dynamic') { @@ -45,8 +47,9 @@ export function flattenInheritedDirectiveMetadata( isDynamic = true; } } - inputs = {...inputs, ...meta.inputs}; - outputs = {...outputs, ...meta.outputs}; + + inputs = ClassPropertyMapping.merge(inputs, meta.inputs); + outputs = ClassPropertyMapping.merge(outputs, meta.outputs); for (const coercedInputField of meta.coercedInputFields) { coercedInputFields.add(coercedInputField); diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/property_mapping.ts b/packages/compiler-cli/src/ngtsc/metadata/src/property_mapping.ts new file mode 100644 index 0000000000..6a78fe1e68 --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/metadata/src/property_mapping.ts @@ -0,0 +1,200 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {InputOutputPropertySet} from '@angular/compiler'; + +/** + * The name of a class property that backs an input or output declared by a directive or component. + * + * This type exists for documentation only. + */ +export type ClassPropertyName = string; + +/** + * The name by which an input or output of a directive or component is bound in an Angular template. + * + * This type exists for documentation only. + */ +export type BindingPropertyName = string; + +/** + * An input or output of a directive that has both a named JavaScript class property on a component + * or directive class, as well as an Angular template property name used for binding. + */ +export interface InputOrOutput { + /** + * The name of the JavaScript property on the component or directive instance for this input or + * output. + */ + readonly classPropertyName: ClassPropertyName; + + /** + * The property name used to bind this input or output in an Angular template. + */ + readonly bindingPropertyName: BindingPropertyName; +} + +/** + * A mapping of component property and template binding property names, for example containing the + * inputs of a particular directive or component. + * + * A single component property has exactly one input/output annotation (and therefore one binding + * property name) associated with it, but the same binding property name may be shared across many + * component property names. + * + * Allows bidirectional querying of the mapping - looking up all inputs/outputs with a given + * property name, or mapping from a specific class property to its binding property name. + */ +export class ClassPropertyMapping implements InputOutputPropertySet { + /** + * Mapping from class property names to the single `InputOrOutput` for that class property. + */ + private forwardMap: Map; + + /** + * Mapping from property names to one or more `InputOrOutput`s which share that name. + */ + private reverseMap: Map; + + private constructor(forwardMap: Map) { + this.forwardMap = forwardMap; + this.reverseMap = reverseMapFromForwardMap(forwardMap); + } + + /** + * Construct a `ClassPropertyMapping` with no entries. + */ + static empty(): ClassPropertyMapping { + return new ClassPropertyMapping(new Map()); + } + + /** + * Construct a `ClassPropertyMapping` from a primitive JS object which maps class property names + * to either binding property names or an array that contains both names, which is used in on-disk + * metadata formats (e.g. in .d.ts files). + */ + static fromMappedObject(obj: { + [classPropertyName: string]: BindingPropertyName|[ClassPropertyName, BindingPropertyName] + }): ClassPropertyMapping { + const forwardMap = new Map(); + + for (const classPropertyName of Object.keys(obj)) { + const value = obj[classPropertyName]; + const bindingPropertyName = Array.isArray(value) ? value[0] : value; + const inputOrOutput: InputOrOutput = {classPropertyName, bindingPropertyName}; + forwardMap.set(classPropertyName, inputOrOutput); + } + + return new ClassPropertyMapping(forwardMap); + } + + /** + * Merge two mappings into one, with class properties from `b` taking precedence over class + * properties from `a`. + */ + static merge(a: ClassPropertyMapping, b: ClassPropertyMapping): ClassPropertyMapping { + const forwardMap = new Map(a.forwardMap.entries()); + for (const [classPropertyName, inputOrOutput] of b.forwardMap) { + forwardMap.set(classPropertyName, inputOrOutput); + } + + return new ClassPropertyMapping(forwardMap); + } + + /** + * All class property names mapped in this mapping. + */ + get classPropertyNames(): ClassPropertyName[] { + return Array.from(this.forwardMap.keys()); + } + + /** + * All binding property names mapped in this mapping. + */ + get propertyNames(): BindingPropertyName[] { + return Array.from(this.reverseMap.keys()); + } + + /** + * Check whether a mapping for the given property name exists. + */ + hasBindingPropertyName(propertyName: BindingPropertyName): boolean { + return this.reverseMap.has(propertyName); + } + + /** + * Lookup all `InputOrOutput`s that use this `propertyName`. + */ + getByBindingPropertyName(propertyName: string): ReadonlyArray|null { + return this.reverseMap.has(propertyName) ? this.reverseMap.get(propertyName)! : null; + } + + /** + * Lookup the `InputOrOutput` associated with a `classPropertyName`. + */ + getByClassPropertyName(classPropertyName: string): InputOrOutput|null { + return this.forwardMap.has(classPropertyName) ? this.forwardMap.get(classPropertyName)! : null; + } + + /** + * Convert this mapping to a primitive JS object which maps each class property directly to the + * binding property name associated with it. + */ + toDirectMappedObject(): {[classPropertyName: string]: BindingPropertyName} { + const obj: {[classPropertyName: string]: BindingPropertyName} = {}; + for (const [classPropertyName, inputOrOutput] of this.forwardMap) { + obj[classPropertyName] = inputOrOutput.bindingPropertyName; + } + return obj; + } + + /** + * Convert this mapping to a primitive JS object which maps each class property either to itself + * (for cases where the binding property name is the same) or to an array which contains both + * names if they differ. + * + * This object format is used when mappings are serialized (for example into .d.ts files). + */ + toJointMappedObject(): + {[classPropertyName: string]: BindingPropertyName|[BindingPropertyName, ClassPropertyName]} { + const obj: { + [classPropertyName: string]: BindingPropertyName|[BindingPropertyName, ClassPropertyName] + } = {}; + for (const [classPropertyName, inputOrOutput] of this.forwardMap) { + if (inputOrOutput.bindingPropertyName as string === classPropertyName as string) { + obj[classPropertyName] = inputOrOutput.bindingPropertyName; + } else { + obj[classPropertyName] = [inputOrOutput.bindingPropertyName, classPropertyName]; + } + } + return obj; + } + + /** + * Implement the iterator protocol and return entry objects which contain the class and binding + * property names (and are useful for destructuring). + */ + * [Symbol.iterator](): IterableIterator<[ClassPropertyName, BindingPropertyName]> { + for (const [classPropertyName, inputOrOutput] of this.forwardMap.entries()) { + yield [classPropertyName, inputOrOutput.bindingPropertyName]; + } + } +} + +function reverseMapFromForwardMap(forwardMap: Map): + Map { + const reverseMap = new Map(); + for (const [_, inputOrOutput] of forwardMap) { + if (!reverseMap.has(inputOrOutput.bindingPropertyName)) { + reverseMap.set(inputOrOutput.bindingPropertyName, []); + } + + reverseMap.get(inputOrOutput.bindingPropertyName)!.push(inputOrOutput); + } + return reverseMap; +} diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/util.ts b/packages/compiler-cli/src/ngtsc/metadata/src/util.ts index 2f465dd977..5d7f6c9841 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/util.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/util.ts @@ -13,6 +13,7 @@ import {ClassDeclaration, ClassMember, ClassMemberKind, isNamedClassDeclaration, import {nodeDebugInfo} from '../../util/src/typescript'; import {DirectiveMeta, DirectiveTypeCheckMeta, MetadataReader, NgModuleMeta, PipeMeta, TemplateGuardMeta} from './api'; +import {ClassPropertyMapping, ClassPropertyName} from './property_mapping'; export function extractReferencesFromType( checker: ts.TypeChecker, def: ts.TypeNode, ngModuleImportedFrom: string|null, @@ -91,7 +92,7 @@ export function readStringArrayType(type: ts.TypeNode): string[] { * making this metadata invariant to changes of inherited classes. */ export function extractDirectiveTypeCheckMeta( - node: ClassDeclaration, inputs: {[fieldName: string]: string|[string, string]}, + node: ClassDeclaration, inputs: ClassPropertyMapping, reflector: ReflectionHost): DirectiveTypeCheckMeta { const members = reflector.getMembersOfClass(node); const staticMembers = members.filter(member => member.isStatic); @@ -102,23 +103,23 @@ export function extractDirectiveTypeCheckMeta( const coercedInputFields = new Set(staticMembers.map(extractCoercedInput) - .filter((inputName): inputName is string => inputName !== null)); + .filter((inputName): inputName is ClassPropertyName => inputName !== null)); - const restrictedInputFields = new Set(); - const stringLiteralInputFields = new Set(); - const undeclaredInputFields = new Set(); + const restrictedInputFields = new Set(); + const stringLiteralInputFields = new Set(); + const undeclaredInputFields = new Set(); - for (const fieldName of Object.keys(inputs)) { - const field = members.find(member => member.name === fieldName); + for (const classPropertyName of inputs.classPropertyNames) { + const field = members.find(member => member.name === classPropertyName); if (field === undefined || field.node === null) { - undeclaredInputFields.add(fieldName); + undeclaredInputFields.add(classPropertyName); continue; } if (isRestricted(field.node)) { - restrictedInputFields.add(fieldName); + restrictedInputFields.add(classPropertyName); } if (field.nameNode !== null && ts.isStringLiteral(field.nameNode)) { - stringLiteralInputFields.add(fieldName); + stringLiteralInputFields.add(classPropertyName); } } diff --git a/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts b/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts index f8b12d5fc0..0216fd8864 100644 --- a/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts +++ b/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts @@ -9,7 +9,7 @@ import * as ts from 'typescript'; import {Reference, ReferenceEmitter} from '../../imports'; -import {CompoundMetadataRegistry, DirectiveMeta, LocalMetadataRegistry, MetadataRegistry, PipeMeta} from '../../metadata'; +import {ClassPropertyMapping, CompoundMetadataRegistry, DirectiveMeta, LocalMetadataRegistry, MetadataRegistry, PipeMeta} from '../../metadata'; import {ClassDeclaration} from '../../reflection'; import {ScopeData} from '../src/api'; import {DtsModuleScopeResolver} from '../src/dependency'; @@ -236,8 +236,8 @@ function fakeDirective(ref: Reference): DirectiveMeta { name, selector: `[${ref.debugName}]`, isComponent: name.startsWith('Cmp'), - inputs: {}, - outputs: {}, + inputs: ClassPropertyMapping.fromMappedObject({}), + outputs: ClassPropertyMapping.fromMappedObject({}), exportAs: null, queries: [], hasNgTemplateContextGuard: false, diff --git a/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts b/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts index 658260fbf1..594475009a 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts @@ -11,7 +11,7 @@ import * as ts from 'typescript'; import {AbsoluteFsPath} from '../../file_system'; import {Reference} from '../../imports'; -import {DirectiveTypeCheckMeta} from '../../metadata'; +import {ClassPropertyMapping, DirectiveTypeCheckMeta} from '../../metadata'; import {ClassDeclaration} from '../../reflection'; @@ -22,6 +22,8 @@ import {ClassDeclaration} from '../../reflection'; export interface TypeCheckableDirectiveMeta extends DirectiveMeta, DirectiveTypeCheckMeta { ref: Reference; queries: string[]; + inputs: ClassPropertyMapping; + outputs: ClassPropertyMapping; } export type TemplateId = string&{__brand: 'TemplateId'}; diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts index fc37c26b78..30c15189bc 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts @@ -241,8 +241,8 @@ export class TypeCheckContextImpl implements TypeCheckContext { // it comes from a .d.ts file. .d.ts declarations don't have bodies. body: !dirNode.getSourceFile().isDeclarationFile, fields: { - inputs: Object.keys(dir.inputs), - outputs: Object.keys(dir.outputs), + inputs: dir.inputs.classPropertyNames, + outputs: dir.outputs.classPropertyNames, // TODO(alxhub): support queries queries: dir.queries, }, diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts index e0d8bb1905..bfa79ad8c4 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts @@ -79,8 +79,8 @@ export class Environment { fnName, body: true, fields: { - inputs: Object.keys(dir.inputs), - outputs: Object.keys(dir.outputs), + inputs: dir.inputs.classPropertyNames, + outputs: dir.outputs.classPropertyNames, // TODO: support queries queries: dir.queries, }, diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts index 5576556a71..e295889c7c 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts @@ -10,6 +10,7 @@ import {AST, BindingPipe, BindingType, BoundTarget, DYNAMIC_TYPE, ImplicitReceiv import * as ts from 'typescript'; import {Reference} from '../../imports'; +import {ClassPropertyName} from '../../metadata'; import {ClassDeclaration} from '../../reflection'; import {TemplateId, TypeCheckableDirectiveMeta, TypeCheckBlockMetadata} from '../api'; @@ -431,7 +432,7 @@ class TcbDirectiveCtorOp extends TcbOp { } // Add unset directive inputs for each of the remaining unset fields. - for (const fieldName of Object.keys(this.dir.inputs)) { + for (const [fieldName] of this.dir.inputs) { if (!genericInputs.has(fieldName)) { genericInputs.set(fieldName, {type: 'unset', field: fieldName}); } @@ -743,22 +744,14 @@ class TcbDirectiveOutputsOp extends TcbOp { execute(): null { let dirId: ts.Expression|null = null; - - - // `dir.outputs` is an object map of field names on the directive class to event names. - // This is backwards from what's needed to match event handlers - a map of event names to field - // names is desired. Invert `dir.outputs` into `fieldByEventName` to create this map. - const fieldByEventName = new Map(); const outputs = this.dir.outputs; - for (const key of Object.keys(outputs)) { - fieldByEventName.set(outputs[key], key); - } for (const output of this.node.outputs) { - if (output.type !== ParsedEventType.Regular || !fieldByEventName.has(output.name)) { + if (output.type !== ParsedEventType.Regular || !outputs.hasBindingPropertyName(output.name)) { continue; } - const field = fieldByEventName.get(output.name)!; + // TODO(alxhub): consider supporting multiple fields with the same property name for outputs. + const field = outputs.getByBindingPropertyName(output.name)![0].classPropertyName; if (this.tcb.env.config.checkTypeOfOutputEvents) { // For strict checking of directive events, generate a call to the `subscribe` method @@ -1225,9 +1218,8 @@ class Scope { if (node instanceof TmplAstElement) { // Go through the directives and remove any inputs that it claims from `elementInputs`. for (const dir of directives) { - for (const fieldName of Object.keys(dir.inputs)) { - const value = dir.inputs[fieldName]; - claimedInputs.add(Array.isArray(value) ? value[0] : value); + for (const propertyName of dir.inputs.propertyNames) { + claimedInputs.add(propertyName); } } @@ -1264,8 +1256,8 @@ class Scope { if (node instanceof TmplAstElement) { // Go through the directives and register any outputs that it claims in `claimedOutputs`. for (const dir of directives) { - for (const outputField of Object.keys(dir.outputs)) { - claimedOutputs.add(dir.outputs[outputField]); + for (const outputProperty of dir.outputs.propertyNames) { + claimedOutputs.add(outputProperty); } } @@ -1276,7 +1268,7 @@ class Scope { interface TcbBoundInput { attribute: TmplAstBoundAttribute|TmplAstTextAttribute; - fieldNames: string[]; + fieldNames: ClassPropertyName[]; } /** @@ -1537,7 +1529,6 @@ function getBoundInputs( tcb: Context): TcbBoundInput[] { const boundInputs: TcbBoundInput[] = []; - const propertyToFieldNames = invertInputs(directive.inputs); const processAttribute = (attr: TmplAstBoundAttribute|TmplAstTextAttribute) => { // Skip non-property bindings. if (attr instanceof TmplAstBoundAttribute && attr.type !== BindingType.Property) { @@ -1550,10 +1541,11 @@ function getBoundInputs( } // Skip the attribute if the directive does not have an input for it. - if (!propertyToFieldNames.has(attr.name)) { + const inputs = directive.inputs.getByBindingPropertyName(attr.name); + if (inputs === null) { return; } - const fieldNames = propertyToFieldNames.get(attr.name)!; + const fieldNames = inputs.map(input => input.classPropertyName); boundInputs.push({attribute: attr, fieldNames}); }; @@ -1580,26 +1572,6 @@ function translateInput( } } -/** - * Inverts the input-mapping from field-to-property name into property-to-field name, to be able - * to match a property in a template with the corresponding field on a directive. - */ -function invertInputs(inputs: {[fieldName: string]: string|[string, string]}): - Map { - const propertyToFieldNames = new Map(); - for (const fieldName of Object.keys(inputs)) { - const propertyNames = inputs[fieldName]; - const propertyName = Array.isArray(propertyNames) ? propertyNames[0] : propertyNames; - - if (propertyToFieldNames.has(propertyName)) { - propertyToFieldNames.get(propertyName)!.push(fieldName); - } else { - propertyToFieldNames.set(propertyName, [fieldName]); - } - } - return propertyToFieldNames; -} - /** * An input binding that corresponds with a field of a directive. */ diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/test/BUILD.bazel index 6e8e8b2f14..523f14ae0d 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/BUILD.bazel +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/BUILD.bazel @@ -16,6 +16,7 @@ ts_library( "//packages/compiler-cli/src/ngtsc/file_system/testing", "//packages/compiler-cli/src/ngtsc/imports", "//packages/compiler-cli/src/ngtsc/incremental", + "//packages/compiler-cli/src/ngtsc/metadata", "//packages/compiler-cli/src/ngtsc/reflection", "//packages/compiler-cli/src/ngtsc/shims", "//packages/compiler-cli/src/ngtsc/testing", diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts index 3c8a5c96b8..248f5ea352 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts @@ -13,6 +13,7 @@ import {absoluteFrom, AbsoluteFsPath, getSourceFileOrError, LogicalFileSystem} f import {TestFile} from '../../file_system/testing'; import {AbsoluteModuleStrategy, LocalIdentifierStrategy, LogicalProjectStrategy, ModuleResolver, Reference, ReferenceEmitter} from '../../imports'; import {NOOP_INCREMENTAL_BUILD} from '../../incremental'; +import {ClassPropertyMapping} from '../../metadata'; import {ClassDeclaration, isNamedClassDeclaration, TypeScriptReflectionHost} from '../../reflection'; import {makeProgram} from '../../testing'; import {getRootDirs} from '../../util/src/typescript'; @@ -180,8 +181,9 @@ export type TestDirective = Partial>>&{ + 'undeclaredInputFields'|'inputs'|'outputs'>>>&{ selector: string, name: string, file?: AbsoluteFsPath, type: 'directive', + inputs?: {[fieldName: string]: string}, outputs?: {[fieldName: string]: string}, coercedInputFields?: string[], restrictedInputFields?: string[], stringLiteralInputFields?: string[], undeclaredInputFields?: string[], isGeneric?: boolean; }; @@ -418,7 +420,7 @@ function prepareDeclarations( ref: new Reference(resolveDeclaration(decl)), exportAs: decl.exportAs || null, hasNgTemplateContextGuard: decl.hasNgTemplateContextGuard || false, - inputs: decl.inputs || {}, + inputs: ClassPropertyMapping.fromMappedObject(decl.inputs || {}), isComponent: decl.isComponent || false, ngTemplateGuards: decl.ngTemplateGuards || [], coercedInputFields: new Set(decl.coercedInputFields || []), @@ -426,7 +428,7 @@ function prepareDeclarations( stringLiteralInputFields: new Set(decl.stringLiteralInputFields || []), undeclaredInputFields: new Set(decl.undeclaredInputFields || []), isGeneric: decl.isGeneric ?? false, - outputs: decl.outputs || {}, + outputs: ClassPropertyMapping.fromMappedObject(decl.outputs || {}), queries: decl.queries || [], }; matcher.addSelectables(selector, meta); diff --git a/packages/compiler/src/render3/view/t2_api.ts b/packages/compiler/src/render3/view/t2_api.ts index e327c55b79..3551396880 100644 --- a/packages/compiler/src/render3/view/t2_api.ts +++ b/packages/compiler/src/render3/view/t2_api.ts @@ -26,6 +26,16 @@ export interface Target { template?: Node[]; } +/** + * A data structure which can indicate whether a given property name is present or not. + * + * This is used to represent the set of inputs or outputs present on a directive, and allows the + * binder to query for the presence of a mapping for property names. + */ +export interface InputOutputPropertySet { + hasBindingPropertyName(propertyName: string): boolean; +} + /** * Metadata regarding a directive that's needed to match it against template elements. This is * provided by a consumer of the t2 APIs. @@ -46,14 +56,14 @@ export interface DirectiveMeta { * * Goes from property names to field names. */ - inputs: {[property: string]: string|[string, string]}; + inputs: InputOutputPropertySet; /** * Set of outputs which this directive claims. * * Goes from property names to field names. */ - outputs: {[property: string]: string}; + outputs: InputOutputPropertySet; /** * Name under which the directive is exported, if any (exportAs in Angular). diff --git a/packages/compiler/src/render3/view/t2_binder.ts b/packages/compiler/src/render3/view/t2_binder.ts index 3523072295..53b363afcc 100644 --- a/packages/compiler/src/render3/view/t2_binder.ts +++ b/packages/compiler/src/render3/view/t2_binder.ts @@ -278,7 +278,7 @@ class DirectiveBinder implements Visitor { type BoundNode = BoundAttribute|BoundEvent|TextAttribute; const setAttributeBinding = (attribute: BoundNode, ioType: keyof Pick) => { - const dir = directives.find(dir => dir[ioType].hasOwnProperty(attribute.name)); + const dir = directives.find(dir => dir[ioType].hasBindingPropertyName(attribute.name)); const binding = dir !== undefined ? dir : node; this.bindings.set(attribute, binding); }; diff --git a/packages/compiler/test/render3/view/binding_spec.ts b/packages/compiler/test/render3/view/binding_spec.ts index 02e9a7ee95..a1c5125454 100644 --- a/packages/compiler/test/render3/view/binding_spec.ts +++ b/packages/compiler/test/render3/view/binding_spec.ts @@ -8,41 +8,56 @@ import * as e from '../../../src/expression_parser/ast'; import * as a from '../../../src/render3/r3_ast'; -import {DirectiveMeta} from '../../../src/render3/view/t2_api'; +import {DirectiveMeta, InputOutputPropertySet} from '../../../src/render3/view/t2_api'; import {R3TargetBinder} from '../../../src/render3/view/t2_binder'; import {parseTemplate} from '../../../src/render3/view/template'; import {CssSelector, SelectorMatcher} from '../../../src/selector'; import {findExpression} from './util'; +/** + * A `InputOutputPropertySet` which only uses an identity mapping for fields and properties. + */ +class IdentityInputMapping implements InputOutputPropertySet { + private names: Set; + + constructor(names: string[]) { + this.names = new Set(names); + } + + hasBindingPropertyName(propertyName: string): boolean { + return this.names.has(propertyName); + } +} + function makeSelectorMatcher(): SelectorMatcher { const matcher = new SelectorMatcher(); matcher.addSelectables(CssSelector.parse('[ngFor][ngForOf]'), { name: 'NgFor', exportAs: null, - inputs: {'ngForOf': 'ngForOf'}, - outputs: {}, + inputs: new IdentityInputMapping(['ngForOf']), + outputs: new IdentityInputMapping([]), isComponent: false, }); matcher.addSelectables(CssSelector.parse('[dir]'), { name: 'Dir', exportAs: null, - inputs: {}, - outputs: {}, + inputs: new IdentityInputMapping([]), + outputs: new IdentityInputMapping([]), isComponent: false, }); matcher.addSelectables(CssSelector.parse('[hasOutput]'), { name: 'HasOutput', exportAs: null, - inputs: {}, - outputs: {'outputBinding': 'outputBinding'}, + inputs: new IdentityInputMapping([]), + outputs: new IdentityInputMapping(['outputBinding']), isComponent: false, }); matcher.addSelectables(CssSelector.parse('[hasInput]'), { name: 'HasInput', exportAs: null, - inputs: {'inputBinding': 'inputBinding'}, - outputs: {}, + inputs: new IdentityInputMapping(['inputBinding']), + outputs: new IdentityInputMapping([]), isComponent: false, }); return matcher; @@ -85,8 +100,8 @@ describe('t2 binding', () => { matcher.addSelectables(CssSelector.parse('text[dir]'), { name: 'Dir', exportAs: null, - inputs: {}, - outputs: {}, + inputs: new IdentityInputMapping([]), + outputs: new IdentityInputMapping([]), isComponent: false, }); const binder = new R3TargetBinder(matcher);