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

Author: alxhub

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<ComponentAnalysisData> = {
       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,

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<Reference<ClassDeclaration>>|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<Reference<ClassDeclaration>>|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<Decorator|null>, reflector: ReflectionHost,
     evaluator: PartialEvaluator, defaultImportRecorder: DefaultImportRecorder, isCore: boolean,
-    flags: HandlerFlags, annotateForClosureCompiler: boolean, defaultSelector: string|null = null):
-    {decorator: Map<string, ts.Expression>, metadata: R3DirectiveMetadata}|undefined {
+    flags: HandlerFlags, annotateForClosureCompiler: boolean,
+    defaultSelector: string|null = null): {
+  decorator: Map<string, ts.Expression>,
+  metadata: R3DirectiveMetadata,
+  inputs: ClassPropertyMapping,
+  outputs: ClassPropertyMapping,
+}|undefined {
   let directive: Map<string, ts.Expression>;
   if (decorator === null || decorator.args === null || decorator.args.length === 0) {
     directive = new Map<string, ts.Expression>();
@@ -331,15 +339,18 @@ 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,
     host,
     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(

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<T2DirectiveMeta>();
+      const dirMeta: T2DirectiveMeta = {
+        exportAs: null,
+        inputs: analysis.inputs,
+        outputs: analysis.outputs,
+        isComponent: false,
+        name: 'Dir',
+      };
+      matcher.addSelectables(CssSelector.parse('[dir]'), dirMeta);
+
+      const {nodes} = parseTemplate('<div dir [propName]="expr"></div>', '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

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

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';

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<string>;
+  coercedInputFields: Set<ClassPropertyName>;
 
   /**
    * The set of input fields which map to `readonly`, `private`, or `protected` members in the
    * Directive's class.
    */
-  restrictedInputFields: Set<string>;
+  restrictedInputFields: Set<ClassPropertyName>;
 
   /**
    * 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<string>;
+  stringLiteralInputFields: Set<ClassPropertyName>;
 
   /**
    * The set of input fields which do not have corresponding members in the Directive's class.
    */
-  undeclaredInputFields: Set<string>;
+  undeclaredInputFields: Set<ClassPropertyName>;
 
   /**
    * Whether the Directive's class is generic, i.e. `class MyDir<T> {...}`.
@@ -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.
    *

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,15 +77,18 @@ 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,
       isComponent: def.name === 'ɵcmp',
       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),

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<string>();
-  const undeclaredInputFields = new Set<string>();
-  const restrictedInputFields = new Set<string>();
-  const stringLiteralInputFields = new Set<string>();
+  const coercedInputFields = new Set<ClassPropertyName>();
+  const undeclaredInputFields = new Set<ClassPropertyName>();
+  const restrictedInputFields = new Set<ClassPropertyName>();
+  const stringLiteralInputFields = new Set<ClassPropertyName>();
   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);