diff --git a/packages/malloy/src/lang/syntax-errors/custom-error-messages.ts b/packages/malloy/src/lang/syntax-errors/custom-error-messages.ts index 3e8b0d318..178ca9b9a 100644 --- a/packages/malloy/src/lang/syntax-errors/custom-error-messages.ts +++ b/packages/malloy/src/lang/syntax-errors/custom-error-messages.ts @@ -29,6 +29,15 @@ export interface ErrorCase { // The error message to show to the user, instead of whatever was default // Supports tokenization: ${currentToken} errorMessage: string; + + // By default, the error checker does lookahead and lookback based on the curentToken + // (the parser's position in the inputStream). However, for many error cases it is + // better to check from the offending token and not from the current token, since the + // error may have been hit while the ATN was looking ahead for alternatives. + // Note: In most cases, if this option is enabled, the 'ruleContext' may be a parser rule + // much higher in the tree than you would expect, and may be better to leave off entirely. + // Note: You can use this option without actually specifying the offendingSymbol match. + lookbackFromOffendingSymbol?: boolean; } export const checkCustomErrorMessage = ( @@ -53,7 +62,15 @@ export const checkCustomErrorMessage = ( // If so, try to check the preceding tokens. if (errorCase.precedingTokenOptions) { const hasPrecedingTokenMatch = errorCase.precedingTokenOptions.some( - sequence => checkTokenSequenceMatch(parser, sequence, 'lookback') + sequence => + checkTokenSequenceMatch( + parser, + sequence, + 'lookback', + errorCase.lookbackFromOffendingSymbol + ? offendingSymbol?.tokenIndex + : undefined + ) ); if (!hasPrecedingTokenMatch) { continue; // Continue to check a different error case @@ -61,7 +78,15 @@ export const checkCustomErrorMessage = ( } if (errorCase.lookAheadOptions) { const hasLookaheadTokenMatch = errorCase.lookAheadOptions.some( - sequence => checkTokenSequenceMatch(parser, sequence, 'lookahead') + sequence => + checkTokenSequenceMatch( + parser, + sequence, + 'lookahead', + errorCase.lookbackFromOffendingSymbol + ? offendingSymbol?.tokenIndex + : undefined + ) ); if (!hasLookaheadTokenMatch) { continue; // Continue to check a different error case @@ -83,13 +108,23 @@ export const checkCustomErrorMessage = ( const checkTokenSequenceMatch = ( parser: Parser, sequence: number[], - direction: 'lookahead' | 'lookback' + direction: 'lookahead' | 'lookback', + anchorTokenPosition: number | undefined ): boolean => { try { for (let i = 0; i < sequence.length; i++) { + let streamToken: number | undefined = undefined; + if (typeof anchorTokenPosition === 'number') { + const tokenOffset = direction === 'lookahead' ? i + 1 : -1 * (i + 1); + streamToken = parser.inputStream.get( + anchorTokenPosition + tokenOffset + ).type; + } else { + const tokenOffset = direction === 'lookahead' ? i + 2 : -1 * (i + 1); + streamToken = parser.inputStream.LA(tokenOffset); + } // Note: positive lookahead starts at '2' because '1' is the current token. - const tokenIndex = direction === 'lookahead' ? i + 2 : -1 * (i + 1); - const streamToken = parser.inputStream.LA(tokenIndex); + // Note: negative checking is < -1 becuase Token.EOF is -1, but below // that we use negatives to indicate "does-not-match" rules. diff --git a/packages/malloy/src/lang/syntax-errors/malloy-parser-error-listener.ts b/packages/malloy/src/lang/syntax-errors/malloy-parser-error-listener.ts index 45bc06f1c..3a2181dbc 100644 --- a/packages/malloy/src/lang/syntax-errors/malloy-parser-error-listener.ts +++ b/packages/malloy/src/lang/syntax-errors/malloy-parser-error-listener.ts @@ -43,6 +43,14 @@ export const commonErrorCases: ErrorCase[] = [ [MalloyParser.SOURCE], ], }, + { + errorMessage: + "'aggregate:' entries must include a name (ex: `some_name is count()`)", + // ruleContextOptions: ['queryFieldEntry'], + precedingTokenOptions: [[MalloyParser.AGGREGATE]], + lookAheadOptions: [[-MalloyParser.IS]], + lookbackFromOffendingSymbol: true, + }, ]; export class MalloyParserErrorListener implements ANTLRErrorListener { diff --git a/packages/malloy/src/lang/test/syntax-errors.spec.ts b/packages/malloy/src/lang/test/syntax-errors.spec.ts index 24981f2de..e25b8a017 100644 --- a/packages/malloy/src/lang/test/syntax-errors.spec.ts +++ b/packages/malloy/src/lang/test/syntax-errors.spec.ts @@ -68,4 +68,33 @@ describe('errors', () => { primary_key: id `).toLogAtLeast(errorMessage("Missing '{' after 'extend'")); }); + + test('missing alias for aggregate entry', () => { + expect(` + run: x -> { + aggregate: count() + } + `).toLogAtLeast( + errorMessage( + "'aggregate:' entries must include a name (ex: `some_name is count()`)" + ) + ); + }); + + test('missing alias for aggregate inside source>view', () => { + expect(` + source: x is aa extend { + measure: airport_count is count() + + view: by_state is { + where: state is not null + aggregate: count() + } + } + `).toLogAtLeast( + errorMessage( + "'aggregate:' entries must include a name (ex: `some_name is count()`)" + ) + ); + }); });