From ae748fc7ddb94aea47f6f29754fa40455f352d6b Mon Sep 17 00:00:00 2001 From: Andrew Ray Date: Sat, 27 Jan 2024 11:14:10 -0800 Subject: [PATCH 1/3] Improving error documentation, test coverage, and adding a type - Updates readme with an errors section. - Adds a `GlslSyntaxError` type alais for peggy errors, and exports it. - Adds tests for errors and an API function test --- README.md | 68 +++++++++++++++++++++++++-- package.json | 2 +- src/error.ts | 3 ++ src/index.ts | 4 +- src/parser/parse.test.ts | 21 ++++++++- src/parser/test-helpers.ts | 4 +- src/preprocessor/preprocessor.test.ts | 34 +++++++++++++- src/preprocessor/preprocessor.ts | 4 +- 8 files changed, 128 insertions(+), 12 deletions(-) create mode 100644 src/error.ts diff --git a/README.md b/README.md index 7aca1e5..1c38ce1 100644 --- a/README.md +++ b/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 diff --git a/package.json b/package.json index 372b086..6cc1243 100644 --- a/package.json +++ b/package.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", diff --git a/src/error.ts b/src/error.ts new file mode 100644 index 0000000..d515b02 --- /dev/null +++ b/src/error.ts @@ -0,0 +1,3 @@ +import { parser } from 'peggy'; + +export type GlslSyntaxError = parser.SyntaxError; diff --git a/src/index.ts b/src/index.ts index 1943577..f8d1902 100644 --- a/src/index.ts +++ b/src/index.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 }; diff --git a/src/parser/parse.test.ts b/src/parser/parse.test.ts index ca65be4..5108551 100644 --- a/src/parser/parse.test.ts +++ b/src/parser/parse.test.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; 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; diff --git a/src/parser/test-helpers.ts b/src/parser/test-helpers.ts index 3a0396e..deb937f 100644 --- a/src/parser/test-helpers.ts +++ b/src/parser/test-helpers.ts @@ -26,7 +26,8 @@ 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, @@ -34,6 +35,7 @@ export const buildParser = () => { }; return { parse, + parser, parseSrc: ps, debugSrc: debugSrc(ctx), debugStatement: debugStatement(ctx), diff --git a/src/preprocessor/preprocessor.test.ts b/src/preprocessor/preprocessor.test.ts index 8492f13..673bf12 100644 --- a/src/preprocessor/preprocessor.test.ts +++ b/src/preprocessor/preprocessor.test.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 diff --git a/src/preprocessor/preprocessor.ts b/src/preprocessor/preprocessor.ts index 1f99134..c6b5535 100644 --- a/src/preprocessor/preprocessor.ts +++ b/src/preprocessor/preprocessor.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) { From da648a97e4c3df0ced6bd5a155229e704b4b9a9e Mon Sep 17 00:00:00 2001 From: Andrew Ray Date: Sat, 27 Jan 2024 11:19:10 -0800 Subject: [PATCH 2/3] Another substring instance --- src/preprocessor/preprocessor.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/preprocessor/preprocessor.ts b/src/preprocessor/preprocessor.ts index c6b5535..e666653 100644 --- a/src/preprocessor/preprocessor.ts +++ b/src/preprocessor/preprocessor.ts @@ -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; From a50626eee0dad2c3c6d48298a3559157c63ee496 Mon Sep 17 00:00:00 2001 From: Andrew Ray Date: Sat, 27 Jan 2024 11:25:12 -0800 Subject: [PATCH 3/3] Typescript version bump --- package-lock.json | 20 ++++++++++---------- package.json | 2 +- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/package-lock.json b/package-lock.json index 2733b4b..f377ae3 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@shaderfrog/glsl-parser", - "version": "1.4.2", + "version": "2.1.0", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "@shaderfrog/glsl-parser", - "version": "1.4.2", + "version": "2.1.0", "license": "ISC", "devDependencies": { "@babel/core": "^7.15.5", @@ -18,7 +18,7 @@ "jest": "^27.0.2", "peggy": "^1.2.0", "prettier": "^2.1.2", - "typescript": "^4.9.5" + "typescript": "^5.3.3" }, "engines": { "node": ">=16" @@ -5114,16 +5114,16 @@ } }, "node_modules/typescript": { - "version": "4.9.5", - "resolved": "https://registry.npmjs.org/typescript/-/typescript-4.9.5.tgz", - "integrity": "sha512-1FXk9E2Hm+QzZQ7z+McJiHL4NW1F2EzMu9Nq9i3zAaGqibafqYwCVU6WyWAuyQRRzOlxou8xZSyXLEN8oKj24g==", + "version": "5.3.3", + "resolved": "https://registry.npmjs.org/typescript/-/typescript-5.3.3.tgz", + "integrity": "sha512-pXWcraxM0uxAS+tN0AG/BF2TyqmHO014Z070UsJ+pFvYuRSq8KH8DmWpnbXe0pEPDHXZV3FcAbJkijJ5oNEnWw==", "dev": true, "bin": { "tsc": "bin/tsc", "tsserver": "bin/tsserver" }, "engines": { - "node": ">=4.2.0" + "node": ">=14.17" } }, "node_modules/unicode-canonical-property-names-ecmascript": { @@ -9212,9 +9212,9 @@ } }, "typescript": { - "version": "4.9.5", - "resolved": "https://registry.npmjs.org/typescript/-/typescript-4.9.5.tgz", - "integrity": "sha512-1FXk9E2Hm+QzZQ7z+McJiHL4NW1F2EzMu9Nq9i3zAaGqibafqYwCVU6WyWAuyQRRzOlxou8xZSyXLEN8oKj24g==", + "version": "5.3.3", + "resolved": "https://registry.npmjs.org/typescript/-/typescript-5.3.3.tgz", + "integrity": "sha512-pXWcraxM0uxAS+tN0AG/BF2TyqmHO014Z070UsJ+pFvYuRSq8KH8DmWpnbXe0pEPDHXZV3FcAbJkijJ5oNEnWw==", "dev": true }, "unicode-canonical-property-names-ecmascript": { diff --git a/package.json b/package.json index 6cc1243..b1cd117 100644 --- a/package.json +++ b/package.json @@ -44,6 +44,6 @@ "jest": "^27.0.2", "peggy": "^1.2.0", "prettier": "^2.1.2", - "typescript": "^4.9.5" + "typescript": "^5.3.3" } }