Changes construction of LineAndColumn to return ParserResult.

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.

partiql-parser/src/lib.rs

@@ -32,8 +32,8 @@
 //!         Keyword(kw) => assert_eq!("FROM", kw),
 //!     }
 //!     // the other thing we can do is get line/column information from a token
-//!     assert_eq!(LineAndColumn::at(1, 8), second.start());
-//!     assert_eq!(LineAndColumn::at(1, 12), second.end());
+//!     assert_eq!(LineAndColumn::try_at(1, 8)?, second.start());
+//!     assert_eq!(LineAndColumn::try_at(1, 12)?, second.end());
 //!
 //!     // this API is built on immutable slices, so we can restart scanning from any token
 //!     scanner = first.into();

partiql-parser/src/peg.rs

@@ -28,7 +28,7 @@ impl<'val, R: RuleType> PairsExt<'val, R> for Pairs<'val, R> {
                 if let Some(other_pair) = self.next() {
                     syntax_error(
                         format!("Expected one token pair, got: {:?}, {:?}", pair, other_pair),
-                        pair.start().into(),
+                        pair.start()?.into(),
                     )?;
                 }
                 Ok(pair)
@@ -44,21 +44,21 @@ impl<'val, R: RuleType> PairsExt<'val, R> for Pairs<'val, R> {
 /// Extension methods for working with [`Pair`].
 pub(crate) trait PairExt<'val, R: RuleType> {
     /// Translates the start position of the [`Pair`] into a [`LineAndColumn`].
-    fn start(&self) -> LineAndColumn;
+    fn start(&self) -> ParserResult<LineAndColumn>;
 
     /// Translates the end position of the [`Pair`] into a [`LineAndColumn`].
-    fn end(&self) -> LineAndColumn;
+    fn end(&self) -> ParserResult<LineAndColumn>;
 }
 
 impl<'val, R: RuleType> PairExt<'val, R> for Pair<'val, R> {
     #[inline]
-    fn start(&self) -> LineAndColumn {
-        self.as_span().start_pos().line_col().into()
+    fn start(&self) -> ParserResult<LineAndColumn> {
+        self.as_span().start_pos().line_col().try_into()
     }
 
     #[inline]
-    fn end(&self) -> LineAndColumn {
-        self.as_span().end_pos().line_col().into()
+    fn end(&self) -> ParserResult<LineAndColumn> {
+        self.as_span().end_pos().line_col().try_into()
     }
 }
 

partiql-parser/src/prelude.rs

@@ -8,3 +8,5 @@ pub use crate::result::ParserError;
 pub use crate::result::ParserResult;
 pub use crate::result::Position;
 pub use crate::scanner::Scanner;
+pub use std::convert::TryFrom;
+pub use std::convert::TryInto;

partiql-parser/src/result.rs

@@ -3,6 +3,7 @@
 //! [`Error`] and [`Result`] types for parsing PartiQL.
 
 use pest::error::{ErrorVariant, LineColLocation};
+use std::convert::TryFrom;
 use std::fmt;
 use std::fmt::Formatter;
 use thiserror::Error;
@@ -14,20 +15,42 @@ use thiserror::Error;
 /// ## Example
 /// ```
 /// # use partiql_parser::prelude::*;
-/// println!("Beginning of a document: {}", LineAndColumn::at(1, 1));
+/// println!("Beginning of a document: {}", LineAndColumn::try_at(1, 1).unwrap());
 /// ```
 #[derive(Copy, Clone, Debug, Eq, PartialEq)]
-pub struct LineAndColumn(pub usize, pub usize);
+pub struct LineAndColumn(usize, usize);
 
 impl LineAndColumn {
+    /// Constructs at [`LineAndColumn`] without verifying 1-position invariant.
+    #[inline]
+    pub(crate) fn at(line: usize, column: usize) -> Self {
+        Self(line, column)
+    }
+
     /// Constructs a [`LineAndColumn`].
     ///
-    /// Note that this function will panic if `line` or `column` is zero.
+    /// Note that this function will return `Err` if `line` or `column` is zero.
     #[inline]
-    pub fn at(line: usize, column: usize) -> Self {
-        assert_ne!(0, line);
-        assert_ne!(0, column);
-        Self(line, column)
+    pub fn try_at(line: usize, column: usize) -> ParserResult<Self> {
+        if line == 0 || column == 0 {
+            invalid_argument(format!(
+                "Cannot create position at line {}, column {}",
+                line, column
+            ))?;
+        }
+        Ok(Self(line, column))
+    }
+
+    /// Returns the line associated with this [`LineAndColumn`].
+    #[inline]
+    pub fn line(&self) -> usize {
+        self.0
+    }
+
+    /// Returns the column associated with this [`LineAndColumn`].
+    #[inline]
+    pub fn column(&self) -> usize {
+        self.1
     }
 
     /// Returns a [`LineAndColumn`] that repositions this position relative
@@ -38,63 +61,77 @@ impl LineAndColumn {
     /// ## Examples
     /// ```
     /// # use partiql_parser::prelude::*;
+    /// # fn main() -> ParserResult<()> {
     /// // we're not repositioning anything!
     /// assert_eq!(
-    ///     LineAndColumn::at(1, 1),
-    ///     LineAndColumn::at(1, 1).position_from(LineAndColumn::at(1, 1))
+    ///     LineAndColumn::try_at(1, 1)?,
+    ///     LineAndColumn::try_at(1, 1)?.position_from(LineAndColumn::try_at(1, 1)?)
     /// );
+    /// # Ok(())
+    /// # }
     /// ```
     ///
     /// ```
     /// # use partiql_parser::prelude::*;
+    /// # fn main() -> ParserResult<()> {
     /// // same here, we're really at the origin
     /// assert_eq!(
-    ///     LineAndColumn::at(1, 2),
-    ///     LineAndColumn::at(1, 2).position_from(LineAndColumn::at(1, 1))
+    ///     LineAndColumn::try_at(1, 2)?,
+    ///     LineAndColumn::try_at(1, 2)?.position_from(LineAndColumn::try_at(1, 1)?)
     /// );
+    /// # Ok(())
+    /// # }
     /// ```
     ///
     /// ```
     /// # use partiql_parser::prelude::*;
+    /// # fn main() -> ParserResult<()> {
     /// // same line from origin, adjust only the column
     /// assert_eq!(
-    ///     LineAndColumn::at(5, 10),
-    ///     LineAndColumn::at(1, 4).position_from(LineAndColumn::at(5, 7))
+    ///     LineAndColumn::try_at(5, 10)?,
+    ///     LineAndColumn::try_at(1, 4)?.position_from(LineAndColumn::try_at(5, 7)?)
     /// );
+    /// # Ok(())
+    /// # }
     /// ```
     ///
     /// ```
     /// # use partiql_parser::prelude::*;
+    /// # fn main() -> ParserResult<()> {
     /// // we're moving lines, adjust the line and take the target column as-is
     /// assert_eq!(
-    ///     LineAndColumn::at(21, 2),
-    ///     LineAndColumn::at(20, 2).position_from(LineAndColumn::at(2, 15))
+    ///     LineAndColumn::try_at(21, 2)?,
+    ///     LineAndColumn::try_at(20, 2)?.position_from(LineAndColumn::try_at(2, 15)?)
     /// );
+    /// # Ok(())
+    /// # }
     /// ```
     pub fn position_from(self, location: LineAndColumn) -> Self {
         match (location, self) {
             (LineAndColumn(base_line, base_column), LineAndColumn(dest_line, dest_column)) => {
                 let diff_line = dest_line - 1;
                 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)
+                    Self(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)
+                    Self(base_line, base_column + diff_column)
                 }
             }
         }
     }
 }
 
-impl From<(usize, usize)> for LineAndColumn {
+impl TryFrom<(usize, usize)> for LineAndColumn {
+    type Error = ParserError;
+
     /// Constructs a [`LineAndColumn`] from a pair.
     ///
-    /// This function will panic if the `line` or `column` is zero.
-    fn from(line_and_column: (usize, usize)) -> Self {
+    /// This function will return `Err` if `line` or `column` is zero.
+    fn try_from(line_and_column: (usize, usize)) -> Result<Self, Self::Error> {
         let (line, column) = line_and_column;
-        Self::at(line, column)
+        Self::try_at(line, column)
     }
 }
 
@@ -116,10 +153,13 @@ pub enum Position {
 impl Position {
     /// Shorthand for creating a [`Position::At`] variant.
     ///
-    /// Note that this will panic if `line` or `column` is zero.
+    /// Note that this will return [`Position::Unknown`] if `line` or `column` is zero.
     #[inline]
     pub fn at(line: usize, column: usize) -> Self {
-        Self::At(LineAndColumn::at(line, column))
+        match LineAndColumn::try_at(line, column) {
+            Ok(location) => Self::At(location),
+            Err(_) => Self::Unknown,
+        }
     }
 }
 
@@ -146,6 +186,10 @@ pub enum ParserError {
     /// Indicates that there was a problem with syntax.
     #[error("Syntax Error: {message} ({position})")]
     SyntaxError { message: String, position: Position },
+
+    /// Indicates that there is a problem with run-time violation of some API.
+    #[error("Invalid Argument: {message}")]
+    InvalidArgument { message: String },
 }
 
 impl ParserError {
@@ -157,14 +201,28 @@ impl ParserError {
             position,
         }
     }
+
+    /// Convenience function to crate a [`InvalidArgument`](ParserError::InvalidArgument).
+    #[inline]
+    pub fn invalid_argument<S: Into<String>>(message: S) -> Self {
+        Self::InvalidArgument {
+            message: message.into(),
+        }
+    }
 }
 
-/// Convenience function to create a `Err([SyntaxError](ParserError::SyntaxError))`.
+/// Convenience function to create an `Err([SyntaxError](ParserError::SyntaxError))`.
 #[inline]
 pub fn syntax_error<T, S: Into<String>>(message: S, position: Position) -> ParserResult<T> {
     Err(ParserError::syntax_error(message, position))
 }
 
+/// Convenience function to crate an `Err([InvalidArgument](ParserError::InvalidArgument))`.
+#[inline]
+pub fn invalid_argument<T, S: Into<String>>(message: S) -> ParserResult<T> {
+    Err(ParserError::invalid_argument(message))
+}
+
 impl<R> From<pest::error::Error<R>> for ParserError
 where
     R: fmt::Debug,
@@ -207,20 +265,29 @@ mod tests {
     }
 
     #[test]
-    #[should_panic]
-    fn bad_position() {
-        Position::at(0, 0);
+    fn bad_position() -> ParserResult<()> {
+        match Position::at(0, 0) {
+            Position::Unknown => {}
+            position => panic!("Bad position {:?}", position),
+        }
+        Ok(())
     }
 
     #[test]
-    #[should_panic]
-    fn bad_line_and_column() {
-        LineAndColumn::at(0, 0);
+    fn bad_line_and_column() -> ParserResult<()> {
+        match LineAndColumn::try_at(0, 0) {
+            Err(ParserError::InvalidArgument { .. }) => {}
+            result => panic!("Bad result {:?}", result),
+        }
+        Ok(())
     }
 
     #[test]
-    #[should_panic]
-    fn bad_line_and_column_from_pair() {
-        LineAndColumn::from((0, 0));
+    fn bad_line_and_column_from_pair() -> ParserResult<()> {
+        match LineAndColumn::try_from((0, 0)) {
+            Err(ParserError::InvalidArgument { .. }) => {}
+            result => panic!("Bad result {:?}", result),
+        }
+        Ok(())
     }
 }

partiql-parser/src/scanner.rs

@@ -110,15 +110,15 @@ impl<'val> PartiQLScanner<'val> {
         // the scanner rule is expected to return a single node
         let pair: Pair<'val, Rule> =
             PartiQLParser::parse(Rule::Scanner, self.remainder.input)?.exactly_one()?;
-        let start = pair.start().position_from(self.remainder.offset);
-        let end = pair.end().position_from(self.remainder.offset);
+        let start = pair.start()?.position_from(self.remainder.offset);
+        let end = pair.end()?.position_from(self.remainder.offset);
         let text = pair.as_str();
         let start_off = pair.as_span().start();
-        self.remainder = self.remainder.consume(start_off + text.len(), pair.end());
+        self.remainder = self.remainder.consume(start_off + text.len(), pair.end()?);
 
         let content = match pair.as_rule() {
             Rule::Keyword => Content::Keyword(text),
-            _ => return syntax_error(format!("Unexpected rule: {:?}", pair), pair.start().into()),
+            _ => return syntax_error(format!("Unexpected rule: {:?}", pair), pair.start()?.into()),
         };
 
         Ok(Token {
@@ -143,6 +143,9 @@ impl<'val> Scanner<'val> for PartiQLScanner<'val> {
                 };
                 ParserError::syntax_error(message, position)
             }
+            // other errors that don't have position information just pass up as position is not
+            // relevant...
+            error => error,
         })
     }
 }