Merge pull request #13 from ShaderFrog/error-improvements

Improving error documentation, test coverage, and adding a type

Author: AndrewRayCode

README.md

@@ -47,10 +47,11 @@ type ParserOptions = {
   // like undefined functions and variables. If `failOnWarn` is set to true,
   // warnings will still cause the parser to raise an error. Defaults to false.
   quiet: boolean;
-  // The origin of the GLSL, for debugging. For example, "main.js", If the
-  // parser raises an error (specifically a GrammarError), and you call
-  // error.format([]) on it, the error shows { source: 'main.js', ... }.
-  // Defaults to null.
+  // An optional string reprsenting the origin of the GLSL, for debugging and
+  // error messages. For example, "main.js". If the parser raises an error, the
+  // grammarSource shows up in the error.source field. If you format the error
+  // (see the errors section), the grammarSource shows up in the formatted error
+  // string. Defaults to undefined.
   grammarSource: string;
   // If true, sets location information on each AST node, in the form of
   // { column: number, line: number, offset: number }. Defaults to false.
@@ -207,6 +208,65 @@ matches based on the name and arity of the functions.
 
 See also [#Utility-Functions] for renaming scope references.
 
+## Errors
+
+If you have invalid GLSL, the parser throws a `GlslSyntaxError`, which is a type
+alias for `peggy.SyntaxError`.
+
+```ts
+import { parser, GlslSyntaxError } from '@shaderfrog/glsl-parser';
+
+let error: GlslSyntaxError | undefined;
+try {
+  // Line without a semicolon
+  c.parse(`float a`);
+} catch (e) {
+  error = e as GlslSyntaxError;
+}
+```
+
+The error class lives on the parser object itself:
+```ts
+console.log(error instanceof parser.SyntaxError)
+// true
+```
+
+The error message is automatically generated by Peggy:
+```ts
+console.log(error.message)
+// 'Expected ",", ";", "=", or array specifier but end of input found'
+```
+
+It includes the location of the error. Note `source` is the `grammarSource`
+string provided to the parser options, which is `undefined` by default.
+```ts
+console.log(error.location)
+/*
+{
+  source: undefined,
+  start: { offset: 7, line: 1, column: 8 },
+  end: { offset: 7, line: 1, column: 8 }
+}
+*/
+```
+
+The standard Peggy error object also has a fairly confusing `format()` method,
+which produces an ASCII formatted string with arrows and underlines. The
+`source` option passed to `.format()` **must** match your `grammarSource` in
+parser options (which is `undefined` by default). This API is awkward and I
+might override it in future versions of the parser.
+
+```ts
+console.log(error.format([{ text, source: undefined }])
+/*
+Error: Expected ",", ";", "=", or array specifier but "f" found.
+  --> undefined:2:1
+  |
+2 | float b
+  | ^
+*/
+```
+
 ## Manipulating and Searching ASTs
 
 ### Visitors

package-lock.json

@@ -3,7 +3,7 @@
   "engines": {
     "node": ">=16"
   },
-  "version": "2.0.1",
+  "version": "2.1.0",
   "description": "A GLSL ES 1.0 and 3.0 parser and preprocessor that can preserve whitespace and comments",
   "scripts": {
     "prepare": "npm run build && ./prepublish.sh",
@@ -44,6 +44,6 @@
     "jest": "^27.0.2",
     "peggy": "^1.2.0",
     "prettier": "^2.1.2",
-    "typescript": "^4.9.5"
+    "typescript": "^5.3.3"
   }
 }

package.json

@@ -0,0 +1,3 @@
+import { parser } from 'peggy';
+
+export type GlslSyntaxError = parser.SyntaxError;

src/error.ts

@@ -1,4 +1,6 @@
 import generate from './parser/generator';
 import parser from './parser/parser';
 
-export = { generate, parser };
+export type * from './error';
+
+export { generate, parser };

src/index.ts

@@ -1,9 +1,26 @@
-import { AstNode, FunctionCallNode, TypeSpecifierNode, visit } from '../ast';
-import { buildParser, inspect } from './test-helpers';
+import { FunctionCallNode, visit } from '../ast';
+import { GlslSyntaxError } from '../error';
+import { buildParser } from './test-helpers';
 
 let c!: ReturnType<typeof buildParser>;
 beforeAll(() => (c = buildParser()));
 
+test('parse error', () => {
+  let error: GlslSyntaxError | undefined;
+  // Missing a semicolon
+  const text = `float a
+float b`;
+  try {
+    c.parse(text);
+  } catch (e) {
+    error = e as GlslSyntaxError;
+  }
+
+  expect(error).toBeInstanceOf(c.parser.SyntaxError);
+  expect(error!.location.start.line).toBe(2);
+  expect(error!.location.end.line).toBe(2);
+});
+
 test('declarations', () => {
   c.expectParsedProgram(`
     float a, b = 1.0, c = a;

src/parser/parse.test.ts

@@ -26,14 +26,16 @@ export const buildParser = () => {
   execSync(
     'npx peggy --cache -o src/parser/parser.js src/parser/glsl-grammar.pegjs'
   );
-  const parse = require('./parser').parse as Parse;
+  const parser = require('./parser');
+  const parse = parser.parse as Parse;
   const ps = parseSrc(parse);
   const ctx: Context = {
     parse,
     parseSrc: ps,
   };
   return {
     parse,
+    parser,
     parseSrc: ps,
     debugSrc: debugSrc(ctx),
     debugStatement: debugStatement(ctx),

src/parser/test-helpers.ts

@@ -1,5 +1,4 @@
 import fs from 'fs';
-import path from 'path';
 import peggy from 'peggy';
 import util from 'util';
 import {
@@ -8,6 +7,7 @@ import {
   PreprocessorProgram,
 } from './preprocessor';
 import generate from './generator';
+import { GlslSyntaxError } from '../error';
 
 const fileContents = (filePath: string): string =>
   fs.readFileSync(filePath).toString();
@@ -37,6 +37,38 @@ const expectParsedProgram = (sourceGlsl: string) => {
 //   expectParsedProgram(fileContents('./preprocess-test-grammar.glsl'));
 // });
 
+test('#preprocessComments', () => {
+  // Should strip comments and replace single-line comments with a single space
+  expect(
+    preprocessComments(`// ccc
+/* cc */aaa/* cc */
+/**
+ * cccc
+ */
+bbb
+`)
+  ).toBe(`
+ aaa 
+
+
+
+bbb
+`);
+});
+
+test('preprocessor error', () => {
+  let error: GlslSyntaxError | undefined;
+  try {
+    parse(`#if defined(#)`);
+  } catch (e) {
+    error = e as GlslSyntaxError;
+  }
+
+  expect(error).toBeInstanceOf(parser.SyntaxError);
+  expect(error!.location.start.line).toBe(1);
+  expect(error!.location.end.line).toBe(1);
+});
+
 test('preprocessor ast', () => {
   expectParsedProgram(`
 #line 0

src/preprocessor/preprocessor.test.ts

@@ -77,8 +77,8 @@ const preprocessComments = (src: string): string => {
   let in_multi = 0;
 
   for (i = 0; i < src.length; i++) {
-    chr = src.substr(i, 1);
-    la = src.substr(i + 1, 1);
+    chr = src.substring(i, i + 1);
+    la = src.substring(i + 1, i + 2);
 
     // Enter single line comment
     if (chr == '/' && la == '/' && !in_single && !in_multi) {
@@ -225,16 +225,16 @@ const expandFunctionMacro = (
 
     // Replace the use of the macro with the expansion
     const processed = current.replace(
-      current.substr(startMatch.index, matchLength),
+      current.substring(startMatch.index, startMatch.index + matchLength),
       expandedReplace
     );
     // Add text up to the end of the expanded macro to what we've procssed
-    expanded += processed.substr(0, endOfReplace);
+    expanded += processed.substring(0, endOfReplace);
 
     // Only work on the rest of the text, not what we already expanded. This is
     // to avoid a nested macro #define foo() foo() where we'll try to expand foo
     // forever. With this strategy, we expand foo() to foo() and move on
-    current = processed.substr(endOfReplace);
+    current = processed.substring(endOfReplace);
   }
 
   return expanded + current;