From 7e724add7eeae318d8a9eb38a552fcb68889976d Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Thu, 12 Nov 2020 15:25:02 -0800 Subject: [PATCH] refactor(compiler-cli): Add additional key/value spans to TCB (#39665) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In order to more accurately map from a node in the TCB to a template position, we need to provide more span information in the TCB. These changes are necessary for the Language Service to map from a TCB node back to a specific locations in the template for actions like "find references" and "refactor/rename". After the TS "find references" returns results, including those in the TCB, we need to map specifically to the matching key/value spans in the template rather than the entire source span. This also has the benefit of producing diagnostics which align more closely with what TypeScript produces. The following example shows TS code and the diagnostic produced by an invalid assignment to a property: ``` let a: {age: number} = {} as any; a.age = 'laksjdf'; ^^^^^ <-- Type 'string' is not assignable to type 'number'. ``` A corollary to this in a template file would be [age]="'someString'". The diagnostic we currently produce for this is: ``` Type 'number' is not assignable to type 'string'. 1 ~~~~~~~~~~~~~~ ``` Notice that the underlined text includes the entire span. If we included the keySpan for the assignment to the property, this diagnostic underline would be more similar to the one produced by TypeScript; that is, it would only underline “greeting”. [design/discussion doc] (https://docs.google.com/document/d/1FtaHdVL805wKe4E6FxVTnVHl38lICoHIjS2nThtRJ6I/edit?usp=sharing) PR Close #39665 --- .../ngtsc/typecheck/src/type_check_block.ts | 19 ++++++++++++++++--- .../ngtsc/typecheck/test/diagnostics_spec.ts | 2 +- .../typecheck/test/span_comments_spec.ts | 2 +- .../test/ngtsc/template_typecheck_spec.ts | 12 ++++++------ 4 files changed, 24 insertions(+), 11 deletions(-) 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 7297da50f0..0d417fae78 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 @@ -15,7 +15,7 @@ import {ClassDeclaration} from '../../reflection'; import {TemplateId, TypeCheckableDirectiveMeta, TypeCheckBlockMetadata} from '../api'; import {addExpressionIdentifier, ExpressionIdentifier, markIgnoreDiagnostics} from './comments'; -import {addParseSpanInfo, addTemplateId, wrapForDiagnostics} from './diagnostics'; +import {addParseSpanInfo, addTemplateId, wrapForDiagnostics, wrapForTypeChecker} from './diagnostics'; import {DomSchemaChecker} from './dom'; import {Environment} from './environment'; import {astToTypescript, NULL_AS_ANY} from './expression'; @@ -176,10 +176,18 @@ class TcbVariableOp extends TcbOp { const initializer = ts.createPropertyAccess( /* expression */ ctx, /* name */ this.variable.value || '$implicit'); - addParseSpanInfo(initializer, this.variable.sourceSpan); + addParseSpanInfo(id, this.variable.keySpan); // Declare the variable, and return its identifier. - this.scope.addStatement(tsCreateVariable(id, initializer)); + let variable: ts.VariableStatement; + if (this.variable.valueSpan !== undefined) { + addParseSpanInfo(initializer, this.variable.valueSpan); + variable = tsCreateVariable(id, wrapForTypeChecker(initializer)); + } else { + variable = tsCreateVariable(id, initializer); + } + addParseSpanInfo(variable.declarationList.declarations[0], this.variable.sourceSpan); + this.scope.addStatement(variable); return id; } } @@ -443,6 +451,7 @@ class TcbReferenceOp extends TcbOp { initializer = ts.createParen(initializer); } addParseSpanInfo(initializer, this.node.sourceSpan); + addParseSpanInfo(id, this.node.keySpan); this.scope.addStatement(tsCreateVariable(id, initializer)); return id; @@ -617,6 +626,9 @@ class TcbDirectiveInputsOp extends TcbOp { ts.createPropertyAccess(dirId, ts.createIdentifier(fieldName)); } + if (input.attribute.keySpan !== undefined) { + addParseSpanInfo(target, input.attribute.keySpan); + } // Finally the assignment is extended by assigning it into the target expression. assignment = ts.createBinary(target, ts.SyntaxKind.EqualsToken, assignment); } @@ -839,6 +851,7 @@ export class TcbDirectiveOutputsOp extends TcbOp { dirId = this.scope.resolve(this.node, this.dir); } const outputField = ts.createElementAccess(dirId, ts.createStringLiteral(field)); + addParseSpanInfo(outputField, output.keySpan); const outputHelper = ts.createCall(this.tcb.env.declareOutputHelper(), undefined, [outputField]); const subscribeFn = ts.createPropertyAccess(outputHelper, 'subscribe'); 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 6662032296..cd7e820cb6 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts @@ -36,7 +36,7 @@ runInEachFileSystem(() => { }]); expect(messages).toEqual( - [`TestComponent.html(1, 10): Type 'string' is not assignable to type 'number'.`]); + [`TestComponent.html(1, 11): Type 'string' is not assignable to type 'number'.`]); }); it('infers type of template variables', () => { 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 8408789547..caa312ce0a 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 @@ -184,7 +184,7 @@ describe('type check blocks diagnostics', () => { }]; const TEMPLATE = ``; expect(tcbWithSpans(TEMPLATE, DIRECTIVES)) - .toContain('_t1.inputA = ("" /*18,20*/) /*8,21*/;'); + .toContain('_t1.inputA /*9,15*/ = ("" /*18,20*/) /*8,21*/;'); }); }); }); diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts index ae6db43321..4e20f958ec 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -1267,11 +1267,11 @@ export declare class AnimationEvent { const diags = env.driveDiagnostics(); expect(diags.length).toBe(3); expect(diags[0].messageText).toBe(`Type 'boolean' is not assignable to type 'number'.`); - expect(getSourceCodeForDiagnostic(diags[0])).toEqual('[fromAbstract]="true"'); + expect(getSourceCodeForDiagnostic(diags[0])).toEqual('fromAbstract'); expect(diags[1].messageText).toBe(`Type 'number' is not assignable to type 'string'.`); - expect(getSourceCodeForDiagnostic(diags[1])).toEqual('[fromBase]="3"'); + expect(getSourceCodeForDiagnostic(diags[1])).toEqual('fromBase'); expect(diags[2].messageText).toBe(`Type 'number' is not assignable to type 'boolean'.`); - expect(getSourceCodeForDiagnostic(diags[2])).toEqual('[fromChild]="4"'); + expect(getSourceCodeForDiagnostic(diags[2])).toEqual('fromChild'); }); it('should properly type-check inherited directives from external libraries', () => { @@ -1324,11 +1324,11 @@ export declare class AnimationEvent { const diags = env.driveDiagnostics(); expect(diags.length).toBe(3); expect(diags[0].messageText).toBe(`Type 'boolean' is not assignable to type 'number'.`); - expect(getSourceCodeForDiagnostic(diags[0])).toEqual('[fromAbstract]="true"'); + expect(getSourceCodeForDiagnostic(diags[0])).toEqual('fromAbstract'); expect(diags[1].messageText).toBe(`Type 'number' is not assignable to type 'string'.`); - expect(getSourceCodeForDiagnostic(diags[1])).toEqual('[fromBase]="3"'); + expect(getSourceCodeForDiagnostic(diags[1])).toEqual('fromBase'); expect(diags[2].messageText).toBe(`Type 'number' is not assignable to type 'boolean'.`); - expect(getSourceCodeForDiagnostic(diags[2])).toEqual('[fromChild]="4"'); + expect(getSourceCodeForDiagnostic(diags[2])).toEqual('fromChild'); }); it('should detect an illegal write to a template variable', () => {