From e07b382d702eb85b247690abfce04875c21baaf6 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Wed, 3 Mar 2021 15:06:21 -0800 Subject: [PATCH] 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 #41042 --- .../src/ngtsc/core/src/compiler.ts | 7 ++++ .../src/ngtsc/diagnostics/src/error_code.ts | 7 ++++ .../src/ngtsc/typecheck/api/api.ts | 7 ++++ .../src/ngtsc/typecheck/src/oob.ts | 37 +++++++++++++++++++ .../ngtsc/typecheck/src/type_check_block.ts | 19 +++++++--- .../src/ngtsc/typecheck/test/test_utils.ts | 3 ++ .../typecheck/test/type_check_block_spec.ts | 1 + .../language-service/ivy/test/BUILD.bazel | 1 + .../ivy/test/diagnostic_spec.ts | 37 +++++++++++++++++++ .../ivy/testing/src/project.ts | 6 +-- 10 files changed, 117 insertions(+), 8 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts index 83e2e61e81aef7..b8ff6f5fa73fbf 100644 --- a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts +++ b/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, }; } 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 89292b56fa377b..671dd81a011230 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts +++ b/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, } /** diff --git a/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts b/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts index 014fa84036742b..e801b85454d0ab 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts +++ b/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; } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts index aaec8d792774d1..cb707feb185785 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.Warning, + ngErrorCode(ErrorCode.WARN_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 4ee589a5f1d555..201b871696d794 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 @@ -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); + } } } } 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 23b82d9fcdf4a4..77142a7d16700b 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts +++ b/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 {} } 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 3c9e7ef0e91cc0..78aa7f3ad7396f 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 @@ -736,6 +736,7 @@ describe('type check blocks', () => { useContextGenericType: true, strictLiteralTypes: true, enableTemplateTypeChecker: false, + warnForSuboptimalTypeInference: false, }; describe('config.applyTemplateContextGuards', () => { diff --git a/packages/language-service/ivy/test/BUILD.bazel b/packages/language-service/ivy/test/BUILD.bazel index 2ffce0d65a5993..2fc4b2a7f56ab0 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 97f7239d33c59a..11c55e0863fc47 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: '
{{user}}
', + }) + 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!); +} diff --git a/packages/language-service/ivy/testing/src/project.ts b/packages/language-service/ivy/testing/src/project.ts index 5f2ca942039b4b..5577f7c705c497 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; export class Project { private tsProject: ts.server.Project;