feat(language-service): modularize error messages (#35678)

This commit performs a modularization of the Language Service's existing
diagnostic messages. Such a modularization has two primary advantages:

- Centralization and decoupling of error messages from the code that
  generates them makes it easy to add/delete/edit diagnostic messages,
  and allows for independent iteration of diagnostic messages and
  diagnostic generation.
- Prepares for additional features like annotating the locations where a
  diagnostic is generated and enabling the configuration of which
  diagnostics should be reported by the language service.

Although it would be preferable to place the diagnostics registry in an
independent JSON file, for ease of typing diagnostic types as an enum
variant of 'ts.DiagnosticCategory', the registry is stored as an object.

Part of #32663.

PR Close #35678
This commit is contained in:
ayazhafiz 2020-02-25 08:32:38 -08:00 committed by atscott
parent 8eb4a9d395
commit 47a1811e0b
8 changed files with 268 additions and 101 deletions

View File

@ -0,0 +1,161 @@
/**
* @license
* Copyright Google Inc. All Rights Reserved.
*
* 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 * as ng from './types';
export interface DiagnosticMessage {
message: string;
kind: keyof typeof ts.DiagnosticCategory;
}
type DiagnosticName = 'directive_not_in_module' | 'missing_template_and_templateurl' |
'both_template_and_templateurl' | 'invalid_templateurl' | 'template_context_missing_member' |
'callable_expression_expected_method_call' | 'call_target_not_callable' |
'expression_might_be_null' | 'expected_a_number_type' | 'expected_a_string_or_number_type' |
'expected_operands_of_similar_type_or_any' | 'unrecognized_operator' |
'unrecognized_primitive' | 'no_pipe_found' | 'unable_to_resolve_compatible_call_signature' |
'unable_to_resolve_signature' | 'could_not_resolve_type' | 'identifier_not_callable' |
'identifier_possibly_undefined' | 'identifier_not_defined_in_app_context' |
'identifier_not_defined_on_receiver' | 'identifier_is_private';
export const Diagnostic: Record<DiagnosticName, DiagnosticMessage> = {
directive_not_in_module: {
message:
`%1 '%2' is not included in a module and will not be available inside a template. Consider adding it to a NgModule declaration.`,
kind: 'Suggestion',
},
missing_template_and_templateurl: {
message: `Component '%1' must have a template or templateUrl`,
kind: 'Error',
},
both_template_and_templateurl: {
message: `Component '%1' must not have both template and templateUrl`,
kind: 'Error',
},
invalid_templateurl: {
message: `URL does not point to a valid file`,
kind: 'Error',
},
template_context_missing_member: {
message: `The template context of '%1' does not define %2.\n` +
`If the context type is a base type or 'any', consider refining it to a more specific type.`,
kind: 'Suggestion',
},
callable_expression_expected_method_call: {
message: 'Unexpected callable expression. Expected a method call',
kind: 'Warning',
},
call_target_not_callable: {
message: 'Call target is not callable',
kind: 'Error',
},
expression_might_be_null: {
message: 'The expression might be null',
kind: 'Error',
},
expected_a_number_type: {
message: 'Expected a number type',
kind: 'Error',
},
expected_a_string_or_number_type: {
message: 'Expected operands to be a string or number type',
kind: 'Error',
},
expected_operands_of_similar_type_or_any: {
message: 'Expected operands to be of similar type or any',
kind: 'Error',
},
unrecognized_operator: {
message: 'Unrecognized operator %1',
kind: 'Error',
},
unrecognized_primitive: {
message: 'Unrecognized primitive %1',
kind: 'Error',
},
no_pipe_found: {
message: 'No pipe of name %1 found',
kind: 'Error',
},
// TODO: Consider a better error message here.
unable_to_resolve_compatible_call_signature: {
message: 'Unable to resolve compatible call signature',
kind: 'Error',
},
unable_to_resolve_signature: {
message: 'Unable to resolve signature for call of %1',
kind: 'Error',
},
could_not_resolve_type: {
message: `Could not resolve the type of '%1'`,
kind: 'Error',
},
identifier_not_callable: {
message: `'%1' is not callable`,
kind: 'Error',
},
identifier_possibly_undefined: {
message:
`'%1' is possibly undefined. Consider using the safe navigation operator (%2) or non-null assertion operator (%3).`,
kind: 'Suggestion',
},
identifier_not_defined_in_app_context: {
message:
`Identifier '%1' is not defined. The component declaration, template variable declarations, and element references do not contain such a member`,
kind: 'Error',
},
identifier_not_defined_on_receiver: {
message: `Identifier '%1' is not defined. '%2' does not contain such a member`,
kind: 'Error',
},
identifier_is_private: {
message: `Identifier '%1' refers to a private member of %2`,
kind: 'Warning',
},
};
/**
* Creates a language service diagnostic.
* @param span location the diagnostic for
* @param dm diagnostic message
* @param formatArgs run-time arguments to format the diagnostic message with (see the messages in
* the `Diagnostic` object for an example).
* @returns a created diagnostic
*/
export function createDiagnostic(
span: ng.Span, dm: DiagnosticMessage, ...formatArgs: string[]): ng.Diagnostic {
// Formats "%1 %2" with formatArgs ['a', 'b'] as "a b"
const formattedMessage =
dm.message.replace(/%(\d+)/g, (_, index: string) => formatArgs[+index - 1]);
return {
kind: ts.DiagnosticCategory[dm.kind],
message: formattedMessage, span,
};
}

View File

@ -11,12 +11,14 @@ import * as path from 'path';
import * as ts from 'typescript';
import {AstResult} from './common';
import {Diagnostic, createDiagnostic} from './diagnostic_messages';
import {getTemplateExpressionDiagnostics} from './expression_diagnostics';
import * as ng from './types';
import {TypeScriptServiceHost} from './typescript_host';
import {findPropertyValueOfType, findTightestNode, offsetSpan, spanOf} from './utils';
/**
* Return diagnostic information for the parsed AST of the template.
* @param ast contains HTML and template AST
@ -41,18 +43,6 @@ export function getTemplateDiagnostics(ast: AstResult): ng.Diagnostic[] {
});
}
/**
* Generate an error message that indicates a directive is not part of any
* NgModule.
* @param name class name
* @param isComponent true if directive is an Angular Component
*/
function missingDirective(name: string, isComponent: boolean) {
const type = isComponent ? 'Component' : 'Directive';
return `${type} '${name}' is not included in a module and will not be ` +
'available inside a template. Consider adding it to a NgModule declaration.';
}
/**
* Performs a variety diagnostics on directive declarations.
*
@ -96,28 +86,22 @@ export function getDeclarationDiagnostics(
span: error.span,
});
}
if (!modules.ngModuleByPipeOrDirective.has(declaration.type)) {
results.push(createDiagnostic(
declarationSpan, Diagnostic.directive_not_in_module,
metadata.isComponent ? 'Component' : 'Directive', type.name));
}
if (metadata.isComponent) {
if (!modules.ngModuleByPipeOrDirective.has(declaration.type)) {
results.push({
kind: ts.DiagnosticCategory.Suggestion,
message: missingDirective(type.name, metadata.isComponent),
span: declarationSpan,
});
}
const {template, templateUrl, styleUrls} = metadata.template !;
if (template === null && !templateUrl) {
results.push({
kind: ts.DiagnosticCategory.Error,
message: `Component '${type.name}' must have a template or templateUrl`,
span: declarationSpan,
});
results.push(createDiagnostic(
declarationSpan, Diagnostic.missing_template_and_templateurl, type.name));
} else if (templateUrl) {
if (template) {
results.push({
kind: ts.DiagnosticCategory.Error,
message: `Component '${type.name}' must not have both template and templateUrl`,
span: declarationSpan,
});
results.push(createDiagnostic(
declarationSpan, Diagnostic.both_template_and_templateurl, type.name));
}
// Find templateUrl value from the directive call expression, which is the parent of the
@ -147,12 +131,6 @@ export function getDeclarationDiagnostics(
results.push(...validateUrls(styleUrlsNode.elements, host.tsLsHost));
}
} else if (!directives.has(declaration.type)) {
results.push({
kind: ts.DiagnosticCategory.Suggestion,
message: missingDirective(type.name, metadata.isComponent),
span: declarationSpan,
});
}
}
@ -188,12 +166,9 @@ function validateUrls(
const url = path.join(path.dirname(curPath), urlNode.text);
if (tsLsHost.fileExists(url)) continue;
allErrors.push({
kind: ts.DiagnosticCategory.Error,
message: `URL does not point to a valid file`,
// Exclude opening and closing quotes in the url span.
span: {start: urlNode.getStart() + 1, end: urlNode.end - 1},
});
// Exclude opening and closing quotes in the url span.
const urlSpan = {start: urlNode.getStart() + 1, end: urlNode.end - 1};
allErrors.push(createDiagnostic(urlSpan, Diagnostic.invalid_templateurl));
}
return allErrors;
}

View File

@ -7,11 +7,11 @@
*/
import {AST, AstPath, Attribute, BoundDirectivePropertyAst, BoundElementPropertyAst, BoundEventAst, BoundTextAst, CompileDirectiveSummary, CompileTypeMetadata, DirectiveAst, ElementAst, EmbeddedTemplateAst, Node, ParseSourceSpan, RecursiveTemplateAstVisitor, ReferenceAst, TemplateAst, TemplateAstPath, VariableAst, identifierName, templateVisitAll, tokenReference} from '@angular/compiler';
import * as ts from 'typescript';
import {Diagnostic, createDiagnostic} from './diagnostic_messages';
import {AstType} from './expression_type';
import {BuiltinType, Definition, Span, Symbol, SymbolDeclaration, SymbolQuery, SymbolTable} from './symbols';
import {Diagnostic} from './types';
import * as ng from './types';
import {findOutputBinding, getPathToNodeAtPosition} from './utils';
export interface DiagnosticTemplateInfo {
@ -23,7 +23,7 @@ export interface DiagnosticTemplateInfo {
templateAst: TemplateAst[];
}
export function getTemplateExpressionDiagnostics(info: DiagnosticTemplateInfo): Diagnostic[] {
export function getTemplateExpressionDiagnostics(info: DiagnosticTemplateInfo): ng.Diagnostic[] {
const visitor = new ExpressionDiagnosticsVisitor(
info, (path: TemplateAstPath) => getExpressionScope(info, path));
templateVisitAll(visitor, info.templateAst);
@ -244,7 +244,7 @@ class ExpressionDiagnosticsVisitor extends RecursiveTemplateAstVisitor {
private path: TemplateAstPath;
private directiveSummary: CompileDirectiveSummary|undefined;
diagnostics: Diagnostic[] = [];
diagnostics: ng.Diagnostic[] = [];
constructor(
private info: DiagnosticTemplateInfo,
@ -291,10 +291,11 @@ class ExpressionDiagnosticsVisitor extends RecursiveTemplateAstVisitor {
if (context && !context.has(ast.value)) {
const missingMember =
ast.value === '$implicit' ? 'an implicit value' : `a member called '${ast.value}'`;
this.reportDiagnostic(
`The template context of '${directive.type.reference.name}' does not define ${missingMember}.\n` +
`If the context type is a base type or 'any', consider refining it to a more specific type.`,
spanOf(ast.sourceSpan), ts.DiagnosticCategory.Suggestion);
const span = this.absSpan(spanOf(ast.sourceSpan));
this.diagnostics.push(createDiagnostic(
span, Diagnostic.template_context_missing_member, directive.type.reference.name,
missingMember));
}
}
}
@ -334,10 +335,9 @@ class ExpressionDiagnosticsVisitor extends RecursiveTemplateAstVisitor {
private diagnoseExpression(ast: AST, offset: number, event: boolean) {
const scope = this.getExpressionScope(this.path, event);
const analyzer = new AstType(scope, this.info.query, {event});
for (const {message, span, kind} of analyzer.getDiagnostics(ast)) {
span.start += offset;
span.end += offset;
this.reportDiagnostic(message as string, span, kind);
for (const diagnostic of analyzer.getDiagnostics(ast)) {
diagnostic.span = this.absSpan(diagnostic.span, offset);
this.diagnostics.push(diagnostic);
}
}
@ -345,11 +345,11 @@ class ExpressionDiagnosticsVisitor extends RecursiveTemplateAstVisitor {
private pop() { this.path.pop(); }
private reportDiagnostic(
message: string, span: Span, kind: ts.DiagnosticCategory = ts.DiagnosticCategory.Error) {
span.start += this.info.offset;
span.end += this.info.offset;
this.diagnostics.push({kind, span, message});
private absSpan(span: Span, additionalOffset: number = 0): Span {
return {
start: span.start + this.info.offset + additionalOffset,
end: span.end + this.info.offset + additionalOffset,
};
}
}

View File

@ -7,8 +7,8 @@
*/
import {AST, AstVisitor, Binary, BindingPipe, Chain, Conditional, FunctionCall, ImplicitReceiver, Interpolation, KeyedRead, KeyedWrite, LiteralArray, LiteralMap, LiteralPrimitive, MethodCall, NonNullAssert, PrefixNot, PropertyRead, PropertyWrite, Quote, SafeMethodCall, SafePropertyRead} from '@angular/compiler';
import * as ts from 'typescript';
import {Diagnostic, createDiagnostic} from './diagnostic_messages';
import {BuiltinType, Signature, Symbol, SymbolQuery, SymbolTable} from './symbols';
import * as ng from './types';
@ -27,9 +27,8 @@ export class AstType implements AstVisitor {
getDiagnostics(ast: AST): ng.Diagnostic[] {
const type: Symbol = ast.visit(this);
if (this.context.event && type.callable) {
this.reportDiagnostic(
'Unexpected callable expression. Expected a method call', ast,
ts.DiagnosticCategory.Warning);
this.diagnostics.push(
createDiagnostic(ast.span, Diagnostic.callable_expression_expected_method_call));
}
return this.diagnostics;
}
@ -58,7 +57,7 @@ export class AstType implements AstVisitor {
// Nullable allowed.
break;
default:
this.reportDiagnostic(`The expression might be null`, ast);
this.diagnostics.push(createDiagnostic(ast.span, Diagnostic.expression_might_be_null));
break;
}
return this.query.getNonNullableType(type);
@ -102,7 +101,8 @@ export class AstType implements AstVisitor {
errorAst = ast.right;
break;
}
this.reportDiagnostic('Expected a numeric type', errorAst);
this.diagnostics.push(
createDiagnostic(errorAst.span, Diagnostic.expected_a_number_type));
return this.anyType;
}
case '+':
@ -129,14 +129,17 @@ export class AstType implements AstVisitor {
return this.query.getBuiltinType(BuiltinType.Number);
case BuiltinType.Boolean << 8 | BuiltinType.Number:
case BuiltinType.Other << 8 | BuiltinType.Number:
this.reportDiagnostic('Expected a number type', ast.left);
this.diagnostics.push(
createDiagnostic(ast.left.span, Diagnostic.expected_a_number_type));
return this.anyType;
case BuiltinType.Number << 8 | BuiltinType.Boolean:
case BuiltinType.Number << 8 | BuiltinType.Other:
this.reportDiagnostic('Expected a number type', ast.right);
this.diagnostics.push(
createDiagnostic(ast.right.span, Diagnostic.expected_a_number_type));
return this.anyType;
default:
this.reportDiagnostic('Expected operands to be a string or number type', ast);
this.diagnostics.push(
createDiagnostic(ast.span, Diagnostic.expected_a_string_or_number_type));
return this.anyType;
}
case '>':
@ -163,7 +166,8 @@ export class AstType implements AstVisitor {
case BuiltinType.Other << 8 | BuiltinType.Other:
return this.query.getBuiltinType(BuiltinType.Boolean);
default:
this.reportDiagnostic('Expected the operants to be of similar type or any', ast);
this.diagnostics.push(
createDiagnostic(ast.span, Diagnostic.expected_operands_of_similar_type_or_any));
return this.anyType;
}
case '&&':
@ -172,7 +176,8 @@ export class AstType implements AstVisitor {
return this.query.getTypeUnion(leftType, rightType);
}
this.reportDiagnostic(`Unrecognized operator ${ast.operation}`, ast);
this.diagnostics.push(
createDiagnostic(ast.span, Diagnostic.unrecognized_operator, ast.operation));
return this.anyType;
}
@ -201,7 +206,7 @@ export class AstType implements AstVisitor {
const args = ast.args.map(arg => this.getType(arg));
const target = this.getType(ast.target !);
if (!target || !target.callable) {
this.reportDiagnostic('Call target is not callable', ast);
this.diagnostics.push(createDiagnostic(ast.span, Diagnostic.call_target_not_callable));
return this.anyType;
}
const signature = target.selectSignature(args);
@ -209,7 +214,8 @@ export class AstType implements AstVisitor {
return signature.result;
}
// TODO: Consider a better error message here.
this.reportDiagnostic('Unable no compatible signature found for call', ast);
this.diagnostics.push(
createDiagnostic(ast.span, Diagnostic.unable_to_resolve_compatible_call_signature));
return this.anyType;
}
@ -290,7 +296,8 @@ export class AstType implements AstVisitor {
case 'number':
return this.query.getBuiltinType(BuiltinType.Number);
default:
this.reportDiagnostic('Unrecognized primitive', ast);
this.diagnostics.push(
createDiagnostic(ast.span, Diagnostic.unrecognized_primitive, typeof ast.value));
return this.anyType;
}
}
@ -305,14 +312,15 @@ export class AstType implements AstVisitor {
// by getPipes() is expected to contain symbols with the corresponding transform method type.
const pipe = this.query.getPipes().get(ast.name);
if (!pipe) {
this.reportDiagnostic(`No pipe by the name ${ast.name} found`, ast);
this.diagnostics.push(createDiagnostic(ast.span, Diagnostic.no_pipe_found, ast.name));
return this.anyType;
}
const expType = this.getType(ast.exp);
const signature =
pipe.selectSignature([expType].concat(ast.args.map(arg => this.getType(arg))));
if (!signature) {
this.reportDiagnostic('Unable to resolve signature for pipe invocation', ast);
this.diagnostics.push(
createDiagnostic(ast.span, Diagnostic.unable_to_resolve_signature, ast.name));
return this.anyType;
}
return signature.result;
@ -376,19 +384,22 @@ export class AstType implements AstVisitor {
}
const methodType = this.resolvePropertyRead(receiverType, ast);
if (!methodType) {
this.reportDiagnostic(`Could not find a type for '${ast.name}'`, ast);
this.diagnostics.push(
createDiagnostic(ast.span, Diagnostic.could_not_resolve_type, ast.name));
return this.anyType;
}
if (this.isAny(methodType)) {
return this.anyType;
}
if (!methodType.callable) {
this.reportDiagnostic(`Member '${ast.name}' is not callable`, ast);
this.diagnostics.push(
createDiagnostic(ast.span, Diagnostic.identifier_not_callable, ast.name));
return this.anyType;
}
const signature = methodType.selectSignature(ast.args.map(arg => this.getType(arg)));
if (!signature) {
this.reportDiagnostic(`Unable to resolve signature for call of method ${ast.name}`, ast);
this.diagnostics.push(
createDiagnostic(ast.span, Diagnostic.unable_to_resolve_signature, ast.name));
return this.anyType;
}
return signature.result;
@ -402,37 +413,28 @@ export class AstType implements AstVisitor {
const member = receiverType.members().get(ast.name);
if (!member) {
if (receiverType.name === '$implicit') {
this.reportDiagnostic(
`Identifier '${ast.name}' is not defined. ` +
`The component declaration, template variable declarations, and element references do not contain such a member`,
ast);
this.diagnostics.push(
createDiagnostic(ast.span, Diagnostic.identifier_not_defined_in_app_context, ast.name));
} else if (receiverType.nullable && ast.receiver instanceof PropertyRead) {
const receiver = ast.receiver.name;
this.reportDiagnostic(
`'${receiver}' is possibly undefined. Consider using the safe navigation operator (${receiver}?.${ast.name}) ` +
`or non-null assertion operator (${receiver}!.${ast.name}).`,
ast, ts.DiagnosticCategory.Suggestion);
this.diagnostics.push(createDiagnostic(
ast.span, Diagnostic.identifier_possibly_undefined, receiver,
`${receiver}?.${ast.name}`, `${receiver}!.${ast.name}`));
} else {
this.reportDiagnostic(
`Identifier '${ast.name}' is not defined. '${receiverType.name}' does not contain such a member`,
ast);
this.diagnostics.push(createDiagnostic(
ast.span, Diagnostic.identifier_not_defined_on_receiver, ast.name, receiverType.name));
}
return this.anyType;
}
if (!member.public) {
this.reportDiagnostic(
`Identifier '${ast.name}' refers to a private member of ${receiverType.name === '$implicit' ? 'the component' : `
'${receiverType.name}'
`}`,
ast, ts.DiagnosticCategory.Warning);
const container =
receiverType.name === '$implicit' ? 'the component' : `'${receiverType.name}'`;
this.diagnostics.push(
createDiagnostic(ast.span, Diagnostic.identifier_is_private, ast.name, container));
}
return member.type;
}
private reportDiagnostic(message: string, ast: AST, kind = ts.DiagnosticCategory.Error) {
this.diagnostics.push({kind, span: ast.span, message});
}
private isAny(symbol: Symbol): boolean {
return !symbol || this.query.getTypeKind(symbol) === BuiltinType.Any ||
(!!symbol.type && this.isAny(symbol.type));

View File

@ -42,6 +42,7 @@ ts_library(
name = "infra_test_lib",
testonly = True,
srcs = [
"diagnostic_messages_spec.ts",
"global_symbols_spec.ts",
"html_info_spec.ts",
"language_service_spec.ts",

View File

@ -0,0 +1,28 @@
/**
* @license
* Copyright Google Inc. All Rights Reserved.
*
* 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 {DiagnosticMessage, createDiagnostic} from '../src/diagnostic_messages';
describe('create diagnostic', () => {
it('should format and create diagnostics correctly', () => {
const diagnosticMessage: DiagnosticMessage = {
message: 'Check that %1 contains %2',
kind: 'Error',
};
const diagnostic =
createDiagnostic({start: 0, end: 1}, diagnosticMessage, 'testCls', 'testMethod');
expect(diagnostic).toEqual({
kind: ts.DiagnosticCategory.Error,
message: 'Check that testCls contains testMethod',
span: {start: 0, end: 1},
});
});
});

View File

@ -85,7 +85,7 @@ describe('diagnostics', () => {
mockHost.override(TEST_TEMPLATE, template);
const diags = ngLS.getSemanticDiagnostics(TEST_TEMPLATE);
expect(diags.length).toBe(1);
expect(diags[0].messageText).toBe('Unable to resolve signature for call of method $any');
expect(diags[0].messageText).toBe('Unable to resolve signature for call of $any');
}
});
@ -129,7 +129,7 @@ describe('diagnostics', () => {
`);
const diags = ngLS.getSemanticDiagnostics(TEST_TEMPLATE);
expect(diags.length).toBe(1);
expect(diags[0].messageText).toBe(`Expected the operants to be of similar type or any`);
expect(diags[0].messageText).toBe(`Expected operands to be of similar type or any`);
});
it('should not report errors for matching exported type', () => {
@ -228,7 +228,7 @@ describe('diagnostics', () => {
it('should report numeric operator errors', () => {
const diags = ngLS.getSemanticDiagnostics(EXPRESSION_CASES).map(d => d.messageText);
expect(diags).toContain('Expected a numeric type');
expect(diags).toContain('Expected a number type');
});
});

View File

@ -180,7 +180,7 @@ describe('expression diagnostics', () => {
it('should reject a misspelled field of a method result',
() => reject('{{getPerson().nume.first}}', 'Identifier \'nume\' is not defined'));
it('should reject calling a uncallable member',
() => reject('{{person().name.first}}', 'Member \'person\' is not callable'));
() => reject('{{person().name.first}}', '\'person\' is not callable'));
it('should accept an event handler',
() => accept('<div (click)="click($event)">{{person.name.first}}</div>'));
it('should reject a misspelled event handler',