Add line/column information consistently to every parser message. #179

Activity

1. nickolay commented on Jun 2, 2020

   

   nickolay

   on Jun 2, 2020

   Contributor

   More actions

   I agree with this goal, and location-aware errors from sqlparser is part of the larger story of good error messages (the other part is keeping track of position in the happy case, so that semantic errors can be reported with a specific location as well - https://github.com/andygrove/sqlparser-rs/issues/161#issuecomment-633619241)

   Note that currently the tokenizer and the parser do two independent passes (with the tokenizer returning a Result<Vec<Token>, TokenizerError>), and only the tokenizer keeps track of the current location.

   I think there are two tasks here:

   • Changing TokenizerError to store the line/column information and introducing a helper to instantiate it based on tokenizer's current position - this would indeed be a "good first issue".
   • Making ParserError report the position (possibly removing the distinction between parser/tokenizer errors) would be more involved.
2. nickolay

   added

   good first issueGood for newcomersGood for newcomers

   on Jun 2, 2020
3. Dandandan commented on Jun 2, 2020

   

   Dandandan

   on Jun 2, 2020

   ContributorAuthor

   More actions

   Sounds great! The first indeed is the more low hanging fruit I meant with this story (TokenizerError). The second one is really valuable but probably requires much more changes.
4. nickolay

   added a commit that references this issue on Jun 10, 2020

   Add line and column number to TokenizerError (#194)

   ...

   d842f49
5. nickolay

   added

   help wantedExtra attention is neededExtra attention is needed

   and removed

   good first issueGood for newcomersGood for newcomers

   on Jun 10, 2020
6. Dandandan commented on Jul 12, 2020

   

   Dandandan

   on Jul 12, 2020

   ContributorAuthor

   More actions

   @nickolay I saw that in the fork of Materialize column / line indexes are supported.
   For that, the following is done:

   • add range to result of tokenizer https://github.com/MaterializeInc/materialize/blob/main/src/sql-parser/src/tokenizer.rs#L328
   • Add range to parse error
   • When showing an error, the line / columns is found and a nice error is displayed https://github.com/MaterializeInc/materialize/blob/main/src/sql-parser/src/parser.rs#L90

   I think this should be relatively simply to implement on the current version, compared to the lossless syntax (although it doesn't address the comment handling, lossless parsing). What do you think?
7. nickolay commented on Jul 14, 2020

   

   nickolay

   on Jul 14, 2020

   Contributor

   More actions

   The source range (or rather the corresponding string) is needed for the Lossless Syntax Tree as well. I used token.to_string() as a workaround, but adding proper infrastructure would certainly be helpful!

   Since the LST needs a data-less enum SyntaxKind to tag its nodes (both the leafs - i.e. tokens - and intermediate ones), I planned on changing Token to store only this tag and a slice of the source text, i.e. make Token = (SyntaxKind, SomeSortOfString). The start position of each token could then be calculated by adding up the lengths of slices for all the preceding tokens. Any processing of the token's text (e.g. reading '\\' as a one-character-long string literal) would have to be moved to the parser.

   Materialize instead returns (Token, Range) tuple from the tokenizer, sometimes calling such tuple a token, which I think is a bit confusing.