fix(ivy): ngcc - recognize accessor members in ES5 (#28357)

Prior to this change, accessor functions for getters and setters would
not be considered as class member, as their declaration is vastly
different from ES2015 syntax.

With this change, the ES5 reflection host has learned to recognize the
downleveled syntax for accessors, allowing for them to be considered as
class member once again.

Fixes #28226

PR Close #28357

packages/compiler-cli/src/ngcc/src/host/esm2015_host.ts

@@ -103,9 +103,9 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
     if (symbol.members) {
       symbol.members.forEach((value, key) => {
         const decorators = removeFromMap(decoratorsMap, key);
-        const member = this.reflectMember(value, decorators);
+        const reflectedMembers = this.reflectMembers(value, decorators);
-        if (member) {
+        if (reflectedMembers) {
-          members.push(member);
+          members.push(...reflectedMembers);
         }
       });
     }
@@ -114,9 +114,9 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
     if (symbol.exports) {
       symbol.exports.forEach((value, key) => {
         const decorators = removeFromMap(decoratorsMap, key);
-        const member = this.reflectMember(value, decorators, true);
+        const reflectedMembers = this.reflectMembers(value, decorators, true);
-        if (member) {
+        if (reflectedMembers) {
-          members.push(member);
+          members.push(...reflectedMembers);
         }
       });
     }
@@ -135,9 +135,9 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
       if (variableSymbol && variableSymbol.exports) {
         variableSymbol.exports.forEach((value, key) => {
           const decorators = removeFromMap(decoratorsMap, key);
-          const member = this.reflectMember(value, decorators, true);
+          const reflectedMembers = this.reflectMembers(value, decorators, true);
-          if (member) {
+          if (reflectedMembers) {
-            members.push(member);
+            members.push(...reflectedMembers);
           }
         });
       }
@@ -724,8 +724,8 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
    * @param isStatic true if this member is static, false if it is an instance property.
    * @returns the reflected member information, or null if the symbol is not a member.
    */
-  protected reflectMember(symbol: ts.Symbol, decorators?: Decorator[], isStatic?: boolean):
+  protected reflectMembers(symbol: ts.Symbol, decorators?: Decorator[], isStatic?: boolean):
-      ClassMember|null {
+      ClassMember[]|null {
     let kind: ClassMemberKind|null = null;
     let value: ts.Expression|null = null;
     let name: string|null = null;
@@ -783,11 +783,11 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
     }
 
     const type: ts.TypeNode = (node as any).type || null;
-    return {
+    return [{
       node,
       implementation: node, kind, type, name, nameNode, value, isStatic,
       decorators: decorators || []
-    };
+    }];
   }
 
   /**

packages/compiler-cli/src/ngcc/src/host/esm5_host.ts

@@ -208,24 +208,69 @@ export class Esm5ReflectionHost extends Esm2015ReflectionHost {
   /**
    * Reflect over a symbol and extract the member information, combining it with the
    * provided decorator information, and whether it is a static member.
+   *
+   * If a class member uses accessors (e.g getters and/or setters) then it gets downleveled
+   * in ES5 to a single `Object.defineProperty()` call. In that case we must parse this
+   * call to extract the one or two ClassMember objects that represent the accessors.
+   *
    * @param symbol the symbol for the member to reflect over.
    * @param decorators an array of decorators associated with the member.
    * @param isStatic true if this member is static, false if it is an instance property.
    * @returns the reflected member information, or null if the symbol is not a member.
    */
-  protected reflectMember(symbol: ts.Symbol, decorators?: Decorator[], isStatic?: boolean):
+  protected reflectMembers(symbol: ts.Symbol, decorators?: Decorator[], isStatic?: boolean):
-      ClassMember|null {
+      ClassMember[]|null {
-    const member = super.reflectMember(symbol, decorators, isStatic);
+    const node = symbol.valueDeclaration || symbol.declarations && symbol.declarations[0];
-    if (member && member.kind === ClassMemberKind.Method && member.isStatic && member.node &&
+    const propertyDefinition = getPropertyDefinition(node);
-        ts.isPropertyAccessExpression(member.node) && member.node.parent &&
+    if (propertyDefinition) {
-        ts.isBinaryExpression(member.node.parent) &&
+      const members: ClassMember[] = [];
-        ts.isFunctionExpression(member.node.parent.right)) {
+      if (propertyDefinition.setter) {
-      // Recompute the implementation for this member:
+        members.push({
-      // ES5 static methods are variable declarations so the declaration is actually the
+          node,
-      // initializer of the variable assignment
+          implementation: propertyDefinition.setter,
-      member.implementation = member.node.parent.right;
+          kind: ClassMemberKind.Setter,
+          type: null,
+          name: symbol.name,
+          nameNode: null,
+          value: null,
+          isStatic: isStatic || false,
+          decorators: decorators || [],
+        });
+
+        // Prevent attaching the decorators to a potential getter. In ES5, we can't tell where the
+        // decorators were originally attached to, however we only want to attach them to a single
+        // `ClassMember` as otherwise ngtsc would handle the same decorators twice.
+        decorators = undefined;
+      }
+      if (propertyDefinition.getter) {
+        members.push({
+          node,
+          implementation: propertyDefinition.getter,
+          kind: ClassMemberKind.Getter,
+          type: null,
+          name: symbol.name,
+          nameNode: null,
+          value: null,
+          isStatic: isStatic || false,
+          decorators: decorators || [],
+        });
+      }
+      return members;
     }
-    return member;
+
+    const members = super.reflectMembers(symbol, decorators, isStatic);
+    members && members.forEach(member => {
+      if (member && member.kind === ClassMemberKind.Method && member.isStatic && member.node &&
+          ts.isPropertyAccessExpression(member.node) && member.node.parent &&
+          ts.isBinaryExpression(member.node.parent) &&
+          ts.isFunctionExpression(member.node.parent.right)) {
+        // Recompute the implementation for this member:
+        // ES5 static methods are variable declarations so the declaration is actually the
+        // initializer of the variable assignment
+        member.implementation = member.node.parent.right;
+      }
+    });
+    return members;
   }
 
   /**
@@ -246,6 +291,62 @@ export class Esm5ReflectionHost extends Esm2015ReflectionHost {
 
 ///////////// Internal Helpers /////////////
 
+/**
+ * Represents the details about property definitions that were set using `Object.defineProperty`.
+ */
+interface PropertyDefinition {
+  setter: ts.FunctionExpression|null;
+  getter: ts.FunctionExpression|null;
+}
+
+/**
+ * In ES5, getters and setters have been downleveled into call expressions of
+ * `Object.defineProperty`, such as
+ *
+ * ```
+ * Object.defineProperty(Clazz.prototype, "property", {
+ *   get: function () {
+ *       return 'value';
+ *   },
+ *   set: function (value) {
+ *       this.value = value;
+ *   },
+ *   enumerable: true,
+ *   configurable: true
+ * });
+ * ```
+ *
+ * This function inspects the given node to determine if it corresponds with such a call, and if so
+ * extracts the `set` and `get` function expressions from the descriptor object, if they exist.
+ *
+ * @param node The node to obtain the property definition from.
+ * @returns The property definition if the node corresponds with accessor, null otherwise.
+ */
+function getPropertyDefinition(node: ts.Node): PropertyDefinition|null {
+  if (!ts.isCallExpression(node)) return null;
+
+  const fn = node.expression;
+  if (!ts.isPropertyAccessExpression(fn) || !ts.isIdentifier(fn.expression) ||
+      fn.expression.text !== 'Object' || fn.name.text !== 'defineProperty')
+    return null;
+
+  const descriptor = node.arguments[2];
+  if (!descriptor || !ts.isObjectLiteralExpression(descriptor)) return null;
+
+  return {
+    setter: readPropertyFunctionExpression(descriptor, 'set'),
+    getter: readPropertyFunctionExpression(descriptor, 'get'),
+  };
+}
+
+function readPropertyFunctionExpression(object: ts.ObjectLiteralExpression, name: string) {
+  const property = object.properties.find(
+      (p): p is ts.PropertyAssignment =>
+          ts.isPropertyAssignment(p) && ts.isIdentifier(p.name) && p.name.text === name);
+
+  return property && ts.isFunctionExpression(property.initializer) && property.initializer || null;
+}
+
 function getIifeBody(declaration: ts.VariableDeclaration): ts.Block|undefined {
   if (!declaration.initializer || !ts.isParenthesizedExpression(declaration.initializer)) {
     return undefined;

packages/compiler-cli/src/ngcc/test/host/esm5_host_spec.ts

@@ -47,6 +47,52 @@ const SOME_DIRECTIVE_FILE = {
     }());
   `,
 };
+const ACCESSORS_FILE = {
+  name: '/accessors.js',
+  contents: `
+    import { Directive, Input, Output } from '@angular/core';
+
+    var SomeDirective = (function() {
+      function SomeDirective() {
+      }
+      Object.defineProperty(SomeDirective.prototype, "setter", {
+          set: function (value) { this.value = value; },
+          enumerable: true,
+          configurable: true
+      });
+      Object.defineProperty(SomeDirective.prototype, "getter", {
+          get: function () { return null; },
+          enumerable: true,
+          configurable: true
+      });
+      Object.defineProperty(SomeDirective.prototype, "setterAndGetter", {
+          get: function () { return null; },
+          set: function (value) { this.value = value; },
+          enumerable: true,
+          configurable: true
+      });
+      Object.defineProperty(SomeDirective, "staticSetter", {
+          set: function (value) { this.value = value; },
+          enumerable: true,
+          configurable: true
+      });
+      Object.defineProperty(SomeDirective.prototype, "none", {
+          enumerable: true,
+          configurable: true
+      });
+      Object.defineProperty(SomeDirective.prototype, "incomplete");
+      SomeDirective.decorators = [
+        { type: Directive, args: [{ selector: '[someDirective]' },] }
+      ];
+      SomeDirective.propDecorators = {
+        "setter": [{ type: Input },],
+        "getter": [{ type: Output },],
+        "setterAndGetter": [{ type: Input },],
+      };
+      return SomeDirective;
+    }());
+  `,
+};
 
 const SIMPLE_CLASS_FILE = {
   name: '/simple_class.js',
@@ -627,7 +673,54 @@ describe('Esm5ReflectionHost', () => {
       const input2 = members.find(member => member.name === 'input2') !;
       expect(input2.kind).toEqual(ClassMemberKind.Property);
       expect(input2.isStatic).toEqual(false);
-      expect(input1.decorators !.map(d => d.name)).toEqual(['Input']);
+      expect(input2.decorators !.map(d => d.name)).toEqual(['Input']);
+    });
+
+    it('should find Object.defineProperty members on a class', () => {
+      const program = makeTestProgram(ACCESSORS_FILE);
+      const host = new Esm5ReflectionHost(false, program.getTypeChecker());
+      const classNode =
+          getDeclaration(program, ACCESSORS_FILE.name, 'SomeDirective', ts.isVariableDeclaration);
+      const members = host.getMembersOfClass(classNode);
+
+      const setter = members.find(member => member.name === 'setter') !;
+      expect(setter.kind).toEqual(ClassMemberKind.Setter);
+      expect(setter.isStatic).toEqual(false);
+      expect(setter.value).toBeNull();
+      expect(setter.decorators !.map(d => d.name)).toEqual(['Input']);
+      expect(ts.isFunctionExpression(setter.implementation !)).toEqual(true);
+      expect((setter.implementation as ts.FunctionExpression).body.statements[0].getText())
+          .toEqual('this.value = value;');
+
+      const getter = members.find(member => member.name === 'getter') !;
+      expect(getter.kind).toEqual(ClassMemberKind.Getter);
+      expect(getter.isStatic).toEqual(false);
+      expect(getter.value).toBeNull();
+      expect(getter.decorators !.map(d => d.name)).toEqual(['Output']);
+      expect(ts.isFunctionExpression(getter.implementation !)).toEqual(true);
+      expect((getter.implementation as ts.FunctionExpression).body.statements[0].getText())
+          .toEqual('return null;');
+
+      const [combinedSetter, combinedGetter] =
+          members.filter(member => member.name === 'setterAndGetter');
+      expect(combinedSetter.kind).toEqual(ClassMemberKind.Setter);
+      expect(combinedSetter.isStatic).toEqual(false);
+      expect(combinedSetter.decorators !.map(d => d.name)).toEqual(['Input']);
+      expect(combinedGetter.kind).toEqual(ClassMemberKind.Getter);
+      expect(combinedGetter.isStatic).toEqual(false);
+      expect(combinedGetter.decorators !.map(d => d.name)).toEqual([]);
+
+      const staticSetter = members.find(member => member.name === 'staticSetter') !;
+      expect(staticSetter.kind).toEqual(ClassMemberKind.Setter);
+      expect(staticSetter.isStatic).toEqual(true);
+      expect(staticSetter.value).toBeNull();
+      expect(staticSetter.decorators !.map(d => d.name)).toEqual([]);
+
+      const none = members.find(member => member.name === 'none');
+      expect(none).toBeUndefined();
+
+      const incomplete = members.find(member => member.name === 'incomplete');
+      expect(incomplete).toBeUndefined();
     });
 
     it('should find non decorated properties on a class', () => {