Skip to content

Library: add run_query_safe exposing parsing errors #3236

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.

Sign up for GitHub

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

Open
exlee wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Library: add run_query_safe exposing parsing errors #3236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions src/machine/lib_machine/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ use crate::machine::{
ArenaHeaderTag, Fixnum, Number, BREAK_FROM_DISPATCH_LOOP_LOC, LIB_QUERY_SUCCESS,
};
use crate::offset_table::*;
use crate::parser::ast::{Var, VarPtr};
pub use crate::parser::ast::ParserError;
use crate::parser::ast::{Term as ASTTerm, Var, VarPtr};
use crate::parser::parser::{Parser, Tokens};
use crate::read::{write_term_to_heap, TermWriteResult};
use crate::types::UntypedArenaPtr;
Expand Down Expand Up @@ -581,16 +582,23 @@ impl Machine {
Ok(())
}

#[deprecated(note = "use non-panicking on parse errors `run_query_string` instead")]
/// Runs a query.
pub fn run_query(&mut self, query: impl Into<String>) -> QueryState<'_> {
self.run_query_string(query).expect("Failed to parse query")
}

/// Runs query after checking parse errors.
fn run_query_string(
&mut self,
query: impl Into<String>,
) -> Result<QueryState<'_>, ParserError> {
let mut parser = Parser::new(
Stream::from_owned_string(query.into(), &mut self.machine_st.arena),
&mut self.machine_st,
);
let op_dir = CompositeOpDir::new(&self.indices.op_dir, None);
let term = parser
.read_term(&op_dir, Tokens::Default)
.expect("Failed to parse query");
let term = parser.read_term(&op_dir, Tokens::Default)?;

self.allocate_stub_choice_point()
.expect("failed to allocate stub choice point");
Expand Down
17 changes: 17 additions & 0 deletions src/parser/ast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,24 +426,38 @@ pub enum ArithmeticError {
UninstantiatedVar,
}

/// Enum for parsing errors
#[allow(dead_code)]
#[non_exhaustive]
#[derive(Debug)]
pub enum ParserError {
Copy link
Copy Markdown
Contributor

@Skgland Skgland Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this already publicly reachable?1 Otherwise we might want to make this #[non_exhaustive] so that we can add new variants without that being a breaking change.

Footnotes

  1. While it was already declared pub it might not have been reachable until the pub use was added in src/machine/lib_machine/mod.rs above ↩

Copy link
Copy Markdown
Author

@exlee exlee Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know my change exposed it . Makes sense to add #[non_exhaustive], I'll add it to the PR.

Copy link
Copy Markdown
Contributor

@Skgland Skgland Jan 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mh, looking at the error variants some more a lot of them have two usize that appear to be a line column number pair.

Maybe it would be good to introduce a Span type e.g.

struct Span {
 line: usize,
 column: usize
}

Copy link
Copy Markdown
Contributor

@Skgland Skgland Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though span would be a range of locations and we only have one, so it should be rather Location instead of Span.

/// Backquoted String error found.
BackQuotedString(usize, usize),
/// IO Error error found.
IO(IOError),
/// Incomplete reduction error found.
IncompleteReduction(usize, usize),
/// Infinite float error found.
InfiniteFloat(usize, usize),
/// Invalid single quoted character found.
InvalidSingleQuotedCharacter(char),
/// Lexical error found.
LexicalError(lexical::Error),
/// Missing quote found.
MissingQuote(usize, usize),
/// Non prolog char found.
NonPrologChar(usize, usize),
/// BigInt parsing error found.
ParseBigInt(usize, usize),
/// Unexpected character found.
UnexpectedChar(char, usize, usize),
// UnexpectedEOF,
/// UTF8 error found.
Copy link
Copy Markdown
Contributor

@triska triska Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These additional comments seem to mostly repeat the types? I would suggest leaving this out.

Copy link
Copy Markdown
Contributor

@Skgland Skgland Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the missing_docs lint is set to deny so either this documentation or an explicit override i.e. #[allow(missing_docs)] on the enum is required now that this type is reachable.

scryer-prolog/src/lib.rs

Line 3 in 453a88f

#![deny(missing_docs)]

Copy link
Copy Markdown
Contributor

@triska triska Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah OK, thank you, I was already wondering why this is suddenly part of the PR.

Copy link
Copy Markdown
Author

@exlee exlee Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree they're superfluous but public enums seems to need to have documentation and I'd rather put them in than mess with the linter requirements.

Utf8Error(usize, usize),
}

impl ParserError {
/// Get line number and column number for error.
pub fn line_and_col_num(&self) -> Option<(usize, usize)> {
match self {
&ParserError::BackQuotedString(line_num, col_num)
Expand All @@ -458,6 +472,7 @@ impl ParserError {
}
}

/// Return error as an atom
pub fn as_atom(&self) -> Atom {
match self {
ParserError::BackQuotedString(..) => atom!("back_quoted_string"),
Expand All @@ -484,11 +499,13 @@ impl ParserError {
}
}

/// Unexpected EOF error
#[inline]
pub fn unexpected_eof() -> Self {
ParserError::IO(std::io::Error::from(ErrorKind::UnexpectedEof))
}

/// Is an unexpected EOF?
#[inline]
pub fn is_unexpected_eof(&self) -> bool {
if let ParserError::IO(e) = self {
Expand Down