Skip to content
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

foldl' and foldr' for Seq are not strict in the starting value #1072

Open
Tracked by #1059
meooow25 opened this issue Nov 21, 2024 · 3 comments · May be fixed by #1077
Open
Tracked by #1059

foldl' and foldr' for Seq are not strict in the starting value #1072

meooow25 opened this issue Nov 21, 2024 · 3 comments · May be fixed by #1077

Comments

@meooow25
Copy link
Contributor

This is unlike every other structure we have.

λ> import qualified Data.Foldable as F
λ> import qualified Data.Set as S -- or Map or IntSet or IntMap
λ> F.foldl' (\_ _ -> ()) undefined (S.singleton ())
*** Exception: Prelude.undefined
CallStack (from HasCallStack):
  ...
λ> F.foldr' (\_ _ -> ()) undefined (S.singleton ())
*** Exception: Prelude.undefined
  ...
λ> import qualified Data.Sequence as Seq
λ> F.foldl' (\_ _ -> ()) undefined (Seq.singleton ())
()
λ> F.foldr' (\_ _ -> ()) undefined (Seq.singleton ())
()

I think we should make these strict.

Perhaps it's worth mentioning that for lists, foldl' is strict in the starting value but foldr' is not (GHC #25508).

@meooow25
Copy link
Contributor Author

@treeowl do you have opinions on this?

@treeowl
Copy link
Contributor

treeowl commented Nov 22, 2024

The whole story is unfortunate. Historically, foldl' was lazy in the starting value for lists. Some other structures made it strict (I think that included HashMap, but I don't remember for sure). Then foldl' for lists became strict—by accident. Do we follow the trend? Probably yes. It's usually better that way, and there probably aren't many relying on the laziness.

@meooow25
Copy link
Contributor Author

Interesting, I found your issue for the accidental change: GHC #12173.
It seems that no one has complained about the change since, and there is one comment in favor of it.
I expect it would be fine to do the same for Seq too then.

@meooow25 meooow25 mentioned this issue Nov 23, 2024
9 tasks
@meooow25 meooow25 linked a pull request Nov 30, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants