From 1eba57eb00b9c2b64509b7a823a67955f2e47d4e Mon Sep 17 00:00:00 2001
From: Alex Rickabaugh <alx+alxhub@alxandria.net>
Date: Wed, 3 Mar 2021 15:06:21 -0800
Subject: [PATCH] fix(language-service): show suggestion when type inference is
suboptimal (#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 #41042
PR Close #41072
---
.../public-api/compiler-cli/error_code.d.ts | 3 +-
.../src/ngtsc/core/src/compiler.ts | 7 ++++
.../src/ngtsc/diagnostics/src/error_code.ts | 13 +++++--
.../src/ngtsc/typecheck/api/api.ts | 14 +++++++
.../src/ngtsc/typecheck/src/oob.ts | 37 +++++++++++++++++++
.../ngtsc/typecheck/src/type_check_block.ts | 19 +++++++---
.../src/ngtsc/typecheck/test/test_utils.ts | 5 ++-
.../typecheck/test/type_check_block_spec.ts | 3 +-
.../language-service/ivy/test/BUILD.bazel | 1 +
.../ivy/test/diagnostic_spec.ts | 37 +++++++++++++++++++
.../ivy/testing/src/project.ts | 6 +--
11 files changed, 131 insertions(+), 14 deletions(-)
diff --git a/goldens/public-api/compiler-cli/error_code.d.ts b/goldens/public-api/compiler-cli/error_code.d.ts
index ccfbd47e71..9116012fe4 100644
--- a/goldens/public-api/compiler-cli/error_code.d.ts
+++ b/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
}
diff --git a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts
index d483243aad..32cedf2641 100644
--- a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts
+++ b/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,
};
}
diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts
index 89292b56fa..910abcbde6 100644
--- a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts
+++ b/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,
}
/**
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts b/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts
index 81b8c998e9..8f3c5a7d07 100644
--- a/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts
+++ b/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;
}
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts
index aaec8d7927..446debb4ee 100644
--- a/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts
+++ b/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(
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
index 6a6a6abdad..bf384afc45 100644
--- a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
+++ b/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);
+ }
}
}
}
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts
index e3935db015..0c8083b357 100644
--- a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts
+++ b/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 {}
}
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts
index 5f06d3a358..9ccb53db84 100644
--- a/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts
+++ b/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', () => {
diff --git a/packages/language-service/ivy/test/BUILD.bazel b/packages/language-service/ivy/test/BUILD.bazel
index 2ffce0d65a..2fc4b2a7f5 100644
--- a/packages/language-service/ivy/test/BUILD.bazel
+++ b/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",
diff --git a/packages/language-service/ivy/test/diagnostic_spec.ts b/packages/language-service/ivy/test/diagnostic_spec.ts
index 97f7239d33..d1264db207 100644
--- a/packages/language-service/ivy/test/diagnostic_spec.ts
+++ b/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!);
+}
diff --git a/packages/language-service/ivy/testing/src/project.ts b/packages/language-service/ivy/testing/src/project.ts
index 5f2ca94203..5577f7c705 100644
--- a/packages/language-service/ivy/testing/src/project.ts
+++ b/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;