From d01c1561d437a3de2be04827844fbd63ce8ece70 Mon Sep 17 00:00:00 2001
From: Kevin Gibbons <bakkot@gmail.com>
Date: Wed, 31 Aug 2022 10:08:31 -0700
Subject: [PATCH] expr-parser: skip tags in records

---
 src/expr-parser.ts  | 61 ++++++++++++++++++++++++++++++++++++++-------
 test/expr-parser.js | 13 +++++++++-
 2 files changed, 64 insertions(+), 10 deletions(-)

diff --git a/src/expr-parser.ts b/src/expr-parser.ts
index a8f5a16a..0d42a407 100644
--- a/src/expr-parser.ts
+++ b/src/expr-parser.ts
@@ -206,8 +206,14 @@ class ExprParser {
               },
             };
       const match = tok.name === 'text' ? tok.contents.match(tokMatcher) : null;
+      // the `tok.name !== 'text'` part in the test below is redundant but makes TS happier
       if (tok.name !== 'text' || match == null) {
-        if (!(tok.name === 'text' && tok.contents.length === 0)) {
+        const empty =
+          (tok.name === 'text' && tok.contents.length === 0) ||
+          tok.name === 'tag' ||
+          tok.name === 'opaqueTag' ||
+          tok.name === 'comment';
+        if (!empty) {
           currentProse.push(tok);
         }
         ++this.srcIndex;
@@ -241,6 +247,36 @@ class ExprParser {
     });
   }
 
+  // returns true if this ate a newline
+  private eatWhitespace(): boolean {
+    let next;
+    let hadNewline = false;
+    while ((next = this.peek())?.type === 'prose') {
+      const firstNonWhitespaceIdx = next.parts.findIndex(
+        part => part.name !== 'text' || /\S/.test(part.contents)
+      );
+      if (firstNonWhitespaceIdx === -1) {
+        hadNewline ||= next.parts.some(
+          part => part.name === 'text' && part.contents.includes('\n')
+        );
+        this.next.shift();
+      } else if (firstNonWhitespaceIdx === 0) {
+        return hadNewline;
+      } else {
+        hadNewline ||= next.parts.some(
+          (part, i) =>
+            i < firstNonWhitespaceIdx && part.name === 'text' && part.contents.includes('\n')
+        );
+        this.next[0] = {
+          type: 'prose',
+          parts: next.parts.slice(firstNonWhitespaceIdx),
+        };
+        return hadNewline;
+      }
+    }
+    return hadNewline;
+  }
+
   // guarantees the next token is an element of close
   parseSeq(close: CloseTokenType[]): Seq {
     const items: NonSeq[] = [];
@@ -421,7 +457,20 @@ class ExprParser {
           let type: 'record' | 'record-spec' | null = null;
           const members: ({ name: string; value: Seq } | { name: string })[] = [];
           while (true) {
+            const hadNewline = this.eatWhitespace();
             const nextTok = this.peek();
+            if (nextTok.type === 'crec') {
+              if (!hadNewline) {
+                // ideally this would be a lint failure, or better yet a formatting thing, but whatever
+                throw new ParseFailure(
+                  members.length > 0
+                    ? 'trailing commas are only allowed when followed by a newline'
+                    : 'records cannot be empty',
+                  nextTok.offset
+                );
+              }
+              break;
+            }
             if (nextTok.type !== 'prose') {
               throw new ParseFailure('expected to find record field name', nextTok.offset);
             }
@@ -434,14 +483,6 @@ class ExprParser {
             const { contents } = nextTok.parts[0];
             const nameMatch = contents.match(/^\s*\[\[(?<name>\w+)\]\]\s*(?<colon>:?)/);
             if (nameMatch == null) {
-              if (members.length > 0 && /^\s*$/.test(contents) && contents.includes('\n')) {
-                // allow trailing commas when followed by a newline
-                this.next.shift(); // eat the whitespace
-                if (this.peek().type === 'crec') {
-                  this.next.shift();
-                  break;
-                }
-              }
               throw new ParseFailure(
                 'expected to find record field',
                 nextTok.parts[0].location.start.offset + contents.match(/^\s*/)![0].length
@@ -581,6 +622,8 @@ class ExprParser {
   }
 }
 
+// Note: this does not necessarily represent the entire input
+// in particular it may omit some whitespace, tags, and comments
 export function parse(src: FragmentNode[], opNames: Set<String>): Seq | Failure {
   const parser = new ExprParser(src, opNames);
   try {
diff --git a/test/expr-parser.js b/test/expr-parser.js
index 56aa8d57..62c68dc0 100644
--- a/test/expr-parser.js
+++ b/test/expr-parser.js
@@ -79,6 +79,17 @@ describe('expression parsing', () => {
         </emu-alg>
       `);
     });
+
+    it('tags in record', async () => {
+      await assertLintFree(`
+        <emu-alg>
+        1. Let _x_ be a new Record { [[Foo]]: 0, <!-- comment --> [[Bar]]: 1 }.
+        1. Let _x_ be a new Record { [[Foo]]: 0, <ins>[[Bar]]: 1</ins> }.
+        1. Let _x_ be a new Record { [[Foo]]: 0, <ins>[[Bar]]: 1, [[Baz]]: 2</ins> }.
+        1. Let _x_ be a new Record { [[Foo]]: 0, <ins>[[Bar]]: 1,</ins> [[Baz]]: 2 }.
+        </emu-alg>
+      `);
+    });
   });
 
   describe('errors', () => {
@@ -193,7 +204,7 @@ describe('expression parsing', () => {
         {
           ruleId: 'expression-parsing',
           nodeType: 'emu-alg',
-          message: 'expected to find record field',
+          message: 'records cannot be empty',
         }
       );
     });