From 1ead8c71c2881b281657acf8ba5752cf87aa0f69 Mon Sep 17 00:00:00 2001 From: Michael Booth Date: Wed, 19 Feb 2025 10:50:14 -0500 Subject: [PATCH 1/2] Add a custom error message for a missing alias after 'aggregate:' --- .../syntax-errors/malloy-parser-error-listener.ts | 7 +++++++ packages/malloy/src/lang/test/syntax-errors.spec.ts | 12 ++++++++++++ 2 files changed, 19 insertions(+) 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..d14b0e354 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,13 @@ 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]], + }, ]; 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..3a263c691 100644 --- a/packages/malloy/src/lang/test/syntax-errors.spec.ts +++ b/packages/malloy/src/lang/test/syntax-errors.spec.ts @@ -68,4 +68,16 @@ 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()`)" + ) + ); + }); }); From 54829cdad3ecc492591212eeb827574c1a881c90 Mon Sep 17 00:00:00 2001 From: Michael Booth Date: Wed, 19 Feb 2025 11:18:33 -0500 Subject: [PATCH 2/2] Update error rewriter with a option --- .../syntax-errors/custom-error-messages.ts | 45 ++++++++++++++++--- .../malloy-parser-error-listener.ts | 3 +- .../src/lang/test/syntax-errors.spec.ts | 17 +++++++ 3 files changed, 59 insertions(+), 6 deletions(-) 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 d14b0e354..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 @@ -46,9 +46,10 @@ export const commonErrorCases: ErrorCase[] = [ { errorMessage: "'aggregate:' entries must include a name (ex: `some_name is count()`)", - ruleContextOptions: ['queryFieldEntry'], + // ruleContextOptions: ['queryFieldEntry'], precedingTokenOptions: [[MalloyParser.AGGREGATE]], lookAheadOptions: [[-MalloyParser.IS]], + lookbackFromOffendingSymbol: true, }, ]; diff --git a/packages/malloy/src/lang/test/syntax-errors.spec.ts b/packages/malloy/src/lang/test/syntax-errors.spec.ts index 3a263c691..e25b8a017 100644 --- a/packages/malloy/src/lang/test/syntax-errors.spec.ts +++ b/packages/malloy/src/lang/test/syntax-errors.spec.ts @@ -80,4 +80,21 @@ describe('errors', () => { ) ); }); + + 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()`)" + ) + ); + }); });