Skip to content

Commit

Permalink
fix(compiler): use FatalDiagnosticError to generate better error mess…
Browse files Browse the repository at this point in the history 
…ages (#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
  • Loading branch information
@AndrewKushnir @mhevery
AndrewKushnir authored and mhevery committed Feb 20, 2020
1 parent bc7a8a8 commit 72664ca
Show file tree
Hide file tree
Showing 3 changed files with 422 additions and 103 deletions.
112 changes: 69 additions & 43 deletions packages/compiler-cli/src/ngtsc/annotations/src/directive.ts
View file
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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(
Expand All @@ -359,17 +359,22 @@ 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')) {
read = new WrappedNodeExpr(options.get('read') !);
}

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;
}
Expand All @@ -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 {
Expand All @@ -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(
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand All @@ -577,17 +597,19 @@ 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') {
hostMetadata[key] = value;
} 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`);
}
});
}
Expand All @@ -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}) => {
Expand All @@ -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;
}
Expand Down
9 changes: 5 additions & 4 deletions packages/compiler-cli/src/ngtsc/annotations/src/injectable.ts
View file
Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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.
Expand All @@ -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));
}
Expand Down

0 comments on commit 72664ca

Please sign in to comment.