fix(compiler): use FatalDiagnosticError to generate better error messages (#35244)

Prior to this commit, decorator handling logic in Ngtsc used `Error` to throw errors. This commit replaces most of these instances with `FatalDiagnosticError` class, which provider a better diagnostics error (including location of the problematic code).

PR Close #35244
This commit is contained in:
Andrew Kushnir 2020-02-07 14:06:52 -08:00 committed by Miško Hevery
parent df75451a0c
commit 646655d09a
3 changed files with 422 additions and 103 deletions

View File

@ -191,7 +191,7 @@ export function extractDirectiveMetadata(
if (!ts.isObjectLiteralExpression(meta)) {
throw new FatalDiagnosticError(
ErrorCode.DECORATOR_ARG_NOT_LITERAL, meta,
`@${decorator.name} argument must be literal.`);
`@${decorator.name} argument must be an object literal`);
}
directive = reflectObjectLiteral(meta);
}
@ -345,7 +345,7 @@ export function extractQueryMetadata(
predicate = new WrappedNodeExpr(node);
} else if (typeof arg === 'string') {
predicate = [arg];
} else if (isStringArrayOrDie(arg, '@' + name)) {
} else if (isStringArrayOrDie(arg, `@${name} predicate`, node)) {
predicate = arg;
} else {
throw new FatalDiagnosticError(
@ -359,7 +359,9 @@ export function extractQueryMetadata(
if (args.length === 2) {
const optionsExpr = unwrapExpression(args[1]);
if (!ts.isObjectLiteralExpression(optionsExpr)) {
throw new Error(`@${name} options must be an object literal`);
throw new FatalDiagnosticError(
ErrorCode.DECORATOR_ARG_NOT_LITERAL, optionsExpr,
`@${name} options must be an object literal`);
}
const options = reflectObjectLiteral(optionsExpr);
if (options.has('read')) {
@ -367,9 +369,12 @@ export function extractQueryMetadata(
}
if (options.has('descendants')) {
const descendantsValue = evaluator.evaluate(options.get('descendants') !);
const descendantsExpr = options.get('descendants') !;
const descendantsValue = evaluator.evaluate(descendantsExpr);
if (typeof descendantsValue !== 'boolean') {
throw new Error(`@${name} options.descendants must be a boolean`);
throw new FatalDiagnosticError(
ErrorCode.VALUE_HAS_WRONG_TYPE, descendantsExpr,
`@${name} options.descendants must be a boolean`);
}
descendants = descendantsValue;
}
@ -385,7 +390,8 @@ export function extractQueryMetadata(
} else if (args.length > 2) {
// Too many arguments.
throw new Error(`@${name} has too many arguments`);
throw new FatalDiagnosticError(
ErrorCode.DECORATOR_ARITY_WRONG, node, `@${name} has too many arguments`);
}
return {
@ -406,17 +412,23 @@ export function extractQueriesFromDecorator(
} {
const content: R3QueryMetadata[] = [], view: R3QueryMetadata[] = [];
if (!ts.isObjectLiteralExpression(queryData)) {
throw new Error(`queries metadata must be an object literal`);
throw new FatalDiagnosticError(
ErrorCode.VALUE_HAS_WRONG_TYPE, queryData,
'Decorator queries metadata must be an object literal');
}
reflectObjectLiteral(queryData).forEach((queryExpr, propertyName) => {
queryExpr = unwrapExpression(queryExpr);
if (!ts.isNewExpression(queryExpr) || !ts.isIdentifier(queryExpr.expression)) {
throw new Error(`query metadata must be an instance of a query type`);
throw new FatalDiagnosticError(
ErrorCode.VALUE_HAS_WRONG_TYPE, queryData,
'Decorator query metadata must be an instance of a query type');
}
const type = reflector.getImportOfIdentifier(queryExpr.expression);
if (type === null || (!isCore && type.from !== '@angular/core') ||
!QUERY_TYPES.has(type.name)) {
throw new Error(`query metadata must be an instance of a query type`);
throw new FatalDiagnosticError(
ErrorCode.VALUE_HAS_WRONG_TYPE, queryData,
'Decorator query metadata must be an instance of a query type');
}
const query = extractQueryMetadata(
@ -430,14 +442,16 @@ export function extractQueriesFromDecorator(
return {content, view};
}
function isStringArrayOrDie(value: any, name: string): value is string[] {
function isStringArrayOrDie(value: any, name: string, node: ts.Expression): value is string[] {
if (!Array.isArray(value)) {
return false;
}
for (let i = 0; i < value.length; i++) {
if (typeof value[i] !== 'string') {
throw new Error(`Failed to resolve ${name}[${i}] to a string`);
throw new FatalDiagnosticError(
ErrorCode.VALUE_HAS_WRONG_TYPE, node,
`Failed to resolve ${name} at position ${i} to a string`);
}
}
return true;
@ -451,9 +465,12 @@ export function parseFieldArrayValue(
}
// Resolve the field of interest from the directive metadata to a string[].
const value = evaluator.evaluate(directive.get(field) !);
if (!isStringArrayOrDie(value, field)) {
throw new Error(`Failed to resolve @Directive.${field}`);
const expression = directive.get(field) !;
const value = evaluator.evaluate(expression);
if (!isStringArrayOrDie(value, field, expression)) {
throw new FatalDiagnosticError(
ErrorCode.VALUE_HAS_WRONG_TYPE, expression,
`Failed to resolve @Directive.${field} to a string array`);
}
return value;
@ -501,13 +518,16 @@ function parseDecoratedFields(
} else if (decorator.args.length === 1) {
const property = evaluator.evaluate(decorator.args[0]);
if (typeof property !== 'string') {
throw new Error(`Decorator argument must resolve to a string`);
throw new FatalDiagnosticError(
ErrorCode.VALUE_HAS_WRONG_TYPE, Decorator.nodeForError(decorator),
`@${decorator.name} decorator argument must resolve to a string`);
}
results[fieldName] = mapValueResolver(property, fieldName);
} else {
// Too many arguments.
throw new Error(
`Decorator must have 0 or 1 arguments, got ${decorator.args.length} argument(s)`);
throw new FatalDiagnosticError(
ErrorCode.DECORATOR_ARITY_WRONG, Decorator.nodeForError(decorator),
`@${decorator.name} can have at most one argument, got ${decorator.args.length} argument(s)`);
}
});
return results;
@ -568,7 +588,7 @@ export function extractHostBindings(
const hostMetaMap = evaluator.evaluate(expr);
if (!(hostMetaMap instanceof Map)) {
throw new FatalDiagnosticError(
ErrorCode.DECORATOR_ARG_NOT_LITERAL, expr, `Decorator host metadata must be an object`);
ErrorCode.VALUE_HAS_WRONG_TYPE, expr, `Decorator host metadata must be an object`);
}
hostMetaMap.forEach((value, key) => {
// Resolve Enum references to their declared value.
@ -577,8 +597,9 @@ export function extractHostBindings(
}
if (typeof key !== 'string') {
throw new Error(
`Decorator host metadata must be a string -> string object, but found unparseable key ${key}`);
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') {
@ -586,8 +607,9 @@ export function extractHostBindings(
} else if (value instanceof DynamicValue) {
hostMetadata[key] = new WrappedNodeExpr(value.node as ts.Expression);
} else {
throw new Error(
`Decorator host metadata must be a string -> string object, but found unparseable value ${value}`);
throw new FatalDiagnosticError(
ErrorCode.VALUE_HAS_WRONG_TYPE, expr,
`Decorator host metadata must be a string -> string object, but found unparseable value`);
}
});
}
@ -605,26 +627,29 @@ export function extractHostBindings(
errors.map((error: ParseError) => error.msg).join('\n'));
}
filterToMembersWithDecorator(members, 'HostBinding', coreModule)
.forEach(({member, decorators}) => {
decorators.forEach(decorator => {
let hostPropertyName: string = member.name;
if (decorator.args !== null && decorator.args.length > 0) {
if (decorator.args.length !== 1) {
throw new Error(`@HostBinding() can have at most one argument`);
}
filterToMembersWithDecorator(members, 'HostBinding', coreModule).forEach(({member, decorators}) => {
decorators.forEach(decorator => {
let hostPropertyName: string = member.name;
if (decorator.args !== null && decorator.args.length > 0) {
if (decorator.args.length !== 1) {
throw new FatalDiagnosticError(
ErrorCode.DECORATOR_ARITY_WRONG, Decorator.nodeForError(decorator),
`@HostBinding can have at most one argument, got ${decorator.args.length} argument(s)`);
}
const resolved = evaluator.evaluate(decorator.args[0]);
if (typeof resolved !== 'string') {
throw new Error(`@HostBinding()'s argument must be a string`);
}
const resolved = evaluator.evaluate(decorator.args[0]);
if (typeof resolved !== 'string') {
throw new FatalDiagnosticError(
ErrorCode.VALUE_HAS_WRONG_TYPE, Decorator.nodeForError(decorator),
`@HostBinding's argument must be a string`);
}
hostPropertyName = resolved;
}
hostPropertyName = resolved;
}
bindings.properties[hostPropertyName] = member.name;
});
});
bindings.properties[hostPropertyName] = member.name;
});
});
filterToMembersWithDecorator(members, 'HostListener', coreModule)
.forEach(({member, decorators}) => {
@ -635,24 +660,25 @@ export function extractHostBindings(
if (decorator.args.length > 2) {
throw new FatalDiagnosticError(
ErrorCode.DECORATOR_ARITY_WRONG, decorator.args[2],
`@HostListener() can have at most two arguments`);
`@HostListener can have at most two arguments`);
}
const resolved = evaluator.evaluate(decorator.args[0]);
if (typeof resolved !== 'string') {
throw new FatalDiagnosticError(
ErrorCode.VALUE_HAS_WRONG_TYPE, decorator.args[0],
`@HostListener()'s event name argument must be a string`);
`@HostListener's event name argument must be a string`);
}
eventName = resolved;
if (decorator.args.length === 2) {
const expression = decorator.args[1];
const resolvedArgs = evaluator.evaluate(decorator.args[1]);
if (!isStringArrayOrDie(resolvedArgs, '@HostListener.args')) {
if (!isStringArrayOrDie(resolvedArgs, '@HostListener.args', expression)) {
throw new FatalDiagnosticError(
ErrorCode.VALUE_HAS_WRONG_TYPE, decorator.args[1],
`@HostListener second argument must be a string array`);
`@HostListener's second argument must be a string array`);
}
args = resolvedArgs;
}

View File

@ -126,8 +126,7 @@ export class InjectableDecoratorHandler implements
/**
* Read metadata from the `@Injectable` decorator and produce the `IvyInjectableMetadata`, the
* input
* metadata needed to run `compileIvyInjectable`.
* input metadata needed to run `compileIvyInjectable`.
*
* A `null` return value indicates this is @Injectable has invalid data.
*/
@ -157,7 +156,9 @@ function extractInjectableMetadata(
// transport references from one location to another. This is the problem that lowering
// used to solve - if this restriction proves too undesirable we can re-implement lowering.
if (!ts.isObjectLiteralExpression(metaNode)) {
throw new Error(`In Ivy, decorator metadata must be inline.`);
throw new FatalDiagnosticError(
ErrorCode.DECORATOR_ARG_NOT_LITERAL, metaNode,
`@Injectable argument must be an object literal`);
}
// Resolve the fields of the literal into a map of field name to expression.
@ -173,7 +174,7 @@ function extractInjectableMetadata(
if (!ts.isArrayLiteralExpression(depsExpr)) {
throw new FatalDiagnosticError(
ErrorCode.VALUE_NOT_LITERAL, depsExpr,
`In Ivy, deps metadata must be an inline array.`);
`@Injectable deps metadata must be an inline array`);
}
userDeps = depsExpr.elements.map(dep => getDep(dep, reflector));
}

View File

@ -1333,70 +1333,362 @@ runInEachFileSystem(os => {
});
});
it('should throw error if content queries share a property with inputs', () => {
env.tsconfig({});
env.write('test.ts', `
import {Component, ContentChild, Input} from '@angular/core';
describe('error handling', () => {
function verifyThrownError(errorCode: ErrorCode, errorMessage: string) {
const errors = env.driveDiagnostics();
expect(errors.length).toBe(1);
const {code, messageText} = errors[0];
expect(code).toBe(ngErrorCode(errorCode));
expect(trim(messageText as string)).toContain(errorMessage);
}
@Component({
selector: 'test-cmp',
template: '<ng-content></ng-content>'
})
export class TestCmp {
@Input() @ContentChild('foo') foo: any;
}
`);
it('should throw if invalid arguments are provided in @NgModule', () => {
env.tsconfig({});
env.write('test.ts', `
import {NgModule} from '@angular/core';
const errors = env.driveDiagnostics();
const {code, messageText} = errors[0];
expect(code).toBe(ngErrorCode(ErrorCode.DECORATOR_COLLISION));
expect(trim(messageText as string))
.toContain('Cannot combine @Input decorators with query decorators');
});
@NgModule('invalidNgModuleArgumentType')
export class MyModule {}
`);
verifyThrownError(
ErrorCode.DECORATOR_ARG_NOT_LITERAL, '@NgModule argument must be an object literal');
});
it('should throw error if multiple query decorators are used on the same field', () => {
env.tsconfig({});
env.write('test.ts', `
import {Component, ContentChild} from '@angular/core';
it('should throw if multiple query decorators are used on the same field', () => {
env.tsconfig({});
env.write('test.ts', `
import {Component, ContentChild} from '@angular/core';
@Component({
selector: 'test-cmp',
template: '...'
})
export class TestCmp {
@ContentChild('bar', {static: true})
@ContentChild('foo')
foo: any;
}
`);
@Component({
selector: 'test-cmp',
template: '...'
})
export class TestCmp {
@ContentChild('bar', {static: true})
@ContentChild('foo')
foo: any;
}
`);
verifyThrownError(
ErrorCode.DECORATOR_COLLISION,
'Cannot have multiple query decorators on the same class member');
});
const errors = env.driveDiagnostics();
const {code, messageText} = errors[0];
expect(code).toBe(ngErrorCode(ErrorCode.DECORATOR_COLLISION));
expect(trim(messageText as string))
.toContain('Cannot have multiple query decorators on the same class member');
});
['ViewChild', 'ViewChildren', 'ContentChild', 'ContentChildren'].forEach(decorator => {
it(`should throw if @Input and @${decorator} decorators are applied to the same property`,
() => {
env.tsconfig({});
env.write('test.ts', `
import {Component, ${decorator}, Input} from '@angular/core';
it('should throw error if query decorators are used on non property-type member', () => {
env.tsconfig({});
env.write('test.ts', `
import {Component, ContentChild} from '@angular/core';
@Component({
selector: 'test-cmp',
template: '<ng-content></ng-content>'
})
export class TestCmp {
@Input() @${decorator}('foo') foo: any;
}
`);
verifyThrownError(
ErrorCode.DECORATOR_COLLISION,
'Cannot combine @Input decorators with query decorators');
});
@Component({
selector: 'test-cmp',
template: '...'
})
export class TestCmp {
@ContentChild('foo')
private someFn() {}
}
`);
it(`should throw if invalid options are provided in ${decorator}`, () => {
env.tsconfig({});
env.write('test.ts', `
import {Component, ${decorator}, Input} from '@angular/core';
const errors = env.driveDiagnostics();
const {code, messageText} = errors[0];
expect(code).toBe(ngErrorCode(ErrorCode.DECORATOR_UNEXPECTED));
expect(trim(messageText as string))
.toContain('Query decorator must go on a property-type member');
@Component({
selector: 'test-cmp',
template: '...'
})
export class TestCmp {
@${decorator}('foo', 'invalidOptionsArgumentType') foo: any;
}
`);
verifyThrownError(
ErrorCode.DECORATOR_ARG_NOT_LITERAL,
`@${decorator} options must be an object literal`);
});
it(`should throw if @${decorator} is used on non property-type member`, () => {
env.tsconfig({});
env.write('test.ts', `
import {Component, ${decorator}} from '@angular/core';
@Component({
selector: 'test-cmp',
template: '...'
})
export class TestCmp {
@${decorator}('foo')
private someFn() {}
}
`);
verifyThrownError(
ErrorCode.DECORATOR_UNEXPECTED, 'Query decorator must go on a property-type member');
});
it(`should throw error if @${decorator} has too many arguments`, () => {
env.tsconfig({});
env.write('test.ts', `
import {Component, ${decorator}} from '@angular/core';
@Component({
selector: 'test-cmp',
template: '...'
})
export class TestCmp {
@${decorator}('foo', {}, 'invalid-extra-arg') foo: any;
}
`);
verifyThrownError(
ErrorCode.DECORATOR_ARITY_WRONG, `@${decorator} has too many arguments`);
});
it(`should throw error if @${decorator} predicate argument has wrong type`, () => {
env.tsconfig({});
env.write('test.ts', `
import {Component, ${decorator}} from '@angular/core';
@Component({
selector: 'test-cmp',
template: '...'
})
export class TestCmp {
@${decorator}({'invalid-predicate-type': true}) foo: any;
}
`);
verifyThrownError(
ErrorCode.VALUE_HAS_WRONG_TYPE, `@${decorator} predicate cannot be interpreted`);
});
it(`should throw error if one of @${decorator}'s predicate has wrong type`, () => {
env.tsconfig({});
env.write('test.ts', `
import {Component, ${decorator}} from '@angular/core';
@Component({
selector: 'test-cmp',
template: '...'
})
export class TestCmp {
@${decorator}(['predicate-a', {'invalid-predicate-type': true}]) foo: any;
}
`);
verifyThrownError(
ErrorCode.VALUE_HAS_WRONG_TYPE,
`Failed to resolve @${decorator} predicate at position 1 to a string`);
});
});
['inputs', 'outputs'].forEach(field => {
it(`should throw error if @Directive.${field} has wrong type`, () => {
env.tsconfig({});
env.write('test.ts', `
import {Directive} from '@angular/core';
@Directive({
selector: 'test-dir',
${field}: 'invalid-field-type',
})
export class TestDir {}
`);
verifyThrownError(
ErrorCode.VALUE_HAS_WRONG_TYPE,
`Failed to resolve @Directive.${field} to a string array`);
});
});
['ContentChild', 'ContentChildren'].forEach(decorator => {
it(`should throw if \`descendants\` field of @${decorator}'s options argument has wrong type`,
() => {
env.tsconfig({});
env.write('test.ts', `
import {Component, ContentChild} from '@angular/core';
@Component({
selector: 'test-cmp',
template: '...'
})
export class TestCmp {
@ContentChild('foo', {descendants: 'invalid'}) foo: any;
}
`);
verifyThrownError(
ErrorCode.VALUE_HAS_WRONG_TYPE,
'@ContentChild options.descendants must be a boolean');
});
});
['Input', 'Output'].forEach(decorator => {
it(`should throw error if @${decorator} decorator argument has unsupported type`, () => {
env.tsconfig({});
env.write('test.ts', `
import {Component, ${decorator}} from '@angular/core';
@Component({
selector: 'test-cmp',
template: '...'
})
export class TestCmp {
@${decorator}(['invalid-arg-type']) foo: any;
}
`);
verifyThrownError(
ErrorCode.VALUE_HAS_WRONG_TYPE,
`@${decorator} decorator argument must resolve to a string`);
});
it(`should throw error if @${decorator} decorator has too many arguments`, () => {
env.tsconfig({});
env.write('test.ts', `
import {Component, ${decorator}} from '@angular/core';
@Component({
selector: 'test-cmp',
template: '...'
})
export class TestCmp {
@${decorator}('name', 'invalid-extra-arg') foo: any;
}
`);
verifyThrownError(
ErrorCode.DECORATOR_ARITY_WRONG,
`@${decorator} can have at most one argument, got 2 argument(s)`);
});
});
it('should throw error if @HostBinding decorator argument has unsupported type', () => {
env.tsconfig({});
env.write('test.ts', `
import {Component, HostBinding} from '@angular/core';
@Component({
selector: 'test-cmp',
template: '...'
})
export class TestCmp {
@HostBinding(['invalid-arg-type']) foo: any;
}
`);
verifyThrownError(
ErrorCode.VALUE_HAS_WRONG_TYPE, `@HostBinding's argument must be a string`);
});
it('should throw error if @HostBinding decorator has too many arguments', () => {
env.tsconfig({});
env.write('test.ts', `
import {Component, HostBinding} from '@angular/core';
@Component({
selector: 'test-cmp',
template: '...'
})
export class TestCmp {
@HostBinding('name', 'invalid-extra-arg') foo: any;
}
`);
verifyThrownError(
ErrorCode.DECORATOR_ARITY_WRONG, '@HostBinding can have at most one argument');
});
it('should throw error if @Directive.host field has wrong type', () => {
env.tsconfig({});
env.write('test.ts', `
import {Directive} from '@angular/core';
@Directive({
selector: 'test-dir',
host: 'invalid-host-type'
})
export class TestDir {}
`);
verifyThrownError(
ErrorCode.VALUE_HAS_WRONG_TYPE, 'Decorator host metadata must be an object');
});
it('should throw error if @Directive.host field is an object with values that have wrong types',
() => {
env.tsconfig({});
env.write('test.ts', `
import {Directive} from '@angular/core';
@Directive({
selector: 'test-dir',
host: {'key': ['invalid-host-value']}
})
export class TestDir {}
`);
verifyThrownError(
ErrorCode.VALUE_HAS_WRONG_TYPE,
'Decorator host metadata must be a string -> string object, but found unparseable value');
});
it('should throw error if @Directive.queries field has wrong type', () => {
env.tsconfig({});
env.write('test.ts', `
import {Directive} from '@angular/core';
@Directive({
selector: 'test-dir',
queries: 'invalid-queries-type'
})
export class TestDir {}
`);
verifyThrownError(
ErrorCode.VALUE_HAS_WRONG_TYPE, 'Decorator queries metadata must be an object');
});
it('should throw error if @Directive.queries object has incorrect values', () => {
env.tsconfig({});
env.write('test.ts', `
import {Directive} from '@angular/core';
@Directive({
selector: 'test-dir',
queries: {
myViewQuery: 'invalid-query-type'
}
})
export class TestDir {}
`);
verifyThrownError(
ErrorCode.VALUE_HAS_WRONG_TYPE,
'Decorator query metadata must be an instance of a query type');
});
it('should throw error if @Directive.queries object has incorrect values (refs to other decorators)',
() => {
env.tsconfig({});
env.write('test.ts', `
import {Directive, Input} from '@angular/core';
@Directive({
selector: 'test-dir',
queries: {
myViewQuery: new Input()
}
})
export class TestDir {}
`);
verifyThrownError(
ErrorCode.VALUE_HAS_WRONG_TYPE,
'Decorator query metadata must be an instance of a query type');
});
it('should throw error if @Injectable has incorrect argument', () => {
env.tsconfig({});
env.write('test.ts', `
import {Injectable} from '@angular/core';
@Injectable('invalid')
export class TestProvider {}
`);
verifyThrownError(
ErrorCode.DECORATOR_ARG_NOT_LITERAL, '@Injectable argument must be an object literal');
});
});
describe('multiple decorators on classes', () => {