relax parse error behavior #44

hendrikvanantwerpen · 2022-02-17T15:17:19Z

Improve display of parse errors
Move parse error check to CLI
Add flag to allow parse errors
Skip tree traversal unless there are errors

Example verbose error:

Unexpected syntax on line 3 column 17:

| type U = { X: { f number } };
                  ^

Example compact error:

Unexpected syntax on line 3 column 17: f

Can be reviewed commit-by-commit

tausbn

I have a few comments and suggestions, but apart from that this looks really nice! I like the way you've moved the check for syntax errors into the main function. That'll make it a lot easier to set up custom handling of syntax errors for my purposes. 👍

tausbn · 2022-02-17T19:13:58Z

src/execution.rs

+}
+
+impl ParseErrors {
+    pub(crate) fn add(&mut self, node: tree_sitter::Node, source: &str) {


Is the intention behind passing in source for each node encountered that this makes it possible to store errors for multiple files in the same ParseErrors struct? In the current setup, source will be the same for all errors in a given file.

tausbn · 2022-02-17T21:09:32Z

src/bin/tree-sitter-graph/util.rs

+    pub(crate) fn is_empty(&self) -> bool {
+        self.errors.is_empty()
+    }
+}


It would be nice to have a way to access the errors, beyond just displaying them or checking that there are none.

The way I currently handle errors is to recognise them in a stanza, which is a bit awkward (since you can't actually query for ERROR nodes). However, it occurs to me that if I could access the list of errors, then I could just produce the relevant graph (consisting of a single "syntax error" node) directly, and do away with needlessly running the tree-sitter-graph machinery. (And in fact, I would be happy with just the first syntax error in the file.)

tausbn · 2022-02-17T21:11:22Z

src/bin/tree-sitter-graph/util.rs

+        } else {
+            None
+        };
+        self.errors.push((start.row, start.column, node_source));


For further handling, it would be nice to have also the end row/column available.
(For displaying the error, however, I don't think this is necessary.)

tausbn · 2022-02-17T21:16:36Z

src/bin/tree-sitter-graph/main.rs

@@ -67,6 +76,17 @@ fn main() -> Result<()> {
    let tree = parser
        .parse(&source, None)
        .ok_or_else(|| anyhow!("Could not parse {}", source_path.display()))?;
+    let allow_parse_errors = matches.is_present("allow-parse-errors");
+    if !allow_parse_errors {
+        let parse_errors = ParseErrors::from_tree(&tree, &source);


I wonder if it would be a bit more smooth to have from_tree return an Option<ParseError> (that could then be decomposed in an if let). That way, you wouldn't need is_empty at all.

Though of course then you have the awkwardness of Some(vec![]) being a potential value that should never appear.

Hm... Maybe it would suffice to just display the first syntax error in the file? (Which I believe will be the first error node encountered during the tree walk.) After all, after the first syntax error, the rest of the parse could be completely out of whack.

hendrikvanantwerpen · 2022-02-18T20:04:02Z

If I understand you correctly, you would also want to use this machinery yourself for finding/reporting errors.

This PR does not actually add the parse error logic to the API, it's only internal. I see that it could be useful for others as well, although that requires a bit more thought---along the lines of your comments. I think such logic should perhaps become part of the tree-sitter API, instead off this one.

On a practical note, I'm off for the next two weeks, so i won't be able to think about these things for a while. I think this PR is quite safe, since it does not change the API, so no external code that we may break later.

hendrikvanantwerpen · 2022-03-18T12:07:30Z

@tausbn I have made some changes so that the error finding and display is now part of the lib instead of the bin, so it should be reusable. There are methods for finding all or just the first error, and error display has a flag that controls whether you get nice multi-line errors, or more compact single line errors.

dcreager

Very nice. Not in scope for this PR, but I've seen recommendations for the miette if in the future we want to make the verbose error output more detailed and rustc-like.

dcreager · 2022-03-18T13:24:23Z

src/util.rs

@@ -0,0 +1,140 @@
+// -*- coding: utf-8 -*-


I would suggest a different filename — util is very generic. Maybe parse_error.rs?

That is a good idea.

hendrikvanantwerpen · 2022-03-18T16:01:47Z

Ooh, miette looks very fancy :) Definitely something to keep in mind if we touch this area again.

Comments addressed by providing a reusable module for parse errors

hendrikvanantwerpen requested a review from tausbn February 17, 2022 15:18

tausbn previously requested changes Feb 17, 2022

View reviewed changes

robrix requested a review from a team February 22, 2022 17:15

hendrikvanantwerpen force-pushed the relax-parse-error-behavior branch 2 times, most recently from fa8899a to 4b649f1 Compare March 17, 2022 19:06

hendrikvanantwerpen changed the base branch from main to shorthands March 17, 2022 19:08

dcreager approved these changes Mar 18, 2022

View reviewed changes

hendrikvanantwerpen force-pushed the shorthands branch from f5a6da2 to e2e7768 Compare March 18, 2022 15:56

Base automatically changed from shorthands to main March 18, 2022 15:57

hendrikvanantwerpen added 7 commits March 18, 2022 17:02

Improve display of parse errors

6101879

Move parse error check to CLI

d1c0032

Add flag to allow parse errors

359e7a4

Skip tree traversal unless there are errors

08e267a

Make finding and displaying errors reusable for library users

1dda16d

util -> parse_error

ee3d423

Update changelog

8065e80

hendrikvanantwerpen force-pushed the relax-parse-error-behavior branch from f26c625 to 8065e80 Compare March 18, 2022 16:12

Limit displayed parse errors to five

56dc40f

hendrikvanantwerpen requested a review from tausbn March 18, 2022 16:21

hendrikvanantwerpen merged commit 766fa6d into main Mar 18, 2022

hendrikvanantwerpen deleted the relax-parse-error-behavior branch March 18, 2022 16:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

relax parse error behavior #44

relax parse error behavior #44

hendrikvanantwerpen commented Feb 17, 2022 •

edited

tausbn left a comment

tausbn Feb 17, 2022

tausbn Feb 17, 2022

tausbn Feb 17, 2022

tausbn Feb 17, 2022

hendrikvanantwerpen commented Feb 18, 2022

hendrikvanantwerpen commented Mar 18, 2022

dcreager left a comment

dcreager Mar 18, 2022

hendrikvanantwerpen Mar 18, 2022

hendrikvanantwerpen commented Mar 18, 2022 •

edited

relax parse error behavior #44

relax parse error behavior #44

Conversation

hendrikvanantwerpen commented Feb 17, 2022 • edited

tausbn left a comment

Choose a reason for hiding this comment

tausbn Feb 17, 2022

Choose a reason for hiding this comment

tausbn Feb 17, 2022

Choose a reason for hiding this comment

tausbn Feb 17, 2022

Choose a reason for hiding this comment

tausbn Feb 17, 2022

Choose a reason for hiding this comment

hendrikvanantwerpen commented Feb 18, 2022

hendrikvanantwerpen commented Mar 18, 2022

dcreager left a comment

Choose a reason for hiding this comment

dcreager Mar 18, 2022

Choose a reason for hiding this comment

hendrikvanantwerpen Mar 18, 2022

Choose a reason for hiding this comment

hendrikvanantwerpen commented Mar 18, 2022 • edited

hendrikvanantwerpen commented Feb 17, 2022 •

edited

hendrikvanantwerpen commented Mar 18, 2022 •

edited