bump combine version

[yihuaf]

Not sure if you are willing to accept this PR, but nonetheless since I already did it. Currently, this crate uses combine crate version 2.5, which is a very old version. The latest version is 4.6.

I understand that using an older version by itself is not an issue. I decided to go down this rabbit hole because I noticed the following security notification from dependabot on one of the project I am working on.

Dependabot cannot update ascii to a non-vulnerable version
The latest possible version of ascii that can be installed is 0.7.1.

The earliest fixed version is 0.9.3.

The older version of combine uses the ascii crate. However, the latest version of combine version 4.6 no longer has this dependency.

Since the major version changed from 2 -> 4, there are breaking changes, which I addressed in this PR. The latest version of combine implemented the display trait for the parser error, so technically we no longer need to format errors ourselves. However, the format is different, so I decided to keep the error format. I am happy to remove these functions if you wish.

With that being said, I had to make changes to the test_error_unexpected_character error messages. The error will container the correct expected end of input, but also contain a number of other errors from previous many parser. I filed the issue: Marwes/combine#355 to clarify with the combine author. As far as I can tell, this is harmless behavior that doesn't impact the correctness of the parsing logic.

There are two commits. The first are cargo fmt and the second contains the actual change.

[qmonnet]

Thanks for this!

Not sure if you are willing to accept this PR, but nonetheless since I already did it. Currently, this crate uses combine crate version 2.5, which is a very old version. The latest version is 4.6.

I understand that using an older version by itself is not an issue. I decided to go down this rabbit hole because I noticed the following security notification from dependabot on one of the project I am working on.

Dependabot cannot update ascii to a non-vulnerable version
The latest possible version of ascii that can be installed is 0.7.1.

The earliest fixed version is 0.9.3.

The older version of combine uses the ascii crate. However, the latest version of combine version 4.6 no longer has this dependency.

Yes, rbpf uses an old version. I haven't seen much interest in updating the dependencies so far, but if it raises security alerts, updates are welcome!

I'm just a bit surprised, because Dependabot raises no alert on ascii on rbpf's repo, I would expect it to report it the same way it did for you?

Since the major version changed from 2 -> 4, there are breaking changes, which I addressed in this PR.

Awesome, thanks! CI tests are all passing so it looks all good from my side.

The latest version of combine implemented the display trait for the parser error, so technically we no longer need to format errors ourselves. However, the format is different, so I decided to keep the error format. I am happy to remove these functions if you wish.

I don't think we mind much the format for those, if it's already implemented in the crate I think we could just use that and avoid keeping another version in rbpf. Would you have a sample output for the two versions, by any chance?

With that being said, I had to make changes to the test_error_unexpected_character error messages. The error will container the correct expected end of input, but also contain a number of other errors from previous many parser. I filed the issue: Marwes/combine#355 to clarify with the combine author. As far as I can tell, this is harmless behavior that doesn't impact the correctness of the parsing logic.

Should not matter much either, but thanks a lot for raising this and let's see what they say.

There are two commits. The first are cargo fmt and the second contains the actual change.

👍

[qmonnet]

Any chance we could have a bit more motivation and context in the commit log (mostly for the 2nd commit) please? I find it easier to check out git logs rather than GitHub PR discussions to understand where a change comes from.

[yihuaf]

Any chance we could have a bit more motivation and context in the commit log (mostly for the 2nd commit) please? I find it easier to check out git logs rather than GitHub PR discussions to understand where a change comes from.

Will do :) I will update the PR later tonight or tomorrow.

[yihuaf]

Would you have a sample output for the two versions, by any chance?

Take the exit\n^ in the test case for example, the current error messages are:

Err("Parse error at line 2 column 1: unexpected '^', expected letter or digit, expected whitespaces, expected 'r', expected '-', expected '+', expected '[', expected end of input")`'

The combine implemented display trait:

Err("Parse error at line: 2, column: 1\nUnexpected `^`\nExpected letter or digit, whitespaces, `r`, `-`, `+`, `[` or end of input\n")`

The output are very similar, so if you don't mind the actual format of the message, I would propose we just use the to_string method instead. I added the change as a different commit for you to consider.

[yihuaf]

PTAL

[qmonnet]

Looks all good, thanks a lot!

The output are very similar, so if you don't mind the actual format of the message, I would propose we just use the to_string method instead. I added the change as a different commit for you to consider.

Definitely, let's get rid of this if combine offers it already.