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

keep fixing warnings in 'core' #571

Merged
merged 7 commits into from
Jan 15, 2023
Merged

Conversation

satorg
Copy link
Contributor

@satorg satorg commented Dec 21, 2022

For #486 (a follow-up to #569).


def unapply[A](heap: Heap[A]): Boolean = heap.isEmpty

final private[collections] case class Leaf[A] private () extends Heap[A] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pretty much the same fix as it was in Diet.scala for the "match may not be exhaustive" warnings.

@satorg
Copy link
Contributor Author

satorg commented Dec 21, 2022

Interesting.. There are no tests reported as failed but rather

FATAL ERROR: Reached heap limit Allocation failed - JavaScript heap out of memory

https://github.com/typelevel/cats-collections/actions/runs/3751710955/jobs/6373035392#step:14:672

@armanbilge
Copy link
Member

Right, that means the Node.js VM crashed. Maybe one of the generators is making too much data?

@satorg
Copy link
Contributor Author

satorg commented Jan 11, 2023

Maybe one of the generators is making too much data?

Actually there was a mistake I made in the Range.toIterator implementation when was getting rid of Stream. The issue led to infinite loop in certain (quite rare) circumstances. A funny thing about it is that it was happening not in RangeSuite but rather in DietSuite.

However, when I started debugging it I noticed that there are similar issues with Range.foreach along with foldLeft and foldRight. The latter bug slipped through unnoticed because there were no tests that could reproduce necessary conditions for the bug to show up. So I decided to rewrite RangeSuite to make it using scalacheck and cover as many possible case as I could come up with.

Comment on lines +111 to +118
// Visit values in range [start, end), i.e. excluding `end`.
while (order.lt(i, end)) {
f(i)
i = discrete.succ(i)
}
// Visit the last (or the only) value, if any.
if (order.eqv(i, end))
f(i)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous implementation was resulting to infinite loops in cases like

Range(123.toByte, Byte.MaxValue).foreach(_ => ())

I.e. due to integer overflows that could not be caught previously.

@satorg
Copy link
Contributor Author

satorg commented Jan 11, 2023

Hmm...

[error]  * the type hierarchy of object cats.collections.PairingHeap#Leaf is different in current version. Missing types {cats.collections.PairingHeap}
[error]    filter with: ProblemFilters.exclude[MissingTypesProblem]("cats.collections.PairingHeap$Leaf$")

not sure, why Mima gets triggered on PairingHeap.Leaf which is private and always has been that way...

Moreover, I did pretty much the same trick for Diet last time and there were no Mima errors about it.
@armanbilge do you have a clue by chance?

@armanbilge
Copy link
Member

not sure, why Mima gets triggered on PairingHeap.Leaf which is private and always has been that way...

MiMa has some issues sometimes with private classes inside objects. It should be safe to exclude.

@armanbilge
Copy link
Member

Hmm, I see MiMa is complaining about many things. We've already discussed breaking bincompat in #277 (comment), maybe we should just bump the base version to 1.0?

@codecov-commenter
Copy link

Codecov Report

Merging #571 (4e32c3a) into master (fd00af8) will not change coverage.
The diff coverage is n/a.

❗ Current head 4e32c3a differs from pull request most recent head 940873a. Consider uploading reports for the commit 940873a to get more accurate results

@@      Coverage Diff      @@
##   master   #571   +/-   ##
=============================
=============================

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@satorg
Copy link
Contributor Author

satorg commented Jan 15, 2023

@armanbilge

Hmm, I see MiMa is complaining about many things.

They are all about PairingHeap.Leaf and its internals. So seems I managed to calm Mima down with just two filters.

But yeah, it is quite weird that we have to have those filters for it after all. The only difference between Diet.EmptyNode and PairingHeap.Leaf I can spot is that the former's visibility is private[collections] wheres the latter's is just private. I can guess that the compiler may optimize private members out more aggressively comparing to package-local ones. But it's just my guess – not sure if I want to dig into it further :)

@armanbilge
Copy link
Member

former's visibility is private[collections] wheres the latter's is just private

Yes, I've noticed similar behavior too. My guess is that private is Java-style package private (i.e. non-public) but private[package] is Scala-private but Java-public. Which is why MiMa considers them differently.

@gemelen gemelen merged commit 5e34f6c into typelevel:master Jan 15, 2023
@satorg satorg deleted the fix-even-more-warns branch January 15, 2023 20:41
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.

4 participants