From 3959511b80ff8abb55b8c8858a6080472bff1589 Mon Sep 17 00:00:00 2001 From: JoostK Date: Sun, 15 Dec 2019 20:53:36 +0100 Subject: [PATCH] fix(ivy): avoid duplicate errors in safe navigations and template guards (#34417) The template type checker generates TypeScript expressions for any expression that occurs in a template, so that TypeScript can check it and produce errors. Some expressions as they occur in a template may be translated into TypeScript code multiple times, for instance a binding to a directive input that has a template guard. One example would be the `NgIf` directive, which has a template guard to narrow the type in the template as appropriate. Given the following template: ```typescript @Component({ template: '
{{ person.name }}
' }) class AppComponent { person?: { name: string }; } ``` A type check block (TCB) with roughly the following structure is created: ```typescript function tcb(ctx: AppComponent) { const t1 = NgIf.ngTypeCtor({ ngIf: ctx.person }); if (ctx.person) { "" + ctx.person.name; } } ``` Notice how the `*ngIf="person"` binding is present twice: once in the type constructor call and once in the `if` guard. As such, TypeScript will check both instances and would produce duplicate errors, if any were found. Another instance is when the safe navigation operator is used, where an expression such as `person?.name` is emitted into the TCB as `person != null ? person!.name : undefined`. As can be seen, the left-hand side expression `person` occurs twice in the TCB. This commit adds the ability to insert markers into the TCB that indicate that any errors within the expression should be ignored. This is similar to `@ts-ignore`, however it can be applied more granularly. PR Close #34417 --- .../src/ngtsc/typecheck/src/diagnostics.ts | 73 ++++++++++++++----- .../src/ngtsc/typecheck/src/expression.ts | 10 ++- .../ngtsc/typecheck/src/type_check_block.ts | 6 +- .../ngtsc/typecheck/test/diagnostics_spec.ts | 46 +++++++++++- .../typecheck/test/span_comments_spec.ts | 5 +- 5 files changed, 111 insertions(+), 29 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/diagnostics.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/diagnostics.ts index 3ed958274b..4e8b6f344c 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/diagnostics.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/diagnostics.ts @@ -46,6 +46,16 @@ export function wrapForDiagnostics(expr: ts.Expression): ts.Expression { return ts.createParen(expr); } +const IGNORE_MARKER = 'ignore'; + +/** + * Adds a marker to the node that signifies that any errors within the node should not be reported. + */ +export function ignoreDiagnostics(node: ts.Node): void { + ts.addSyntheticTrailingComment( + node, ts.SyntaxKind.MultiLineCommentTrivia, IGNORE_MARKER, /* hasTrailingNewLine */ false); +} + /** * Adds a synthetic comment to the expression that represents the parse span of the provided node. * This comment can later be retrieved as trivia of a node to recover original source locations. @@ -214,17 +224,20 @@ interface SourceLocation { span: AbsoluteSourceSpan; } +/** + * Traverses up the AST starting from the given node to extract the source location from comments + * that have been emitted into the TCB. If the node does not exist within a TCB, or if an ignore + * marker comment is found up the tree, this function returns null. + */ function findSourceLocation(node: ts.Node, sourceFile: ts.SourceFile): SourceLocation|null { // Search for comments until the TCB's function declaration is encountered. while (node !== undefined && !ts.isFunctionDeclaration(node)) { - const span = - ts.forEachTrailingCommentRange(sourceFile.text, node.getEnd(), (pos, end, kind) => { - if (kind !== ts.SyntaxKind.MultiLineCommentTrivia) { - return null; - } - const commentText = sourceFile.text.substring(pos, end); - return parseSourceSpanComment(commentText); - }) || null; + if (hasIgnoreMarker(node, sourceFile)) { + // There's an ignore marker on this node, so the diagnostic should not be reported. + return null; + } + + const span = readSpanComment(sourceFile, node); if (span !== null) { // Once the positional information has been extracted, search further up the TCB to extract // the unique id that is attached with the TCB's function declaration. @@ -243,17 +256,21 @@ function findSourceLocation(node: ts.Node, sourceFile: ts.SourceFile): SourceLoc function getTemplateId(node: ts.Node, sourceFile: ts.SourceFile): TemplateId|null { // Walk up to the function declaration of the TCB, the file information is attached there. - let tcb = node; - while (!ts.isFunctionDeclaration(tcb)) { - tcb = tcb.parent; + while (!ts.isFunctionDeclaration(node)) { + if (hasIgnoreMarker(node, sourceFile)) { + // There's an ignore marker on this node, so the diagnostic should not be reported. + return null; + } + node = node.parent; // Bail once we have reached the root. - if (tcb === undefined) { + if (node === undefined) { return null; } } - return ts.forEachLeadingCommentRange(sourceFile.text, tcb.getFullStart(), (pos, end, kind) => { + const start = node.getFullStart(); + return ts.forEachLeadingCommentRange(sourceFile.text, start, (pos, end, kind) => { if (kind !== ts.SyntaxKind.MultiLineCommentTrivia) { return null; } @@ -262,13 +279,29 @@ function getTemplateId(node: ts.Node, sourceFile: ts.SourceFile): TemplateId|nul }) as TemplateId || null; } -const parseSpanComment = /^\/\*(\d+),(\d+)\*\/$/; +const parseSpanComment = /^(\d+),(\d+)$/; -function parseSourceSpanComment(commentText: string): AbsoluteSourceSpan|null { - const match = commentText.match(parseSpanComment); - if (match === null) { - return null; - } +function readSpanComment(sourceFile: ts.SourceFile, node: ts.Node): AbsoluteSourceSpan|null { + return ts.forEachTrailingCommentRange(sourceFile.text, node.getEnd(), (pos, end, kind) => { + if (kind !== ts.SyntaxKind.MultiLineCommentTrivia) { + return null; + } + const commentText = sourceFile.text.substring(pos + 2, end - 2); + const match = commentText.match(parseSpanComment); + if (match === null) { + return null; + } - return new AbsoluteSourceSpan(+match[1], +match[2]); + return new AbsoluteSourceSpan(+match[1], +match[2]); + }) || null; +} + +function hasIgnoreMarker(node: ts.Node, sourceFile: ts.SourceFile): boolean { + return ts.forEachTrailingCommentRange(sourceFile.text, node.getEnd(), (pos, end, kind) => { + if (kind !== ts.SyntaxKind.MultiLineCommentTrivia) { + return null; + } + const commentText = sourceFile.text.substring(pos + 2, end - 2); + return commentText === IGNORE_MARKER; + }) === true; } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts index 99a6365175..2bb19db850 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts @@ -10,7 +10,7 @@ import {AST, ASTWithSource, AstVisitor, Binary, BindingPipe, Chain, Conditional, import * as ts from 'typescript'; import {TypeCheckingConfig} from './api'; -import {addParseSpanInfo, wrapForDiagnostics} from './diagnostics'; +import {addParseSpanInfo, ignoreDiagnostics, wrapForDiagnostics} from './diagnostics'; export const NULL_AS_ANY = ts.createAsExpression(ts.createNull(), ts.createKeywordTypeNode(ts.SyntaxKind.AnyKeyword)); @@ -222,11 +222,13 @@ class AstTranslator implements AstVisitor { // See the comment in SafePropertyRead above for an explanation of the need for the non-null // assertion here. const receiver = wrapForDiagnostics(this.translate(ast.receiver)); + const guard = ts.getMutableClone(receiver); + ignoreDiagnostics(guard); const method = ts.createPropertyAccess(ts.createNonNullExpression(receiver), ast.name); const args = ast.args.map(expr => this.translate(expr)); const expr = ts.createCall(method, undefined, args); const whenNull = this.config.strictSafeNavigationTypes ? UNDEFINED : NULL_AS_ANY; - const node = safeTernary(receiver, expr, whenNull); + const node = safeTernary(guard, expr, whenNull); addParseSpanInfo(node, ast.sourceSpan); return node; } @@ -237,9 +239,11 @@ class AstTranslator implements AstVisitor { // assertion is necessary because in practice 'a' may be a method call expression, which won't // have a narrowed type when repeated in the ternary true branch. const receiver = wrapForDiagnostics(this.translate(ast.receiver)); + const guard = ts.getMutableClone(receiver); + ignoreDiagnostics(guard); const expr = ts.createPropertyAccess(ts.createNonNullExpression(receiver), ast.name); const whenNull = this.config.strictSafeNavigationTypes ? UNDEFINED : NULL_AS_ANY; - const node = safeTernary(receiver, expr, whenNull); + const node = safeTernary(guard, expr, whenNull); addParseSpanInfo(node, ast.sourceSpan); return node; } 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 b358ef3300..a71944c421 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 @@ -13,7 +13,7 @@ import {Reference} from '../../imports'; import {ClassDeclaration} from '../../reflection'; import {TemplateId, TypeCheckBlockMetadata, TypeCheckableDirectiveMeta} from './api'; -import {addParseSpanInfo, addTemplateId, wrapForDiagnostics} from './diagnostics'; +import {addParseSpanInfo, addTemplateId, ignoreDiagnostics, wrapForDiagnostics} from './diagnostics'; import {DomSchemaChecker} from './dom'; import {Environment} from './environment'; import {NULL_AS_ANY, astToTypescript} from './expression'; @@ -217,6 +217,10 @@ class TcbTemplateBodyOp extends TcbOp { // If there is such a binding, generate an expression for it. const expr = tcbExpression(boundInput.value, this.tcb, this.scope); + // The expression has already been checked in the type constructor invocation, so + // it should be ignored when used within a template guard. + ignoreDiagnostics(expr); + if (guard.type === 'binding') { // Use the binding expression itself as guard. directiveGuards.push(expr); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts index b00c2efd65..1c281971de 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts @@ -43,7 +43,7 @@ runInEachFileSystem(() => { `
{{ render(idx) }}
`, ` class TestComponent { persons: {}[]; - + render(input: string): string { return input; } }`, [ngForDeclaration()], [ngForDts()]); @@ -58,7 +58,7 @@ runInEachFileSystem(() => { `
{{ render(person.namme) }}
`, ` class TestComponent { persons: any; - + render(input: string): string { return input; } }`, [ngForDeclaration()], [ngForDts()]); @@ -194,6 +194,46 @@ runInEachFileSystem(() => { ]); }); + it('does not repeat diagnostics for errors within LHS of safe-navigation operator', () => { + const messages = diagnose(`{{ personn?.name }} {{ personn?.getName() }}`, ` + class TestComponent { + person: { + name: string; + getName: () => string; + } | null; + }`); + + expect(messages).toEqual([ + `synthetic.html(1, 4): Property 'personn' does not exist on type 'TestComponent'. Did you mean 'person'?`, + `synthetic.html(1, 24): Property 'personn' does not exist on type 'TestComponent'. Did you mean 'person'?`, + ]); + }); + + it('does not repeat diagnostics for errors used in template guard expressions', () => { + const messages = diagnose( + `
`, ` + class GuardDir { + static ngTemplateGuard_guard: 'binding'; + } + + class TestComponent { + person: { + name: string; + }; + }`, + [{ + type: 'directive', + name: 'GuardDir', + selector: '[guard]', + inputs: {'guard': 'guard'}, + ngTemplateGuards: [{inputName: 'guard', type: 'binding'}] + }]); + + expect(messages).toEqual([ + `synthetic.html(1, 14): Property 'personn' does not exist on type 'TestComponent'. Did you mean 'person'?`, + ]); + }); + it('does not produce diagnostics for user code', () => { const messages = diagnose(`{{ person.name }}`, ` class TestComponent { @@ -313,7 +353,7 @@ runInEachFileSystem(() => {
-
+ `, ` class TestComponent { diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/span_comments_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/span_comments_spec.ts index 81627c40ce..35c3de00a6 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/span_comments_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/span_comments_spec.ts @@ -97,14 +97,15 @@ describe('type check blocks diagnostics', () => { it('should annotate safe property access', () => { const TEMPLATE = `{{ a?.b }}`; expect(tcbWithSpans(TEMPLATE)) - .toContain('(((ctx).a /*3,4*/) != null ? ((ctx).a /*3,4*/)!.b : undefined) /*3,8*/;'); + .toContain( + '(((ctx).a /*3,4*/) /*ignore*/ != null ? ((ctx).a /*3,4*/)!.b : undefined) /*3,8*/;'); }); it('should annotate safe method calls', () => { const TEMPLATE = `{{ a?.method(b) }}`; expect(tcbWithSpans(TEMPLATE)) .toContain( - '(((ctx).a /*3,4*/) != null ? ((ctx).a /*3,4*/)!.method((ctx).b /*13,14*/) : undefined) /*3,16*/;'); + '(((ctx).a /*3,4*/) /*ignore*/ != null ? ((ctx).a /*3,4*/)!.method((ctx).b /*13,14*/) : undefined) /*3,16*/;'); }); it('should annotate $any casts', () => {