-
Notifications
You must be signed in to change notification settings - Fork 198
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
Add optional arbitrary
integration in geo-types.
#622
Conversation
geo-types/src/arbitrary.rs
Outdated
let x = u.arbitrary::<T>()?; | ||
if x.is_nan() { | ||
return Err(arbitrary::Error::IncorrectFormat); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is value in creating geometries with NaN, but considering we don't have a validity story for geo
, the NaN values get in the way of fuzz testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the NaN values get in the way of fuzz testing
Can you expound on what you mean by "get in the way of fuzz testing"? Is it that we panic
when we plumb through NaN valued Geometries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You know what, let's aim high and include NaN
for now. If it NaN
becomes burdensome, we can always filter them out in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in ffa3f05
let coords = u.arbitrary::<Vec<Coordinate<T>>>()?; | ||
|
||
if coords.len() < 2 { | ||
return Err(arbitrary::Error::IncorrectFormat); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with arbitrary
, but it seems like, to ensure we construct a valid linestring, we do a sort of "guess and check" strategy.
If the guessed vector is less than 2 coords, we return an error, and presumably(?), we'll try again until a Vec of at least length 2 is created.
Is that about right?
Did you look at https://docs.rs/arbitrary/0.4.7/arbitrary/trait.Arbitrary.html#method.size_hint - it seems like it exists to avoid this guess-and-check approach if I'm understanding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great great catch. Addressed in 1c4399f. I think I did that right...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scratch that, maybe a7ad921 makes more sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually no we definitely don't want an upper bound: c2a3a4a
I promise I will stop force pushing now :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh what am I doing t
is not a variable here. Going back to c381187
git_commit_amend_force_push2_last_FINAL_FINAL.doc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a couple of questions, but I think this would be a great (optional) addition!
geo-types/Cargo.toml
Outdated
@@ -15,6 +15,7 @@ use-rstar = ["rstar", "approx"] | |||
|
|||
[dependencies] | |||
approx = { version = "0.4.0", optional = true } | |||
arbitrary = { version = "1.0.0-rc1", optional = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know how pre-release suffixes work with semver?
Are breaking changes allowed between a release candidate and the eventual release? In other words, if we merge this as is, does semver require us to do a breaking rev of geo-types once arbitrary 1.0.0
is released? 😬
I tried looking for a little bit and haven't yet found an answer...
edited for clarity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dunno what semver's official policy is but my guess is yes it would be considered a breaking change. I thought about what you said as I opened this pull request and my feeling is that since it's very unlikely anyone will use this besides us, and that the breakage between 1.0.0-rc1 and 1.0.0 is going to be small, that we should be okay lying a little and just upgrade to 1.0.0 in a geo-types minor release 🤷🏻
If people feel otherwise we can wait until 1.0.0 is released rust-fuzz/arbitrary#62
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nm!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but if the SemVer Police show up, I'm claiming I was coerced.
Status update: gonna wait for the arbitrary 1.0.0 release rust-fuzz/arbitrary#72 |
arbitrary 1.0.0 was released today: https://twitter.com/fitzgen/status/1364644251099820036 i'll try to wrap this pr up in the next day or two |
519fcd1
to
ffa3f05
Compare
a7ad921
to
c2a3a4a
Compare
c2a3a4a
to
c381187
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
bors r=michaelkirk |
Build succeeded: |
CHANGES.md
if knowledge of this change could be valuable to users.Extracted from #532
If people feel this is too niche, this could also just be a separate crate that lives in this repo or a separate one 🤷🏻