Initial API for `Scanner`. #12

almann · 2021-05-10T05:33:45Z

Provides an interface for lexing PartiQL. Traditionally we would
factor this as the low-level interface for the parser, but with the PEG
based implementation, we just surface the relevant parts of the parser
as a rule to parse with.

This commit helps explore how to effectively integrate with the somewhat
low-level APIs for parsing that Pest provides.

Notes:

Adds scanner module.
Adds Scanner trait to the prelude.
Makes PartiQLParser public to crate.
Refactors LineAndColumn as a tuple for Position::At.
- Adds this to the prelude.
Changes entry point for PEG to Query and added Scanner as the
entry point rules for implementing the Scanner API.
Adds PairsExt/PairExt trait/impl to add utility methods for working
with Pest Pairs/Pair.
Adds LineAndColumn::move_relative and cleans up some doc/doc tests.

Pest links for reference:

https://pest.rs/book/parser_api.html - API description
https://docs.rs/pest/2.1.3/pest/index.html - API Reference

This PR is dependent on #8 and as such is targeting a staging branch but I did not want to prevent review.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov · 2021-05-10T05:39:02Z

Codecov Report

Merging #12 (ba4398c) into main (d748ebd) will decrease coverage by 1.06%.
The diff coverage is 91.28%.

@@            Coverage Diff             @@
##             main      #12      +/-   ##
==========================================
- Coverage   91.37%   90.31%   -1.07%     
==========================================
  Files           6        7       +1     
  Lines          58      258     +200     
==========================================
+ Hits           53      233     +180     
- Misses          5       25      +20

Impacted Files	Coverage Δ
partiql-parser/src/lib.rs	`100.00% <ø> (ø)`
partiql-parser/src/result.rs	`82.94% <81.81%> (-6.42%)`	⬇️
partiql-parser/src/scanner.rs	`97.45% <97.45%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d748ebd...ba4398c. Read the comment docs.

partiql-parser/src/partiql.pest

almann · 2021-05-10T05:38:52Z

partiql-parser/src/peg.rs

+/// Extension methods for working with [`Pairs`].
+pub(crate) trait PairsExt<'val, R: RuleType> {
+    /// Consumes a [`Pairs`] as a singleton, returning an error if there are less or more than
+    /// one [`Pair`].
+    fn exactly_one(self) -> ParserResult<Pair<'val, R>>;
+}
+
+impl<'val, R: RuleType> PairsExt<'val, R> for Pairs<'val, R> {
+    fn exactly_one(mut self) -> ParserResult<Pair<'val, R>> {
+        match self.next() {
+            Some(pair) => {
+                // make sure there isn't something more...
+                if let Some(other_pair) = self.next() {
+                    syntax_error(
+                        format!("Expected one token pair, got: {:?}, {:?}", pair, other_pair),
+                        pair.start_loc().into(),
+                    )?;
+                }
+                Ok(pair)
+            }
+            None => syntax_error(
+                "Expected at one token pair, got nothing!",
+                Position::Unknown,
+            ),
+        }
+    }
+}


A successful parse from Pest returns Pairs which is an iterator of Pair which itself is a terrible name for a rule that matched some span of text (I believe the term "pair" alluding to the start and end of a match).

A rule match returns zero or more Pair, this API says, return an error unless it is exactly one Pair.

almann · 2021-05-10T05:39:43Z

partiql-parser/src/peg.rs

+pub(crate) trait PairExt<'val, R: RuleType> {
+    /// Translates the start position of the [`Pair`] into a [`LineAndColumn`].
+    fn start_loc(&self) -> LineAndColumn;
+
+    /// Translates the end position of the [`Pair`] into a [`LineAndColumn`].
+    fn end_loc(&self) -> LineAndColumn;
+}
+
+impl<'val, R: RuleType> PairExt<'val, R> for Pair<'val, R> {
+    #[inline]
+    fn start_loc(&self) -> LineAndColumn {
+        self.as_span().start_pos().line_col().into()
+    }
+
+    #[inline]
+    fn end_loc(&self) -> LineAndColumn {
+        self.as_span().end_pos().line_col().into()
+    }
+}


Translates the baroque Pair/Span to start/end line positioning.

almann · 2021-05-10T05:39:56Z

partiql-parser/src/peg.rs

@@ -18,7 +69,7 @@ struct PartiQLParser;
 ///
 /// This API will be replaced with one that produces an AST in the future.
 pub fn recognize_partiql(input: &str) -> ParserResult<()> {
-    PartiQLParser::parse(Rule::Keywords, input)?;
+    PartiQLParser::parse(Rule::Query, input)?;


Better entry point name.

almann · 2021-05-10T05:41:42Z

partiql-parser/src/result.rs

+    /// Returns a [`LineAndColumn`] that moves to the given position relative
+    /// to this one as a sort of "origin."
+    ///
+    /// Note that this positioning is 1-based, so moving `(1, 1)` to `(1, 1)` is a no-op.
+    ///
+    /// ## Examples
+    /// ```
+    /// # use partiql_parser::prelude::*;
+    /// assert_eq!(
+    ///     LineAndColumn::at(1, 1),
+    ///     LineAndColumn::at(1, 1).move_relative(LineAndColumn::at(1, 1))
+    /// );
+    /// ```
+    ///
+    /// ```
+    /// # use partiql_parser::prelude::*;
+    /// assert_eq!(
+    ///     LineAndColumn::at(1, 2),
+    ///     LineAndColumn::at(1, 1).move_relative(LineAndColumn::at(1, 2))
+    /// );
+    /// ```
+    ///
+    /// ```
+    /// # use partiql_parser::prelude::*;
+    /// assert_eq!(
+    ///     LineAndColumn::at(5, 10),
+    ///     LineAndColumn::at(5, 7).move_relative(LineAndColumn::at(1, 4))
+    /// );
+    /// ```
+    ///
+    /// ```
+    /// # use partiql_parser::prelude::*;
+    /// assert_eq!(
+    ///     LineAndColumn::at(21, 2),
+    ///     LineAndColumn::at(2, 15).move_relative(LineAndColumn::at(20, 2))
+    /// );
+    /// ```
+    pub fn move_relative(self, location: LineAndColumn) -> Self {


This API is needed so we can use the PEG "continuation" style. Call in, remember where we left off, and re-translate the parsers line/column positioning because every time we call into the PEG, it thinks we're starting at (1, 1).

almann · 2021-05-10T05:42:30Z

partiql-parser/src/result.rs

+impl From<(usize, usize)> for LineAndColumn {
+    fn from(line_and_column: (usize, usize)) -> Self {
+        let (line, column) = line_and_column;
+        Self::at(line, column)
+    }
+}


This makes it so we can convert a raw tuple into our typed one by just saying something like (x, y).into().

Should this panic/fail if either value is 0? Same question for LineAndColumn::at.

It is a good question, I am really gun shy around panics, but using an error hear also feels like overkill, I think the answer is probably error, but this is pretty close to where I'd use an assert in C.

almann · 2021-05-10T05:43:14Z

partiql-parser/src/result.rs

    Unknown,
-    At { line: usize, column: usize },
+    /// Variant indicating that there *is* a known location in source for some context.
+    At(LineAndColumn),


This is split out of the enum, because we have contexts where there is always a LineAndColumn and other contexts where there may not be.

almann · 2021-05-10T05:44:52Z

partiql-parser/src/scanner.rs

+        let start_location = self.remainder.offset.move_relative(pair.start_loc());
+        let end_location = self.remainder.offset.move_relative(pair.end_loc());


Notice here we translate the parser which has a logical line/column from its start, to where we left off.

almann · 2021-05-10T05:46:32Z

partiql-parser/src/scanner.rs

+                    // make sure to translate line/column position from where we started
+                    Position::At(location) => Position::At(start_loc.move_relative(location)),


Here, we translate the line information from our continuation.

almann · 2021-05-10T17:08:39Z

I added a doc test to illustrate usage of the Scanner API, here it is rendered as an image

dlurton · 2021-05-10T17:36:05Z

partiql-parser/src/scanner.rs

+    start_location: LineAndColumn,
+    /// End location for this token.
+    end_location: LineAndColumn,


Should these be in a struct by themselves?

struct Span { /// Start location for this token. start_location: LineAndColumn, /// End location for this token. end_location: LineAndColumn }

??

They could be--though the question is to what end in the API? Do you want to see Token::span() -> Span

Added #14 to track this.

Token::span() -> Span is the idea.

partiql-parser/src/lib.rs

marcbowes · 2021-05-14T02:34:15Z

partiql-parser/src/lib.rs

+//!     }
+//!     // the entire text of a token can be fetched--which looks the roughly the
+//!     // same for a keyword.
+//!     assert_eq!("SELECT", first.text());


was expecting as_str() here

Interesting, we have a similar API in SymbolToken that is like this, do you think we should make that as_str()? I only ask, because I do think there is an aesthetics question here. I want to emphasize that the &str being returned is the text of the token, maybe that is easily just as_str(), but I hadn't thought of it as a conversion though, which is what I think as_nnn tends to mean.

SymbolToken can not have a str, right ($0)? so maybe that's an important difference - that text() might not be meaningful. Whereas here it seems obvious that the keyword can be "viewed as a str".

I see your pov. though, and I agree it makes sense.

FWIW, not that this is canonical or anything, but I've been more or less inspired by:

https://rust-lang.github.io/api-guidelines/naming.html

I think what I have here is more like a getter versus a conversion:

https://rust-lang.github.io/api-guidelines/naming.html#getter-names-follow-rust-convention-c-getter

The difference between getters and conversions (C-CONV) can be subtle and is not always clear-cut. For example TempDir::path can be understood as a getter for the filesystem path of the temporary directory, while TempDir::into_path is a conversion that transfers responsibility for deleting the temporary directory to the caller. Since path is a getter, it would not be correct to call it get_path or as_path.

partiql-parser/src/partiql.pest

marcbowes · 2021-05-14T02:35:34Z

partiql-parser/src/peg.rs

+            }
+            None => syntax_error(
+                "Expected at one token pair, got nothing!",
+                Position::Unknown,


FIXME for what? This is the case where the Pairs iterator type is empty, are you saying that it should have a Position? If so, I might need to check the APIs for this.

sorry that was a lame comment!

is it always the case there is no position? I guess it could be none because you're at EOF or something..?

I think the only way this could happen is if I wrote a Pest grammar with a rule that matched something but it and all sub-rules were silent and did not produce a Pair at all. This would mean that I as the rule writer wrote something pretty insane and I think it is almost certainly a bug.

Add this to the reason why I think Pest is a little too low-level for my tastes as far as its APIs are concerned (though I think logically consistent).

marcbowes · 2021-05-14T02:38:04Z

partiql-parser/src/result.rs

+    /// Returns a [`LineAndColumn`] that moves to the given position relative
+    /// to this one as a sort of "origin."
+    ///
+    /// Note that this positioning is 1-based, so moving `(1, 1)` to `(1, 1)` is a no-op.


Why is the move 1-based? "Move 1 line" not moving 1 line seems weird

Maybe the name is not right--this isn't taking a delta, change location to use self as the origin. I am not sure what I should call that, but move_relative is not right--let me think about that.

partiql-parser/src/result.rs

zslayton · 2021-05-14T17:38:37Z

partiql-parser/src/result.rs

+impl From<(usize, usize)> for LineAndColumn {
+    fn from(line_and_column: (usize, usize)) -> Self {
+        let (line, column) = line_and_column;
+        Self::at(line, column)
+    }
+}


Should this panic/fail if either value is 0? Same question for LineAndColumn::at.

zslayton · 2021-05-14T17:40:58Z

partiql-parser/src/result.rs

+                if diff_line > 0 {
+                    // we're moving lines, adjust the line and take the target column as-is
+                    LineAndColumn::at(base_line + diff_line, dest_column)
+                } else {
+                    // same line from base, adjust only the column
+                    let diff_column = dest_column - 1;
+                    LineAndColumn::at(base_line, base_column + diff_column)


Why do we only adjust the column if we're staying on the same line? If we're treating the base position as the new origin, wouldn't the dest_column always be relative to the base_column?

EDIT: Having read ahead to Remainder, I now see what this is driving at. I think the content of these code comments should be promoted to / reproduced in the doc comment to call out that this is the expected behavior. The last doc comment example shows this, but it's easy to miss and I'd worry that other people would read the word "origin" the way that I did.

Promoted these comments into the doc tests.

partiql-parser/src/scanner.rs

zslayton · 2021-05-14T18:07:56Z

partiql-parser/src/scanner.rs

+    }
+
+    /// Returns the remainder of the input after this token.
+    pub fn rem(&self) -> &str {


I'd prefer to spell this out if you don't object.

Suggested change

pub fn rem(&self) -> &str {

pub fn remainder(&self) -> &str {

(CTRL+F searching shows one usage of it elsewhere in the diff.) If/when #14 is added, I think we can simplify start_loc and end_loc to start and end.

Should this function return a copy of self.remainder instead of just a &str?

It is an interesting question--I wasn't really planning on exposing Remainder as a public API. How about

text_after(&self) -> &str

?

Also, I will rename the various start_loc and end_loc to just start/end.

almann · 2021-05-19T18:39:12Z

partiql-parser/src/peg.rs

+            None => syntax_error(
+                "Expected at one token pair, got nothing!",
+                Position::Unknown,
+            ),


This is probably better modeled as illegal state.

almann · 2021-05-19T18:40:14Z

partiql-parser/src/peg.rs

 #[derive(Parser)]
 #[grammar = "partiql.pest"]
-struct PartiQLParser;
+pub(crate) struct PartiQLParser;


Might want to consider excluding this from coverage metrics.

Already done as #22

Provides an interface for _lexing_ PartiQL. Traditionally we would factor this as the low-level interface for the parser, but with the PEG based implementation, we just surface the relevant parts of the parser as a rule to parse with. This commit helps explore how to effectively integrate with the somewhat low-level APIs for parsing that Pest provides. * Adds `scanner` module. * Adds `Scanner` trait to the prelude. * Makes `PartiQLParser` public to crate. * Refactors `LineAndColumn` as a tuple for `Position::At`. - Adds this to the `prelude`. * Changes entry point for PEG to `Query` and added `Scanner` as the entry point rules for implementing the `Scanner` API. * Adds `PairsExt`/`PairExt` trait/impl to add utility methods for working with Pest `Pairs`/`Pair`. * Adds `LineAndColumn::position_from` and cleans up some doc/doc tests. Resolves partiql#13.

Co-authored-by: Zack Slayton <zack.slayton@gmail.com>

* Renames `start_loc`/`end_loc` to just `start`/`end`. * Renames `start_location`\`end_location` similarly. * Adds doc comments about how `position_from` works.

The rationale is that even though there are some aspects of this that we could assert as internal logic problems, we rely on external APIs to construct this location and we should not panic as a result of those. Refactors relevant APIs/tests to use this new return result. Adds `TryFrom` and `TryInto` into the prelude. This also cleans up field access for the `LineAndColumn` struct.

almann requested review from marcbowes, dlurton, abhikuhikar and alancai98 May 10, 2021 05:33

almann commented May 10, 2021

View reviewed changes

almann mentioned this pull request May 10, 2021

Implement a Scanner Trait #13

Closed

almann linked an issue May 10, 2021 that may be closed by this pull request

Implement a Scanner Trait #13

Closed

almann force-pushed the scanner branch from f09ebff to 109eae7 Compare May 10, 2021 15:30

dlurton reviewed May 10, 2021

View reviewed changes

This was referenced May 10, 2021

Consider adding a Span API #14

Closed

Adds support for string literals and identifiers in Scanner. #15

Merged

marcbowes approved these changes May 14, 2021

View reviewed changes

almann mentioned this pull request May 14, 2021

Adds initial PEG, basic recognizer API, and Error/Result APIs. #8

Merged

almann changed the base branch from scanner-base to main May 14, 2021 16:20

almann force-pushed the scanner branch from 265685e to 4e47ac3 Compare May 14, 2021 19:17

zslayton reviewed May 14, 2021

View reviewed changes

almann force-pushed the scanner branch from 4e47ac3 to 265685e Compare May 14, 2021 20:03

almann changed the base branch from main to scanner-base May 14, 2021 20:04

zslayton approved these changes May 15, 2021

View reviewed changes

marcbowes approved these changes May 17, 2021

View reviewed changes

almann commented May 25, 2021

View reviewed changes

almann force-pushed the scanner branch from dcc9e0e to 4aa9ce2 Compare May 26, 2021 03:42

almann and others added 5 commits May 25, 2021 20:42

Apply suggestions from @zslayton.

1fe92bd

Co-authored-by: Zack Slayton <zack.slayton@gmail.com>

Feedback as per @zslayton

8716ff2

* Renames `start_loc`/`end_loc` to just `start`/`end`. * Renames `start_location`\`end_location` similarly. * Adds doc comments about how `position_from` works.

Adds panic when creating a position at (0, 0).

f6ef87e

almann force-pushed the scanner branch from 4aa9ce2 to ba4398c Compare May 26, 2021 03:43

almann changed the base branch from scanner-base to main May 26, 2021 03:43

almann mentioned this pull request May 26, 2021

Add IllegalState Error #23

Closed

almann merged commit f227d0d into partiql:main May 26, 2021

almann deleted the scanner branch May 26, 2021 04:23

tbmreza mentioned this pull request May 28, 2021

First issue #24

Merged

		let start_location = self.remainder.offset.move_relative(pair.start_loc());
		let end_location = self.remainder.offset.move_relative(pair.end_loc());

		// make sure to translate line/column position from where we started
		Position::At(location) => Position::At(start_loc.move_relative(location)),

	pub fn rem(&self) -> &str {
	pub fn remainder(&self) -> &str {

Initial API for Scanner. #12

Initial API for Scanner. #12

Conversation

almann commented May 10, 2021

codecov bot commented May 10, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

almann commented May 10, 2021

dlurton May 10, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

almann May 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

almann May 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Initial API for `Scanner`. #12

Initial API for `Scanner`. #12

codecov bot commented May 10, 2021 •

edited

dlurton May 10, 2021 •

edited

almann May 14, 2021 •

edited

almann May 14, 2021 •

edited