feat(compiler): support safe keyed read expressions (#41911)

Currently we support safe property (`a?.b`) and method (`a?.b()`) accesses, but we don't handle safe keyed reads (`a?.[0]`) which is inconsistent. These changes expand the compiler in order to support safe key read expressions as well. PR Close #41911
2021-05-01 18:46:34 +02:00
parent 47270d9e63
commit ba084857ea
21 changed files with 385 additions and 50 deletions
@@ -6,7 +6,7 @@
 * found in the LICENSE file at https://angular.io/license
 */

-import {AST, AstVisitor, ASTWithSource, Binary, BindingPipe, Chain, Conditional, EmptyExpr, FunctionCall, ImplicitReceiver, Interpolation, KeyedRead, KeyedWrite, LiteralArray, LiteralMap, LiteralPrimitive, MethodCall, NonNullAssert, PrefixNot, PropertyRead, PropertyWrite, Quote, SafeMethodCall, SafePropertyRead, ThisReceiver, Unary} from '@angular/compiler';
+import {AST, AstVisitor, ASTWithSource, Binary, BindingPipe, Chain, Conditional, EmptyExpr, FunctionCall, ImplicitReceiver, Interpolation, KeyedRead, KeyedWrite, LiteralArray, LiteralMap, LiteralPrimitive, MethodCall, NonNullAssert, PrefixNot, PropertyRead, PropertyWrite, Quote, SafeKeyedRead, SafeMethodCall, SafePropertyRead, ThisReceiver, Unary} from '@angular/compiler';
 import * as ts from 'typescript';

 import {TypeCheckingConfig} from '../api';
@@ -156,7 +156,7 @@ class AstTranslator implements AstVisitor {
  }

  visitKeyedRead(ast: KeyedRead): ts.Expression {
-    const receiver = wrapForDiagnostics(this.translate(ast.obj));
+    const receiver = wrapForDiagnostics(this.translate(ast.receiver));
    const key = this.translate(ast.key);
    const node = ts.createElementAccess(receiver, key);
    addParseSpanInfo(node, ast.sourceSpan);
@@ -164,7 +164,7 @@ class AstTranslator implements AstVisitor {
  }

  visitKeyedWrite(ast: KeyedWrite): ts.Expression {
-    const receiver = wrapForDiagnostics(this.translate(ast.obj));
+    const receiver = wrapForDiagnostics(this.translate(ast.receiver));
    const left = ts.createElementAccess(receiver, this.translate(ast.key));
    // TODO(joost): annotate `left` with the span of the element access, which is not currently
    //  available on `ast`.
@@ -330,6 +330,30 @@ class AstTranslator implements AstVisitor {
    addParseSpanInfo(node, ast.sourceSpan);
    return node;
  }
+
+  visitSafeKeyedRead(ast: SafeKeyedRead): ts.Expression {
+    const receiver = wrapForDiagnostics(this.translate(ast.receiver));
+    const key = this.translate(ast.key);
+    let node: ts.Expression;
+
+    // The form of safe property reads depends on whether strictness is in use.
+    if (this.config.strictSafeNavigationTypes) {
+      // "a?.[...]" becomes (null as any ? a![...] : undefined)
+      const expr = ts.createElementAccess(ts.createNonNullExpression(receiver), key);
+      addParseSpanInfo(expr, ast.sourceSpan);
+      node = ts.createParen(ts.createConditional(NULL_AS_ANY, expr, UNDEFINED));
+    } else if (VeSafeLhsInferenceBugDetector.veWillInferAnyFor(ast)) {
+      // "a?.[...]" becomes (a as any)[...]
+      node = ts.createElementAccess(tsCastToAny(receiver), key);
+    } else {
+      // "a?.[...]" becomes (a!.[...] as any)
+      const expr = ts.createElementAccess(ts.createNonNullExpression(receiver), key);
+      addParseSpanInfo(expr, ast.sourceSpan);
+      node = tsCastToAny(expr);
+    }
+    addParseSpanInfo(node, ast.sourceSpan);
+    return node;
+  }
 }

 /**
@@ -348,8 +372,9 @@ class AstTranslator implements AstVisitor {
 class VeSafeLhsInferenceBugDetector implements AstVisitor {
  private static SINGLETON = new VeSafeLhsInferenceBugDetector();

-  static veWillInferAnyFor(ast: SafeMethodCall|SafePropertyRead) {
-    return ast.receiver.visit(VeSafeLhsInferenceBugDetector.SINGLETON);
+  static veWillInferAnyFor(ast: SafeMethodCall|SafePropertyRead|SafeKeyedRead) {
+    const visitor = VeSafeLhsInferenceBugDetector.SINGLETON;
+    return ast instanceof SafeKeyedRead ? ast.receiver.visit(visitor) : ast.receiver.visit(visitor);
  }

  visitUnary(ast: Unary): boolean {
@@ -418,4 +443,7 @@ class VeSafeLhsInferenceBugDetector implements AstVisitor {
  visitSafePropertyRead(ast: SafePropertyRead): boolean {
    return false;
  }
+  visitSafeKeyedRead(ast: SafeKeyedRead): boolean {
+    return false;
+  }
 }
@@ -412,6 +412,35 @@ runInEachFileSystem(() => {

        expect(messages).toEqual([]);
      });
+
+      it('does not produce diagnostic for safe keyed access', () => {
+        const messages =
+            diagnose(`<div [class.red-text]="person.favoriteColors?.[0] === 'red'"></div>`, `
+              export class TestComponent {
+                person: {
+                  favoriteColors?: string[];
+                };
+              }`);
+
+        expect(messages).toEqual([]);
+      });
+
+      it('infers a safe keyed read as undefined', () => {
+        const messages = diagnose(`<div (click)="log(person.favoriteColors?.[0])"></div>`, `
+          export class TestComponent {
+            person: {
+              favoriteColors?: string[];
+            };
+
+            log(color: string) {
+              console.log(color);
+            }
+          }`);
+
+        expect(messages).toEqual([
+          `TestComponent.html(1, 19): Argument of type 'string | undefined' is not assignable to parameter of type 'string'.`
+        ]);
+      });
    });

    it('computes line and column offsets', () => {
@@ -44,6 +44,7 @@ export function typescriptLibDts(): TestFile {
        call(...args: any[]): any;
      }
      declare interface Array<T> {
+        [index: number]: T;
        length: number;
      }
      declare interface String {
@@ -997,12 +997,13 @@ describe('type check blocks', () => {
    });

    describe('config.strictSafeNavigationTypes', () => {
-      const TEMPLATE = `{{a?.b}} {{a?.method()}}`;
+      const TEMPLATE = `{{a?.b}} {{a?.method()}} {{a?.[0]}}`;

      it('should use undefined for safe navigation operations when enabled', () => {
        const block = tcb(TEMPLATE, DIRECTIVES);
        expect(block).toContain('(null as any ? (((ctx).a))!.method() : undefined)');
        expect(block).toContain('(null as any ? (((ctx).a))!.b : undefined)');
+        expect(block).toContain('(null as any ? (((ctx).a))![0] : undefined)');
      });
      it('should use an \'any\' type for safe navigation operations when disabled', () => {
        const DISABLED_CONFIG:
@@ -1010,15 +1011,17 @@ describe('type check blocks', () => {
        const block = tcb(TEMPLATE, DIRECTIVES, DISABLED_CONFIG);
        expect(block).toContain('((((ctx).a))!.method() as any)');
        expect(block).toContain('((((ctx).a))!.b as any)');
+        expect(block).toContain('(((((ctx).a))![0] as any)');
      });
    });

    describe('config.strictSafeNavigationTypes (View Engine bug emulation)', () => {
-      const TEMPLATE = `{{a.method()?.b}} {{a()?.method()}}`;
+      const TEMPLATE = `{{a.method()?.b}} {{a()?.method()}} {{a.method()?.[0]}}`;
      it('should check the presence of a property/method on the receiver when enabled', () => {
        const block = tcb(TEMPLATE, DIRECTIVES);
        expect(block).toContain('(null as any ? ((((ctx).a)).method())!.b : undefined)');
        expect(block).toContain('(null as any ? ((ctx).a())!.method() : undefined)');
+        expect(block).toContain('(null as any ? ((((ctx).a)).method())![0] : undefined)');
      });
      it('should not check the presence of a property/method on the receiver when disabled', () => {
        const DISABLED_CONFIG:
@@ -1026,6 +1029,7 @@ describe('type check blocks', () => {
        const block = tcb(TEMPLATE, DIRECTIVES, DISABLED_CONFIG);
        expect(block).toContain('(((((ctx).a)).method()) as any).b');
        expect(block).toContain('(((ctx).a()) as any).method()');
+        expect(block).toContain('(((((ctx).a)).method()) as any)[0]');
      });
    });

@@ -0,0 +1,65 @@
+/****************************************************************************************************
+ * PARTIAL FILE: safe_keyed_read.js
+ ****************************************************************************************************/
+import { Component, NgModule } from '@angular/core';
+import * as i0 from "@angular/core";
+export class MyApp {
+    constructor() {
+        this.unknownNames = null;
+        this.knownNames = [['Frodo', 'Bilbo']];
+        this.species = null;
+        this.keys = null;
+        this.speciesMap = { key: 'unknown' };
+    }
+}
+MyApp.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyApp, deps: [], target: i0.ɵɵFactoryTarget.Component });
+MyApp.ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", type: MyApp, selector: "ng-component", ngImport: i0, template: `
+    <span [title]="'Your last name is ' + (unknownNames?.[0] || 'unknown')">
+      Hello, {{ knownNames?.[0]?.[1] }}!
+      You are a Balrog: {{ species?.[0]?.[1]?.[2]?.[3]?.[4]?.[5] || 'unknown' }}
+      You are an Elf: {{ speciesMap?.[keys?.[0] ?? 'key'] }}
+      You are an Orc: {{ speciesMap?.['key'] }}
+    </span>
+`, isInline: true });
+i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyApp, decorators: [{
+            type: Component,
+            args: [{
+                    template: `
+    <span [title]="'Your last name is ' + (unknownNames?.[0] || 'unknown')">
+      Hello, {{ knownNames?.[0]?.[1] }}!
+      You are a Balrog: {{ species?.[0]?.[1]?.[2]?.[3]?.[4]?.[5] || 'unknown' }}
+      You are an Elf: {{ speciesMap?.[keys?.[0] ?? 'key'] }}
+      You are an Orc: {{ speciesMap?.['key'] }}
+    </span>
+`
+                }]
+        }] });
+export class MyModule {
+}
+MyModule.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyModule, deps: [], target: i0.ɵɵFactoryTarget.NgModule });
+MyModule.ɵmod = i0.ɵɵngDeclareNgModule({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyModule, declarations: [MyApp] });
+MyModule.ɵinj = i0.ɵɵngDeclareInjector({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyModule });
+i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyModule, decorators: [{
+            type: NgModule,
+            args: [{ declarations: [MyApp] }]
+        }] });
+
+/****************************************************************************************************
+ * PARTIAL FILE: safe_keyed_read.d.ts
+ ****************************************************************************************************/
+import * as i0 from "@angular/core";
+export declare class MyApp {
+    unknownNames: string[] | null;
+    knownNames: string[][];
+    species: null;
+    keys: null;
+    speciesMap: Record<string, string>;
+    static ɵfac: i0.ɵɵFactoryDeclaration<MyApp, never>;
+    static ɵcmp: i0.ɵɵComponentDeclaration<MyApp, "ng-component", never, {}, {}, never, never>;
+}
+export declare class MyModule {
+    static ɵfac: i0.ɵɵFactoryDeclaration<MyModule, never>;
+    static ɵmod: i0.ɵɵNgModuleDeclaration<MyModule, [typeof MyApp], never, never>;
+    static ɵinj: i0.ɵɵInjectorDeclaration<MyModule>;
+}
+
@@ -0,0 +1,22 @@
+{
+  "$schema": "../../test_case_schema.json",
+  "cases": [
+    {
+      "description": "should handle safe keyed reads inside templates",
+      "inputFiles": [
+        "safe_keyed_read.ts"
+      ],
+      "expectations": [
+        {
+          "files": [
+            {
+              "expected": "safe_keyed_read_template.js",
+              "generated": "safe_keyed_read.js"
+            }
+          ],
+          "failureMessage": "Incorrect template"
+        }
+      ]
+    }
+  ]
+}
@@ -0,0 +1,23 @@
+import {Component, NgModule} from '@angular/core';
+
+@Component({
+  template: `
+    <span [title]="'Your last name is ' + (unknownNames?.[0] || 'unknown')">
+      Hello, {{ knownNames?.[0]?.[1] }}!
+      You are a Balrog: {{ species?.[0]?.[1]?.[2]?.[3]?.[4]?.[5] || 'unknown' }}
+      You are an Elf: {{ speciesMap?.[keys?.[0] ?? 'key'] }}
+      You are an Orc: {{ speciesMap?.['key'] }}
+    </span>
+`
+})
+export class MyApp {
+  unknownNames: string[]|null = null;
+  knownNames: string[][] = [['Frodo', 'Bilbo']];
+  species = null;
+  keys = null;
+  speciesMap: Record<string, string> = {key: 'unknown'};
+}
+
+@NgModule({declarations: [MyApp]})
+export class MyModule {
+}
@@ -0,0 +1,13 @@
+template: function MyApp_Template(rf, ctx) {
+  if (rf & 1) {
+    i0.ɵɵelementStart(0, "span", 0);
+    i0.ɵɵtext(1);
+    i0.ɵɵelementEnd();
+  }
+  if (rf & 2) {
+    let $tmp_0_0$;
+    i0.ɵɵproperty("title", "Your last name is " + ((ctx.unknownNames == null ? null : ctx.unknownNames[0]) || "unknown"));
+    i0.ɵɵadvance(1);
+    i0.ɵɵtextInterpolate4(" Hello, ", ctx.knownNames == null ? null : ctx.knownNames[0] == null ? null : ctx.knownNames[0][1], "! You are a Balrog: ", (ctx.species == null ? null : ctx.species[0] == null ? null : ctx.species[0][1] == null ? null : ctx.species[0][1][2] == null ? null : ctx.species[0][1][2][3] == null ? null : ctx.species[0][1][2][3][4] == null ? null : ctx.species[0][1][2][3][4][5]) || "unknown", " You are an Elf: ", ctx.speciesMap == null ? null : ctx.speciesMap[($tmp_0_0$ = ctx.keys == null ? null : ctx.keys[0]) !== null && $tmp_0_0$ !== undefined ? $tmp_0_0$ : "key"], " You are an Orc: ", ctx.speciesMap == null ? null : ctx.speciesMap["key"], " ");
+  }
+}