Skip to content
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.

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

Merge InputLength into Input can introduce extra trait requirement #1809

Open
tisonkun opened this issue Jan 29, 2025 · 5 comments
Open

Merge InputLength into Input can introduce extra trait requirement #1809

tisonkun opened this issue Jan 29, 2025 · 5 comments

Comments

@tisonkun
Copy link
Contributor

tisonkun commented Jan 29, 2025

See andylokandy/nom-rule#8

Previously, we have:

nom/src/multi/mod.rs

Lines 52 to 58 in 869f897

#[cfg_attr(feature = "docsrs", doc(cfg(feature = "alloc")))]
pub fn many0<I, O, E, F>(mut f: F) -> impl FnMut(I) -> IResult<I, Vec<O>, E>
where
I: Clone + InputLength,
F: Parser<I, O, E>,
E: ParseError<I>,
{

and

pub trait InputLength {
  fn input_len(&self) -> usize;
}

impl<'a, T> InputLength for &'a [T] {
  #[inline]
  fn input_len(&self) -> usize {
    self.len()
  }
}

So InputLength is implemented for &[proc_macro2::TokenTree].

But now we have:

nom/src/multi/mod.rs

Lines 61 to 67 in 2cec1b3

#[cfg(feature = "alloc")]
#[cfg_attr(feature = "docsrs", doc(cfg(feature = "alloc")))]
pub fn many0<I, F>(
f: F,
) -> impl Parser<I, Output = Vec<<F as Parser<I>>::Output>, Error = <F as Parser<I>>::Error>
where
I: Clone + Input,

So &[proc_macro2::TokenTree] can no longer directly adapted in many0.

Perhaps we can wrap a newtype to work it around. But I'd first report this use case and see if there is better/idiomatic way.

cc @Geal

@andylokandy
Copy link

Thank you @tisonkun for investigating the problem. Seems we are not able to add the impls by ourself except we add a new type which is not so ergonomic 🤔.

@marcdejonge
Copy link
Contributor

I think there should be an implementation for Input for all slices of objects. The problem is the current implementations are in the way, because they use a copied version of the Input::Item. But if the Input::Item would return a reference to the thing in the array, nothing would really break and you would have a nice generic implementation.

The main downside is that it's another breaking API change that could have an impact on code that people are using.

@tisonkun
Copy link
Contributor Author

I make a new type wrapper as a workaround: https://github.com/andylokandy/nom-rule/pull/8/files/62e78a4a0475eba6494b49820e91b414474e432e..6fcd576dc7022955f90dc837476d5907331ad621

But it's not so ergonomic, yes.

I think there should be an implementation for Input for all slices of objects. The problem is the current implementations are in the way, because they use a copied version of the Input::Item. But if the Input::Item would return a reference to the thing in the array, nothing would really break and you would have a nice generic implementation.

Yes. I'm not sure if Input::Item can return a ref. And perhaps a wrapper template for downstream users to wrap their slices as in the patch above would help.

@marcdejonge
Copy link
Contributor

https://github.com/rust-bakery/nom/pull/1810/files#diff-172747fc43e38d33de743b2d5f564ad6440570670af7aa912235399448a55c7cR192

Something like this actually passes all the tests and should work for you. The main issue is that is changes the external API and it will brake stuff for some people.

@tisonkun
Copy link
Contributor Author

@marcdejonge That should help. Thank you!

We're using a wrapper Input type inside our database application, which provide the Span and Backtrace during parsing to provide located error reports.

Perhaps that would be an supplement for a general Input wrapper or nom's Input trait can require a span info.

I found that error reporting is an essential part when writing an end-user parser, otherwise users would only accept "Syntax error" without which part is the source of error. Context can help in providing a global context, but a located span can help a lot.

I know nom_locate but perhaps @andylokandy can tell why we don't use it (perhaps the lack of backtrace report?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants