Merge pull request #3406 from quantified-uncertainty/comment-duplicat…

…ion-bug

Comment duplication bug

packages/squiggle-lang/__tests__/ast/parse_test.ts

@@ -4,6 +4,7 @@ import {
   testEvalError,
   testEvalToBe,
   testParse,
+  testParseComments,
 } from "../helpers/reducerHelpers.js";
 
 describe("Peggy parse", () => {
@@ -143,11 +144,6 @@ describe("Peggy parse", () => {
     );
   });
 
-  describe("multi-line", () => {
-    testParse("x=1; 2", "(Program (LetStatement :x 1) 2)");
-    testParse("x=1; y=2", "(Program (LetStatement :x 1) (LetStatement :y 2))");
-  });
-
   describe("variables", () => {
     testParse("x = 1", "(Program (LetStatement :x 1))");
     testParse("x", "(Program :x)");
@@ -285,6 +281,16 @@ describe("Peggy parse", () => {
     );
     testParse("/* comment * with * stars */ 1", "(Program 1)");
     testParse("/* /* */ 1", "(Program 1)");
+
+    testParseComments(
+      `foo() = {
+        foo = 5
+        // myComment
+        bar = 4
+        bar
+      }`,
+      [" myComment"]
+    );
   });
 
   describe("ternary operator", () => {
@@ -673,9 +679,40 @@ describe("Parsing new line", () => {
  `,
     "(Program (InfixCall + (Pipe (Pipe (Pipe :a :b) :c) :d) :e))"
   );
+
+  // allow new lines in `a to b` since 0.10.0
+  testParse(
+    `2
+to
+3`,
+    "(Program (InfixCall to 2 3))"
+  );
+
   describe("strings", () => {
     testParse(`"test\\"\\'\\u1234-\\b\\n"`, "(Program 'test\"'\u1234-\b\n')");
     testParse(`'test\\"\\'\\u1234-\\b\\n'`, "(Program 'test\"'\u1234-\b\n')");
     testEvalError("'\\h'");
   });
+
+  describe("statement separators", () => {
+    testParse("x=1; 2", "(Program (LetStatement :x 1) 2)");
+
+    testParse("x=1; y=2", "(Program (LetStatement :x 1) (LetStatement :y 2))");
+    testParse(
+      "x=1   ; y=2",
+      "(Program (LetStatement :x 1) (LetStatement :y 2))"
+    );
+    testParse(
+      "x=1   ;; ;y=2",
+      "(Program (LetStatement :x 1) (LetStatement :y 2))"
+    );
+
+    // whitespace before semicolon on new line allowed since 0.10.0
+    testParse(
+      `x = 1
+ ;
+y = 2`,
+      "(Program (LetStatement :x 1) (LetStatement :y 2))"
+    );
+  });
 });

packages/squiggle-lang/__tests__/ast/unit_type_check_test.ts

@@ -3,7 +3,7 @@ import {
   TypeConstraint,
   VariableUnitTypes,
 } from "../../src/analysis/unitTypeChecker.js";
-import { parse as peggyParse } from "../../src/ast/peggyParser.js";
+import { parse } from "../../src/ast/parse.js";
 
 const {
   checkTypeConstraints,
@@ -15,11 +15,19 @@ const {
 
 type IdNameMapping = string[];
 
+function _findTypeConstraints(sourceCode: string) {
+  const node = parse(sourceCode, "test");
+  if (!node.ok) {
+    throw new Error("Parse failed");
+  }
+  return findTypeConstraints(node.value);
+}
+
 function findTypeConstraintsHelper(
   sourceCode: string
 ): [TypeConstraint[], IdNameMapping] {
-  const node = peggyParse(sourceCode, { grammarSource: "test", comments: [] });
-  const [typeConstraints, scopes] = findTypeConstraints(node);
+  const [typeConstraints, scopes] = _findTypeConstraints(sourceCode);
+
   const plainConstraints = typeConstraints.map((pair) => pair[0]);
   const idNameMapping = scopes.variableNodes.map(
     (node) => (node as { value: string }).value
@@ -28,8 +36,8 @@ function findTypeConstraintsHelper(
 }
 
 function getUnitTypes(sourceCode: string): [VariableUnitTypes, IdNameMapping] {
-  const node = peggyParse(sourceCode, { grammarSource: "test", comments: [] });
-  const [typeConstraints, scopes] = findTypeConstraints(node);
+  const [typeConstraints, scopes] = _findTypeConstraints(sourceCode);
+
   const idNameMapping = scopes.variableNodes
     .filter((node) => ["Identifier", "LambdaParameter"].includes(node.kind))
     .map((node) => getIdentifierName(node));

packages/squiggle-lang/__tests__/helpers/reducerHelpers.ts

@@ -32,9 +32,18 @@ export async function evaluateStringToResult(
   }
 }
 
+const expectParseCommentsToBe = (expr: string, comments: string[]) => {
+  const result = parse(expr, "test");
+  if (result?.value && "comments" in result.value) {
+    expect(result.value.comments.map((c) => c.value)).toEqual(comments);
+  } else {
+    fail("Parsing failed or result does not contain comments");
+  }
+};
+
 const expectParseToBe = (expr: string, answer: string) => {
-  expect(astResultToString(parse(expr, "test"), { pretty: false })).toBe(
-    answer
+  expect(answer).toBe(
+    astResultToString(parse(expr, "test"), { pretty: false })
   );
 };
 
@@ -52,6 +61,9 @@ const resultToString = (
   }
 };
 
+export const testParseComments = (code: string, comments: string[]) =>
+  test(code, () => expectParseCommentsToBe(code, comments));
+
 export const testParse = (code: string, answer: string) =>
   test(code, () => expectParseToBe(code, answer));
 

packages/squiggle-lang/src/ast/parse.ts

@@ -11,7 +11,13 @@ import {
   parse as peggyParse,
   SyntaxError as PeggySyntaxError,
 } from "./peggyParser.js";
-import { AST, type ASTCommentNode, ASTNode, LocationRange } from "./types.js";
+import {
+  AST,
+  type ASTCommentNode,
+  ASTNode,
+  LocationRange,
+  ParseOptions,
+} from "./types.js";
 
 export type ParseError = {
   type: "SyntaxError";
@@ -43,16 +49,21 @@ function codeToFullLocationRange(
 
 export function parse(expr: string, sourceId: string): ASTResult {
   try {
-    const comments: ASTCommentNode[] = [];
+    const comments = new Map<string, ASTCommentNode>();
+
     const parsed: AST = peggyParse(expr, {
       grammarSource: sourceId,
-      comments,
-    });
+      addComment: (comment) => {
+        // deduplicate comments - Peggy can backtrack, so we can't collect comments in an array
+        const key = `${comment.location.start.offset}-${comment.location.end.offset}`;
+        comments.set(key, comment);
+      },
+    } satisfies ParseOptions);
     if (parsed.kind !== "Program") {
       throw new Error("Expected parse to result in a Program node");
     }
 
-    parsed.comments = comments;
+    parsed.comments = [...comments.values()];
 
     return Result.Ok(parsed);
   } catch (e) {

packages/squiggle-lang/src/ast/peggyParser.peggy

@@ -5,7 +5,7 @@
 }}
 
 start
-  = _nl start:outerBlock _nl finalComment?
+  = _nl start:outerBlock _nl lineComment? // not just `_nl`, because last line doesn't need to end in new line
     { return start; }
 
 outerBlock
@@ -24,7 +24,7 @@ outerBlock
 importStatementsList = (@importStatement __nl)*
 
 importStatement
-  = _nl 'import' __ file:string variable:(__ 'as' __ @identifier)
+  = _nl 'import' __nl file:string variable:(__nl 'as' __nl @identifier)
     { return h.nodeImport(file, variable, location()); }
 
 innerBlockOrExpression
@@ -146,7 +146,7 @@ relational
   relationalOp "operator" = '<=' / '<' / '>=' / '>'
 
 credibleInterval
-  = head:additive tail:(__ @credibleIntervalOp __nl @additive)*
+  = head:additive tail:(__nl @credibleIntervalOp __nl @additive)*
     { return h.makeInfixChain(head, tail, location()); }
   credibleIntervalOp "operator" = 'to'
 
@@ -397,36 +397,36 @@ dictConstructor 'dict'
 
 // Separators
 
+statementSeparator ';'
+  = (_ ';' / _ newLineWithPossibleComment)+ _nl
+
+commaSeparator ',' = _ ',' _nl
+
+// optional whitespace without line breaks (line breaks are still allowed inside block comments)
 _ 'whitespace'
   = whiteSpaceCharactersOrComment*
 
+// optional whitespace with possible line breaks and comments
 _nl 'whitespace'
-  = (whiteSpaceCharactersOrComment / commentOrNewLine)*
-
-__ 'whitespace'
-  = whiteSpaceCharactersOrComment+
+  = (whiteSpaceCharactersOrComment / newLineWithPossibleComment)*
 
+// required whitespace with possible line breaks and comments
 __nl 'whitespace'
-  = (whiteSpaceCharactersOrComment / commentOrNewLine)+
-
-statementSeparator ';'
-  = _ (';' / commentOrNewLine)+ _nl
-
-commaSeparator ',' = _ ',' _nl
+  = (whiteSpaceCharactersOrComment / newLineWithPossibleComment)+
 
-commentOrNewLine = finalComment? newLine
+newLineWithPossibleComment = lineComment? newLine
   newLine "newline" = [\n\r]
 
-finalComment "line comment"
-  = _ ('//') comment:($([^\r\n]*))
-    { options.comments.push(h.lineComment(comment, location())); }
+lineComment "line comment"
+  = '//' comment:($([^\r\n]*))
+    { options.addComment(h.lineComment(comment, location())); }
 
-whiteSpaceCharactersOrComment = whiteSpaceCharacters / delimitedComment
+whiteSpaceCharactersOrComment = whiteSpaceCharacters / blockComment
 
-  delimitedComment  "comment"
+  blockComment "comment"
     = '/*' comment:($(
         ([^*] / [*][^/])*
       )) '*/'
-      { options.comments.push(h.blockComment(comment, location())); }
+      { options.addComment(h.blockComment(comment, location())); }
 
   whiteSpaceCharacters = [ \t]

packages/squiggle-lang/src/ast/types.ts

@@ -26,6 +26,16 @@ export type LocationRange = {
   end: Location;
 };
 
+/*
+ * Peggy options that we expect to pass. This type exists mostly as
+ * documentation, since inline JS fragments in the parser are not checked by
+ * TypeScript.
+ */
+export type ParseOptions = {
+  grammarSource: string;
+  addComment: (comment: ASTCommentNode) => void;
+};
+
 export type InfixOperator = keyof typeof infixFunctions;
 
 export type UnaryOperator = keyof typeof unaryFunctions;