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

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

Merged
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
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
@@ -83,7 +83,9 @@ class SeqMonoid[A] extends Monoid[Seq[A]] {
}

object SeqMonoid {
Copy link
Contributor Author

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]:

@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

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

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

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
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

@@ -83,7 +83,9 @@ class SeqMonoid[A] extends Monoid[Seq[A]] {
}

object SeqMonoid {
Copy link
Member

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 merged commit 62c0dba into typelevel:main Jun 13, 2024
16 checks passed
@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
Labels
behind-the-scenes appreciated, but not user-facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants