fix(language-service): show suggestion when type inference is suboptimal (angular#41072)

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 suggestion 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

PR Close angular#41072

Author: alxhub

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

@@ -38,5 +38,6 @@ export declare enum ErrorCode {
     INLINE_TCB_REQUIRED = 8900,
     INLINE_TYPE_CTOR_REQUIRED = 8901,
     INJECTABLE_DUPLICATE_PROV = 9001,
-    SUGGEST_STRICT_TEMPLATES = 10001
+    SUGGEST_STRICT_TEMPLATES = 10001,
+    SUGGEST_SUBOPTIMAL_TYPE_INFERENCE = 10002
 }

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

@@ -692,6 +692,10 @@ export class NgCompiler {
         strictLiteralTypes: true,
         enableTemplateTypeChecker: this.enableTemplateTypeChecker,
         useInlineTypeConstructors,
+        // 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.
+        suggestionsForSuboptimalTypeInference: this.enableTemplateTypeChecker && !strictTemplates,
       };
     } else {
       typeCheckingConfig = {
@@ -717,6 +721,9 @@ export class NgCompiler {
         strictLiteralTypes: false,
         enableTemplateTypeChecker: this.enableTemplateTypeChecker,
         useInlineTypeConstructors,
+        // In "basic" template type-checking mode, no warnings are produced since most things are
+        // not checked anyways.
+        suggestionsForSuboptimalTypeInference: false,
       };
     }
 

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

@@ -171,15 +171,22 @@ export enum ErrorCode {
    */
   INJECTABLE_DUPLICATE_PROV = 9001,
 
-  // 10XXX error codes are reserved for diagnostics with category
-  // `ts.DiagnosticCategory.Suggestion`. These diagnostics are generated by
-  // language service.
+  // 10XXX error codes are reserved for diagnostics with categories other than
+  // `ts.DiagnosticCategory.Error`. These diagnostics are generated by the compiler when configured
+  // to do so by a tool such as the Language Service, or by the Language Service itself.
 
   /**
    * Suggest users to enable `strictTemplates` to make use of full capabilities
    * 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.
+   */
+  SUGGEST_SUBOPTIMAL_TYPE_INFERENCE = 10002,
 }
 
 /**

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

@@ -275,6 +275,20 @@ export interface TypeCheckingConfig {
    * inlining, this must be set to `false`.
    */
   useInlineTypeConstructors: boolean;
+
+  /**
+   * Whether or not to produce diagnostic suggestions in cases where the compiler could have
+   * inferred a better type for a construct, but was prevented from doing so by the current type
+   * checking configuration.
+   *
+   * For example, if the compiler could have used a template context guard to infer a better type
+   * for a structural directive's context and `let-` variables, but the user is in
+   * `fullTemplateTypeCheck` mode and such guards are therefore disabled.
+   *
+   * This mode is useful for clients like the Language Service which want to inform users of
+   * opportunities to improve their own developer experience.
+   */
+  suggestionsForSuboptimalTypeInference: 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.Suggestion,
+        ngErrorCode(ErrorCode.SUGGEST_SUBOPTIMAL_TYPE_INFERENCE), message));
+  }
 }
 
 function makeInlineDiagnostic(

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

@@ -286,11 +286,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.suggestionsForSuboptimalTypeInference) {
+            // 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

@@ -188,7 +188,8 @@ export const ALL_ENABLED_CONFIG: Readonly<TypeCheckingConfig> = {
   useContextGenericType: true,
   strictLiteralTypes: true,
   enableTemplateTypeChecker: false,
-  useInlineTypeConstructors: true
+  useInlineTypeConstructors: true,
+  suggestionsForSuboptimalTypeInference: false,
 };
 
 // Remove 'ref' from TypeCheckableDirectiveMeta and add a 'selector' instead.
@@ -272,6 +273,7 @@ export function tcb(
     strictLiteralTypes: true,
     enableTemplateTypeChecker: false,
     useInlineTypeConstructors: true,
+    suggestionsForSuboptimalTypeInference: false,
     ...config
   };
   options = options || {
@@ -690,4 +692,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

@@ -745,7 +745,8 @@ describe('type check blocks', () => {
       useContextGenericType: true,
       strictLiteralTypes: true,
       enableTemplateTypeChecker: false,
-      useInlineTypeConstructors: true
+      useInlineTypeConstructors: true,
+      suggestionsForSuboptimalTypeInference: 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.SUGGEST_SUBOPTIMAL_TYPE_INFERENCE));
+    expect(diag.category).toBe(ts.DiagnosticCategory.Suggestion);
+    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!);
+}