From e4c7c747abe2979cb6ea09c07f7d460e1adb2de1 Mon Sep 17 00:00:00 2001 From: stefandd Date: Fri, 27 May 2022 16:48:44 +0200 Subject: [PATCH 1/7] Fixes #4126 - bug in Range 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) --- packages/collections/range.pony | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/collections/range.pony b/packages/collections/range.pony index 6846ad34a8..59ac2789ef 100644 --- a/packages/collections/range.pony +++ b/packages/collections/range.pony @@ -93,11 +93,12 @@ class Range[A: (Real[A] val & Number) = USize] is Iterator[A] or ((_min > _max) and (_inc > 0)) fun has_next(): Bool => - if _forward then - _idx < _max - else - _idx > _max - end + _infinite or + if _forward then + _idx < _max + else + _idx > _max + end fun ref next(): A ? => if has_next() then From 31068cc50f227ffb18c5cb93a3a026f1af951353 Mon Sep 17 00:00:00 2001 From: stefandd Date: Fri, 27 May 2022 20:17:02 +0200 Subject: [PATCH 2/7] New tests related to bug #4127 The extens the _assert_infinite() function in packages/collections/_test.pony to detect bugs in infinite Ranges --- packages/collections/_test.pony | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/packages/collections/_test.pony b/packages/collections/_test.pony index 878b15465d..8992418ead 100644 --- a/packages/collections/_test.pony +++ b/packages/collections/_test.pony @@ -663,7 +663,17 @@ class \nodoc\ iso _TestRange is UnitTest let range: Range[N] = Range[N](min, max, step) let range_str = "Range(" + min.string() + ", " + max.string() + ", " + step.string() + ")" - h.assert_true(range.is_infinite(), range_str + " should be infinite") + h.assert_true(range.is_infinite(), range_str + " should be infinite") + try + range.next()? + else + h.fail(range_str + ".next(): first call should not fail but should produce " + min.string()) + end + try + range.next()? + else + h.fail(range_str + ".next(): subsequent call should not fail") + end fun _assert_range[N: (Real[N] val & Number)]( h: TestHelper, From 2f8269805e0c867de7950a2fa6071df680b7e8f9 Mon Sep 17 00:00:00 2001 From: stefandd Date: Fri, 27 May 2022 20:21:11 +0200 Subject: [PATCH 3/7] Adjusted style in has_next() --- packages/collections/range.pony | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/packages/collections/range.pony b/packages/collections/range.pony index 59ac2789ef..1b22cefb40 100644 --- a/packages/collections/range.pony +++ b/packages/collections/range.pony @@ -93,12 +93,14 @@ class Range[A: (Real[A] val & Number) = USize] is Iterator[A] or ((_min > _max) and (_inc > 0)) fun has_next(): Bool => - _infinite or - if _forward then - _idx < _max - else - _idx > _max - end + if _infinite then + return true + end + if _forward then + _idx < _max + else + _idx > _max + end fun ref next(): A ? => if has_next() then From 091fa71e46f925ab0af3630f2720871383b6aa5f Mon Sep 17 00:00:00 2001 From: stefandd Date: Fri, 27 May 2022 21:46:39 +0200 Subject: [PATCH 4/7] Further small improvement to _test.pony In addition to test the first .next() to not fail, a value assertion for the expected result this call (min) was added --- packages/collections/_test.pony | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/collections/_test.pony b/packages/collections/_test.pony index 8992418ead..60d03c0295 100644 --- a/packages/collections/_test.pony +++ b/packages/collections/_test.pony @@ -652,8 +652,6 @@ class \nodoc\ iso _TestRange is UnitTest _assert_infinite[F64](h, 0, 10, p_inf) _assert_infinite[F64](h, 100, 10, n_inf) - - fun _assert_infinite[N: (Real[N] val & Number)]( h: TestHelper, min: N, @@ -665,7 +663,8 @@ class \nodoc\ iso _TestRange is UnitTest h.assert_true(range.is_infinite(), range_str + " should be infinite") try - range.next()? + let nextval = range.next()? + h.assert_eq[N](nextval, min) else h.fail(range_str + ".next(): first call should not fail but should produce " + min.string()) end From b0bc28c6987a32dc01f2f169a00250676a474b97 Mon Sep 17 00:00:00 2001 From: stefandd Date: Sat, 28 May 2022 16:59:26 +0200 Subject: [PATCH 5/7] Create 4127.md --- .release-notes/4127.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 .release-notes/4127.md diff --git a/.release-notes/4127.md b/.release-notes/4127.md new file mode 100644 index 0000000000..5173820917 --- /dev/null +++ b/.release-notes/4127.md @@ -0,0 +1,13 @@ +## Fix for infinite Ranges + +Even though all Range objects that contain NaN parameters are considered infinite, some did not iterate at all or produced an error after the first iteration. For example, the code from the Range documentation: + +```pony + // this Range will produce 0 at first, then infinitely NaN + let nan: F64 = F64(0) / F64(0) + for what_am_i in Range[F64](0, 1000, nan) do + wild_guess(what_am_i) + end +``` + +did in fact not run as expected, but prodcued an error on the first iteration. This is now fixed, and .next() calls on the above `Range[F64](0, 1000, nan)` now first yields 0 and subsequently indefinetely NaN values. From 584b784c8ab36edb38dffeb5faac6ec40f1ba952 Mon Sep 17 00:00:00 2001 From: stefandd Date: Sat, 28 May 2022 17:02:00 +0200 Subject: [PATCH 6/7] Update 4127.md --- .release-notes/4127.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.release-notes/4127.md b/.release-notes/4127.md index 5173820917..51ac0fcf7f 100644 --- a/.release-notes/4127.md +++ b/.release-notes/4127.md @@ -10,4 +10,4 @@ Even though all Range objects that contain NaN parameters are considered infinit end ``` -did in fact not run as expected, but prodcued an error on the first iteration. This is now fixed, and .next() calls on the above `Range[F64](0, 1000, nan)` now first yields 0 and subsequently indefinetely NaN values. +did not run as expected, but rather produced an error on the first iteration. This is now fixed, and .next() calls on the above `Range[F64](0, 1000, nan)` now first yields 0 and subsequently indefinetely NaN values. From 3a1769c360ad4d0dcbca9e6b41d7642c58ceed94 Mon Sep 17 00:00:00 2001 From: stefandd Date: Tue, 7 Jun 2022 01:08:12 +0200 Subject: [PATCH 7/7] Update 4127.md --- .release-notes/4127.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.release-notes/4127.md b/.release-notes/4127.md index 51ac0fcf7f..dbdac65cf4 100644 --- a/.release-notes/4127.md +++ b/.release-notes/4127.md @@ -1,6 +1,6 @@ ## Fix for infinite Ranges -Even though all Range objects that contain NaN parameters are considered infinite, some did not iterate at all or produced an error after the first iteration. For example, the code from the Range documentation: +Even though all Range objects that contain NaN parameters or a zero step parameter are considered infinite, some did not iterate at all or produced an error after the first iteration. For example, the code from the Range documentation: ```pony // this Range will produce 0 at first, then infinitely NaN @@ -10,4 +10,4 @@ Even though all Range objects that contain NaN parameters are considered infinit end ``` -did not run as expected, but rather produced an error on the first iteration. This is now fixed, and .next() calls on the above `Range[F64](0, 1000, nan)` now first yields 0 and subsequently indefinetely NaN values. +did not run as expected, but rather produced an error on the first iteration. This is now fixed, and .next() calls on the above `Range[F64](0, 1000, nan)` now first yields 0 and subsequently indefinetely NaN values. Likewise, `Range(10, 10, 0)` will now indefinitely yield `10`.