Add a custom error message for a missing alias after 'aggregate:' (#2144

) * Add a custom error message for a missing alias after 'aggregate:' * Update error rewriter with a "lookbackFromOffendingSymbol" option
malloydata · Feb 20, 2025 · 79ac867 · 79ac867
1 parent 30123a6
commit 79ac867
Show file tree

Hide file tree

Showing 3 changed files with 77 additions and 5 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
@@ -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,15 +62,31 @@ 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
         }
       }
       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
@@ -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<Token> {

diff --git 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()`)"
+      )
+    );
+  });
 });