Repository: rust-cli/config-rs (language: rust)

Issue:
Nom is included in public API
See [`ConfigError::ParsePath`](https://docs.rs/config/latest/config/enum.ConfigError.html#variant.PathParse).

This makes upgrading nom or moving away from it a breaking change.


Discussion / hints:
Was looking at switching from `nom` to `winnow` to reduce the number of dependencies.

This would drop the total dependency count by 2-3:
- `winnow` is already pulled in via `toml` (if enabled)
- `nom` has 2 direct dependencies (one isn't even used by `nom`, the other is only used by one parser which most people likely don't include).
That's indeed an issue, yes. Thanks for raising this!

I am happy with either, a patch to fix the issue you raised here, or a patch(set) to move from `nom` to another parser implementation.

---

That said, I would like to offer you maintainership on this crate, if you want it. I am only passively maintaining this and I had little motivation to drive this crate forward in the past... even my `config-rs-ng` (https://github.com/matthiasbeyer/config-rs-ng) efforts stalled way too early. So I'd love to share maintainership, or actually even step down from maintaining this crate!
I'm fine doing the change but it is a breaking change and unsure how you feel about doing one so soon after the last...

As for maintainership, I would likely be passive about it as I don't even use this crate.  I was making these changes mostly because they are low hanging and this is used by so many people.
If I help out or take over, I would update the crate to conform to all of my other crates so that the incremental overhead of taking care of this crate is dramatically lower.

To achieve that, i would like full privileges so I can make any changes to the github side of this to support the above (e.g. install "apps").
I do not even have full access myself, so for that we would need to talk to @mehcode ... I hope they respond. In any case, if @epage wants to take over, I'd be fine with it and 👍 
If you do get approval, before tackling all that could you please give me a heads-up with a window / notice of about a week?

I'm planning to wrap up my open PRs this month and the less friction to deal with from rebasing those the easier it'll be to get them resolved 😅 
Hopefully as part of this change, the `Display` implementation for `ConfigError::ParseParse` can be improved.

Currently, it just prints the description of the inner error which is often quite useless
https://github.com/rust-cli/config-rs/blob/51acc67acf45f23bbcd50abd27376b0933277eda/src/error.rs#L190

As an example, if you try to parse the path `a..b` then the error message you get is just
> Char

I feel like it should at least tell you that it was an error in parsing the path.
Is the concern just with the nom error or errors in general?
Just the Nom error. The other errors I've seen are succinct and informative.

Fix patch:
diff --git a/Cargo.lock b/Cargo.lock
index ada170a1..686a3d7e 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -246,7 +246,6 @@ dependencies = [
  "indexmap 2.2.2",
  "json5",
  "log",
- "nom",
  "notify",
  "pathdiff",
  "reqwest",
@@ -260,6 +259,7 @@ dependencies = [
  "tokio",
  "toml",
  "warp",
+ "winnow",
  "yaml-rust2",
 ]
 
@@ -1030,12 +1030,6 @@ dependencies = [
  "unicase",
 ]
 
-[[package]]
-name = "minimal-lexical"
-version = "0.2.1"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "68354c5c6bd36d73ff3feceb05efa59b6acb7626617f4962be322a825e61f79a"
-
 [[package]]
 name = "miniz_oxide"
 version = "0.7.1"
@@ -1104,16 +1098,6 @@ dependencies = [
  "tempfile",
 ]
 
-[[package]]
-name = "nom"
-version = "7.1.3"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "d273983c5a657a70a3e8f2a01329822f3b8c8172b73826411a55751e404a0a4a"
-dependencies = [
- "memchr",
- "minimal-lexical",
-]
-
 [[package]]
 name = "normalize-line-endings"
 version = "0.3.0"
@@ -2517,9 +2501,9 @@ checksum = "589f6da84c646204747d1270a2a5661ea66ed1cced2631d546fdfb155959f9ec"
 
 [[package]]
 name = "winnow"
-version = "0.6.5"
+version = "0.6.20"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "dffa400e67ed5a4dd237983829e66475f0a4a26938c4b04c21baede6262215b8"
+checksum = "36c1fec1a2bb5866f07c25f68c26e565c4c200aebb96d7e55710c19d3e8ac49b"
 dependencies = [
  "memchr",
 ]
diff --git a/Cargo.toml b/Cargo.toml
index 50cae5f4..cba1717b 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -122,7 +122,6 @@ async = ["async-trait"]
 
 [dependencies]
 serde = "1.0"
-nom = "7"
 
 async-trait = { version = "0.1", optional = true }
 toml = { version = "0.8", optional = true }
@@ -134,6 +133,7 @@ json5_rs = { version = "0.4", optional = true, package = "json5" }
 indexmap = { version = "2.2", features = ["serde"], optional = true }
 convert_case = { version = "0.6", optional = true }
 pathdiff = "0.2"
+winnow = "0.6.20"
 
 [dev-dependencies]
 serde_derive = "1.0"
diff --git a/src/error.rs b/src/error.rs
index 8f3363b8..13e13b82 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -38,6 +38,7 @@ impl fmt::Display for Unexpected {
 
 /// Represents all possible errors that can occur when working with
 /// configuration.
+#[non_exhaustive]
 pub enum ConfigError {
     /// Configuration is frozen and no further mutations can be made.
     Frozen,
@@ -46,7 +47,7 @@ pub enum ConfigError {
     NotFound(String),
 
     /// Configuration path could not be parsed.
-    PathParse(nom::error::ErrorKind),
+    PathParse { cause: Box<dyn Error + Send + Sync> },
 
     /// Configuration could not be parsed from file.
     FileParse {
@@ -187,7 +188,7 @@ impl fmt::Display for ConfigError {
         match *self {
             ConfigError::Frozen => write!(f, "configuration is frozen"),
 
-            ConfigError::PathParse(ref kind) => write!(f, "{}", kind.description()),
+            ConfigError::PathParse { ref cause } => write!(f, "{cause}"),
 
             ConfigError::Message(ref s) => write!(f, "{s}"),
 
diff --git a/src/path/mod.rs b/src/path/mod.rs
index 52089de1..3c0b4213 100644
--- a/src/path/mod.rs
+++ b/src/path/mod.rs
@@ -17,10 +17,29 @@ impl FromStr for Expression {
     type Err = ConfigError;
 
     fn from_str(s: &str) -> Result<Self> {
-        parser::from_str(s).map_err(ConfigError::PathParse)
+        parser::from_str(s).map_err(|e| ConfigError::PathParse {
+            cause: Box::new(ParseError::new(e)),
+        })
     }
 }
 
+#[derive(Debug)]
+struct ParseError(String);
+
+impl ParseError {
+    fn new(inner: winnow::error::ContextError) -> Self {
+        Self(inner.to_string())
+    }
+}
+
+impl std::fmt::Display for ParseError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        self.0.fmt(f)
+    }
+}
+
+impl std::error::Error for ParseError {}
+
 fn sindex_to_uindex(index: isize, len: usize) -> usize {
     if index >= 0 {
         index as usize
diff --git a/src/path/parser.rs b/src/path/parser.rs
index fbb8ba24..6666e21f 100644
--- a/src/path/parser.rs
+++ b/src/path/parser.rs
@@ -1,83 +1,72 @@
 use std::str::FromStr;
 
-use nom::{
-    branch::alt,
-    bytes::complete::{is_a, tag},
-    character::complete::{char, digit1, space0},
-    combinator::{map, map_res, opt, recognize},
-    error::ErrorKind,
-    sequence::{delimited, pair, preceded},
-    Err, IResult,
-};
+use winnow::ascii::digit1;
+use winnow::ascii::space0;
+use winnow::combinator::dispatch;
+use winnow::combinator::eof;
+use winnow::combinator::fail;
+use winnow::combinator::opt;
+use winnow::combinator::repeat;
+use winnow::combinator::seq;
+use winnow::error::ContextError;
+use winnow::prelude::*;
+use winnow::token::any;
+use winnow::token::take_while;
 
 use crate::path::Expression;
 
-fn raw_ident(i: &str) -> IResult<&str, String> {
-    map(
-        is_a(
-            "abcdefghijklmnopqrstuvwxyz \
-         ABCDEFGHIJKLMNOPQRSTUVWXYZ \
-         0123456789 \
-         _-",
-        ),
-        ToString::to_string,
-    )(i)
+pub(crate) fn from_str(mut input: &str) -> Result<Expression, ContextError> {
+    let input = &mut input;
+    path(input).map_err(|e| e.into_inner().unwrap())
 }
 
-fn integer(i: &str) -> IResult<&str, isize> {
-    map_res(
-        delimited(space0, recognize(pair(opt(tag("-")), digit1)), space0),
-        FromStr::from_str,
-    )(i)
+fn path(i: &mut &str) -> PResult<Expression> {
+    let root = ident.parse_next(i)?;
+    let expr = repeat(0.., postfix)
+        .fold(
+            || root.clone(),
+            |prev, cur| match cur {
+                Child::Key(k) => Expression::Child(Box::new(prev), k),
+                Child::Index(k) => Expression::Subscript(Box::new(prev), k),
+            },
+        )
+        .parse_next(i)?;
+    eof.parse_next(i)?;
+    Ok(expr)
 }
 
-fn ident(i: &str) -> IResult<&str, Expression> {
-    map(raw_ident, Expression::Identifier)(i)
+fn ident(i: &mut &str) -> PResult<Expression> {
+    raw_ident.map(Expression::Identifier).parse_next(i)
 }
 
-fn postfix<'a>(expr: Expression) -> impl FnMut(&'a str) -> IResult<&'a str, Expression> {
-    let e2 = expr.clone();
-    let child = map(preceded(tag("."), raw_ident), move |id| {
-        Expression::Child(Box::new(expr.clone()), id)
-    });
-
-    let subscript = map(delimited(char('['), integer, char(']')), move |num| {
-        Expression::Subscript(Box::new(e2.clone()), num)
-    });
+fn postfix(i: &mut &str) -> PResult<Child> {
+    dispatch! {any;
+        '[' => seq!(integer.map(Child::Index), _: ']').map(|(i,)| i),
+        '.' => raw_ident.map(Child::Key),
+        _ => fail,
+    }
+    .parse_next(i)
+}
 
-    alt((child, subscript))
+enum Child {
+    Key(String),
+    Index(isize),
 }
 
-pub(crate) fn from_str(input: &str) -> Result<Expression, ErrorKind> {
-    match ident(input) {
-        Ok((mut rem, mut expr)) => {
-            while !rem.is_empty() {
-                match postfix(expr)(rem) {
-                    Ok((rem_, expr_)) => {
-                        rem = rem_;
-                        expr = expr_;
-                    }
-
-                    // Forward Incomplete and Error
-                    result => {
-                        return result.map(|(_, o)| o).map_err(to_error_kind);
-                    }
-                }
-            }
-
-            Ok(expr)
-        }
-
-        // Forward Incomplete and Error
-        result => result.map(|(_, o)| o).map_err(to_error_kind),
-    }
+fn raw_ident(i: &mut &str) -> PResult<String> {
+    take_while(1.., ('a'..='z', 'A'..='Z', '0'..='9', '_', '-'))
+        .map(ToString::to_string)
+        .parse_next(i)
 }
 
-pub(crate) fn to_error_kind(e: Err<nom::error::Error<&str>>) -> ErrorKind {
-    match e {
-        Err::Incomplete(_) => ErrorKind::Complete,
-        Err::Failure(e) | Err::Error(e) => e.code,
-    }
+fn integer(i: &mut &str) -> PResult<isize> {
+    seq!(
+        _: space0,
+        (opt('-'), digit1).take().try_map(FromStr::from_str),
+        _: space0
+    )
+    .map(|(i,)| i)
+    .parse_next(i)
 }
 
 #[cfg(test)]


Test patch: