Avoid syntax methods in implementations in cats-core

[travisbrown]

This weekend I ran into a Dotty bug that turned up because of our use of cats.syntax methods in cats.data. The issues are easy enough to work around, but it's a good reminder that value classes are a mess and we might want to avoid extension methods in our own implementations.

This PR replaces all use of cats.syntax extension methods in cats/*.scala and cats/data/*.scala. Most of the replacements have a fairly neutral effect on readability, like using Ior.fromEither instead of _.toIor, or algebra.remove(x, y) instead of x |-| y, etc. In these cases we get to avoid some indirection and exposure to value class nonsense without bloating the code.

In a few cases the non-syntax version is a significantly more verbose—in particular we use bimap on Either a lot in cats.data. This is one case where the syntax method does have a non-trivial runtime cost. I've added a benchmark for two implementations of map for EitherK: the current one:

EitherK(e.run.bimap(F.lift(f), G.lift(f)))

…and one that doesn't use the bimap extension method:

EitherK(
  e.run match {
    case Right(ga) => Right(G.map(ga)(f))
    case Left(fa)  => Left(F.map(fa)(f))
  }
)

The non-syntax one gets 30% more throughput in this benchmark:

EitherKMapBench.incMap1  thrpt   20  19348653.553 ±  75679.278  ops/s
EitherKMapBench.incMap2  thrpt   20  25152245.244 ± 161475.884  ops/s

Given that the faster one is still arguably more readable, even if it's a little more verbose, I think avoiding bimap in our own implementations is reasonable.

[travisbrown]

For the record this isn't something I feel particularly strongly about—I just thought it was worth seeing how much noise this would add, and it turns out it isn't much. I'm happy to change the PR to only include the easy replacements if people would prefer that.

[codecov-io]

Codecov Report

Merging #3263 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3263      +/-   ##
==========================================
+ Coverage   93.08%   93.09%   +0.01%     
==========================================
  Files         376      376              
  Lines        7428     7483      +55     
  Branches      196      197       +1     
==========================================
+ Hits         6914     6966      +52     
- Misses        514      517       +3

Flag	Coverage Δ
#scala_version_212	93.42% <100%> (ø)	⬆️
#scala_version_213	92.86% <99.12%> (+0.01%)	⬆️

Impacted Files	Coverage Δ
core/src/main/scala/cats/data/NonEmptyList.scala	98.72% <100%> (+0.01%)	⬆️
core/src/main/scala/cats/Reducible.scala	100% <100%> (ø)	⬆️
core/src/main/scala/cats/data/WriterT.scala	91.26% <100%> (ø)	⬆️
...in/scala/cats/data/IndexedReaderWriterStateT.scala	97.02% <100%> (+0.02%)	⬆️
core/src/main/scala/cats/data/IndexedStateT.scala	90.19% <100%> (+0.19%)	⬆️
core/src/main/scala/cats/data/OptionT.scala	95.9% <100%> (ø)	⬆️
core/src/main/scala/cats/data/OneAnd.scala	95.45% <100%> (ø)	⬆️
core/src/main/scala/cats/data/EitherK.scala	94.54% <100%> (+1.36%)	⬆️
core/src/main/scala/cats/Eval.scala	98.83% <100%> (ø)	⬆️
core/src/main/scala/cats/data/NonEmptyVector.scala	99.17% <100%> (+0.02%)	⬆️
... and 4 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95cb4ae...39a721c. Read the comment docs.

[djspiewak]

Interestingly, Scala 2.13 itself seems to be somewhat stricter on the cycles which are caused by using syntax imports within a library. I ran into that problem in spades with Matryoshka. Granted, some of that was just being generated by package objects, but still. It's an important warning, and I'm glad Dotty is stricter still on it.