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

Implement toList and toNonEmpty for SCC #1057

Merged
merged 1 commit into from
Oct 27, 2024
Merged

Conversation

meooow25
Copy link
Contributor

@meooow25 meooow25 commented Oct 26, 2024

The default implementations perform an avoidable list copy for NECyclicSCC. Also

  • Implement null, which is always False
  • Fix flattenSCC being made too lazy accidentally in a previous commit

@meooow25
Copy link
Contributor Author

The laziness bug slipped in in #987.
Note to self: Be extra careful whenever any Data.List.NonEmpty function is involved (haskell/core-libraries-committee#107)

@@ -220,11 +220,18 @@ instance F.Foldable SCC where
foldr c n (AcyclicSCC v) = c v n
foldr c n (NECyclicSCC vs) = foldr c n vs

null _ = False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine GHC can figure this out from the default, but I agree it's nice to make it clear.

Copy link
Contributor Author

@meooow25 meooow25 Oct 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently it doesn't. However, I realized that we're making null lazier here. The default definition using foldr would need to inspect the SCC. Is this the right thing to do? I'm not sure now, though many Foldable instances in base do this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh. I hate those kinds of decisions. I think the change is probably fine, but must be logged. null isn't normally used to force computations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some thought, I feel it's best to let this lie. It is very unlikely someone is using null on an SCC, so this is unlikely to help someone with performance. But in case they are, this is an unnecessary breaking change to deal with.
Let me know if you feel otherwise.

@treeowl
Copy link
Contributor

treeowl commented Oct 26, 2024

I've said it before and I'll say it again: NonEmpty is the wrong default representation for a non-empty list in a lazy-by-default language. Having conversions to and from standard lists be so cheap is a cute party trick, but actually working with the things is one painful judgement call after another.

@meooow25
Copy link
Contributor Author

Isn't this just a case of functions being too lazy? I don't see a problem with the definition of the type. What would you prefer?

@treeowl
Copy link
Contributor

treeowl commented Oct 26, 2024

Isn't this just a case of functions being too lazy? I don't see a problem with the definition of the type. What would you prefer?

Modulo naming, I would personally prefer

data NE a = End a | a :< NE a

instance Foldable1 NE where
  -- foldrMap1 :: (a -> b) -> (a -> b -> b) -> NE a -> b
  foldrMap1 e _c (End a) = e a
  foldrMap1 e c (a :< as) = a `c` foldrMap1 e c as

There would still be judgement calls for some things, but I suspect there would be more "natural" defaults. There are two options for uncons:

uncons1, uncons2 :: NE a -> (a, Maybe (NE a))

uncons1 (End a) = (a, Nothing) 
uncons1 (a :< as) = (a, Just as)

uncons2 q = (a, r)
  where
    (a, r) = uncons1 q

Doesn't uncons1 feel more obvious, with uncons2 going above and beyond? With NonEmpty, there's a nested judgement call.

Zips are a bit more subtle, because we have the option of being somewhat lazy in the second list. Let's recons for convenience:

recons :: a -> Maybe (NE a) -> NE a
recons a Nothing = End a
recons a (Just as) = a :< as

zipWith1 f (uncons1 -> (a, mas)) (uncons1 -> (b, mbs)) = recons (f a b) $ do
  as <- mas
  bs <- mbs
  Just $ zipWith1 f as bs

zipWith2 f (uncons1 -> (a, mas)) (uncons1 -> ~(b, mbs)) = recons (f a b) $ do
  as <- mas
  bs <- mbs
  Just $ zipWith2 f as bs

zipWith1 feels much more natural to me. zipWith2 has "short circuiting" like list zips, but if the second argument hits bottom when the first ends, there will usually be a bottom in the last element of the zipped list—weird.


Obviously, there's no objective way to choose between one and another, but the relationship between NE and foldrMap1 is a particularly pretty thing.

@meooow25
Copy link
Contributor Author

I see, certainly this type is better suited for some situations, though I'm not convinced that it should be the default.
Anyway, we're stuck with the NonEmpty we have today.

The default implementations perform an avoidable list copy for
NECyclicSCC. Also fix flattenSCC being made too lazy accidentally in a
previous commit.
@meooow25 meooow25 merged commit 9a9e210 into haskell:master Oct 27, 2024
12 checks passed
@meooow25 meooow25 deleted the scc-tolist branch October 27, 2024 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants