From ba8fa7454bb01bb56042cfadcdae3d358fa42889 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 --- .../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 | 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 +-- 11 files changed, 122 insertions(+), 12 deletions(-) diff --git a/goldens/public-api/compiler-cli/error_code.d.ts b/goldens/public-api/compiler-cli/error_code.d.ts index ccfbd47e710e8..0a3294ed28f55 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, + WARN_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 311d768f71ad3..7055a3d6d7f98 100644 --- a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts +++ b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts @@ -689,6 +689,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 = { @@ -713,6 +717,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 89292b56fa377..c6892640a2b97 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. + */ + 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 014fa84036742..e801b85454d0a 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 aaec8d792774d..cb707feb18578 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 4ee589a5f1d55..201b871696d79 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 23b82d9fcdf4a..77142a7d16700 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 3c9e7ef0e91cc..78aa7f3ad7396 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 2ffce0d65a599..2fc4b2a7f56ab 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 97f7239d33c59..11c55e0863fc4 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 5f2ca942039b4..5577f7c705c49 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;