fix(language-service): show warning when type inference is suboptimal

The Ivy Language Service uses the compiler's template type-checking engine,
which honors the configuration in the user's tsconfig.json. We recommend
that users upgrade to `strictTemplates` mode in their projects to take
advantage of the best possible type inference, and thus to have the best
experience in Language Service.

If a project is not using `strictTemplates`, then the compiler will not
leverage certain type inference options it has. One case where this is very
noticeable is the inference of let- variables for structural directives that
provide a template context guard (such as NgFor). Without `strictTemplates`,
these guards will not be applied and such variables will be inferred as
'any', degrading the user experience within Language Service.

This is working as designed, since the Language Service _should_ reflect
types exactly as the compiler sees them. However, the View Engine Language
Service used its own type system that _would_ infer these types even when
the compiler did not. As a result, it's confusing to some users why the
Ivy Language Service has "worse" type inference.

To address this confusion, this commit implements a warning diagnostic which
is shown in the Language Service for variables which could have been
narrowed via a context guard, but the type checking configuration didn't
allow it. This should make the reason why variables receive the 'any' type
as well as the action needed to improve the typings much more obvious,
improving the Language Service experience.

Fixes angular/vscode-ng-language-service#1155
Closes angular#41042

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

@@ -690,6 +690,10 @@ export class NgCompiler {
         useContextGenericType: strictTemplates,
         strictLiteralTypes: true,
         enableTemplateTypeChecker: this.enableTemplateTypeChecker,
+        // Warnings for suboptimal type inference are only enabled if in Language Service mode
+        // (providing the full TemplateTypeChecker API) and if strict mode is not enabled. In strict
+        // mode, the user is in full control of type inference.
+        warnForSuboptimalTypeInference: this.enableTemplateTypeChecker && !strictTemplates,
       };
     } else {
       typeCheckingConfig = {
@@ -714,6 +718,9 @@ export class NgCompiler {
         useContextGenericType: false,
         strictLiteralTypes: false,
         enableTemplateTypeChecker: this.enableTemplateTypeChecker,
+        // In "basic" template type-checking mode, no warnings are produced since most things are
+        // not checked anyways.
+        warnForSuboptimalTypeInference: false,
       };
     }
 

packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts

@@ -180,6 +180,13 @@ export enum ErrorCode {
    * provided by Angular language service.
    */
   SUGGEST_STRICT_TEMPLATES = 10001,
+
+  /**
+   * Indicates that a particular structural directive provides advanced type narrowing
+   * functionality, but the current template type-checking configuration does not allow its usage in
+   * type inference.
+   */
+  WARN_SUBOPTIMAL_TYPE_INFERENCE = 10002,
 }
 
 /**

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

@@ -260,6 +260,13 @@ export interface TypeCheckingConfig {
    * literals are cast to `any` when declared.
    */
   strictLiteralTypes: boolean;
+
+  /**
+   * Whether or not to produce warnings in cases where the compiler could have used a template
+   * context guard to infer a better type for 'let' variables in a template, but was prohibited from
+   * doing so by the template type-checking configuration.
+   */
+  warnForSuboptimalTypeInference: boolean;
 }
 
 

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

@@ -68,6 +68,12 @@ export interface OutOfBandDiagnosticRecorder {
 
   requiresInlineTypeConstructors(
       templateId: TemplateId, node: ClassDeclaration, directives: ClassDeclaration[]): void;
+
+  /**
+   * Report a warning when structural directives support context guards, but the current
+   * type-checking configuration prohibits their usage.
+   */
+  suboptimalTypeInference(templateId: TemplateId, variables: TmplAstVariable[]): void;
 }
 
 export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecorder {
@@ -174,6 +180,37 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor
         directives.map(
             dir => makeRelatedInformation(dir.name, `Requires an inline type constructor.`))));
   }
+
+  suboptimalTypeInference(templateId: TemplateId, variables: TmplAstVariable[]): void {
+    const mapping = this.resolver.getSourceMapping(templateId);
+
+    // Select one of the template variables that's most suitable for reporting the diagnostic. Any
+    // variable will do, but prefer one bound to the context's $implicit if present.
+    let diagnosticVar: TmplAstVariable|null = null;
+    for (const variable of variables) {
+      if (diagnosticVar === null || (variable.value === '' || variable.value === '$implicit')) {
+        diagnosticVar = variable;
+      }
+    }
+    if (diagnosticVar === null) {
+      // There is no variable on which to report the diagnostic.
+      return;
+    }
+
+    let varIdentification = `'${diagnosticVar.name}'`;
+    if (variables.length === 2) {
+      varIdentification += ` (and 1 other)`;
+    } else if (variables.length > 2) {
+      varIdentification += ` ( and ${variables.length - 1} others)`;
+    }
+    const message =
+        `This structural directive supports advanced type inference, but the current compiler configuration prevents its usage. The variable ${
+            varIdentification} will have type 'any' as a result.\n\nConsider enabling the 'strictTemplates' option in your tsconfig.json for better type inference within this template.`;
+
+    this._diagnostics.push(makeTemplateDiagnostic(
+        templateId, mapping, diagnosticVar.keySpan, ts.DiagnosticCategory.Warning,
+        ngErrorCode(ErrorCode.WARN_SUBOPTIMAL_TYPE_INFERENCE), message));
+  }
 }
 
 function makeInlineDiagnostic(

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

@@ -285,11 +285,20 @@ class TcbTemplateBodyOp extends TcbOp {
 
         // The second kind of guard is a template context guard. This guard narrows the template
         // rendering context variable `ctx`.
-        if (dir.hasNgTemplateContextGuard && this.tcb.env.config.applyTemplateContextGuards) {
-          const ctx = this.scope.resolve(this.template);
-          const guardInvoke = tsCallMethod(dirId, 'ngTemplateContextGuard', [dirInstId, ctx]);
-          addParseSpanInfo(guardInvoke, this.template.sourceSpan);
-          directiveGuards.push(guardInvoke);
+        if (dir.hasNgTemplateContextGuard) {
+          if (this.tcb.env.config.applyTemplateContextGuards) {
+            const ctx = this.scope.resolve(this.template);
+            const guardInvoke = tsCallMethod(dirId, 'ngTemplateContextGuard', [dirInstId, ctx]);
+            addParseSpanInfo(guardInvoke, this.template.sourceSpan);
+            directiveGuards.push(guardInvoke);
+          } else if (
+              this.template.variables.length > 0 &&
+              this.tcb.env.config.warnForSuboptimalTypeInference) {
+            // The compiler could have inferred a better type for the variables in this template,
+            // but was prevented from doing so by the type-checking configuration. Issue a warning
+            // diagnostic.
+            this.tcb.oobRecorder.suboptimalTypeInference(this.tcb.id, this.template.variables);
+          }
         }
       }
     }

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

@@ -187,6 +187,7 @@ export const ALL_ENABLED_CONFIG: TypeCheckingConfig = {
   useContextGenericType: true,
   strictLiteralTypes: true,
   enableTemplateTypeChecker: false,
+  warnForSuboptimalTypeInference: false,
 };
 
 // Remove 'ref' from TypeCheckableDirectiveMeta and add a 'selector' instead.
@@ -246,6 +247,7 @@ export function tcb(
     useContextGenericType: true,
     strictLiteralTypes: true,
     enableTemplateTypeChecker: false,
+    warnForSuboptimalTypeInference: false,
   };
   options = options || {
     emitSpans: false,
@@ -644,4 +646,5 @@ export class NoopOobRecorder implements OutOfBandDiagnosticRecorder {
   duplicateTemplateVar(): void {}
   requiresInlineTcb(): void {}
   requiresInlineTypeConstructors(): void {}
+  suboptimalTypeInference(): void {}
 }

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

@@ -736,6 +736,7 @@ describe('type check blocks', () => {
       useContextGenericType: true,
       strictLiteralTypes: true,
       enableTemplateTypeChecker: false,
+      warnForSuboptimalTypeInference: false,
     };
 
     describe('config.applyTemplateContextGuards', () => {

packages/language-service/ivy/test/BUILD.bazel

@@ -7,6 +7,7 @@ ts_library(
     deps = [
         "//packages/compiler",
         "//packages/compiler-cli/src/ngtsc/core:api",
+        "//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/testing",

packages/language-service/ivy/test/diagnostic_spec.ts

@@ -6,6 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
+import {ErrorCode, ngErrorCode} from '@angular/compiler-cli/src/ngtsc/diagnostics';
 import {initMockFileSystem} from '@angular/compiler-cli/src/ngtsc/file_system/testing';
 import * as ts from 'typescript';
 
@@ -245,4 +246,40 @@ describe('getSemanticDiagnostics', () => {
       'component is missing a template',
     ]);
   });
+
+  it('reports a warning when the project configuration prevents good type inference', () => {
+    const files = {
+      'app.ts': `
+        import {Component, NgModule} from '@angular/core';
+        import {CommonModule} from '@angular/common';
+
+        @Component({
+          template: '<div *ngFor="let user of users">{{user}}</div>',
+        })
+        export class MyComponent {
+          users = ['Alpha', 'Beta'];
+        }
+      `
+    };
+
+    const project = createModuleAndProjectWithDeclarations(env, 'test', files, {
+      // Disable `strictTemplates`.
+      strictTemplates: false,
+      // Use `fullTemplateTypeCheck` mode instead.
+      fullTemplateTypeCheck: true,
+    });
+    const diags = project.getDiagnosticsForFile('app.ts');
+    expect(diags.length).toBe(1);
+    const diag = diags[0];
+    expect(diag.code).toBe(ngErrorCode(ErrorCode.WARN_SUBOPTIMAL_TYPE_INFERENCE));
+    expect(diag.category).toBe(ts.DiagnosticCategory.Warning);
+    expect(getTextOfDiagnostic(diag)).toBe('user');
+  });
 });
+
+function getTextOfDiagnostic(diag: ts.Diagnostic): string {
+  expect(diag.file).not.toBeUndefined();
+  expect(diag.start).not.toBeUndefined();
+  expect(diag.length).not.toBeUndefined();
+  return diag.file!.text.substring(diag.start!, diag.start! + diag.length!);
+}

packages/language-service/ivy/testing/src/project.ts

@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {StrictTemplateOptions} from '@angular/compiler-cli/src/ngtsc/core/api';
+import {LegacyNgcOptions, StrictTemplateOptions} from '@angular/compiler-cli/src/ngtsc/core/api';
 import {absoluteFrom, AbsoluteFsPath, FileSystem, getFileSystem, getSourceFileOrError} from '@angular/compiler-cli/src/ngtsc/file_system';
 import {OptimizeFor, TemplateTypeChecker} from '@angular/compiler-cli/src/ngtsc/typecheck/api';
 import * as ts from 'typescript/lib/tsserverlibrary';
@@ -19,7 +19,7 @@ export type ProjectFiles = {
 
 function writeTsconfig(
     fs: FileSystem, tsConfigPath: AbsoluteFsPath, entryFiles: AbsoluteFsPath[],
-    options: StrictTemplateOptions): void {
+    options: TestableOptions): void {
   fs.writeFile(
       tsConfigPath,
       JSON.stringify(
@@ -44,7 +44,7 @@ function writeTsconfig(
           null, 2));
 }
 
-export type TestableOptions = StrictTemplateOptions;
+export type TestableOptions = StrictTemplateOptions&Pick<LegacyNgcOptions, 'fullTemplateTypeCheck'>;
 
 export class Project {
   private tsProject: ts.server.Project;