Scalafix rule to remove instance imports when upgrading to 2.2.0

[cb372]

Fixes #3563

A Scalafix rule for removing instance imports that become unnecessary in v2.2.0 thanks to #3043

Behaviour is as follows:

• import cats.instances.foo._
  • remove unconditionally
• import cats.implicits._
  • rewrite to import cats.syntax.all._ if there is any use of a syntax extension method within the lexical range of the import statement
  • otherwise remove

What I mean by "lexical range" is the range where the import statement can affect lexical scope, i.e. from the end of the import statement to the end of its enclosing block, method body or class/object body. Not sure if there's an accepted name for this.

Other changes

• Moved the existing v1.0.0 tests and test data into a subdirectory and created separate sbt projects for them. This gives us a template to follow if we want to add Scalafix rules for future migrations (v3.0.0?).
• Bumped sbt-scalafix to the latest version <- Gave up on this is as it caused one of the existing rules to break. Let's save that for another PR.
• Updated the readme for the Scalafix rules

[codecov-commenter]

Codecov Report

Merging #3566 into master will increase coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3566      +/-   ##
==========================================
+ Coverage   91.27%   91.31%   +0.04%     
==========================================
  Files         386      386              
  Lines        8593     8617      +24     
  Branches      255      243      -12     
==========================================
+ Hits         7843     7869      +26     
+ Misses        750      748       -2     

[travisbrown]

This looks good to me, with a couple of comments.

Can you give more details about this?

Gave up on this is as it caused one of the existing rules to break. Let's save that for another PR.

Also the fact that there's so much special-casing needed here for the Future instances makes me think that maybe they should just go in implicit scope too? (Personally I think I'd prefer just to banish them to alleycats, but I also don't think it's the right time to do that.)

[cb372]

Can you give more details about this?

The failing test is the one for the ContraMapToLMap rule. In this line the rule inspects the denotation of a term and checks whether it contains a certain value.

It looks like something in the behaviour of scalafix/scalameta changed between sbt-scalafix 0.9.16 and 0.9.17. The expected value no longer appears in the denotation, so the rule never applies any patches.

I had a quick look at the scalafix and scalameta changelogs but couldn't find a likely culprit for the change.

In the grand scheme of things I don't think this is very important - how many people out there are still running Cats v0.9 and looking to migrate to 1.0.0, and make use of contramap syntax? Probably the simplest solution is just to delete that rule.

Also the fact that there's so much special-casing needed here for the Future instances makes me think that maybe they should just go in implicit scope too? (Personally I think I'd prefer just to banish them to alleycats, but I also don't think it's the right time to do that.)

I don't have a strong opinion on whether the Future instances should remain in core or move to alleycats, especially as I rarely use Future myself. I know this has been discussed at length and I don't really have any insight to add.

But I think keeping them in core but leaving them out of implicit scope, just for the sake of making them a second class citizen, is probably the worst of both worlds. It'll confuse people and lead to increased support burden on gitter.

I would suggest we add them to implicit scope for v2.2.0, and you lobby to get them moved to alleycats in v3.0.0.

[travisbrown]

I was just looking at the Future instances issue again, and remembered that part of the original reason I didn't add them was because there's not currently any Future machinery at all in cats-kernel, and the Monoid and Semigroup instances would have to go there. IIRC when we discussed that last year the consensus was not to introduce the scala.concurrent stuff into cats-kernel just to get the Future instances into implicit scope, which still makes sense to me.

I'm still definitely open to discussion about these instances, but I think this is a real reason to exclude them from implicit scope that isn't just "nobody really likes them".

[cb372]

I see. If there’s a valid technical reason not to add them, that’s fair enough.

In that case I guess there are no changes needed on this PR?

[barambani]

* `import cats.instances.all._`
  
  * remove only if there is no use of a Future instance within the lexical range (see below) of the import statement

assuming (if there are no oversights) that Future's instances are the only one left out of scope, I was wondering if a possible strategy could be to replace the import with

import cats.instances.future._

instead of leaving there all in case Future's are used in the range.
I'm not super familiar with scalafix and I'm not sure how complex that is, so I hope that it makes sense. Thanks.

[cb372]

Good idea. That should be pretty easy to do.

I was also wondering whether, in the case that import cats.implicits._ is used for syntax extensions (but not for Future instances), we should replace it with import cats.syntax.all._.

[mpilquist]

I'm in favor of the Future instances being implicitly available.

[LukaJCB]

Yeah I agree with @mpilquist we should add them to cats-kernel

[cb372]

I've updated the rule in light of Future instances now being available in implicit scope, so this is ready for another review.

Also updated the PR description to match the new behaviour.

[cb372]

@fthomas I think you ran into scalacenter/scalafix#1123. I don't think there's much we can do about it, but as a workaround I added a try-catch to log it when it happens and stop it from crashing scalafix.

Also added another test.

[fthomas]

Thanks for adding the workaround, @cb372! I'll try out the new version again ASAP.

[fthomas]

@cb372 The scalafix now succeeds but the build still fails because some imports of cats.implicits._ are not replaced with imports of cats.syntax.all._. One example is https://github.com/scala-steward-org/scala-steward/pull/1582/files#diff-b90dc7bb760a09e4a4e86ba83ab0715a where flatMap on F[_] is required for the for comprehension:

[error] /home/frank/data/code/scala-steward/core/modules/core/src/main/scala/org/scalasteward/core/application/SelfCheckAlg.scala:29:23: value flatMap is not a member of type parameter F[Unit]
[error]       _ <- logger.info("Run self checks")
[error]                       ^
[error] /home/frank/data/code/scala-steward/core/modules/core/src/main/scala/org/scalasteward/core/application/SelfCheckAlg.scala:30:12: value map is not a member of type parameter F[Unit]
[error]       _ <- checkHttpExistenceClient
[error]            ^

Adding import cats.syntax.all._ to this file fixes these errors.

[fthomas]

a1b8d09 fixed 100 errors in my test, only 9 errors are remaining. They are all similar to this (source file):

[error] /home/travis/build/scala-steward-org/scala-steward/modules/core/src/main/scala/org/scalasteward/core/buildtool/sbt/parser.scala:36:47: No implicit Ordering defined for org.scalasteward.core.data.Resolver.
[error]       else Some(Scope(dependencies, resolvers.sorted))
[error]                                               ^

In this file cats.implicits._ was replaced with cats.syntax.all._ and it seems that the Order.catsKernelOrderingForOrder is now missing in the implicit scope.

[cb372]

@fthomas your build should pass this time 🤞

[fthomas]

Yes it did! Many thanks for the quick fixes, @cb372! ❤️

[fthomas]

This rule can now be tested really easily by anyone who is using Scala Steward: just bump Cats to 2.2.0-RC3 and Scala Steward will use this rule for the update to 2.2.0-RC4.

My tests with refined (fthomas/refined#804) and scala-steward (scala-steward-org/scala-steward#1590) were successful.

[travisbrown]

Thanks, @fthomas and @cb372! Should we go ahead and merge this?

[cb372]

@travisbrown Yep, I'd say this is ready to merge

Sorry I think I confused the GitHub check by putting [skip ci] in my final commit message. I'll just do a force-push to try and fix that.

[mzuehlke]

You can use this query to get a list of all migrations done by Scala Steward to cats 2.2.0-RC4:

https://github.com/search?q=author%3Ascala-steward+is%3Apr+cats+2.2.0-RC4+Applied+Migrations

[LukaJCB]

This is pretty cool, thanks!