Skip to content

Fix/suppress warnings in kernel and kernel-laws #4614

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 13, 2024
Merged

Fix/suppress warnings in kernel and kernel-laws #4614

merged 2 commits into from
Jun 13, 2024

Conversation

@satorg
Copy link
Contributor

@satorg satorg commented Jun 9, 2024

Supersedes #4187.

This PR fixes or suppresses compiler warnings in the kernel and kernel-laws modules.

@satorg satorg self-assigned this Jun 9, 2024
@satorg satorg mentioned this pull request Jun 9, 2024
Closed
@satorg
Copy link
Contributor Author

satorg commented Jun 9, 2024

Note: not all the warnings have been fixed. There are a bunch of warnings like these below that still remain:

1 |import scala.{specialized => sp}
  |              ^^^^^^^^^^^^^^^^^
  |              unused import

Such warnings get emitted by Scala3 only and the renamed @sp annotations are actually in use. So I believe it is a kind of a bug in Scala3, I filed the issue here: scala/scala3#20536

We can fix them by removing those import renames and using @specialized directly. But I would like to hear from the Scala3 team first.

@satorg satorg added the behind-the-scenes appreciated, but not user-facing label Jun 9, 2024
satorg
satorg commented Jun 9, 2024
View reviewed changes
kernel/src/main/scala/cats/kernel/instances/SeqInstances.scala
StaticMethods.combineAllIterable(Seq.newBuilder[A], xs)
}

object SeqMonoid {
Copy link
Contributor Author

@satorg satorg Jun 9, 2024

Choose a reason for hiding this comment

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

I wonder if we want to not just deprecate SeqMonoid but also make it private [cats]:

cats/kernel/src/main/scala/cats/kernel/instances/SeqInstances.scala

Lines 73 to 74 in 17d7e95

@deprecated("Use SeqMonoid.apply, which does not allocate a new instance", "2.9.0")
class SeqMonoid[A] extends Monoid[Seq[A]] {

Looks like SeqMonoid is not supposed to be user-facing, so it should be a fairly safe change.
It would be out of scope of this PR, of course, but if it makes sense, I can do it here, because it is still related to the warnings cause.

Copy link
Member

@rossabaker rossabaker Jun 12, 2024

Choose a reason for hiding this comment

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

That'd be a source breaking change for whoever used it that way. Even though they shouldn't use it, it's nice to make it very clear how to fix it. I don't tend to like to remove deprecations until keeping them causes pain.

Copy link
Contributor Author

@satorg satorg Jun 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I feel the best strategy here would be to provide a scalafix rule for that. But it would definitely come out of the scope of this PR.

@satorg satorg changed the title [CLEANUP] Fix/suppress warnings in kernel and kernel-laws Fix/suppress warnings in kernel and kernel-laws Jun 10, 2024
@satorg
Copy link
Contributor Author

satorg commented Jun 10, 2024

Removed the [CLEANUP] prefix from the title. I feel the behind-the-scenes label is just enough for it.

@som-snytt
Copy link

som-snytt commented Jun 11, 2024

I believe it is a kind of a bug

and not even the most exotic species.

Thanks for the ticket, I seized the opportunity and submitted a fix for the unused import.

I wonder if they would accept a SIP to add 2-letter aliases for all the standard annotations.

danicheg
danicheg previously approved these changes Jun 11, 2024
View reviewed changes
Copy link
Member

@danicheg danicheg left a comment

Choose a reason for hiding this comment

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

lgtm

rossabaker
rossabaker reviewed Jun 12, 2024
View reviewed changes
kernel/src/main/scala/cats/kernel/instances/SeqInstances.scala
StaticMethods.combineAllIterable(Seq.newBuilder[A], xs)
}

object SeqMonoid {
Copy link
Member

@rossabaker rossabaker Jun 12, 2024

Choose a reason for hiding this comment

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

That'd be a source breaking change for whoever used it that way. Even though they shouldn't use it, it's nice to make it very clear how to fix it. I don't tend to like to remove deprecations until keeping them causes pain.

@satorg satorg force-pushed the suppress-kernel-warnings branch from 1a8aa58 to c2bdebf Compare June 12, 2024 18:15
@satorg satorg requested review from danicheg and rossabaker June 12, 2024 18:36
@satorg satorg mentioned this pull request Jun 13, 2024
Open
@satorg satorg merged commit 62c0dba into typelevel:main Jun 13, 2024
@satorg satorg deleted the suppress-kernel-warnings branch June 13, 2024 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rossabaker rossabaker rossabaker approved these changes

@danicheg danicheg Awaiting requested review from danicheg

Assignees

@satorg satorg

Labels

behind-the-scenes appreciated, but not user-facing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@satorg @som-snytt @rossabaker @danicheg