Repository navigation
Stream correctness and doc fixes - #3260
Merged
Merged
Conversation
harendra-kumar
commented
Oct 16, 2026
harendra-kumar
commented
Member
- enumerateFromToNum could go past the limit for non-integral steps. 1.1 .. 3.2 gave an extra 4.1.
- Enumerating Natural downwards no longer underflows.
- Ratio and Fixed now enumerate the same way as their Enum instances.
- foldr1 combined the elements in the wrong order.
- isInfixOf returned False for an empty infix on short streams.
- The Show instances of Stream.Nested and StreamK.Nested now respect precedence.
- modifyIndex and unsafeModifyIndex are now inlined.
- Corrected the docs of concatEffect, fromFoldableM, unionBy, deleteFirstsBy and insertBy.
The instances defined only show, so the default showsPrec ignored the precedence: show (Just n) produced "Just fromList [1,2]", which the Read instance cannot parse. Delegate showsPrec to the Stream and StreamK instances, which use showParen, the same as the Array fix. Keep show, delegating to an explicit show in the Stream and StreamK instances, the same as the Array instance.
foldr1 used the head of the stream as the seed of a right fold over the tail, so the head was combined last: foldr1 (-) on [1,2,3] computed 2 - (3 - 1) = 0 instead of 1 - (2 - 3) = 2, and foldr1 const returned the second element. The existing tests used only (+). Fold the whole stream with a Maybe accumulator instead.
isInfixOf splits the stream on the infix and checks for a second segment. An empty pattern splits the stream at every element, so a stream of fewer than two elements has fewer than two segments: isInfixOf nil nil and isInfixOf nil (fromList [1]) returned False. An empty infix occurs in every stream; return True for it directly.
enumerateFromToNum yielded x and continued with x + 1 whenever x < to, without checking x + 1 <= to. When to - from is not a whole number the last element exceeded the limit: enumerateFromToNum 1.1 3.2 gave [1.1,2.1,3.1,4.1], and enumerateFromTo (1 :: Rational) (12 % 5), which the Ratio and Fixed Enumerable instances route to enumerateFromToNum, gave [1,2,3] instead of [1,2]. enumerateDownFromToNum had the mirror bug. Fix the Stream functions and the Producer copies used by Unfold.
enumerateFromThenToNum computed the stride as next - from, which throws an arithmetic underflow for Natural when next < from: enumerateFromThenTo (5 :: Natural) 3 0 threw instead of returning [5,3,1]. enumerateFromThen for Natural had the same problem and, being unbounded below, could not stop at 0. For the downward direction use the positive stride from - next and the limit to + stride, stepping by subtraction. For fixed width types this gives the same values modulo 2^n as before, so bounded types are unaffected. Give Natural its own Enumerable instances whose enumerateFromThen stops at 0 when enumerating downwards. Fix the Stream functions and the Producer copy used by Unfold.
The Ratio and Fixed Enumerable instances used enumerateFromToNum and enumerateFromThenToNum, which stop at the last element not beyond the limit. Their Enum instances do not: - Ratio rounds the limit to the nearest step, [1 .. 5 % 2] is [1,2,3]. enumerateFromTo and enumerateFromThenTo now keep the elements up to to + stride / 2, using repeated addition since Ratio is exact. - Fixed enumerates the underlying Integer, i.e. in steps of the resolution, [1.5 .. 1.503 :: Milli] is [1.500,1.501,1.502,1.503]. All four methods now map over the Integer instance. enumerateFromToNum and enumerateDownFromToNum (and the Producer copies used by Unfold) checked both to > x and x + 1 <= to on every element. They now compute to - 1 (to + 1 downwards) once at the start and compare x <= to - 1 in the loop, one comparison per element as before the non-integral fix. to - 1 is computed only when from + 1 <= to, so neither to - 1 nor x + 1 overflows for bounded types or underflows for Natural. In the Stream versions the loop compares x <= to - 1 directly, to - 1 is neither a field of the state nor a binding in the closure. A larger state would be allocated on each element of an unfused stream, increasing the allocation of the equations/concatFor (bounded) benchmark by 2.4%. A strict binding would wrap the Stream constructor in a case, a stream used twice then does not fuse with StreamK.fromStream, increasing the allocation of the equations/fairConcatForK (bounded) benchmark by 9.5%. The extra Init branches are duplicated in each copy of the producer, the core size of the equations/unfoldCross (bounded) benchmark grows from about 17.8k to 20.5k terms, 23.6k with GHC 9.14.1, raise its MaxCoreSize from 20000 to 26000. Its run time is unchanged.
…insertBy docs The concatEffect note referred to Streamly.Internal.Data.Stream.Nesting.ifThenElse, which does not exist; ifThenElse is defined in Stream.Type. The fromFoldableM doc said it constructs a stream from a Foldable containing pure values, copied from fromFoldable; its input is a Foldable of monadic actions. The documented unionBy definition passed the arguments of deleteFirstsBy in the wrong order, deleting the elements of the second stream from the first stream. For s1 = [1,2,2,4] and s2 = [1,1,2,3,3] it gave [1,2,2,4,2,4] instead of [1,2,2,4,3], which is what unionBy returns. In deleteFirstsBy each element of the second stream deletes one occurrence, so the documented laws did not hold: - deleteFirstsBy (==) (ordNub s2 `append` s1) s2 with s2 = [1,1] and s1 = [1] gives [], not [1]. Without ordNub the law holds for any s2. - deleteFirstsBy (==) (ordNub s2 `interleave` s1) s2 with s2 = [9,8,7,1] and s1 = [1,2] gives [2,1], not [1,2], because the 1 from s1 comes first in the interleaved stream and is deleted. Keep the append law without ordNub and drop the interleave law. The implementation buffers the second stream, not the first, so the space is O(n) in the second stream. The insertBy docs said the element is inserted before the first element that is less than it. insertBy inserts it before the first element x with cmp elem x /= GT, i.e. the first element greater than or equal to it, and the doc example insertBy compare 2 [1,3,5] = [1,2,3,5] shows that.
Without INLINE, modifyIndex was called through the Unbox dictionary and boxed its result. In a BFS over a graph of 10^6 nodes using a MutArray Bool visited bitmap, inlining it reduced the time from 270-290 ms to about 100 ms and the allocation about 9 times.
harendra-kumar
force-pushed
the
stream-fixes
branch
from
October 16, 2026 23:49
6e0c72d to
b83929c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.