refactor(compiler-cli): support enforcing field access for initializer APIs (#55070)

devversion · dylhunn · commit 75d1cae616f1 · 2024-03-28T09:13:07.000-07:00
An initializer API like `input`, `output`, or signal queries may not be compatible with certain access levels. E.g. queries cannot work with ES private class fields. This commit introduces a check for access levels into the initializer API recognition— enforcing that every initializer API *clearly* specifies what type of access is allowed. PR Close #55070
diff --git a/goldens/public-api/compiler-cli/error_code.md b/goldens/public-api/compiler-cli/error_code.md
@@ -56,6 +56,7 @@ export enum ErrorCode {
     INACCESSIBLE_DEFERRED_TRIGGER_ELEMENT = 8010,
     INCORRECTLY_DECLARED_ON_STATIC_MEMBER = 1100,
     INITIALIZER_API_DECORATOR_METADATA_COLLISION = 1051,
+    INITIALIZER_API_DISALLOWED_MEMBER_VISIBILITY = 1053,
     INITIALIZER_API_NO_REQUIRED_FUNCTION = 1052,
     INITIALIZER_API_WITH_DISALLOWED_DECORATOR = 1050,
     INJECTABLE_DUPLICATE_PROV = 9001,
diff --git a/packages/compiler-cli/src/ngtsc/annotations/directive/src/initializer_function_access.ts b/packages/compiler-cli/src/ngtsc/annotations/directive/src/initializer_function_access.ts
@@ -0,0 +1,34 @@
+/**
+ * @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 {ErrorCode, FatalDiagnosticError, makeDiagnosticChain} from '../../../diagnostics';
+import {ClassMember} from '../../../reflection';
+import {classMemberAccessLevelToString} from '../../../reflection/src/util';
+
+import {InitializerFunctionMetadata} from './initializer_functions';
+
+/**
+ * Validates that the initializer member is compatible with the given class
+ * member in terms of field access and visibility.
+ *
+ * @throws {FatalDiagnosticError} If the recognized initializer API is
+ *   incompatible.
+ */
+export function validateAccessOfInitializerApiMember(
+    {api, call}: InitializerFunctionMetadata, member: Pick<ClassMember, 'accessLevel'>): void {
+  if (!api.allowedAccessLevels.some(level => level === member.accessLevel)) {
+    throw new FatalDiagnosticError(
+        ErrorCode.INITIALIZER_API_DISALLOWED_MEMBER_VISIBILITY, call,
+        makeDiagnosticChain(
+            `Cannot use "${api.functionName}" on a class member that is declared as ${
+                classMemberAccessLevelToString(member.accessLevel)}.`,
+            [makeDiagnosticChain(
+                `Update the class field to be either: ` +
+                api.allowedAccessLevels.map(l => classMemberAccessLevelToString(l)).join(', '))]));
+  }
+}
diff --git a/packages/compiler-cli/src/ngtsc/annotations/directive/src/initializer_functions.ts b/packages/compiler-cli/src/ngtsc/annotations/directive/src/initializer_functions.ts
@@ -9,7 +9,7 @@
 import ts from 'typescript';
 
 import {ImportedSymbolsTracker} from '../../../imports';
-import {ClassMember, ReflectionHost} from '../../../reflection';
+import {ClassMemberAccessLevel, ReflectionHost} from '../../../reflection';
 
 /**
  * @fileoverview
@@ -24,16 +24,20 @@ import {ClassMember, ReflectionHost} from '../../../reflection';
  */
 
 export interface InitializerApiFunction {
+  /** Module name where the initializer function is imported from. */
   owningModule: '@angular/core'|'@angular/core/rxjs-interop';
+  /** Export name of the initializer function. */
   functionName: ('input'|'model'|'output'|'outputFromObservable'|'viewChild'|'viewChildren'|
                  'contentChild'|'contentChildren');
+  /** Class member access levels compatible with the API. */
+  allowedAccessLevels: ClassMemberAccessLevel[];
 }
 
 /**
  * Metadata describing an Angular class member that was recognized through
  * a function initializer. Like `input`, `input.required` or `viewChild`.
  */
-interface InitializerFunctionMetadata {
+export interface InitializerFunctionMetadata {
   /** Initializer API function that was recognized. */
   api: InitializerApiFunction;
   /** Node referring to the call expression. */
@@ -58,11 +62,12 @@ interface StaticInitializerData {
 }
 
 /**
- * Attempts to identify an Angular class member that is declared via
- * its initializer referring to a given initializer API function.
+ * Attempts to identify an Angular initializer function call.
  *
  * Note that multiple possible initializer API function names can be specified,
  * allowing for checking multiple types in one pass.
+ *
+ * @returns The parsed initializer API, or null if none was found.
  */
 export function tryParseInitializerApi<Functions extends InitializerApiFunction[]>(
     functions: Functions, expression: ts.Expression, reflector: ReflectionHost,
@@ -80,19 +85,21 @@ export function tryParseInitializerApi<Functions extends InitializerApiFunction[
     return null;
   }
 
+  const {api, apiReference, isRequired} = staticResult;
+
   // Once we've statically determined that the initializer is one of the APIs we're looking for, we
   // need to verify it using the type checker which accounts for things like shadowed variables.
   // This should be done as the absolute last step since using the type check can be expensive.
-  const resolvedImport = reflector.getImportOfIdentifier(staticResult.apiReference);
-  if (resolvedImport === null || staticResult.api.functionName !== resolvedImport.name ||
-      staticResult.api.owningModule !== resolvedImport.from) {
+  const resolvedImport = reflector.getImportOfIdentifier(apiReference);
+  if (resolvedImport === null || api.functionName !== resolvedImport.name ||
+      api.owningModule !== resolvedImport.from) {
     return null;
   }
 
   return {
-    api: staticResult.api,
+    api,
     call: expression,
-    isRequired: staticResult.isRequired,
+    isRequired,
   };
 }
 
diff --git a/packages/compiler-cli/src/ngtsc/annotations/directive/test/BUILD.bazel b/packages/compiler-cli/src/ngtsc/annotations/directive/test/BUILD.bazel
@@ -13,6 +13,7 @@ ts_library(
         "//packages/compiler",
         "//packages/compiler-cli/src/ngtsc/annotations/common",
         "//packages/compiler-cli/src/ngtsc/annotations/directive",
+        "//packages/compiler-cli/src/ngtsc/diagnostics",
         "//packages/compiler-cli/src/ngtsc/file_system",
         "//packages/compiler-cli/src/ngtsc/file_system/testing",
         "//packages/compiler-cli/src/ngtsc/imports",
diff --git a/packages/compiler-cli/src/ngtsc/annotations/directive/test/initializer_functions_spec.ts b/packages/compiler-cli/src/ngtsc/annotations/directive/test/initializer_functions_spec.ts
@@ -8,18 +8,22 @@
 
 import ts from 'typescript';
 
+import {ErrorCode, FatalDiagnosticError} from '../../../diagnostics';
 import {absoluteFrom} from '../../../file_system';
 import {runInEachFileSystem} from '../../../file_system/testing';
 import {ImportedSymbolsTracker} from '../../../imports';
-import {ClassMember, TypeScriptReflectionHost} from '../../../reflection';
+import {ClassMember, ClassMemberAccessLevel, TypeScriptReflectionHost} from '../../../reflection';
+import {reflectClassMember} from '../../../reflection/src/typescript';
 import {makeProgram} from '../../../testing';
+import {validateAccessOfInitializerApiMember} from '../src/initializer_function_access';
 import {InitializerApiFunction, tryParseInitializerApi} from '../src/initializer_functions';
 
 
 runInEachFileSystem(() => {
   const modelApi: InitializerApiFunction = {
     functionName: 'model',
     owningModule: '@angular/core',
+    allowedAccessLevels: [ClassMemberAccessLevel.PublicWritable],
   };
 
   describe('initializer function detection', () => {
@@ -54,7 +58,11 @@ runInEachFileSystem(() => {
 
       const result = tryParseInitializerApi(
           [
-            {functionName: 'input', owningModule: '@angular/core'},
+            {
+              functionName: 'input',
+              owningModule: '@angular/core',
+              allowedAccessLevels: [ClassMemberAccessLevel.PublicWritable],
+            },
             modelApi,
           ],
           member.value!, reflector, importTracker);
@@ -80,14 +88,19 @@ runInEachFileSystem(() => {
       const result = tryParseInitializerApi(
           [
             modelApi,
-            {functionName: 'outputFromObservable', owningModule: '@angular/core/rxjs-interop'},
+            {
+              functionName: 'outputFromObservable',
+              owningModule: '@angular/core/rxjs-interop',
+              allowedAccessLevels: [ClassMemberAccessLevel.PublicWritable],
+            },
           ],
           member.value!, reflector, importTracker);
 
       expect(result).toEqual({
         api: {
           functionName: 'outputFromObservable',
           owningModule: '@angular/core/rxjs-interop',
+          allowedAccessLevels: [ClassMemberAccessLevel.PublicWritable],
         },
         isRequired: false,
         call: jasmine.objectContaining({kind: ts.SyntaxKind.CallExpression}),
@@ -286,40 +299,142 @@ runInEachFileSystem(() => {
          call: jasmine.objectContaining({kind: ts.SyntaxKind.CallExpression}),
        });
      });
+
+  describe('`validateAccessOfInitializerApiMember`', () => {
+    it('should report errors if a private field is used, but not allowed', () => {
+      const {member, reflector, importTracker} = setup(`
+        import {Directive, model} from '@angular/core';
+
+        @Directive()
+        class Dir {
+          private test = model(1);
+        }
+      `);
+
+      const result = tryParseInitializerApi([modelApi], member.value!, reflector, importTracker);
+
+      expect(result).not.toBeNull();
+      expect(() => validateAccessOfInitializerApiMember(result!, member))
+          .toThrowMatching(
+              err => err instanceof FatalDiagnosticError &&
+                  err.code === ErrorCode.INITIALIZER_API_DISALLOWED_MEMBER_VISIBILITY);
+    });
+
+    it('should report errors if a protected field is used, but not allowed', () => {
+      const {member, reflector, importTracker} = setup(`
+        import {Directive, model} from '@angular/core';
+
+        @Directive()
+        class Dir {
+          protected test = model(1);
+        }
+      `);
+
+      const result = tryParseInitializerApi([modelApi], member.value!, reflector, importTracker);
+
+      expect(result).not.toBeNull();
+      expect(() => validateAccessOfInitializerApiMember(result!, member))
+          .toThrowMatching(
+              err => err instanceof FatalDiagnosticError &&
+                  err.code === ErrorCode.INITIALIZER_API_DISALLOWED_MEMBER_VISIBILITY);
+    });
+
+    it('should report errors if an ECMAScript private field is used, but not allowed', () => {
+      const {member, reflector, importTracker} = setup(`
+        import {Directive, model} from '@angular/core';
+
+        @Directive()
+        class Dir {
+          #test = model(1);
+        }
+      `);
+
+      const result = tryParseInitializerApi([modelApi], member.value!, reflector, importTracker);
+
+      expect(result).not.toBeNull();
+      expect(() => validateAccessOfInitializerApiMember(result!, member))
+          .toThrowMatching(
+              err => err instanceof FatalDiagnosticError &&
+                  err.code === ErrorCode.INITIALIZER_API_DISALLOWED_MEMBER_VISIBILITY);
+    });
+
+    it('should report errors if a readonly public field is used, but not allowed', () => {
+      const {member, reflector, importTracker} = setup(`
+        import {Directive, model} from '@angular/core';
+
+        @Directive()
+        class Dir {
+          // test model initializer API definition doesn't even allow readonly!
+          readonly test = model(1);
+        }
+      `);
+
+      const result = tryParseInitializerApi([modelApi], member.value!, reflector, importTracker);
+
+      expect(result).not.toBeNull();
+      expect(() => validateAccessOfInitializerApiMember(result!, member))
+          .toThrowMatching(
+              err => err instanceof FatalDiagnosticError &&
+                  err.code === ErrorCode.INITIALIZER_API_DISALLOWED_MEMBER_VISIBILITY);
+    });
+
+    it('should allow private field if API explicitly allows it', () => {
+      const {member, reflector, importTracker} = setup(`
+        import {Directive, model} from '@angular/core';
+
+        @Directive()
+        class Dir {
+          // test model initializer API definition doesn't even allow readonly!
+          private test = model(1);
+        }
+      `);
+
+      const result = tryParseInitializerApi(
+          [{...modelApi, allowedAccessLevels: [ClassMemberAccessLevel.Private]}], member.value!,
+          reflector, importTracker);
+
+      expect(result?.api).toEqual(jasmine.objectContaining<InitializerApiFunction>({
+        functionName: 'model'
+      }));
+      expect(() => validateAccessOfInitializerApiMember(result!, member)).not.toThrow();
+    });
+  });
 });
 
 
 function setup(contents: string) {
   const fileName = absoluteFrom('/test.ts');
-  const {program} = makeProgram([
-    {
-      name: absoluteFrom('/node_modules/@angular/core/index.d.ts'),
-      contents: `
+  const {program} = makeProgram(
+      [
+        {
+          name: absoluteFrom('/node_modules/@angular/core/index.d.ts'),
+          contents: `
         export const Directive: any;
         export const input: any;
         export const model: any;
       `,
-    },
-    {
-      name: absoluteFrom('/node_modules/@angular/core/rxjs-interop/index.d.ts'),
-      contents: `
+        },
+        {
+          name: absoluteFrom('/node_modules/@angular/core/rxjs-interop/index.d.ts'),
+          contents: `
         export const outputFromObservable: any;
       `,
-    },
-    {
-      name: absoluteFrom('/node_modules/@unknown/utils/index.d.ts'),
-      contents: `
+        },
+        {
+          name: absoluteFrom('/node_modules/@unknown/utils/index.d.ts'),
+          contents: `
         export declare function toString(value: any): string;
       `,
-    },
-    {
-      name: absoluteFrom('/node_modules/@not-angular/core/index.d.ts'),
-      contents: `
+        },
+        {
+          name: absoluteFrom('/node_modules/@not-angular/core/index.d.ts'),
+          contents: `
         export const model: any;
       `,
-    },
-    {name: fileName, contents}
-  ]);
+        },
+        {name: fileName, contents}
+      ],
+      {target: ts.ScriptTarget.ESNext});
   const sourceFile = program.getSourceFile(fileName);
   const importTracker = new ImportedSymbolsTracker();
   const reflector = new TypeScriptReflectionHost(program.getTypeChecker());
@@ -328,11 +443,13 @@ function setup(contents: string) {
     throw new Error(`Cannot resolve test file ${fileName}`);
   }
 
-  let member: Pick<ClassMember, 'value'>|null = null;
+  let member: Pick<ClassMember, 'value'|'accessLevel'>|null = null;
 
   (function walk(node: ts.Node) {
-    if (ts.isPropertyDeclaration(node) && ts.isIdentifier(node.name) && node.name.text === 'test') {
-      member = {value: node.initializer ?? null};
+    if (ts.isPropertyDeclaration(node) &&
+        (ts.isIdentifier(node.name) && node.name.text === 'test' ||
+         ts.isPrivateIdentifier(node.name) && node.name.text === '#test')) {
+      member = reflectClassMember(node);
     } else {
       ts.forEachChild(node, walk);
     }
diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts
@@ -44,6 +44,12 @@ export enum ErrorCode {
    */
   INITIALIZER_API_NO_REQUIRED_FUNCTION = 1052,
 
+  /**
+   * Raised whenever an initializer API is used on a class member
+   * and the given access modifiers (e.g. `private`) are not allowed.
+   */
+  INITIALIZER_API_DISALLOWED_MEMBER_VISIBILITY = 1053,
+
   /**
    * An Angular feature, like inputs, outputs or queries is incorrectly
    * declared on a static member.
