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

Tune Scala compiler warnings #344

Merged
merged 4 commits into from
Jul 18, 2022
Merged

Conversation

satorg
Copy link
Contributor

@satorg satorg commented Jul 18, 2022

The intent is to keep as many warnings as possible while still preserving backward compatibility between Scala 2.12 and 2.13 series (which is not always possible though).

For more details see a discussion in typelevel/cats-effect#3104.

@satorg
Copy link
Contributor Author

satorg commented Jul 18, 2022

@armanbilge as you created #343 this one may not be actual any more.
Although it still might be useful to understand which compiler warnings we want to keep enabled by default (because apparently sbt-tpolecat won't figure it out automatically).

@armanbilge
Copy link
Member

I just glanced and I like your work here! #343 will take a while to land so I'm very happy to have this working now, it's definitely an improvement. If it works well in Cats and Cats Effect then maybe we can backport to 0.4.x line.

to understand which compiler warnings we want to keep enabled by default (because apparently sbt-tpolecat won't figure it out automatically

What do you mean by this?

@satorg
Copy link
Contributor Author

satorg commented Jul 18, 2022

This PR was tested against the latest cats-effect/series/3.x and has revealed one potential issue that I think is worth to be addressed in the code out there:

[warn] .../typelevel/cats-effect/core/jvm/src/main/scala/cats/effect/IOApp.scala:425:9: match may not be exhaustive.
[warn] It would fail on the following input: (x: AnyRef forSome x not in (Runnable, Throwable, cats.effect.ExitCode, concurrent.CancellationException))
[warn]         result match {
[warn]         ^
[warn] one warning found

Apparently, this match is not exhaustive indeed:
https://github.com/typelevel/cats-effect/blob/e116186a3d008be12f9e814ce77f491e469352f7/core/jvm/src/main/scala/cats/effect/IOApp.scala#L425

Therefore it could be either provided with a default case or marked as @unchecked explicitly.

@satorg
Copy link
Contributor Author

satorg commented Jul 18, 2022

@armanbilge

What do you mean by this?

I mean, for example consider the issue that you ran into recently:

[warn] .../typelevel/cats-effect/core/shared/src/main/scala/cats/effect/Thunk.scala:24:16: private object Thunk in package effect is never used
[warn] private object Thunk {
[warn]                ^

So this warning gets triggered by -Xlint:privates which is available for both 2.12 and 2.13. But the warning is only got triggered for 2.12. I guess it may be due to some 2.12 compiler bug which makes 2.12 detecting some private stuff incorrectly. Therefore most likely we cannot fix it on our side completely, but as a work-around we can disable -Xlint:privates for 2.12 only. But I think it still maybe useful to keep that option for 2.13.

In fact, similar considerations are applicable for -Ywarn-unused:nowarn which has its counterpart for 2.13 -Wunused:nowarn but since the 2.13''s version is way more precise, we may want to disable that 'nowarn' for 2.12.

@armanbilge
Copy link
Member

Got it, thanks for those examples, that's very helpful. cc @DavidGregory084 are these worth adjusting in sbt-tpolecat?

@satorg your changes look good here, I'm going to merge and cut a new milestone. thank you so much!

@armanbilge armanbilge merged commit f588a7c into typelevel:main Jul 18, 2022
@satorg satorg deleted the tune-scalac-warnings branch July 18, 2022 06:43
@satorg satorg mentioned this pull request Jul 19, 2022
@DavidGregory084
Copy link
Member

Sorry, I didn't spot your @ @armanbilge.

-Wunused:privates / -Ywarn-unused:privates have been enabled in sbt-tpolecat since the very first release and I haven't had any other feedback from users suggesting that they should be disabled.

I'm not sure about disabling a warning globally because of the risk of a false positive. Can we adjust these on a case-by-case basis instead? Definitely open to discussing this though - perhaps we should raise an issue on sbt-tpolecat?

@armanbilge
Copy link
Member

@DavidGregory084 no problem, thanks for chiming in.

I haven't had any other feedback from users suggesting that they should be disabled.

Awesome, this is exactly what I was wondering. So not sure why it was annoying for us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants