test.check

clojure.test.check.generators/rand-range includes upper bound

Details

  • Type: Defect Defect
  • Status: Resolved Resolved
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None

Description

While playing with the rand-range function in the clojure.test.check.generators namespace, I was surprised that

(range-range lower upper)

can generate integers equal to the upper bound. This behavior is inconsistent with the zero-based numbering convention that ranges of integers are lower-inclusive, upper-exclusive.

I know that this function appears in a section that is commented

;; Internal Helpers

but it isn't a private function, and it is useful for users who need to implement custom generators. Can it be changed? It looks like the choose function in the same namespace is the only other function that would need to be changed too, along with unit tests.

If others agree that this is a bug that ought to be fixed, I volunteer to submit a patch, after I provide my signed Contributor Agreement.

Activity

Hide
Steve Kim added a comment -

Sorry for the typo. I meant

(rand-range rnd lower upper)
Show
Steve Kim added a comment - Sorry for the typo. I meant
(rand-range rnd lower upper)
Hide
Reid Draper added a comment -

Both Erlang and Haskell QuickCheck do this too. I'll admit I mostly just followed in their steps, and that's it's a bit unexpected, compared to Clojure functions like 'range'. However, without being inclusive on the top-end, you wouldn't be able to create generators that fill an entire 'type range'. For example: (gen/sample (gen/choose Integer/MIN_VALUE Integer/MAX_VALUE)). Thoughts?

Show
Reid Draper added a comment - Both Erlang and Haskell QuickCheck do this too. I'll admit I mostly just followed in their steps, and that's it's a bit unexpected, compared to Clojure functions like 'range'. However, without being inclusive on the top-end, you wouldn't be able to create generators that fill an entire 'type range'. For example: (gen/sample (gen/choose Integer/MIN_VALUE Integer/MAX_VALUE)). Thoughts?
Hide
Chas Emerick added a comment -

I agree that it's surprising at first, but the utility is undeniable (compared to e.g. range as mentioned, the behaviour of which you need to compensate for in various ways in order to include upper bounds).

Show
Chas Emerick added a comment - I agree that it's surprising at first, but the utility is undeniable (compared to e.g. range as mentioned, the behaviour of which you need to compensate for in various ways in order to include upper bounds).
Hide
Jean Niklas L'orange added a comment -

On the visibility of this function: As far as I can see, rand-range is just used once in generators, and not used anywhere else. Whereas it is a useful function in general, I don't think it is that useful for people who need to implement custom generators (You can bind/fmap over choose instead).

I assume it wouldn't be a problem to make this private.

Show
Jean Niklas L'orange added a comment - On the visibility of this function: As far as I can see, rand-range is just used once in generators, and not used anywhere else. Whereas it is a useful function in general, I don't think it is that useful for people who need to implement custom generators (You can bind/fmap over choose instead). I assume it wouldn't be a problem to make this private.
Hide
Steve Kim added a comment -

I did not know the QuickCheck convention, and I had not considered the need to fill an entire type range like

(gen/sample (gen/choose Integer/MIN_VALUE Integer/MAX_VALUE))
. Thanks for the explanation!

The docstring for choose states that it is inclusive, whereas rand-range is not documented. I think that new users would be less confused if either (1) rand-range had a docstring, or (2) rand-range were made private, because it only makes sense as a helper to implement the choose function. This is a trivial nitpick; I won't complain if this issue is resolved as Won't fix.

Show
Steve Kim added a comment - I did not know the QuickCheck convention, and I had not considered the need to fill an entire type range like
(gen/sample (gen/choose Integer/MIN_VALUE Integer/MAX_VALUE))
. Thanks for the explanation! The docstring for choose states that it is inclusive, whereas rand-range is not documented. I think that new users would be less confused if either (1) rand-range had a docstring, or (2) rand-range were made private, because it only makes sense as a helper to implement the choose function. This is a trivial nitpick; I won't complain if this issue is resolved as Won't fix.
Hide
Reid Draper added a comment -

Sounding like we're all in favor of making range-range private. I'm happy to take a patch if someone wants to contribute. Otherwise I'll fix it today or tomorrow.

Show
Reid Draper added a comment - Sounding like we're all in favor of making range-range private. I'm happy to take a patch if someone wants to contribute. Otherwise I'll fix it today or tomorrow.
Hide
Jean Niklas L'orange added a comment -

I have attached the straightforward patch to this issue.

There is not much to elaborate on, only that the test suite now refers to the var containing the function, not the function itself (as it now is private).

Show
Jean Niklas L'orange added a comment - I have attached the straightforward patch to this issue. There is not much to elaborate on, only that the test suite now refers to the var containing the function, not the function itself (as it now is private).
Hide
Reid Draper added a comment -

Fixed in 364710da692e66fca7310bade4388cf9439e3ef1, thanks!

Show
Reid Draper added a comment - Fixed in 364710da692e66fca7310bade4388cf9439e3ef1, thanks!
Hide
Reid Draper added a comment -

364710da692e66fca7310bade4388cf9439e3ef1

Show
Reid Draper added a comment - 364710da692e66fca7310bade4388cf9439e3ef1

People

Vote (1)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: