Closed yihuaf closed 1 year ago
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 is4.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 theascii
crate. However, the latest version ofcombine
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 thedisplay
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 correctexpected end of input
, but also contain a number of other errors from previousmany
parser. I filed the issue: Marwes/combine#355 to clarify with thecombine
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.
:+1:
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.
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.
PTAL
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 is4.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.
The older version of
combine
uses theascii
crate. However, the latest version ofcombine
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 thedisplay
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 correctexpected end of input
, but also contain a number of other errors from previousmany
parser. I filed the issue: https://github.com/Marwes/combine/issues/355 to clarify with thecombine
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.