Library: add run_query_safe exposing parsing errors by exlee · Pull Request #3236 · mthom/scryer-prolog

exlee · 2026-01-21T14:00:31Z

Split run_query into run_query and run_query_impl (shared logic)
Add run_query_safe that returns QueryState or parser::ast::ParserError
Expose ParserError in lib
Add documentation string for ParserError and associated methods

Skgland

I am doing a similar thing¹ as part of #3145.
You might want to have a look at what I am doing there.

Also left a two comments regarding the name choice and code duplication.

Other than that I think propagating the error instead of panic-ing is a desirable improvement.

See https://github.com/mthom/scryer-prolog/pull/3145/changes#diff-72c8cab85361f6b46e74c79156bcd786ba5256e250bbe566b3a53e127fe2c17dR563-R591 ↩

exlee · 2026-01-21T18:30:21Z

src/machine/lib_machine/mod.rs

    }

+    /// Runs query after checking parse errors.
+    pub fn run_query_safe(


I was wondering about the same thing but decided to go with gut feeling. It's not a difficult change :)

src/machine/lib_machine/mod.rs

@@ -591,7 +607,10 @@ impl Machine {
        let term = parser
            .read_term(&op_dir, Tokens::Default)
            .expect("Failed to parse query");
+        self.run_query_impl(term)


Skgland · 2026-01-21T16:36:01Z

src/parser/ast.rs

+/// Enum for parsing errors
 #[allow(dead_code)]
 #[derive(Debug)]
 pub enum ParserError {


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

Footnotes

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 ↩

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

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 }

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

triska · 2026-01-21T18:44:18Z

src/parser/ast.rs

+    /// Unexpected character found.
    UnexpectedChar(char, usize, usize),
    // UnexpectedEOF,
+    /// UTF8 error found.


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

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)]

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

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.

exlee · 2026-01-23T13:46:21Z

Just to check in, is there anything that should still be done on this PR?

From what I understand there are comments, but I'm not sure if they should be implemented or not.

Skgland · 2026-01-23T16:12:12Z

Just to check in, is there anything that should still be done on this PR?

From what I understand there are comments, but I'm not sure if they should be implemented or not.

I think leaving the documentation as-is is fine as it can be changed at any point.
The more I look at the ParseError type the more I want to completely overhaul it first (i.e. before this PR makes it part of the public API, sorry).

The following isn't really directed at this PR but more a collection of my thought of what I think would be good to change about the ParseError type

use of tuple variants, struct variant would allow giving the fields a meaningful name
all but three variants contain a location
- makes me think maybe this should be changed to
```
struct ParseError{
    location: Option<Location>, 
    kind: ParseErrorKind
}
```
  with ParseErrorKind being the current ParseError enum but with the location moved into the struct.
- why don't we have a location for InvalidSingleQuotedCharacter or LexicalError?
variant LexicalError is exposing a type of a dependency which is often not a good idea
- am I remembering correctly that lexical was the library we use for integer/float parsing?

exlee mentioned this pull request Jan 21, 2026

run_query panics on incomplete Prolog queries #3235

Open

Skgland suggested changes Jan 21, 2026

View reviewed changes

Library: add run_query_safe exposing parsing errors

a7c7f92

exlee force-pushed the run_query_safe branch from 0f18a8d to a7c7f92 Compare January 21, 2026 18:37

triska reviewed Jan 21, 2026

View reviewed changes

Skgland mentioned this pull request Jan 24, 2026

change ParserError type #3241

Open

Conversation

exlee commented Jan 21, 2026

Uh oh!

Skgland left a comment

Choose a reason for hiding this comment

Footnotes

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Skgland Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

exlee commented Jan 23, 2026

Uh oh!

Skgland commented Jan 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Skgland Jan 21, 2026 •

edited

Loading

Skgland commented Jan 23, 2026 •

edited

Loading