adhocteam / pushup

Pushup is for making modern, page-oriented web apps in Go
https://pushup.adhoc.dev
MIT License
840 stars 30 forks source link

Implement better parse error strategy #53

Closed paulsmith closed 1 year ago

paulsmith commented 1 year ago

The Pushup parser is a hand-written recursive descent parser, and its current strategy for handling syntax errors is to log them to stderr, allow parsing to continue, and only check for an error in the top-level parse() method after attempting to parse the document.

Parsing continues blindly after an error, potentially returning a nil AST node from a parse method, which is risky: we could panic from a nil deference or wind up in some other unexpected state.

A better approach might be to attempt to synchronize on the next statement/keyword. It may be slightly tricky in our case because we have both HTML and Go tokenizers that we switch between depending on context, but should be doable.

Since recursive-descent uses the host lang's call stack for its parse state, if we want to implement synchronizing we need to unwind the stack on a parse error. This may be done in other languages with exceptions. We don't have those in Go, but we could panic at the site of the syntax error and recover in the top-level parse() method, checking for a sentinel value or type so we can distinguish between syntax errors and all other regular panic conditions like slice out of bounds. Go itself does this at least here, and this blog post argues for this as a valid use-case of panic/recover as a quasi-exception system.

paulsmith commented 1 year ago

The error handling piece only was addressed by #54 but the synchronization still needs to be implemented separately.