Log source location with all errors (#295)

Everywhere we throw an error now reports a location. Use
throwDiagnostic plus getSourceLocationOfXxx helpers going forward
for new errors. (We still need to do error recovery and not just
throw at the first sign of trouble, but that isn't part of this
initial pass.)

Fix duplicate name reporting in binder to report duplicates in
the same file as an error and to report locations of all
duplicates up to the first bound source file that introduced at
least one.

Fix issue where failed compilation was saying "Compilation
completed successfully" and ensure process exits with non-zero
code on failure.
This commit is contained in:
Nick Guerrera 2021-02-12 15:33:02 -08:00 коммит произвёл GitHub
Родитель d0e9927db2
Коммит ba844ea8e5
11 изменённых файлов: 244 добавлений и 95 удалений

Просмотреть файл

@ -1,11 +1,29 @@
import { Suite } from 'mocha';
import { format } from '../lib/decorators.js';
import { DiagnosticError, formatDiagnostic, getSourceLocationOfNode, throwDiagnostic } from './diagnostics.js';
import { visitChildren } from './parser.js';
import { ADLSourceFile, Program } from './program.js';
import { NamespaceStatementNode, ModelStatementNode, Node, SyntaxKind, TemplateParameterDeclarationNode } from './types.js';
import { createSourceFile } from './scanner.js';
import { NamespaceStatementNode, ModelStatementNode, Node, SyntaxKind, TemplateParameterDeclarationNode, SourceLocation } from './types.js';
// trying to avoid masking built-in Symbol
export type Sym = DecoratorSymbol | TypeSymbol;
export type SymbolTable = Map<string, Sym>;
export class SymbolTable extends Map<string, Sym> {
duplicates = new Set<Sym>();
// First set for a given key wins, but record all duplicates for diagnostics.
set(key: string, value: Sym) {
const existing = this.get(key);
if (existing === undefined) {
super.set(key, value);
} else {
this.duplicates.add(existing);
this.duplicates.add(value);
}
return this;
}
}
export interface DecoratorSymbol {
kind: 'decorator';
@ -17,6 +35,7 @@ export interface DecoratorSymbol {
export interface TypeSymbol {
kind: 'type';
node: Node;
name: string;
}
export interface Binder {
@ -40,22 +59,17 @@ export function createBinder(): Binder {
// everything is global
if (globalScope) {
for (const name of sourceFile.symbols.keys()) {
if (program.globalSymbols.has(name)) {
// todo: collect all the redeclarations of
// this binding and mark each as errors
throw new Error('Duplicate binding ' + name);
}
program.globalSymbols.set(name, sourceFile.symbols.get(name)!);
for (const [name, sym] of sourceFile.symbols) {
program.globalSymbols.set(name, sym);
}
reportDuplicateSymbols(program.globalSymbols);
}
}
function bindNode(node: Node) {
if (!node) return;
// set the node's parent since we're going for a walk anyway
(<any>node).parent = parentNode;
node.parent = parentNode;
switch (node.kind) {
case SyntaxKind.ModelStatement:
@ -90,6 +104,7 @@ export function createBinder(): Binder {
(<ModelStatementNode>scope).locals!.set(node.sv, {
kind: 'type',
node: node,
name: node.sv,
});
}
@ -97,10 +112,11 @@ export function createBinder(): Binder {
currentFile.symbols.set(node.id.sv, {
kind: 'type',
node: node,
name: node.id.sv,
});
// initialize locals for type parameters.
node.locals = new Map();
node.locals = new SymbolTable();
}
function bindInterfaceStatement(
@ -109,10 +125,59 @@ export function createBinder(): Binder {
currentFile.symbols.set(statement.id.sv, {
kind: 'type',
node: statement,
name: statement.id.sv,
});
}
function reportDuplicateSymbols(globalSymbols: SymbolTable) {
let reported = new Set<Sym>();
let messages = new Array<string>();
for (const symbol of currentFile.symbols.duplicates) {
report(symbol);
}
for (const symbol of globalSymbols.duplicates) {
report(symbol);
}
if (messages.length > 0) {
// TODO: We're now reporting all duplicates up to the binding of the first file
// that introduced one, but still bailing the compilation rather than
// recovering and reporting other issues including the possibility of more
// duplicates.
//
// That said, decorators are entered into the global symbol table before
// any source file is bound and therefore this will include all duplicate
// decorator implementations.
throw new DiagnosticError(messages.join('\n'));
}
function report(symbol: Sym) {
if (!reported.has(symbol)) {
reported.add(symbol);
const message = formatDiagnostic('Duplicate name: ' + symbol.name, getSourceLocationOfSymbol(symbol));
messages.push(message);
}
}
}
}
function getSourceLocationOfSymbol(symbol: Sym): SourceLocation {
switch (symbol.kind) {
case 'decorator':
return {
// We currently report all decorators at line 1 of defining .js path.
file: createSourceFile("", symbol.path),
pos: 0,
end: 0
};
case 'type':
return getSourceLocationOfNode(symbol.node);
}
}
function hasScope(node: Node) {
switch (node.kind) {
case SyntaxKind.ModelStatement:

Просмотреть файл

@ -1,7 +1,7 @@
import { getSourceLocationOfNode, throwDiagnostic } from './diagnostics.js';
import { ADLSourceFile, Program } from './program.js';
import {
ArrayExpressionNode,
ArrayType,
BooleanLiteralNode,
BooleanLiteralType,
IdentifierNode,
@ -121,7 +121,7 @@ export function createChecker(program: Program) {
return checkTemplateParameterDeclaration(node);
}
throw new Error('cant eval ' + SyntaxKind[node.kind]);
throwDiagnostic('Cannot evaluate ' + SyntaxKind[node.kind], getSourceLocationOfNode(node));
}
function getTypeName(type: Type): string {
@ -190,11 +190,11 @@ export function createChecker(program: Program) {
*/
function instantiateTemplate(templateNode: ModelStatementNode, args: Array<Type>): ModelType {
if (templateNode.templateParameters!.length < args.length) {
throw new Error('Too few template arguments provided.');
throwDiagnostic('Too few template arguments provided.', getSourceLocationOfNode(templateNode));
}
if (templateNode.templateParameters!.length > args.length) {
throw new Error('Too many template arguments provided.');
throwDiagnostic('Too many template arguments provided.', getSourceLocationOfNode(templateNode));
}
const oldTis = templateInstantiation;
@ -229,7 +229,7 @@ export function createChecker(program: Program) {
function checkIntersectionExpression(node: IntersectionExpressionNode) {
const optionTypes = node.options.map(getTypeForNode);
if (!allModelTypes(optionTypes)) {
throw new Error('Cannot intersect non-model types (including union types).');
throwDiagnostic('Cannot intersect non-model types (including union types).', getSourceLocationOfNode(node));
}
const properties = new Map<string, ModelTypeProperty>();
@ -237,7 +237,7 @@ export function createChecker(program: Program) {
const allProps = walkPropertiesInherited(option);
for (const prop of allProps) {
if (properties.has(prop.name)) {
throw new Error(`Intersection contains duplicate property definitions for ${prop.name}`);
throwDiagnostic(`Intersection contains duplicate property definitions for ${prop.name}`, getSourceLocationOfNode(node));
}
const newPropType = createType({
@ -330,7 +330,7 @@ export function createChecker(program: Program) {
}
if (!binding) {
throw new Error('Unknown identifier ' + node.sv);
throwDiagnostic('Unknown identifier ' + node.sv, getSourceLocationOfNode(node));
}
return binding;
@ -377,7 +377,7 @@ export function createChecker(program: Program) {
for (const newProp of newProperties) {
if (properties.has(newProp.name)) {
throw new Error(`Model already has a property named ${newProp.name}`)
throwDiagnostic(`Model already has a property named ${newProp.name}`, getSourceLocationOfNode(node));
}
properties.set(newProp.name, newProp);
@ -420,7 +420,7 @@ export function createChecker(program: Program) {
const heritageType = getTypeForNode(heritageRef);
if (heritageType.kind !== "Model") {
throw new Error("Models must extend other models.")
throwDiagnostic("Models must extend other models.", getSourceLocationOfNode(heritageRef))
}
return heritageType;
@ -434,7 +434,7 @@ export function createChecker(program: Program) {
if (targetType.kind != 'TemplateParameter') {
if (targetType.kind !== 'Model') {
throw new Error('Cannot spread properties of non-model type.');
throwDiagnostic('Cannot spread properties of non-model type.', getSourceLocationOfNode(targetNode));
}
// copy each property

Просмотреть файл

@ -8,6 +8,7 @@ import { readFileSync } from "fs";
import { compile } from "../compiler/program.js";
import { spawnSync } from "child_process";
import { CompilerOptions } from "../compiler/options.js";
import { DiagnosticError } from "./diagnostics.js";
const adlVersion = getVersion();
@ -70,14 +71,20 @@ const args = yargs(process.argv.slice(2))
.demandCommand(1, "You must use one of the supported commands.")
.argv;
async function compileInput(compilerOptions: CompilerOptions): Promise<void> {
async function compileInput(compilerOptions: CompilerOptions): Promise<boolean> {
try {
await compile(args.path!, compilerOptions);
return true;
} catch (err) {
console.error(`An error occurred while compiling path '${args.path}':\n\n${err.message}`);
if (args["debug"]) {
console.error(`Stack trace:\n\n${err.stack}`);
if (err instanceof DiagnosticError) {
console.error(err.message);
if (args.debug) {
console.error(`Stack trace:\n\n${err.stack}`);
}
} else {
throw err; // let non-diagnostic errors go to top-level bug handler.
}
return false;
}
}
@ -107,11 +114,15 @@ async function main() {
if (args._[0] === "compile") {
const options = await getCompilerOptions();
await compileInput(options);
if (!(await compileInput(options))) {
process.exit(1);
}
console.log(`Compilation completed successfully, output files are in ${options.outputPath}.`)
} else if (args._[0] === "generate") {
const options = await getCompilerOptions();
await compileInput(options);
if (!(await compileInput(options))) {
process.exit(1);
}
if (args.client) {
const clientPath = path.resolve(args["output-path"], "client");
@ -146,8 +157,12 @@ async function main() {
main()
.then(() => {})
.catch((err) => {
console.error(`An unknown error occurred:\n\n${err.message}`);
if (args["debug"]) {
console.error(`Stack trace:\n\n${err.stack}`);
}
// NOTE: An expected error, like one thrown for bad input, shouldn't reach
// here, but be handled somewhere else. If we reach here, it should be
// considered a bug and therefore we should not suppress the stack trace as
// that risks losing it in the case of a bug that does not repro easily.
console.error("Internal compiler error!");
console.error("File issue at https://github.com/azure/adl\n");
console.error(err.stack); // includes message.
process.exit(1);
});

Просмотреть файл

@ -0,0 +1,58 @@
import { Message, Node, SourceLocation, SyntaxKind, SourceFile, Type } from "./types.js";
/**
* Represents an error in the code input that is fatal and bails the compilation.
*
* This isn't meant to be kept long term, but we currently do this on all errors.
*/
export class DiagnosticError extends Error {
constructor(message: string) {
super(message);
}
}
export type ErrorHandler = (message: Message | string, location: SourceLocation, ...args: Array<string | number>) => void;
export const throwOnError: ErrorHandler = throwDiagnostic;
export function throwDiagnostic(message: Message | string, location: SourceLocation, ...args: Array<string | number>): never {
throw new DiagnosticError(formatDiagnostic(message, location, ...args));
}
export function formatDiagnostic(message: Message | string, location: SourceLocation, ...args: Array<string | number>) {
if (typeof message === 'string') {
// Temporarily allow ad-hoc strings as error messages.
message = { code: -1, text: message, category: 'error' }
}
const code = message.code < 0 ? "" : ` ADL${message.code}`;
const pos = location.file.getLineAndCharacterOfPosition(location.pos);
const msg = format(message.text, ...args);
return `${location.file.path}:${pos.line + 1}:${pos.character + 1} - ${message.category}${code}: ${msg}`;
}
export function getSourceFileOfNode(node: Node): SourceFile {
while (node.parent !== undefined) {
node = node.parent;
}
if (node.kind !== SyntaxKind.ADLScript) {
throw new Error("Cannot obtain source file of unbound node.");
}
return node.file;
}
export function getSourceLocationOfNode(node: Node): SourceLocation {
return {
file: getSourceFileOfNode(node),
pos: node.pos,
end: node.end,
}
}
export function getSourceLocationOfType(type: Type): SourceLocation {
return getSourceLocationOfNode(type.node);
}
function format(text: string, ...args: Array<string | number>): string {
return text.replace(/{(\d+)}/g, (_match, index: string) => '' + args[+index] || '<ARGMISSING>');
}

Просмотреть файл

Просмотреть файл

@ -1,27 +1,14 @@
export enum MessageCategory {
Warning,
Error,
Suggestion,
Message
}
export interface Message {
code: number;
category: MessageCategory;
text: string;
}
import { Message } from "./types";
export const messages = {
DigitExpected: { code: 1100, category: MessageCategory.Error, text: 'Digit expected (0-9)' },
HexDigitExpected: { code: 1101, category: MessageCategory.Error, text: 'Hex Digit expected (0-F,0-f)' },
BinaryDigitExpected: { code: 1102, category: MessageCategory.Error, text: 'Binary Digit expected (0,1)' },
UnexpectedEndOfFile: { code: 1103, category: MessageCategory.Error, text: 'Unexpected end of file while searching for \'{0}\'' },
InvalidEscapeSequence: { code: 1104, category: MessageCategory.Error, text: 'Invalid escape sequence' },
NoNewLineAtStartOfTripleQuotedString: { code: 1105, category: MessageCategory.Error, text: 'String content in triple quotes must begin on a new line' },
NoNewLineAtEndOfTripleQuotedString: { code: 1106, category: MessageCategory.Error, text: 'Closing triple quotes must begin on a new line' },
InconsistentTripleQuoteIndentation: { code: 1107, category: MessageCategory.Error, text: 'All lines in triple-quoted string lines must have the same indentation as closing triple quotes' },
UnexpectedToken: { code: 1108, category: MessageCategory.Error, text: 'Unexpected token: \'{0}\'' }
export const messages: { [key: string]: Message } = {
// Scanner errors
DigitExpected: { code: 1100, category: 'error', text: 'Digit expected (0-9)' },
HexDigitExpected: { code: 1101, category: 'error', text: 'Hex Digit expected (0-F,0-f)' },
BinaryDigitExpected: { code: 1102, category: 'error', text: 'Binary Digit expected (0,1)' },
UnexpectedEndOfFile: { code: 1103, category: 'error', text: 'Unexpected end of file while searching for \'{0}\'' },
InvalidEscapeSequence: { code: 1104, category: 'error', text: 'Invalid escape sequence' },
NoNewLineAtStartOfTripleQuotedString: { code: 1105, category: 'error', text: 'String content in triple quotes must begin on a new line' },
NoNewLineAtEndOfTripleQuotedString: { code: 1106, category: 'error', text: 'Closing triple quotes must begin on a new line' },
InconsistentTripleQuoteIndentation: { code: 1107, category: 'error', text: 'All lines in triple-quoted string lines must have the same indentation as closing triple quotes' },
UnexpectedToken: { code: 1108, category: 'error', text: 'Unexpected token: \'{0}\'' },
};
export function format(text: string, ...args: Array<string | number>): string {
return text.replace(/{(\d+)}/g, (_match, index: string) => '' + args[+index] || '<ARGMISSING>');
}

Просмотреть файл

@ -1,16 +1,17 @@
import { format } from './messages.js';
import { DiagnosticError, throwDiagnostic } from './diagnostics.js';
import { createScanner, Token } from './scanner.js';
import * as Types from './types.js';
export function parse(code: string | Types.SourceFile) {
const scanner = createScanner(code, (msg, params) => error(format(msg.text, ...params)));
const scanner = createScanner(code);
nextToken();
return parseADLScript();
function parseADLScript(): Types.ADLScriptNode {
const script: Types.ADLScriptNode = {
kind: Types.SyntaxKind.ADLScript,
file: scanner.file,
statements: [],
pos: 0,
end: 0
@ -597,7 +598,7 @@ export function parse(code: string | Types.SourceFile) {
}
}
function finishNode<T>(o: T, pos: number): T & { pos: number; end: number } {
function finishNode<T>(o: T, pos: number): T & Types.TextRange {
return {
...o,
pos,
@ -605,9 +606,12 @@ export function parse(code: string | Types.SourceFile) {
};
}
function error(msg: string) {
const pos = scanner.source.getLineAndCharacterOfPosition(scanner.tokenPosition);
throw new Error(`${scanner.source.path}:${pos.line + 1}:${pos.character + 1} - error: ${msg}`);
function error(message: string) {
throwDiagnostic(message, {
file: scanner.file,
pos: scanner.tokenPosition,
end: scanner.position
});
}
function parseExpected(expectedToken: Token) {

Просмотреть файл

@ -15,9 +15,12 @@ import {
ModelStatementNode,
ModelType,
SyntaxKind,
Type
Type,
SourceFile,
} from './types.js';
import { createSourceFile } from "./scanner.js";
import { getSourceLocationOfNode, getSourceLocationOfType, throwDiagnostic } from "./diagnostics.js";
export interface Program {
compilerOptions: CompilerOptions;
@ -33,9 +36,8 @@ export interface Program {
executeDecorators(type: Type): void;
}
export interface ADLSourceFile {
export interface ADLSourceFile extends SourceFile {
ast: ADLScriptNode;
path: string;
symbols: SymbolTable;
models: Array<ModelType>;
interfaces: Array<Namespace>;
@ -46,7 +48,7 @@ export async function compile(rootDir: string, options?: CompilerOptions) {
const program: Program = {
compilerOptions: options || {},
globalSymbols: new Map(),
globalSymbols: new SymbolTable(),
sourceFiles: [],
typeCache: new MultiKeyMap(),
literalTypes: new Map(),
@ -116,7 +118,7 @@ export async function compile(rootDir: string, options?: CompilerOptions) {
function executeDecorator(dec: DecoratorExpressionNode, program: Program, type: Type) {
if (dec.target.kind !== SyntaxKind.Identifier) {
throw new Error('Decorator must be identifier');
throwDiagnostic('Decorator must be identifier', getSourceLocationOfNode(dec));
}
const decName = dec.target.sv;
@ -125,7 +127,7 @@ export async function compile(rootDir: string, options?: CompilerOptions) {
);
const decBinding = <DecoratorSymbol>program.globalSymbols.get(decName);
if (!decBinding) {
throw new Error(`Can't find decorator ${decName}`);
throwDiagnostic(`Can't find decorator ${decName}`, getSourceLocationOfNode(dec));
}
const decFn = decBinding.value;
decFn(program, type, ...args);
@ -220,13 +222,15 @@ export async function compile(rootDir: string, options?: CompilerOptions) {
// virtual file path
function evalAdlScript(adlScript: string, filePath?: string): void {
filePath = filePath ?? `__virtual_file_${++virtualFileCount}`;
const ast = parse(createSourceFile(adlScript, filePath));
const unparsedFile = createSourceFile(adlScript, filePath);
const ast = parse(unparsedFile);
const sourceFile = {
...unparsedFile,
ast,
path: filePath,
interfaces: [],
models: [],
symbols: new Map(),
symbols: new SymbolTable(),
};
program.sourceFiles.push(sourceFile);

Просмотреть файл

@ -1,6 +1,7 @@
import { CharacterCodes, isBinaryDigit, isDigit, isHexDigit, isIdentifierPart, isIdentifierStart, isLineBreak, isWhiteSpaceSingleLine } from './character-codes.js';
import { format, Message, messages } from './messages.js';
import { SourceFile } from './types.js';
import { throwOnError } from './diagnostics.js';
import { messages } from './messages.js';
import { Message, SourceFile } from './types.js';
// All conflict markers consist of the same character repeated seven times. If it is
// a <<<<<<< or >>>>>>> marker then it is also followed by a space.
@ -68,7 +69,7 @@ const keywords = new Map([
export interface Scanner {
/** The source code being scanned. */
readonly source: SourceFile;
readonly file: SourceFile;
/** The offset in UTF-16 code units to the current position at the start of the next token. */
readonly position: number;
@ -97,10 +98,6 @@ export interface Scanner {
getTokenValue(): string;
}
export function throwOnError(msg: Message, params: Array<string | number>) {
throw new Error(format(msg.text, ...params));
}
const enum TokenFlags {
HasCrlf = 1 << 0,
Escaped = 1 << 1,
@ -108,11 +105,8 @@ const enum TokenFlags {
}
export function createScanner(source: string | SourceFile, onError = throwOnError): Scanner {
if (typeof source === 'string') {
source = createSourceFile(source, '<anonymous file>');
}
const input = source.text;
const file = typeof source === 'string' ? createSourceFile(source, '<anonymous file>') : source;
const input = file.text;
let position = 0;
let token = Token.Unknown;
let tokenPosition = -1;
@ -123,7 +117,7 @@ export function createScanner(source: string | SourceFile, onError = throwOnErro
get position() { return position; },
get token() { return token; },
get tokenPosition() { return tokenPosition; },
source,
file,
scan,
eof,
getTokenText,
@ -314,8 +308,8 @@ export function createScanner(source: string | SourceFile, onError = throwOnErro
return false;
}
function error(msg: Message, ...params: Array<string | number>) {
onError(msg, params);
function error(msg: Message, ...args: Array<string | number>) {
onError(msg, { file, pos: tokenPosition, end: position }, ...args);
}
function scanWhitespace(): Token {

Просмотреть файл

@ -1,4 +1,4 @@
import { SymbolTable } from './binder';
import { SymbolTable } from "./binder";
/**
* Type System types
@ -128,10 +128,8 @@ export enum SyntaxKind {
TemplateParameterDeclaration
}
export interface BaseNode {
export interface BaseNode extends TextRange {
kind: SyntaxKind;
pos: number;
end: number;
parent?: Node;
}
@ -150,6 +148,7 @@ export type Node =
export interface ADLScriptNode extends BaseNode {
kind: SyntaxKind.ADLScript;
statements: Array<Statement>;
file: SourceFile;
}
export type Statement =
@ -345,3 +344,26 @@ export interface SourceFile {
getLineAndCharacterOfPosition(position: number): LineAndCharacter;
}
export interface TextRange {
/**
* The starting position of the ranger measured in UTF-16 code units from the
* start of the full string. Inclusive.
*/
pos: number;
/**
* The ending position measured in UTF-16 code units from the start of the
* full string. Exclusive.
*/
end: number;
}
export interface SourceLocation extends TextRange {
file: SourceFile;
}
export interface Message {
code: number;
text: string;
category: 'error' | 'warning';
}

Просмотреть файл

@ -1,8 +1,8 @@
import { deepStrictEqual, strictEqual } from 'assert';
import { readFile } from 'fs/promises';
import { URL } from 'url';
import { format } from '../compiler/messages.js';
import { createScanner, throwOnError, Token } from '../compiler/scanner.js';
import { throwOnError } from '../compiler/diagnostics.js';
import { createScanner, Token } from '../compiler/scanner.js';
import { LineAndCharacter } from '../compiler/types.js';
type TokenEntry = [Token, string?, number?, LineAndCharacter?];
@ -17,7 +17,7 @@ function tokens(text: string, onError = throwOnError): Array<TokenEntry> {
scanner.token,
scanner.getTokenText(),
scanner.tokenPosition,
scanner.source.getLineAndCharacterOfPosition(scanner.tokenPosition),
scanner.file.getLineAndCharacterOfPosition(scanner.tokenPosition),
]);
} while (!scanner.eof());
@ -136,7 +136,7 @@ describe('scanner', () => {
});
function scanString(text: string, expectedValue: string) {
const scanner = createScanner(text, (msg, params) => { throw new Error(format(msg.text, ...params)); });
const scanner = createScanner(text);
strictEqual(scanner.scan(), Token.StringLiteral);
strictEqual(scanner.token, Token.StringLiteral);
strictEqual(scanner.getTokenText(), text);
@ -205,6 +205,6 @@ describe('scanner', () => {
it('scans this file', async () => {
const text = await readFile(new URL(import.meta.url), 'utf-8');
tokens(text, function(msg, params) { /* ignore errors */});
tokens(text, function() { /* ignore errors */});
});
});