nom icon indicating copy to clipboard operation
nom copied to clipboard

Merge InputLength into Input can introduce extra trait requirement

Open tisonkun opened this issue 11 months ago • 6 comments

See https://github.com/andylokandy/nom-rule/pull/8

Previously, we have:

https://github.com/rust-bakery/nom/blob/869f8972a4383b13cf89574fda28cb7dbfd56517/src/multi/mod.rs#L52-L58

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:

https://github.com/rust-bakery/nom/blob/2cec1b3e4c9ccac62c902d60c00de6d1549ccbe1/src/multi/mod.rs#L61-L67

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

tisonkun avatar Jan 29 '25 14:01 tisonkun

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 🤔.

andylokandy avatar Jan 30 '25 06:01 andylokandy

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.

marcdejonge avatar Jan 30 '25 09:01 marcdejonge

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.

tisonkun avatar Jan 30 '25 13:01 tisonkun

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.

marcdejonge avatar Jan 30 '25 14:01 marcdejonge

@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?)

tisonkun avatar Jan 31 '25 10:01 tisonkun

I tried to change the default implementation of the &[u8] implementation to accept &[T: Clone]` instead. The fork is here: https://github.com/asibahi/nom/tree/impl-clone

I think using clone() and Cloned on Copy types just copies them. u8 at least should do that.

All the tests pass without changing any of them. It would also make it easier for using parsers on arbitrary Token trees.

asibahi avatar Feb 25 '25 22:02 asibahi