[WIP] Propagated error type. #413

ghost · 2016-12-30T10:44:54Z

Error types are implicitly passed around, and gives hints about type of error to rustc.

Some macros are currently broken.

Fix:

I need some help to fix delimited!(), what does tuple_parser!() do?
How can I get it's error value?

- Error types are implicitly passed around. - Some macros are currently broken (delimited!) Fixed: many1, many0, separated_list, separated_nonempty_list. - Tests which use assert_eq!(mac!(&b"input"[..], arg)); was broken, and fixed. - Fixed some tests which was wrong in error type. (Child matcher might return same error type. If not, caller must use .map_err() )

Geal · 2016-12-30T13:50:39Z

hi!

before going further, can you add tests to confirm that it fixes the issue? And explain how it works?

Geal · 2016-12-30T13:51:07Z

src/bits.rs

-    assert_eq!(take_bits!( (sl, 0), u32, 20 ), IResult::Done((&sl[2..], 4), 700163));
-    assert_eq!(take_bits!( (sl, 4), u32, 20 ), IResult::Done((&sl[3..], 0), 716851));
-    assert_eq!(take_bits!( (sl, 4), u32, 22 ), IResult::Incomplete(Needed::Size(22)));
+    assert_eq!(take_bits!( (sl, 0), u8,   0 ), done((sl, 0), 0));


why do you replace IResult::Done with done here?

As commented below, type cannot be inferred if macro is used directly from assert_eq!()

Geal · 2016-12-30T13:52:33Z

src/branch.rs

-        $crate::IResult::Error(_)      => {
-          alt!(__impl $i, $($rest)*)
+        $crate::IResult::Error(_alt_err)      => {
+          $crate::propagate_error_type(_alt_err, alt!(__impl $i, $($rest)*))


I'm a bit uneasy about this. Especially, I don't know how it will interact with the whitespace parsing combinators

Geal · 2016-12-30T13:53:52Z

src/traits.rs

@@ -17,6 +17,13 @@ pub trait InputLength {
  fn input_len(&self) -> usize;
 }

+impl<T> InputLength for [T] {


I'm not sure about it. Maybe crlf? I configured vscode to use lf always, but it somehow prefers crlf. I'll try to remove it from pull request.

Geal · 2016-12-30T13:54:30Z

src/util.rs

@@ -235,7 +235,7 @@ pub fn slice_to_offsets(input: &[u8], s: &[u8]) -> (usize, usize) {

 #[cfg(not(feature = "core"))]
 #[cfg(feature = "verbose-errors")]
-pub fn prepare_errors<O,E: Clone>(input: &[u8], res: IResult<&[u8],O,E>) -> Option<Vec<(ErrorKind<E>, usize, usize)> > {
+pub fn prepare_errors<O,E: Clone>(input: &[u8], res: IResult<&[u8], O, E>) -> Option<Vec<(ErrorKind<E>, usize, usize)> > {


please avoid cosmetic changes in unrelated parts of the code

Oh, sorry for that. It's a mistake done while experimenting,

Geal · 2016-12-30T13:54:55Z

tests/issues.rs

@@ -98,7 +98,9 @@ fn issue_142(){

 #[test]
 fn usize_length_bytes_issue(){
-  length_bytes!(b"012346", be_u16);
+  named!(typed_iresult, length_bytes!(be_u16));


why touch that test?

Geal · 2016-12-30T13:55:15Z

tests/test1.rs

@@ -40,5 +40,6 @@ fn is_not() {
 #[test]
 fn exported_public_method_defined_by_macro() {
  let a = &b"ab12cd\nefgh"[..];
-  assert_eq!(not_line_ending(a), IResult::Done(&b"\nefgh"[..], &b"ab12cd"[..]));
+  let expected: IResult<_, _> = IResult::Done(&b"\nefgh"[..], &b"ab12cd"[..]);


why change that test?

default parameter does not works for function, so return types for codes that calls macro directly from assert_eq() cannot be inferred.

In real world usecase, it would not affect any usage, unless users write code like

assert_eq!(macro_in_nom!("args.."), Done(...))

ghost · 2016-12-31T02:41:09Z

I'm stucked while making escaped function. Does original macro accept only slices? And it seems to match against u8, but matching non-ascii character as byte isn't safe for utf8. UTF8 extended characters have 1 on it's first bit for ascii-compatibility, so it's safe to match as byte only if it's in [0, 127].

I have a proposal about it.
Byte array can be decoded as char iterator with https://doc.rust-lang.org/src/core/up/src/libcore/char.rs.html#744-818 , and char.len_utf8() can be used for proceeding.

Implementation would be

pub trait FirstChar {
  fn first_char<E>() -> IResult<Self, char, E>
}

// alternative
pub trait AsChars {
  type CharIter;

  fn as_chars(&self) -> Self::CharIter;
}

/// maybe result. 'Incomplete' is exactly
fn decode_utf8<E>(i: &[u8]) -> IResult<&[u8], char, E> {
// ...
}

// in char related matchers
fn use_it(i: I) {
    let index = sub_parser(i);
    // handle incomplete from char decoding
    i.slice(index..).first_char()
}

In this way, also &str can be used for it.

Edit: added questions about usage of char.

Geal · 2017-01-06T10:59:15Z

ok, sorry for the delay, I will now get to work on this :)

escaped cannot work yet with &str, I planned to do that for 2.0, but didn't have the time to do it: #300

Geal · 2017-01-06T14:15:13Z

fixing inference for escaped will be difficult :/

Geal · 2017-01-06T14:45:52Z

I really don't know how to fix that one, except by specifying the type directly like this: named!(esc, escaped!(call!(alpha::<&[u8],u32>), '\\', one_of!("\"n\\")));

I found a better way to write escaped in the process, though

Geal · 2017-01-06T15:11:59Z

I have to stop for today. I have a feeling the solution will be in fixing the error type for basic parsers like alpha or tag, and let users do their fix_error above them, instead of trying to adapt in a parent parser to the custom error type coming from the child parser.

ghost · 2017-01-09T14:26:10Z

I'm really sorry, but I can't help because I'm far away from my home and I can't use any pc.
I'm not sure when it would be allowed.. Maybe on march? But even this depends.

I agree that propagating error types from basical parsers is much important task.

Geal · 2017-01-20T13:36:31Z

@kdy1997 why did you close this PR?

ghost · 2017-01-20T13:42:41Z

Oh.. sorry for my mistake. I think I accidentally touched it while reading from mobile.

Geal · 2017-01-20T13:43:43Z

ok, I'm relieved :)
I might have time to work on it next week

fflorent · 2017-07-15T11:59:56Z

Any news on this?

Florent

Geal · 2017-11-26T11:56:58Z

hey, so, there was a huge refactoring for nom 4 to fix this issue, so I don't think I'll be able to merge that PR in the end. Thank you for your work still!

Geal reviewed Dec 30, 2016

View reviewed changes

ghost closed this Jan 20, 2017

ghost reopened this Jan 20, 2017

Geal closed this Nov 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Propagated error type. #413

[WIP] Propagated error type. #413

ghost commented Dec 30, 2016 •

edited by ghost

Loading

Geal commented Dec 30, 2016

Geal Dec 30, 2016

ghost Dec 30, 2016

Geal Dec 30, 2016

Geal Dec 30, 2016

ghost Dec 30, 2016

Geal Dec 30, 2016

ghost Dec 30, 2016

Geal Dec 30, 2016

Geal Dec 30, 2016

ghost Dec 30, 2016 •

edited by ghost

Loading

ghost commented Dec 31, 2016 •

edited by ghost

Loading

Geal commented Jan 6, 2017

Geal commented Jan 6, 2017

Geal commented Jan 6, 2017

Geal commented Jan 6, 2017

ghost commented Jan 9, 2017

Geal commented Jan 20, 2017

ghost commented Jan 20, 2017

Geal commented Jan 20, 2017

fflorent commented Jul 15, 2017

Geal commented Nov 26, 2017

[WIP] Propagated error type. #413

[WIP] Propagated error type. #413

Conversation

ghost commented Dec 30, 2016 • edited by ghost Loading

Fix:

Geal commented Dec 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Dec 30, 2016 • edited by ghost Loading

Choose a reason for hiding this comment

ghost commented Dec 31, 2016 • edited by ghost Loading

Geal commented Jan 6, 2017

Geal commented Jan 6, 2017

Geal commented Jan 6, 2017

Geal commented Jan 6, 2017

ghost commented Jan 9, 2017

Geal commented Jan 20, 2017

ghost commented Jan 20, 2017

Geal commented Jan 20, 2017

fflorent commented Jul 15, 2017

Geal commented Nov 26, 2017

ghost commented Dec 30, 2016 •

edited by ghost

Loading

ghost Dec 30, 2016 •

edited by ghost

Loading

ghost commented Dec 31, 2016 •

edited by ghost

Loading