From cf3548a02f7c1e070d2a51540727a49d98b52cd8 Mon Sep 17 00:00:00 2001 From: Chuck Jazdzewski Date: Fri, 3 Jun 2016 15:43:09 -0700 Subject: [PATCH] fix(compiler): Improved error reporting of the static reflector. StaticReflector provides more context on errors reported by the collector. The metadata collector now records the line and character of the node that caused it to report the error. Includes other minor fixes to error reporting and a wording change. Fixes #8978 Closes #9011 --- .../compiler-cli/src/static_reflector.ts | 41 +++++++++++- .../test/static_reflector_spec.ts | 57 ++++++++++++++++ tools/@angular/tsc-wrapped/src/collector.ts | 22 +++---- tools/@angular/tsc-wrapped/src/evaluator.ts | 39 ++++++++--- tools/@angular/tsc-wrapped/src/schema.ts | 23 +++++++ .../tsc-wrapped/test/collector.spec.ts | 40 ++++++++---- .../tsc-wrapped/test/evaluator.spec.ts | 65 +++++++++++++++++-- 7 files changed, 248 insertions(+), 39 deletions(-) diff --git a/modules/@angular/compiler-cli/src/static_reflector.ts b/modules/@angular/compiler-cli/src/static_reflector.ts index f5ec5cd615..aae9382c86 100644 --- a/modules/@angular/compiler-cli/src/static_reflector.ts +++ b/modules/@angular/compiler-cli/src/static_reflector.ts @@ -30,7 +30,7 @@ import { keyframes } from "@angular/core"; import {ReflectorReader} from "./core_private"; - + const SUPPORTED_SCHEMA_VERSION = 1; /** @@ -390,7 +390,11 @@ export class StaticReflector implements ReflectorReader { return context; } case "error": - throw new Error(expression['message']); + let message = produceErrorMessage(expression); + if (expression['line']) { + message = `${message} (position ${expression['line']}:${expression['character']} in the original .ts file)`; + } + throw new Error(message); } return null; } @@ -399,7 +403,11 @@ export class StaticReflector implements ReflectorReader { return null; } - return simplify(value); + try { + return simplify(value); + } catch(e) { + throw new Error(`${e.message}, resolving symbol ${context.name} in ${context.filePath}`); + } } /** @@ -431,6 +439,33 @@ export class StaticReflector implements ReflectorReader { } return result; } + +} + +function expandedMessage(error: any): string { + switch (error.message) { + case 'Reference to non-exported class': + if (error.context && error.context.className) { + return `Reference to a non-exported class ${error.context.className}`; + } + break; + case 'Variable not initialized': + return 'Only initialized variables and constants can be referenced'; + case 'Destructuring not supported': + return 'Referencing an exported destructured variable or constant is not supported'; + case 'Could not resolve type': + if (error.context && error.context.typeName) { + return `Could not resolve type ${error.context.typeName}`; + } + break; + case 'Function call not supported': + return 'Function calls are not supported. Consider replacing the function or lambda with a reference to an exported function'; + } + return error.message; +} + +function produceErrorMessage(error: any): string { + return `Error encountered resolving symbol values statically. ${expandedMessage(error)}`; } function mapStringMap(input: {[key: string]: any}, diff --git a/modules/@angular/compiler-cli/test/static_reflector_spec.ts b/modules/@angular/compiler-cli/test/static_reflector_spec.ts index d1c75ba09f..42528866fe 100644 --- a/modules/@angular/compiler-cli/test/static_reflector_spec.ts +++ b/modules/@angular/compiler-cli/test/static_reflector_spec.ts @@ -109,6 +109,11 @@ describe('StaticReflector', () => { expect(parameters).toEqual([]); }); + it('should provide context for errors reported by the collector', () => { + let SomeClass = host.findDeclaration('src/error-reporting', 'SomeClass'); + expect(() => reflector.annotations(SomeClass)).toThrow(new Error('Error encountered resolving symbol values statically. A reasonable error message (position 12:33 in the original .ts file), resolving symbol ErrorSym in /tmp/src/error-references.d.ts, resolving symbol Link2 in /tmp/src/error-references.d.ts, resolving symbol Link1 in /tmp/src/error-references.d.ts, resolving symbol SomeClass in /tmp/src/error-reporting.d.ts, resolving symbol SomeClass in /tmp/src/error-reporting.d.ts')); + }); + it('should simplify primitive into itself', () => { expect(simplify(noContext, 1)).toBe(1); expect(simplify(noContext, true)).toBe(true); @@ -537,6 +542,58 @@ class MockReflectorHost implements StaticReflectorHost { }, '/src/extern.d.ts': {"__symbolic": "module", "version": 1, metadata: {s: "s"}}, '/tmp/src/version-error.d.ts': {"__symbolic": "module", "version": 100, metadata: {e: "s"}}, + '/tmp/src/error-reporting.d.ts': { + __symbolic: "module", + version: 1, + metadata: { + SomeClass: { + __symbolic: "class", + decorators: [ + { + __symbolic: "call", + expression: { + __symbolic: "reference", + name: "Component", + module: "angular2/src/core/metadata" + }, + arguments: [ + { + directives: [ + { + __symbolic: "reference", + module: "src/error-references", + name: "Link1", + } + ] + } + ] + } + ], + } + } + }, + '/tmp/src/error-references.d.ts': { + __symbolic: "module", + version: 1, + metadata: { + Link1: { + __symbolic: "reference", + module: "src/error-references", + name: "Link2" + }, + Link2: { + __symbolic: "reference", + module: "src/error-references", + name: "ErrorSym" + }, + ErrorSym: { + __symbolic: "error", + message: "A reasonable error message", + line: 12, + character: 33 + } + } + } }; return data[moduleId]; } diff --git a/tools/@angular/tsc-wrapped/src/collector.ts b/tools/@angular/tsc-wrapped/src/collector.ts index 6ca076c084..83c1eda9b7 100644 --- a/tools/@angular/tsc-wrapped/src/collector.ts +++ b/tools/@angular/tsc-wrapped/src/collector.ts @@ -1,6 +1,6 @@ import * as ts from 'typescript'; -import {Evaluator, ImportMetadata, ImportSpecifierMetadata, isPrimitive} from './evaluator'; +import {Evaluator, ImportMetadata, ImportSpecifierMetadata, errorSymbol, isPrimitive} from './evaluator'; import {ClassMetadata, ConstructorMetadata, ModuleMetadata, MemberMetadata, MetadataError, MetadataMap, MetadataSymbolicExpression, MetadataSymbolicReferenceExpression, MetadataValue, MethodMetadata, isMetadataError, isMetadataSymbolicReferenceExpression, VERSION} from './schema'; import {Symbols} from './symbols'; @@ -23,6 +23,11 @@ export class MetadataCollector { return evaluator.evaluateNode(decoratorNode.expression); } + function errorSym( + message: string, node?: ts.Node, context?: {[name: string]: string}): MetadataError { + return errorSymbol(message, node, context, sourceFile); + } + function classMetadataOf(classDeclaration: ts.ClassDeclaration): ClassMetadata { let result: ClassMetadata = {__symbolic: 'class'}; @@ -37,7 +42,7 @@ export class MetadataCollector { if (isMetadataError(result) || isMetadataSymbolicReferenceExpression(result)) { return result; } else { - return {__symbolic: 'error', message: 'Symbol reference expected'}; + return errorSym('Symbol reference expected', node); } } @@ -128,8 +133,7 @@ export class MetadataCollector { locals.define(className, {__symbolic: 'reference', name: className}); } else { locals.define( - className, - {__symbolic: 'error', message: `Reference to non-exported class ${className}`}); + className, errorSym('Reference to non-exported class', node, {className})); } break; } @@ -156,10 +160,7 @@ export class MetadataCollector { if (variableDeclaration.initializer) { varValue = evaluator.evaluateNode(variableDeclaration.initializer); } else { - varValue = { - __symbolic: 'error', - message: 'Only intialized variables and constants can be referenced statically' - }; + varValue = errorSym('Variable not initialized', nameNode); } if (variableStatement.flags & ts.NodeFlags.Export || variableDeclaration.flags & ts.NodeFlags.Export) { @@ -175,14 +176,11 @@ export class MetadataCollector { // or // var [[, ; // are not supported. - let varValue = { - __symbolc: 'error', - message: 'Destructuring declarations cannot be referenced statically' - }; const report = (nameNode: ts.Node) => { switch (nameNode.kind) { case ts.SyntaxKind.Identifier: const name = nameNode; + const varValue = errorSym('Destructuring not supported', nameNode); locals.define(name.text, varValue); if (node.flags & ts.NodeFlags.Export) { if (!metadata) metadata = {}; diff --git a/tools/@angular/tsc-wrapped/src/evaluator.ts b/tools/@angular/tsc-wrapped/src/evaluator.ts index 28dad992a9..3c9a6b5039 100644 --- a/tools/@angular/tsc-wrapped/src/evaluator.ts +++ b/tools/@angular/tsc-wrapped/src/evaluator.ts @@ -54,6 +54,28 @@ export interface ImportMetadata { from: string; // from 'place' } + +function getSourceFileOfNode(node: ts.Node): ts.SourceFile { + while (node && node.kind != ts.SyntaxKind.SourceFile) { + node = node.parent + } + return node; +} + +/* @internal */ +export function errorSymbol( + message: string, node?: ts.Node, context?: {[name: string]: string}, + sourceFile?: ts.SourceFile): MetadataError { + if (node) { + sourceFile = sourceFile || getSourceFileOfNode(node); + if (sourceFile) { + let {line, character} = ts.getLineAndCharacterOfPosition(sourceFile, node.pos); + return {__symbolic: 'error', message, line, character, context}; + }; + } + return {__symbolic: 'error', message, context}; +} + /** * Produce a symbolic representation of an expression folding values into their final value when * possible. @@ -69,10 +91,7 @@ export class Evaluator { if (isMetadataError(result) || typeof result === 'string') { return result; } else { - return { - __symbolic: 'error', - message: `Name expected a string or an identifier but received "${node.getText()}""` - }; + return errorSymbol('Name expected', node, {received: node.getText()}); } } @@ -185,7 +204,8 @@ export class Evaluator { const assignment = child; const propertyName = this.nameOf(assignment.name); if (isMetadataError(propertyName)) { - return propertyName; + error = propertyName; + return true; } const propertyValue = this.evaluateNode(assignment.initializer); if (isMetadataError(propertyValue)) { @@ -306,13 +326,13 @@ export class Evaluator { const typeReferenceNode = node; const typeNameNode = typeReferenceNode.typeName; if (typeNameNode.kind != ts.SyntaxKind.Identifier) { - return { __symbolic: 'error', message: 'Qualified type names not supported' } + return errorSymbol('Qualified type names not supported', node); } const typeNameIdentifier = typeReferenceNode.typeName; const typeName = typeNameIdentifier.text; const typeReference = this.symbols.resolve(typeName); if (!typeReference) { - return {__symbolic: 'error', message: `Could not resolve type ${typeName}`}; + return errorSymbol('Could not resolve type', node, {typeName}); } if (typeReferenceNode.typeArguments && typeReferenceNode.typeArguments.length) { const args = typeReferenceNode.typeArguments.map(element => this.evaluateNode(element)); @@ -452,7 +472,10 @@ export class Evaluator { }; } break; + case ts.SyntaxKind.FunctionExpression: + case ts.SyntaxKind.ArrowFunction: + return errorSymbol('Function call not supported', node); } - return {__symbolic: 'error', message: 'Expression is too complex to resolve statically'}; + return errorSymbol('Expression form not supported', node); } } diff --git a/tools/@angular/tsc-wrapped/src/schema.ts b/tools/@angular/tsc-wrapped/src/schema.ts index 5aa008a9a2..e6a1fdcd8d 100644 --- a/tools/@angular/tsc-wrapped/src/schema.ts +++ b/tools/@angular/tsc-wrapped/src/schema.ts @@ -191,7 +191,30 @@ export function isMetadataSymbolicSelectExpression(value: any): export interface MetadataError { __symbolic: 'error'; + + /** + * This message should be short and relatively discriptive and should be fixed once it is created. + * If the reader doesn't recognize the message, it will display the message unmodified. If the + * reader recognizes the error message is it free to use substitute message the is more + * descriptive and/or localized. + */ message: string; + + /** + * The line number of the error in the .ts file the metadata was created for. + */ + line?: number; + + /** + * The number of utf8 code-units from the beginning of the file of the error. + */ + character?: number; + + /** + * Context information that can be used to generate a more descriptive error message. The content + * of the context is dependent on the error message. + */ + context?: {[name: string]: string}; } export function isMetadataError(value: any): value is MetadataError { return value && value.__symbolic === 'error'; diff --git a/tools/@angular/tsc-wrapped/test/collector.spec.ts b/tools/@angular/tsc-wrapped/test/collector.spec.ts index dc7ab3d1fa..0e6655b079 100644 --- a/tools/@angular/tsc-wrapped/test/collector.spec.ts +++ b/tools/@angular/tsc-wrapped/test/collector.spec.ts @@ -33,6 +33,7 @@ describe('Collector', () => { const metadata = collector.getMetadata(sourceFile); expect(metadata).toEqual({ __symbolic: 'module', + version: 1, metadata: { HeroDetailComponent: { __symbolic: 'class', @@ -73,6 +74,7 @@ describe('Collector', () => { const metadata = collector.getMetadata(sourceFile); expect(metadata).toEqual({ __symbolic: 'module', + version: 1, metadata: { AppComponent: { __symbolic: 'class', @@ -126,6 +128,7 @@ describe('Collector', () => { const metadata = collector.getMetadata(sourceFile); expect(metadata).toEqual({ __symbolic: 'module', + version: 1, metadata: { HEROES: [ {'id': 11, 'name': 'Mr. Nice'}, {'id': 12, 'name': 'Narco'}, @@ -206,26 +209,37 @@ describe('Collector', () => { let metadata = collector.getMetadata(unsupported1); expect(metadata).toEqual({ __symbolic: 'module', + version: 1, metadata: { a: { - __symbolc: 'error', - message: 'Destructuring declarations cannot be referenced statically' + __symbolic: 'error', + message: 'Destructuring declarations cannot be referenced statically', + line: 1, + character: 16 }, b: { - __symbolc: 'error', - message: 'Destructuring declarations cannot be referenced statically' + __symbolic: 'error', + message: 'Destructuring declarations cannot be referenced statically', + line: 1, + character: 18 }, c: { - __symbolc: 'error', - message: 'Destructuring declarations cannot be referenced statically' + __symbolic: 'error', + message: 'Destructuring declarations cannot be referenced statically', + line: 2, + character: 16 }, d: { - __symbolc: 'error', - message: 'Destructuring declarations cannot be referenced statically' + __symbolic: 'error', + message: 'Destructuring declarations cannot be referenced statically', + line: 2, + character: 18 }, e: { __symbolic: 'error', - message: 'Only intialized variables and constants can be referenced statically' + message: 'Only intialized variables and constants can be referenced statically', + line: 3, + character: 14 } } }); @@ -237,8 +251,12 @@ describe('Collector', () => { let barClass = metadata.metadata['Bar']; let ctor = barClass.members['__ctor__'][0]; let parameter = ctor.parameters[0]; - expect(parameter).toEqual( - {__symbolic: 'error', message: 'Reference to non-exported class Foo'}); + expect(parameter).toEqual({ + __symbolic: 'error', + message: 'Reference to non-exported class Foo', + line: 1, + character: 45 + }); }); }); diff --git a/tools/@angular/tsc-wrapped/test/evaluator.spec.ts b/tools/@angular/tsc-wrapped/test/evaluator.spec.ts index 76f58ac5ae..dd509f4b7c 100644 --- a/tools/@angular/tsc-wrapped/test/evaluator.spec.ts +++ b/tools/@angular/tsc-wrapped/test/evaluator.spec.ts @@ -7,6 +7,7 @@ import {Symbols} from '../src/symbols'; import {Directory, Host, expectNoDiagnostics, findVar} from './typescript.mocks'; describe('Evaluator', () => { + let documentRegistry = ts.createDocumentRegistry(); let host: ts.LanguageServiceHost; let service: ts.LanguageService; let program: ts.Program; @@ -17,9 +18,9 @@ describe('Evaluator', () => { beforeEach(() => { host = new Host(FILES, [ 'expressions.ts', 'consts.ts', 'const_expr.ts', 'forwardRef.ts', 'classes.ts', - 'newExpression.ts' + 'newExpression.ts', 'errors.ts' ]); - service = ts.createLanguageService(host); + service = ts.createLanguageService(host, documentRegistry); program = service.getProgram(); typeChecker = program.getTypeChecker(); symbols = new Symbols(null); @@ -30,7 +31,10 @@ describe('Evaluator', () => { expectNoDiagnostics(service.getCompilerOptionsDiagnostics()); for (const sourceFile of program.getSourceFiles()) { expectNoDiagnostics(service.getSyntacticDiagnostics(sourceFile.fileName)); - expectNoDiagnostics(service.getSemanticDiagnostics(sourceFile.fileName)); + if (sourceFile.fileName != 'errors.ts') { + // Skip errors.ts because we it has intentional semantic errors that we are testing for. + expectNoDiagnostics(service.getSemanticDiagnostics(sourceFile.fileName)); + } } }); @@ -141,6 +145,46 @@ describe('Evaluator', () => { arguments: ['name', 12] }); }); + + it('should return errors for unsupported expressions', () => { + let errors = program.getSourceFile('errors.ts'); + let aDecl = findVar(errors, 'a'); + expect(evaluator.evaluateNode(aDecl.type)).toEqual({ + __symbolic: 'error', + message: 'Qualified type names not supported', + line: 5, + character: 10 + }); + let fDecl = findVar(errors, 'f'); + expect(evaluator.evaluateNode(fDecl.initializer)).toEqual({ + __symbolic: 'error', + message: + 'Functions cannot be evaluated statically; consider replacing with a reference to an exported function', + line: 6, + character: 11 + }); + let eDecl = findVar(errors, 'e'); + expect(evaluator.evaluateNode(eDecl.type)).toEqual({ + __symbolic: 'error', + message: 'Could not resolve type NotFound', + line: 7, + character: 10 + }); + let sDecl = findVar(errors, 's'); + expect(evaluator.evaluateNode(sDecl.initializer)).toEqual({ + __symbolic: 'error', + message: 'Name expected a string or an identifier but received "1"', + line: 8, + character: 13 + }); + let tDecl = findVar(errors, 't'); + expect(evaluator.evaluateNode(tDecl.initializer)).toEqual({ + __symbolic: 'error', + message: 'Expression form not supported statically', + line: 9, + character: 11 + }); + }); }); const FILES: Directory = { @@ -193,7 +237,7 @@ const FILES: Directory = { export var bShiftLeft = 1 << 2; // 0x04 export var bShiftRight = -1 >> 2; // -1 export var bShiftRightU = -1 >>> 2; // 0x3fffffff - + export var recursiveA = recursiveB; export var recursiveB = recursiveA; `, @@ -224,5 +268,16 @@ const FILES: Directory = { function forwardRef(value: any) { return value; } export const someValue = new Value("name", 12); export const complex = CONST_EXPR(new Value("name", forwardRef(() => 12))); - ` + `, + 'errors.ts': ` + declare namespace Foo { + type A = string; + } + + let a: Foo.A = 'some value'; + let f = () => 1; + let e: NotFound; + let s = { 1: 1, 2: 2 }; + let t = typeof 12; + `, };