feat(compiler-cli): Add compiler option to report errors when assigning to restricted input fields (angular#38249)

The compiler does not currently report errors when there's an `@Input()`
for a `private`, `protected`, or `readonly` directive/component class member.
This change adds an option to enable reporting errors when a template
attempts to bind to one of these restricted input fields.

PR Close angular#38249

Author: atscott

aio/content/guide/template-typecheck.md

@@ -114,6 +114,7 @@ In case of a false positive like these, there are a few options:
 |Strictness flag|Effect|
 |-|-|
 |`strictInputTypes`|Whether the assignability of a binding expression to the `@Input()` field is checked. Also affects the inference of directive generic types. |
+|`strictInputAccessModifiers`|Whether access modifiers such as `private`/`protected`/`readonly` are honored when assigning a binding expression to an `@Input()`. If disabled, the access modifiers of the `@Input` are ignored; only the type is checked.|
 |`strictNullInputTypes`|Whether `strictNullChecks` is honored when checking `@Input()` bindings (per `strictInputTypes`). Turning this off can be useful when using a library that was not built with `strictNullChecks` in mind.|
 |`strictAttributeTypes`|Whether to check `@Input()` bindings that are made using text attributes (for example, `<mat-tab label="Step 1">` vs `<mat-tab [label]="'Step 1'">`).
 |`strictSafeNavigationTypes`|Whether the return type of safe navigation operations (for example, `user?.name`) will be correctly inferred based on the type of `user`). If disabled, `user?.name` will be of type `any`.

goldens/public-api/compiler-cli/compiler_options.d.ts

@@ -35,6 +35,7 @@ export interface StrictTemplateOptions {
     strictContextGenerics?: boolean;
     strictDomEventTypes?: boolean;
     strictDomLocalRefTypes?: boolean;
+    strictInputAccessModifiers?: boolean;
     strictInputTypes?: boolean;
     strictLiteralTypes?: boolean;
     strictNullInputTypes?: boolean;

packages/compiler-cli/src/ngtsc/core/api/src/public_options.ts

@@ -147,6 +147,18 @@ export interface StrictTemplateOptions {
    */
   strictInputTypes?: boolean;
 
+  /**
+   * Whether to check if the input binding attempts to assign to a restricted field (readonly,
+   * private, or protected) on the directive/component.
+   *
+   * Defaults to `false`, even if "fullTemplateTypeCheck", "strictTemplates" and/or
+   * "strictInputTypes" is set. Note that if `strictInputTypes` is not set, or set to `false`, this
+   * flag has no effect.
+   *
+   * Tracking issue for enabling this by default: https://github.com/angular/angular/issues/38400
+   */
+  strictInputAccessModifiers?: boolean;
+
   /**
    * Whether to use strict null types for input bindings for directives.
    *

packages/compiler-cli/src/ngtsc/core/src/compiler.ts

@@ -415,6 +415,7 @@ export class NgCompiler {
         checkQueries: false,
         checkTemplateBodies: true,
         checkTypeOfInputBindings: strictTemplates,
+        honorAccessModifiersForInputBindings: false,
         strictNullInputBindings: strictTemplates,
         checkTypeOfAttributes: strictTemplates,
         // Even in full template type-checking mode, DOM binding checks are not quite ready yet.
@@ -442,6 +443,7 @@ export class NgCompiler {
         checkTemplateBodies: false,
         checkTypeOfInputBindings: false,
         strictNullInputBindings: false,
+        honorAccessModifiersForInputBindings: false,
         checkTypeOfAttributes: false,
         checkTypeOfDomBindings: false,
         checkTypeOfOutputEvents: false,
@@ -462,6 +464,10 @@ export class NgCompiler {
       typeCheckingConfig.checkTypeOfInputBindings = this.options.strictInputTypes;
       typeCheckingConfig.applyTemplateContextGuards = this.options.strictInputTypes;
     }
+    if (this.options.strictInputAccessModifiers !== undefined) {
+      typeCheckingConfig.honorAccessModifiersForInputBindings =
+          this.options.strictInputAccessModifiers;
+    }
     if (this.options.strictNullInputTypes !== undefined) {
       typeCheckingConfig.strictNullInputBindings = this.options.strictNullInputTypes;
     }

packages/compiler-cli/src/ngtsc/metadata/src/api.ts

@@ -60,6 +60,13 @@ export interface DirectiveTypeCheckMeta {
    */
   restrictedInputFields: Set<string>;
 
+  /**
+   * 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>;
+
   /**
    * The set of input fields which do not have corresponding members in the Directive's class.
    */

packages/compiler-cli/src/ngtsc/metadata/src/inheritance.ts

@@ -27,9 +27,10 @@ export function flattenInheritedDirectiveMetadata(
 
   let inputs: {[key: string]: string|[string, string]} = {};
   let outputs: {[key: string]: string} = {};
-  let coercedInputFields = new Set<string>();
-  let undeclaredInputFields = new Set<string>();
-  let restrictedInputFields = new Set<string>();
+  const coercedInputFields = new Set<string>();
+  const undeclaredInputFields = new Set<string>();
+  const restrictedInputFields = new Set<string>();
+  const stringLiteralInputFields = new Set<string>();
   let isDynamic = false;
 
   const addMetadata = (meta: DirectiveMeta): void => {
@@ -56,6 +57,9 @@ export function flattenInheritedDirectiveMetadata(
     for (const restrictedInputField of meta.restrictedInputFields) {
       restrictedInputFields.add(restrictedInputField);
     }
+    for (const field of meta.stringLiteralInputFields) {
+      stringLiteralInputFields.add(field);
+    }
   };
 
   addMetadata(topMeta);
@@ -67,6 +71,7 @@ export function flattenInheritedDirectiveMetadata(
     coercedInputFields,
     undeclaredInputFields,
     restrictedInputFields,
+    stringLiteralInputFields,
     baseClass: isDynamic ? 'dynamic' : null,
   };
 }

packages/compiler-cli/src/ngtsc/metadata/src/util.ts

@@ -98,15 +98,21 @@ export function extractDirectiveTypeCheckMeta(
                   .filter((inputName): inputName is string => inputName !== null));
 
   const restrictedInputFields = new Set<string>();
+  const stringLiteralInputFields = new Set<string>();
   const undeclaredInputFields = new Set<string>();
 
   for (const fieldName of Object.keys(inputs)) {
     const field = members.find(member => member.name === fieldName);
     if (field === undefined || field.node === null) {
       undeclaredInputFields.add(fieldName);
-    } else if (isRestricted(field.node)) {
+      continue;
+    }
+    if (isRestricted(field.node)) {
       restrictedInputFields.add(fieldName);
     }
+    if (field.nameNode !== null && ts.isStringLiteral(field.nameNode)) {
+      stringLiteralInputFields.add(fieldName);
+    }
   }
 
   const arity = reflector.getGenericArityOfClass(node);
@@ -116,6 +122,7 @@ export function extractDirectiveTypeCheckMeta(
     ngTemplateGuards,
     coercedInputFields,
     restrictedInputFields,
+    stringLiteralInputFields,
     undeclaredInputFields,
     isGeneric: arity !== null && arity > 0,
   };

packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts

@@ -244,6 +244,7 @@ function fakeDirective(ref: Reference<ClassDeclaration>): DirectiveMeta {
     ngTemplateGuards: [],
     coercedInputFields: new Set<string>(),
     restrictedInputFields: new Set<string>(),
+    stringLiteralInputFields: new Set<string>(),
     undeclaredInputFields: new Set<string>(),
     isGeneric: false,
     baseClass: null,

packages/compiler-cli/src/ngtsc/typecheck/api/api.ts

@@ -90,6 +90,14 @@ export interface TypeCheckingConfig {
    */
   checkTypeOfInputBindings: boolean;
 
+  /**
+   * Whether to honor the access modifiers on input bindings for the component/directive.
+   *
+   * If a template binding attempts to assign to an input that is private/protected/readonly,
+   * this will produce errors when enabled but will not when disabled.
+   */
+  honorAccessModifiersForInputBindings: boolean;
+
   /**
    * Whether to use strict null types for input bindings for directives.
    *

packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts

@@ -453,8 +453,13 @@ class TcbDirectiveInputsOp extends TcbOp {
           // declared in a `@Directive` or `@Component` decorator's `inputs` property) there is no
           // assignment target available, so this field is skipped.
           continue;
-        } else if (this.dir.restrictedInputFields.has(fieldName)) {
-          // To ignore errors, assign to temp variable with type of the field
+        } else if (
+            !this.tcb.env.config.honorAccessModifiersForInputBindings &&
+            this.dir.restrictedInputFields.has(fieldName)) {
+          // If strict checking of access modifiers is disabled and the field is restricted
+          // (i.e. private/protected/readonly), generate an assignment into a temporary variable
+          // that has the type of the field. This achieves type-checking but circumvents the access
+          // modifiers.
           const id = this.tcb.allocateId();
           const dirTypeRef = this.tcb.env.referenceType(this.dir.ref);
           if (!ts.isTypeReferenceNode(dirTypeRef)) {
@@ -464,23 +469,16 @@ class TcbDirectiveInputsOp extends TcbOp {
           const type = ts.createIndexedAccessTypeNode(
               ts.createTypeQueryNode(dirId as ts.Identifier),
               ts.createLiteralTypeNode(ts.createStringLiteral(fieldName)));
-          const temp = tsCreateVariable(id, ts.createNonNullExpression(ts.createNull()), type);
-          addParseSpanInfo(temp, input.attribute.sourceSpan);
+          const temp = tsDeclareVariable(id, type);
           this.scope.addStatement(temp);
           target = id;
-
-          // TODO: To get errors assign directly to the fields on the instance, using dot access
-          // when possible
-
         } else {
-          // Otherwise, a declaration exists in which case the `dir["fieldName"]` syntax is used
-          // as assignment target. An element access is used instead of a property access to
-          // support input names that are not valid JavaScript identifiers. Additionally, using
-          // element access syntax does not produce
-          // TS2341 "Property $prop is private and only accessible within class $class." nor
-          // TS2445 "Property $prop is protected and only accessible within class $class and its
-          //         subclasses."
-          target = ts.createElementAccess(dirId, ts.createStringLiteral(fieldName));
+          // To get errors assign directly to the fields on the instance, using property access
+          // when possible. String literal fields may not be valid JS identifiers so we use
+          // literal element access instead for those cases.
+          target = this.dir.stringLiteralInputFields.has(fieldName) ?
+              ts.createElementAccess(dirId, ts.createStringLiteral(fieldName)) :
+              ts.createPropertyAccess(dirId, ts.createIdentifier(fieldName));
         }
 
         // Finally the assignment is extended by assigning it into the target expression.

packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts

@@ -157,6 +157,7 @@ export const ALL_ENABLED_CONFIG: TypeCheckingConfig = {
   checkQueries: false,
   checkTemplateBodies: true,
   checkTypeOfInputBindings: true,
+  honorAccessModifiersForInputBindings: true,
   strictNullInputBindings: true,
   checkTypeOfAttributes: true,
   // Feature is still in development.
@@ -178,10 +179,11 @@ export type TestDirective = Partial<Pick<
     TypeCheckableDirectiveMeta,
     Exclude<
         keyof TypeCheckableDirectiveMeta,
-        'ref'|'coercedInputFields'|'restrictedInputFields'|'undeclaredInputFields'>>>&{
+        'ref'|'coercedInputFields'|'restrictedInputFields'|'stringLiteralInputFields'|
+        'undeclaredInputFields'>>>&{
   selector: string, name: string, file?: AbsoluteFsPath, type: 'directive',
       coercedInputFields?: string[], restrictedInputFields?: string[],
-      undeclaredInputFields?: string[], isGeneric?: boolean;
+      stringLiteralInputFields?: string[], undeclaredInputFields?: string[], isGeneric?: boolean;
 };
 export type TestPipe = {
   name: string,
@@ -212,6 +214,7 @@ export function tcb(
     applyTemplateContextGuards: true,
     checkQueries: false,
     checkTypeOfInputBindings: true,
+    honorAccessModifiersForInputBindings: false,
     strictNullInputBindings: true,
     checkTypeOfAttributes: true,
     checkTypeOfDomBindings: false,
@@ -420,6 +423,7 @@ function prepareDeclarations(
       ngTemplateGuards: decl.ngTemplateGuards || [],
       coercedInputFields: new Set<string>(decl.coercedInputFields || []),
       restrictedInputFields: new Set<string>(decl.restrictedInputFields || []),
+      stringLiteralInputFields: new Set<string>(decl.stringLiteralInputFields || []),
       undeclaredInputFields: new Set<string>(decl.undeclaredInputFields || []),
       isGeneric: decl.isGeneric ?? false,
       outputs: decl.outputs || {},