Fix for infinite Ranges

[stefandd]

The has_next() function in Range relies entirely on the variable _forward to decide whether or not another iteration exists. However, _forward is too narrowly defined to properly cover some or some _infinite == true cases. Currently some cases of step being 0 or NaN depending on min > or < max were broken (ie. not infinite)

[SeanTAllen]

Can you include a test to prevent regression in the future? I assume that it is possible to write such a test.

[ponylang-main]

Hi @stefandd,

The changelog - fixed label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do.

Release notes are added by creating a uniquely named file in the .release-notes directory. We suggest you call the file 4127.md to match the number of this pull request.

The basic format of the release notes (using markdown) should be:

## Title

End user description of changes, why it's important,
problems it solves etc.

If a breaking change, make sure to include 1 or more
examples what code would look like prior to this change
and how to update it to work after this change.

Thanks.

[SeanTAllen]

The title for this PR will be in the CHANGELOG and as such should have a human meaningful title that is descriptive unto itself. @stefandd do you have suggestions for such a title/changelog entry?

[stefandd]

As for the title: the fix addresses the cases of infinite Ranges in which NaN was used for the step. The problem is due to the lack of a sign of NaN (unlike +Inf or -Inf) and the dependence of the has_next() function on the `step` parameter being either > 0 or < 0-- which NaN can't be. Therefore, I'd say "Fix for Range with NaN step parameters" or something similar would get pretty close.

[stefandd]

Can you include a test to prevent regression in the future? I assume that it is possible to write such a test.

I extended the _assert_infinite() function in packages/collections/_test.pony to now test for both the first AND the second call to .next() being valid in case of infinite Ranges. So far, tests for .next() calls were badly missing for infinite Ranges (see number of failed tests below)!

I test for the second .next() to be valid since firstly, one could infer from 2 to infinity :-), but secondly and more importantly because a side effect of the _forward = (_min < _max) and (_inc > 0) is that it is always false for step=NaN or =0. When now a Range has a _min > _max such as for example Range[F64](1000, 0, nan), it gets through the first .next() call due to this by "luck", but starts failing on the second .next() call. So the first Range[F64](1000, 0, nan).next() call returns 1000, but the next one causes an error. With the modified _assert_infinite(), this is now caught!

...
**** FAILED: collections/Range
Range(0, 1, -1).next(): first call should not fail but should produce 0
Range(0, 1, -1).next(): subsequent call should not fail
Range(0, 1, 0).next(): first call should not fail but should produce 0
Range(0, 1, 0).next(): subsequent call should not fail
Range(-0.1, 0.2, -0.1).next(): first call should not fail but should produce -0.1
Range(-0.1, 0.2, -0.1).next(): subsequent call should not fail
Range(nan, 0, 1).next(): first call should not fail but should produce nan
Range(nan, 0, 1).next(): subsequent call should not fail
Range(0, nan, 1).next(): first call should not fail but should produce 0
Range(0, nan, 1).next(): subsequent call should not fail
Range(0, 100, nan).next(): first call should not fail but should produce 0
Range(0, 100, nan).next(): subsequent call should not fail
Range(10, inf, 0).next(): first call should not fail but should produce 10
Range(10, inf, 0).next(): subsequent call should not fail
Range(0, 10, inf).next(): subsequent call should not fail
Range(100, 10, -inf).next(): subsequent call should not fail
---- Passed: collections/RingBuffer
---- Passed: collections/Sort
----
---- 25 tests ran.
---- Passed: 24
**** FAILED: 1 test, listed below:
**** FAILED: collections/Range

With the fix, all tests pass!

[jemc]

I'm not sure why the "arm64 Apple Darwin" job failed, so I've rerun the CI job.

[stefandd]

@jemc I am not sure about the GitHub and PonyC etiquette: was it proper to request the review by all 3 reviewers after you approved? I saw 3 assigned reviewers and thought that this wass the right thing to do.

[ergl]

@stefandd It's fine. I've been a bit busy so I won't probably get the time to properly review this until Sunday.