From 03169cc406f4a67747ab053834b1b093e6c09ce0 Mon Sep 17 00:00:00 2001 From: Jessica Janiuk Date: Fri, 1 Jul 2022 17:07:34 -0700 Subject: [PATCH] feat(compiler): Add extended diagnostic to warn when missing let on ngForOf In the case that a user accidentally forgot the let keyword, they dont get a very clear indicator of there being a problem. They get an issue in the template iteration at runtime. This diagnostic will warn the user when the let keyword is missing. --- goldens/public-api/compiler-cli/error_code.md | 1 + .../extended_template_diagnostic_name.md | 4 +- .../src/ngtsc/diagnostics/src/error_code.ts | 11 ++++ .../src/extended_template_diagnostic_name.ts | 1 + .../src/ngtsc/typecheck/extended/BUILD.bazel | 1 + .../checks/missing_ngforof_let/BUILD.bazel | 17 +++++ .../checks/missing_ngforof_let/index.ts | 43 ++++++++++++ .../src/ngtsc/typecheck/extended/index.ts | 2 + .../checks/missing_ngforof_let/BUILD.bazel | 27 ++++++++ .../missing_ngforof_let_spec.ts | 66 +++++++++++++++++++ 10 files changed, 172 insertions(+), 1 deletion(-) create mode 100644 packages/compiler-cli/src/ngtsc/typecheck/extended/checks/missing_ngforof_let/BUILD.bazel create mode 100644 packages/compiler-cli/src/ngtsc/typecheck/extended/checks/missing_ngforof_let/index.ts create mode 100644 packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/missing_ngforof_let/BUILD.bazel create mode 100644 packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/missing_ngforof_let/missing_ngforof_let_spec.ts diff --git a/goldens/public-api/compiler-cli/error_code.md b/goldens/public-api/compiler-cli/error_code.md index a98edb2a7f8cfd..d6052b8fe20116 100644 --- a/goldens/public-api/compiler-cli/error_code.md +++ b/goldens/public-api/compiler-cli/error_code.md @@ -46,6 +46,7 @@ export enum ErrorCode { INLINE_TYPE_CTOR_REQUIRED = 8901, INVALID_BANANA_IN_BOX = 8101, MISSING_CONTROL_FLOW_DIRECTIVE = 8103, + MISSING_NGFOROF_LET = 8105, MISSING_PIPE = 8004, MISSING_REFERENCE_TARGET = 8003, NGMODULE_BOOTSTRAP_IS_STANDALONE = 6009, diff --git a/goldens/public-api/compiler-cli/extended_template_diagnostic_name.md b/goldens/public-api/compiler-cli/extended_template_diagnostic_name.md index 8241cb63a5739b..53010626a52250 100644 --- a/goldens/public-api/compiler-cli/extended_template_diagnostic_name.md +++ b/goldens/public-api/compiler-cli/extended_template_diagnostic_name.md @@ -13,7 +13,9 @@ export enum ExtendedTemplateDiagnosticName { // (undocumented) NULLISH_COALESCING_NOT_NULLABLE = "nullishCoalescingNotNullable", // (undocumented) - TEXT_ATTRIBUTE_NOT_BINDING = "textAttributeNotBinding" + TEXT_ATTRIBUTE_NOT_BINDING = "textAttributeNotBinding", + // (undocumented) + MISSING_NGFOROF_LET = "missingNgForOfLet" } // (No @packageDocumentation comment for this package) 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 9b112352967961..c2aefd56cfe202 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts @@ -256,6 +256,17 @@ export enum ErrorCode { */ TEXT_ATTRIBUTE_NOT_BINDING = 8104, + /** + * NgForOf is used in a template, but the user forgot to include let + * in their statement. + * + * For example: + * ``` + * + * ``` + */ + MISSING_NGFOROF_LET = 8105, + /** * The template type-checking engine would need to generate an inline type check block for a * component, but the current type-checking environment doesn't support it. diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/extended_template_diagnostic_name.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/extended_template_diagnostic_name.ts index 8002e7a9ffbf76..a3d3f5630cad9c 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/extended_template_diagnostic_name.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/extended_template_diagnostic_name.ts @@ -20,4 +20,5 @@ export enum ExtendedTemplateDiagnosticName { NULLISH_COALESCING_NOT_NULLABLE = 'nullishCoalescingNotNullable', MISSING_CONTROL_FLOW_DIRECTIVE = 'missingControlFlowDirective', TEXT_ATTRIBUTE_NOT_BINDING = 'textAttributeNotBinding', + MISSING_NGFOROF_LET = 'missingNgForOfLet' } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel index 9109bdd8b6a803..c9299e9573295e 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel @@ -14,6 +14,7 @@ ts_library( "//packages/compiler-cli/src/ngtsc/typecheck/extended/api", "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/invalid_banana_in_box", "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/missing_control_flow_directive", + "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/missing_ngforof_let", "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/nullish_coalescing_not_nullable", "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/text_attribute_not_binding", "@npm//typescript", diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/missing_ngforof_let/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/missing_ngforof_let/BUILD.bazel new file mode 100644 index 00000000000000..7403a86983fced --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/missing_ngforof_let/BUILD.bazel @@ -0,0 +1,17 @@ +load("//tools:defaults.bzl", "ts_library") + +ts_library( + name = "missing_ngforof_let", + srcs = ["index.ts"], + visibility = [ + "//packages/compiler-cli/src/ngtsc:__subpackages__", + "//packages/compiler-cli/test/ngtsc:__pkg__", + ], + deps = [ + "//packages/compiler", + "//packages/compiler-cli/src/ngtsc/diagnostics", + "//packages/compiler-cli/src/ngtsc/typecheck/api", + "//packages/compiler-cli/src/ngtsc/typecheck/extended/api", + "@npm//typescript", + ], +) diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/missing_ngforof_let/index.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/missing_ngforof_let/index.ts new file mode 100644 index 00000000000000..ede14fed9852b8 --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/missing_ngforof_let/index.ts @@ -0,0 +1,43 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {AST, TmplAstNode, TmplAstTemplate} from '@angular/compiler'; +import ts from 'typescript'; + +import {ErrorCode, ExtendedTemplateDiagnosticName} from '../../../../diagnostics'; +import {NgTemplateDiagnostic} from '../../../api'; +import {TemplateCheckFactory, TemplateCheckWithVisitor, TemplateContext} from '../../api'; + +/** + * Ensures a user doesn't forget to omit `let` when using ngfor. + * Will return diagnostic information when `let` is missing. + */ +class MissingNgForOfLetCheck extends TemplateCheckWithVisitor { + override code = ErrorCode.MISSING_NGFOROF_LET as const; + + override visitNode( + ctx: TemplateContext, component: ts.ClassDeclaration, + node: TmplAstNode|AST): NgTemplateDiagnostic[] { + const isTemplate = node instanceof TmplAstTemplate; + if (!(isTemplate) || (isTemplate && !(node.templateAttrs.length))) return []; + const attr = node.templateAttrs.find(x => x.name === 'ngForOf'); + if (attr == null) return []; + + if (node.variables.length > 0) return []; + const errorString = 'Your ngForOf is missing a value. Did you forget to add the `let` keyword?'; + const diagnostic = ctx.makeTemplateDiagnostic(node.sourceSpan, errorString); + return [diagnostic]; + } +} + +export const factory: TemplateCheckFactory< + ErrorCode.MISSING_NGFOROF_LET, ExtendedTemplateDiagnosticName.MISSING_NGFOROF_LET> = { + code: ErrorCode.MISSING_NGFOROF_LET, + name: ExtendedTemplateDiagnosticName.MISSING_NGFOROF_LET, + create: () => new MissingNgForOfLetCheck(), +}; diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/index.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/index.ts index 5823b60bcfeef6..a260610172d225 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/extended/index.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/index.ts @@ -11,6 +11,7 @@ import {ErrorCode, ExtendedTemplateDiagnosticName} from '../../diagnostics'; import {TemplateCheckFactory} from './api'; import {factory as invalidBananaInBoxFactory} from './checks/invalid_banana_in_box'; import {factory as missingControlFlowDirectiveFactory} from './checks/missing_control_flow_directive'; +import {factory as missingNgForOfLetFactory} from './checks/missing_ngforof_let'; import {factory as nullishCoalescingNotNullableFactory} from './checks/nullish_coalescing_not_nullable'; import {factory as textAttributeNotBindingFactory} from './checks/text_attribute_not_binding'; @@ -22,4 +23,5 @@ export const ALL_DIAGNOSTIC_FACTORIES: nullishCoalescingNotNullableFactory, missingControlFlowDirectiveFactory, textAttributeNotBindingFactory, + missingNgForOfLetFactory, ]; diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/missing_ngforof_let/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/missing_ngforof_let/BUILD.bazel new file mode 100644 index 00000000000000..9011ebd5fdb9e0 --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/missing_ngforof_let/BUILD.bazel @@ -0,0 +1,27 @@ +load("//tools:defaults.bzl", "jasmine_node_test", "ts_library") + +ts_library( + name = "test_lib", + testonly = True, + srcs = ["missing_ngforof_let_spec.ts"], + 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/compiler-cli/src/ngtsc/typecheck/extended", + "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/missing_ngforof_let", + "//packages/compiler-cli/src/ngtsc/typecheck/testing", + "@npm//typescript", + ], +) + +jasmine_node_test( + name = "test", + bootstrap = ["//tools/testing:node_no_angular_es2015"], + deps = [ + ":test_lib", + ], +) diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/missing_ngforof_let/missing_ngforof_let_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/missing_ngforof_let/missing_ngforof_let_spec.ts new file mode 100644 index 00000000000000..5b85ab46910dac --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/missing_ngforof_let/missing_ngforof_let_spec.ts @@ -0,0 +1,66 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import ts from 'typescript'; + +import {ErrorCode, ExtendedTemplateDiagnosticName, ngErrorCode} from '../../../../../diagnostics'; +import {absoluteFrom, getSourceFileOrError} from '../../../../../file_system'; +import {runInEachFileSystem} from '../../../../../file_system/testing'; +import {getSourceCodeForDiagnostic} from '../../../../../testing'; +import {getClass, setup} from '../../../../testing'; +import {factory as missingNgForOfLet} from '../../../checks/missing_ngforof_let/index'; +import {ExtendedTemplateCheckerImpl} from '../../../src/extended_template_checker'; + +runInEachFileSystem(() => { + describe('TemplateChecks', () => { + it('binds the error code to its extended template diagnostic name', () => { + expect(missingNgForOfLet.code).toBe(ErrorCode.MISSING_NGFOROF_LET); + expect(missingNgForOfLet.name).toBe(ExtendedTemplateDiagnosticName.MISSING_NGFOROF_LET); + }); + + it('should produce missing ngforof let warning', () => { + const fileName = absoluteFrom('/main.ts'); + const {program, templateTypeChecker} = setup([{ + fileName, + templates: { + 'TestCmp': '
  • {{thing["name"]}}
', + }, + source: + 'export class TestCmp { items: [] = [{\'name\': \'diana\'}, {\'name\': \'prince\'}] }' + }]); + const sf = getSourceFileOrError(program, fileName); + const component = getClass(sf, 'TestCmp'); + const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( + templateTypeChecker, program.getTypeChecker(), [missingNgForOfLet], {} /* options */); + const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); + expect(diags.length).toBe(1); + expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning); + expect(diags[0].code).toBe(ngErrorCode(ErrorCode.MISSING_NGFOROF_LET)); + expect(getSourceCodeForDiagnostic(diags[0])) + .toBe('
  • {{thing["name"]}}
    • '); + }); + + it('should not produce missing ngforof let warning if written correctly', () => { + const fileName = absoluteFrom('/main.ts'); + const {program, templateTypeChecker} = setup([{ + fileName, + templates: { + 'TestCmp': '
    • {{item["name"]}};
    ', + }, + source: + 'export class TestCmp { items: [] = [{\'name\': \'diana\'}, {\'name\': \'prince\'}] }' + }]); + const sf = getSourceFileOrError(program, fileName); + const component = getClass(sf, 'TestCmp'); + const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( + templateTypeChecker, program.getTypeChecker(), [missingNgForOfLet], {} /* options */); + const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); + expect(diags.length).toBe(0); + }); + }); +});