From e893c5a3301fa54ef1d21a16dd30461d994ccc17 Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Tue, 23 Jul 2019 12:32:14 -0700 Subject: [PATCH] fix(compiler-cli): pass real source spans where they are empty (#31805) Some consumers of functions that take `ParseSourceSpan`s currently pass empty and incorrect source spans. This fixes those cases. PR Close #31805 --- .../src/ngtsc/annotations/src/directive.ts | 91 ++++++++------- .../src/ngtsc/annotations/src/util.ts | 16 ++- .../ngtsc/annotations/test/directive_spec.ts | 107 +++++++++++------- .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 25 ++++ packages/compiler/src/parse_util.ts | 4 - 5 files changed, 153 insertions(+), 90 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts index a0caee7461..9a20073865 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {ConstantPool, EMPTY_SOURCE_SPAN, Expression, Identifiers, ParseError, ParsedHostBindings, R3DependencyMetadata, R3DirectiveMetadata, R3FactoryTarget, R3QueryMetadata, Statement, WrappedNodeExpr, compileDirectiveFromMetadata, makeBindingParser, parseHostBindings, verifyHostBindings} from '@angular/compiler'; +import {ConstantPool, Expression, Identifiers, ParseError, ParsedHostBindings, R3DependencyMetadata, R3DirectiveMetadata, R3FactoryTarget, R3QueryMetadata, Statement, WrappedNodeExpr, compileDirectiveFromMetadata, makeBindingParser, parseHostBindings, verifyHostBindings} from '@angular/compiler'; import * as ts from 'typescript'; import {ErrorCode, FatalDiagnosticError} from '../../diagnostics'; @@ -21,7 +21,7 @@ import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerFl import {getDirectiveDiagnostics, getProviderDiagnostics} from './diagnostics'; import {compileNgFactoryDefField} from './factory'; import {generateSetClassMetadataCall} from './metadata'; -import {findAngularDecorator, getConstructorDependencies, isAngularDecorator, readBaseClass, resolveProvidersRequiringFactory, unwrapConstructorDependencies, unwrapExpression, unwrapForwardRef, validateConstructorDependencies, wrapFunctionExpressionsInParens, wrapTypeReference} from './util'; +import {createSourceSpan, findAngularDecorator, getConstructorDependencies, isAngularDecorator, readBaseClass, resolveProvidersRequiringFactory, unwrapConstructorDependencies, unwrapExpression, unwrapForwardRef, validateConstructorDependencies, wrapFunctionExpressionsInParens, wrapTypeReference} from './util'; const EMPTY_OBJECT: {[key: string]: string} = {}; const FIELD_DECORATORS = [ @@ -320,7 +320,7 @@ export function extractDirectiveMetadata( outputs: {...outputsFromMeta, ...outputsFromFields}, queries, viewQueries, selector, fullInheritance: !!(flags & HandlerFlags.FULL_INHERITANCE), type, internalType, typeArgumentCount: reflector.getGenericArityOfClass(clazz) || 0, - typeSourceSpan: EMPTY_SOURCE_SPAN, usesInheritance, exportAs, providers + typeSourceSpan: createSourceSpan(clazz.name), usesInheritance, exportAs, providers }; return {decorator: directive, metadata}; } @@ -580,54 +580,61 @@ type StringMap = { [key: string]: T; }; -export function extractHostBindings( - members: ClassMember[], evaluator: PartialEvaluator, coreModule: string | undefined, - metadata?: Map): ParsedHostBindings { - let hostMetadata: StringMap = {}; - if (metadata && metadata.has('host')) { - const expr = metadata.get('host') !; - const hostMetaMap = evaluator.evaluate(expr); - if (!(hostMetaMap instanceof Map)) { - throw new FatalDiagnosticError( - ErrorCode.VALUE_HAS_WRONG_TYPE, expr, `Decorator host metadata must be an object`); - } - hostMetaMap.forEach((value, key) => { - // Resolve Enum references to their declared value. - if (value instanceof EnumValue) { - value = value.resolved; - } - - if (typeof key !== 'string') { - throw new FatalDiagnosticError( - ErrorCode.VALUE_HAS_WRONG_TYPE, expr, - `Decorator host metadata must be a string -> string object, but found unparseable key`); - } - - if (typeof value == 'string') { - hostMetadata[key] = value; - } else if (value instanceof DynamicValue) { - hostMetadata[key] = new WrappedNodeExpr(value.node as ts.Expression); - } else { - throw new FatalDiagnosticError( - ErrorCode.VALUE_HAS_WRONG_TYPE, expr, - `Decorator host metadata must be a string -> string object, but found unparseable value`); - } - }); +function evaluateHostExpressionBindings( + hostExpr: ts.Expression, evaluator: PartialEvaluator): ParsedHostBindings { + const hostMetaMap = evaluator.evaluate(hostExpr); + if (!(hostMetaMap instanceof Map)) { + throw new FatalDiagnosticError( + ErrorCode.VALUE_HAS_WRONG_TYPE, hostExpr, `Decorator host metadata must be an object`); } + const hostMetadata: StringMap = {}; + hostMetaMap.forEach((value, key) => { + // Resolve Enum references to their declared value. + if (value instanceof EnumValue) { + value = value.resolved; + } + + if (typeof key !== 'string') { + throw new FatalDiagnosticError( + ErrorCode.VALUE_HAS_WRONG_TYPE, hostExpr, + `Decorator host metadata must be a string -> string object, but found unparseable key`); + } + + if (typeof value == 'string') { + hostMetadata[key] = value; + } else if (value instanceof DynamicValue) { + hostMetadata[key] = new WrappedNodeExpr(value.node as ts.Expression); + } else { + throw new FatalDiagnosticError( + ErrorCode.VALUE_HAS_WRONG_TYPE, hostExpr, + `Decorator host metadata must be a string -> string object, but found unparseable value`); + } + }); const bindings = parseHostBindings(hostMetadata); - // TODO: create and provide proper sourceSpan to make error message more descriptive (FW-995) - // For now, pass an incorrect (empty) but valid sourceSpan. - const errors = verifyHostBindings(bindings, EMPTY_SOURCE_SPAN); + const errors = verifyHostBindings(bindings, createSourceSpan(hostExpr)); if (errors.length > 0) { throw new FatalDiagnosticError( - // TODO: provide more granular diagnostic and output specific host expression that triggered - // an error instead of the whole host object - ErrorCode.HOST_BINDING_PARSE_ERROR, metadata !.get('host') !, + // TODO: provide more granular diagnostic and output specific host expression that + // triggered an error instead of the whole host object. + ErrorCode.HOST_BINDING_PARSE_ERROR, hostExpr, errors.map((error: ParseError) => error.msg).join('\n')); } + return bindings; +} + +export function extractHostBindings( + members: ClassMember[], evaluator: PartialEvaluator, coreModule: string | undefined, + metadata?: Map): ParsedHostBindings { + let bindings: ParsedHostBindings; + if (metadata && metadata.has('host')) { + bindings = evaluateHostExpressionBindings(metadata.get('host') !, evaluator); + } else { + bindings = parseHostBindings({}); + } + filterToMembersWithDecorator(members, 'HostBinding', coreModule).forEach(({member, decorators}) => { decorators.forEach(decorator => { let hostPropertyName: string = member.name; diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/util.ts b/packages/compiler-cli/src/ngtsc/annotations/src/util.ts index 7cddaae9f8..c4a88684fc 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/util.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/util.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {Expression, ExternalExpr, LiteralExpr, R3DependencyMetadata, R3Reference, R3ResolvedDependencyType, WrappedNodeExpr} from '@angular/compiler'; +import {Expression, ExternalExpr, LiteralExpr, ParseLocation, ParseSourceFile, ParseSourceSpan, R3DependencyMetadata, R3Reference, R3ResolvedDependencyType, WrappedNodeExpr} from '@angular/compiler'; import * as ts from 'typescript'; import {ErrorCode, FatalDiagnosticError, makeDiagnostic} from '../../diagnostics'; @@ -475,3 +475,17 @@ export function wrapTypeReference(reflector: ReflectionHost, clazz: ClassDeclara value; return {value, type}; } + +/** Creates a ParseSourceSpan for a TypeScript node. */ +export function createSourceSpan(node: ts.Node): ParseSourceSpan { + const sf = node.getSourceFile(); + const [startOffset, endOffset] = [node.getStart(), node.getEnd()]; + const {line: startLine, character: startCol} = sf.getLineAndCharacterOfPosition(startOffset); + const {line: endLine, character: endCol} = sf.getLineAndCharacterOfPosition(endOffset); + const parseSf = new ParseSourceFile(sf.getFullText(), sf.fileName); + + // +1 because values are zero-indexed. + return new ParseSourceSpan( + new ParseLocation(parseSf, startOffset, startLine + 1, startCol + 1), + new ParseLocation(parseSf, endOffset, endLine + 1, endCol + 1)); +} diff --git a/packages/compiler-cli/src/ngtsc/annotations/test/directive_spec.ts b/packages/compiler-cli/src/ngtsc/annotations/test/directive_spec.ts index b15e85ca63..62ecab624e 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/test/directive_spec.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/test/directive_spec.ts @@ -5,6 +5,7 @@ * 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 * as ts from 'typescript'; import {absoluteFrom} from '../../file_system'; import {runInEachFileSystem} from '../../file_system/testing'; import {NOOP_DEFAULT_IMPORT_RECORDER, ReferenceEmitter} from '../../imports'; @@ -16,10 +17,10 @@ import {getDeclaration, makeProgram} from '../../testing'; import {DirectiveDecoratorHandler} from '../src/directive'; runInEachFileSystem(() => { - describe('DirectiveDecoratorHandler', () => { - let _: typeof absoluteFrom; - beforeEach(() => _ = absoluteFrom); + let _: typeof absoluteFrom; + beforeEach(() => _ = absoluteFrom); + describe('DirectiveDecoratorHandler', () => { it('should use the `ReflectionHost` to detect class inheritance', () => { const {program} = makeProgram([ { @@ -40,53 +41,73 @@ runInEachFileSystem(() => { }, ]); - const checker = program.getTypeChecker(); - const reflectionHost = new TestReflectionHost(checker); - const evaluator = new PartialEvaluator(reflectionHost, checker, /* dependencyTracker */ null); - const metaReader = new LocalMetadataRegistry(); - const dtsReader = new DtsMetadataReader(checker, reflectionHost); - const scopeRegistry = new LocalModuleScopeRegistry( - metaReader, new MetadataDtsModuleScopeResolver(dtsReader, null), new ReferenceEmitter([]), - null); - const injectableRegistry = new InjectableClassRegistry(reflectionHost); - const handler = new DirectiveDecoratorHandler( - reflectionHost, evaluator, scopeRegistry, scopeRegistry, metaReader, - NOOP_DEFAULT_IMPORT_RECORDER, injectableRegistry, - /* isCore */ false, /* annotateForClosureCompiler */ false); - - const analyzeDirective = (dirName: string) => { - const DirNode = getDeclaration(program, _('/entry.ts'), dirName, isNamedClassDeclaration); - - const detected = - handler.detect(DirNode, reflectionHost.getDecoratorsOfDeclaration(DirNode)); - if (detected === undefined) { - throw new Error(`Failed to recognize @Directive (${dirName}).`); - } - - const {analysis} = handler.analyze(DirNode, detected.metadata); - if (analysis === undefined) { - throw new Error(`Failed to analyze @Directive (${dirName}).`); - } - - return analysis; - }; - - // By default, `TestReflectionHost#hasBaseClass()` returns `false`. - const analysis1 = analyzeDirective('TestDir1'); + const analysis1 = analyzeDirective(program, 'TestDir1', /*hasBaseClass*/ false); expect(analysis1.meta.usesInheritance).toBe(false); - // Tweak `TestReflectionHost#hasBaseClass()` to return true. - reflectionHost.hasBaseClassReturnValue = true; - - const analysis2 = analyzeDirective('TestDir2'); + const analysis2 = analyzeDirective(program, 'TestDir2', /*hasBaseClass*/ true); expect(analysis2.meta.usesInheritance).toBe(true); }); + + it('should record the source span of a Directive class type', () => { + const src = ` + import {Directive} from '@angular/core'; + + @Directive({selector: 'test-dir'}) + export class TestDir {} + `; + const {program} = makeProgram([ + { + name: _('/node_modules/@angular/core/index.d.ts'), + contents: 'export const Directive: any;', + }, + { + name: _('/entry.ts'), + contents: src, + }, + ]); + + const analysis = analyzeDirective(program, 'TestDir'); + const span = analysis.meta.typeSourceSpan; + expect(span.toString()).toBe('TestDir'); + expect(span.start.toString()).toContain('/entry.ts@5:22'); + expect(span.end.toString()).toContain('/entry.ts@5:29'); + }); }); // Helpers - class TestReflectionHost extends TypeScriptReflectionHost { - hasBaseClassReturnValue = false; + function analyzeDirective(program: ts.Program, dirName: string, hasBaseClass: boolean = false) { + class TestReflectionHost extends TypeScriptReflectionHost { + constructor(checker: ts.TypeChecker) { super(checker); } - hasBaseClass(clazz: ClassDeclaration): boolean { return this.hasBaseClassReturnValue; } + hasBaseClass(_class: ClassDeclaration): boolean { return hasBaseClass; } + } + + const checker = program.getTypeChecker(); + const reflectionHost = new TestReflectionHost(checker); + const evaluator = new PartialEvaluator(reflectionHost, checker, /*dependencyTracker*/ null); + const metaReader = new LocalMetadataRegistry(); + const dtsReader = new DtsMetadataReader(checker, reflectionHost); + const scopeRegistry = new LocalModuleScopeRegistry( + metaReader, new MetadataDtsModuleScopeResolver(dtsReader, null), new ReferenceEmitter([]), + null); + const injectableRegistry = new InjectableClassRegistry(reflectionHost); + const handler = new DirectiveDecoratorHandler( + reflectionHost, evaluator, scopeRegistry, scopeRegistry, metaReader, + NOOP_DEFAULT_IMPORT_RECORDER, injectableRegistry, /*isCore*/ false, + /*annotateForClosureCompiler*/ false); + + const DirNode = getDeclaration(program, _('/entry.ts'), dirName, isNamedClassDeclaration); + + const detected = handler.detect(DirNode, reflectionHost.getDecoratorsOfDeclaration(DirNode)); + if (detected === undefined) { + throw new Error(`Failed to recognize @Directive (${dirName}).`); + } + + const {analysis} = handler.analyze(DirNode, detected.metadata); + if (analysis === undefined) { + throw new Error(`Failed to analyze @Directive (${dirName}).`); + } + + return analysis; } }); diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index db54ed8e05..6131de87df 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -37,6 +37,10 @@ const setClassMetadataRegExp = (expectedType: string): RegExp => const testFiles = loadStandardTestFiles(); +function getDiagnosticSourceCode(diag: ts.Diagnostic): string { + return diag.file !.text.substr(diag.start !, diag.length !); +} + runInEachFileSystem(os => { describe('ngtsc behavioral tests', () => { let env !: NgtscTestEnvironment; @@ -2897,6 +2901,27 @@ runInEachFileSystem(os => { `Unexpected global target 'UnknownTarget' defined for 'click' event. Supported list of global targets: window,document,body.`); }); + it('should provide error location for invalid host properties', () => { + env.write('test.ts', ` + import {Component} from '@angular/core'; + + @Component({ + selector: 'test', + template: '...', + host: { + '(click)': 'act() | pipe', + } + }) + class FooCmp {} + `); + + const errors = env.driveDiagnostics(); + expect(getDiagnosticSourceCode(errors[0])).toBe(`{ + '(click)': 'act() | pipe', + }`); + expect(errors[0].messageText).toContain('/test.ts@7:17'); + }); + it('should throw in case pipes are used in host listeners', () => { env.write(`test.ts`, ` import {Component} from '@angular/core'; diff --git a/packages/compiler/src/parse_util.ts b/packages/compiler/src/parse_util.ts index 8935dfc26a..e6e7c30758 100644 --- a/packages/compiler/src/parse_util.ts +++ b/packages/compiler/src/parse_util.ts @@ -7,7 +7,6 @@ */ import * as chars from './chars'; import {CompileIdentifierMetadata, identifierModuleUrl, identifierName} from './compile_metadata'; -import {error} from './util'; export class ParseLocation { constructor( @@ -109,9 +108,6 @@ export class ParseSourceSpan { } } -export const EMPTY_PARSE_LOCATION = new ParseLocation(new ParseSourceFile('', ''), 0, 0, 0); -export const EMPTY_SOURCE_SPAN = new ParseSourceSpan(EMPTY_PARSE_LOCATION, EMPTY_PARSE_LOCATION); - export enum ParseErrorLevel { WARNING, ERROR,