Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nicer parse errors using miette #618

Merged
merged 1 commit into from
Jan 5, 2025
Merged

Conversation

Samyak2
Copy link
Contributor

@Samyak2 Samyak2 commented Jan 5, 2025

I noticed that the parse errors were a bit hard to read - only the nearest token and the line/col offsets were printed.

I made a first attempt at improving the errors using miette.

  • Added derive for miette::Diagnostic to both the parser's error type and LimboError.
  • Added miette dependency to both sqlite3_parser and core. The fancy feature is only enabled for the CLI. So the overhead on the libraries (core, parser) should be minimal.

Some future improvements that can be made further:

  • Add spans to AST nodes so that errors can better point to the correct token. See upstream issue: Spanned AST nodes gwenn/lemon-rs#33
  • Construct more errors with offset information. I noticed that most parser errors are constructed with None as the offset.
  • The messages are a bit redundant (example "syntax error at (1, 6)"). This can improved.

Comparisons.
Before:

❯ cargo run --package limbo --bin limbo database.db --output-mode pretty
...
limbo> selet * from a;
[2025-01-05T11:22:55Z ERROR sqlite3Parser] near "Token([115, 101, 108, 101, 116])": syntax error
Parse error: near "selet": syntax error at (1, 6)
image

After:

❯ cargo run --package limbo --bin limbo database.db --output-mode pretty
...
limbo> selet * from a;
[2025-01-05T12:25:52Z ERROR sqlite3Parser] near "Token([115, 101, 108, 101, 116])": syntax error

  × near "selet": syntax error at (1, 6)
   ╭────
 1 │ selet * from a
   ·     ▲
   ·     ╰── syntax error
   ╰────

image

I noticed that the parse errors were a bit hard to read - only the nearest token and the line/col offsets were printed.

I made a first attempt at improving the errors using [miette](https://github.com/zkat/miette).
- Added derive for `miette::Diagnostic` to both the parser's error type and LimboError.
- Added miette dependency to both sqlite3_parser and core. The `fancy` feature is only enabled for CLI.

Some future improvements that can be made further:
- Add spans to AST nodes so that errors can better point to the correct token. See upstream issue: gwenn/lemon-rs#33
- Construct more errors with offset information. I noticed that most parser errors are constructed with `None` as the offset.

Comparisons.
Before:
```
❯ cargo run --package limbo --bin limbo database.db --output-mode pretty
...
limbo> selet * from a;
[2025-01-05T11:22:55Z ERROR sqlite3Parser] near "Token([115, 101, 108, 101, 116])": syntax error
Parse error: near "selet": syntax error at (1, 6)
```

After:
```
❯ cargo run --package limbo --bin limbo database.db --output-mode pretty
...
limbo> selet * from a;
[2025-01-05T12:25:52Z ERROR sqlite3Parser] near "Token([115, 101, 108, 101, 116])": syntax error

  × near "selet": syntax error at (1, 6)
   ╭────
 1 │ selet * from a
   ·     ▲
   ·     ╰── syntax error
   ╰────

```
@Samyak2 Samyak2 marked this pull request as ready for review January 5, 2025 12:28
@penberg
Copy link
Collaborator

penberg commented Jan 5, 2025

Btw, we should suppress that error logging from the parser to make it even nicer.

@penberg penberg merged commit 2cd3c47 into tursodatabase:main Jan 5, 2025
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants