Update ExplicitImplicitTypes

Ref scalacenter/scalafix#1180

README.md

@@ -3,34 +3,32 @@
 
 This project contains scalafix rules I create when migrating [akka](https://github.com/akka/akka/) to dotty.
 
-#### FinalObject
-Remove redundant `final` modifier for objects:
-```scala
-final object Abc
-```
-
 #### ConstructorProcedureSyntax
 Remove constructor procedure syntax: `def this(..) {..}` => `def this(..) = {..}`
 
-This rule complement to the built-in [ProcedureSyntax](https://github.com/scalacenter/scalafix/blob/master/scalafix-rules/src/main/scala/scalafix/internal/rule/ProcedureSyntax.scala) rule
-
-#### ExplicitNonNullaryApply
-Compare to [fix.scala213.ExplicitNonNullaryApply](https://github.com/scala/scala-rewrites/blob/1cea92d/rewrites/src/main/scala/fix/scala213/ExplicitNonNullaryApply.scala)
- from scala-rewrites project:
-  - Don't need scala.meta.internal.pc.ScalafixGlobal to hook into scala-compiler.
-
-    So this rule also add `()` to methods that is defined in java and be overridden in scala, eg the calls in [ExplicitNonNullaryApplyJavaPending](scalafix/input/src/main/scala/fix/scala213/ExplicitNonNullaryApplyJavaPending.scala) test. 
-  - When I try scala-rewrites' ExplicitNonNullaryApply for akka sources, it crashed!
-  - Don't need to publishLocal before use. [scala/scala-rewrites#32](https://github.com/scala/scala-rewrites/issues/32)
-
-    [Just add](https://github.com/ohze/akka/blob/dotty/wip/.scalafix.conf) `"github:ohze/scalafix-rules/ExplicitNonNullaryApply"` to `.scalafix.conf`
+This rule complement to the built-in [ProcedureSyntax](https://github.com/scalacenter/scalafix/blob/master/scalafix-rules/src/main/scala/scalafix/internal/rule/ProcedureSyntax.scala) rule.
 
 #### Any2StringAdd
 - ~~Fix https://github.com/scala/scala-rewrites/issues/18~~ (now fixed)
 - Plus some improvements
 
+#### ParensAroundLambda
+Fix: parentheses are required around the parameter of a lambda
+```scala
+Seq(1).map { i: Int => // rewrite to: Seq(1).map { (i: Int) =>
+ i + 1
+}
+Seq(1).map { i => i + 1 } // keep
+```
+
+#### FinalObject
+Remove redundant `final` modifier for objects:
+```scala
+final object Abc
+```
+
 #### NullaryOverride
-Consistent nullary overriding, eg:
+Consistent nullary overriding
 ```scala
 trait A {
   def i: Int
@@ -42,40 +40,72 @@ trait B {
 }
 ```
 
-#### ParensAroundLambda
-Fix: parentheses are required around the parameter of a lambda
-```scala
-Seq(1).map { i: Int => // rewrite to: Seq(1).map { (i: Int) =>
- i + 1
-}
-Seq(1).map { i => i + 1 } // keep
-```
+#### ExplicitNonNullaryApply
+Compare to [fix.scala213.ExplicitNonNullaryApply](https://github.com/scala/scala-rewrites/blob/1cea92d/rewrites/src/main/scala/fix/scala213/ExplicitNonNullaryApply.scala)
+ from scala-rewrites project:
++ Pros
+  - When I try scala-rewrites' ExplicitNonNullaryApply for akka sources, it crashed!
+  - This rule run faster than scala-rewrites' one.
+  - Don't need to publishLocal to use. [scala/scala-rewrites#32](https://github.com/scala/scala-rewrites/issues/32)
+
+    [Just add](https://github.com/ohze/akka/blob/dotty/wip/.scalafix.conf) `"github:ohze/scalafix-rules/ExplicitNonNullaryApply"` to `.scalafix.conf`
++ Cons    
+  - This rule also add `()` to methods that are defined in java and be overridden in scala,
+    eg the calls in [ExplicitNonNullaryApplyJavaPending](scalafix/input/src/main/scala/fix/scala213/ExplicitNonNullaryApplyJavaPending.scala) test.
+    Those `()`s are not required by dotty.
++ Technical note: This rule don't need `scala.meta.internal.pc.ScalafixGlobal` to hook into scala-compiler.
 
 #### ExplicitImplicitTypes
-To explicitly add type to `implicit def/val/var`s (required by dotty) you need:
-1. Use the built-in ExplicitResultTypes rule with config:
+Before scalacenter/scalafix#1180 is fixed, to explicitly add type to `implicit def/val/var`s (required by dotty) you need:
++ Use the built-in [ExplicitResultTypes](https://scalacenter.github.io/scalafix/docs/rules/ExplicitResultTypes.html) rule with config:
 ```hocon
 rules = [
   ExplicitResultTypes
 ]
 ExplicitResultTypes {
   memberVisibility = [] # only rewrite implicit members
-  skipSimpleDefinitions = []
 }
 ```
 => Add type to implicit members of `class`es, `trait`s
+```scala
+trait T {
+  // rewrite to `implicit val s: Seq[String] = Nil`
+  implicit val s = Seq.empty[String]
+  // rewrite to `implicit def i: Int = 1`
+  implicit def i = 1
+}
+```
 
-2. And this rule
++ *And* use this rule
 ```hocon
 rules = [
   "github:ohze/scalafix-rules/ExplicitImplicitTypes"
 ]
-# maybe need
+# Optinal
 ExplicitImplicitTypes.symbolReplacements {
   "scala/concurrent/ExecutionContextExecutor#" = "scala/concurrent/ExecutionContext#"
 }
 ```
-=> Add type to implicit local `def/val/var`s
+=> Add type to implicit local `def/val/var`s that its parent is a trait/class
+```scala
+import scala.concurrent.ExecutionContextExecutor
+
+trait T
+trait A {
+  def f() = {
+    class C {
+      // rewrite to `implicit val i: Int = 1`
+      implicit val i = 1
+    }
+    ???
+  }
+  def someEc(): ExecutionContextExecutor
+  def g() = new T {
+    // rewrite to `implicit def ec: ExecutionContext = someEc()`
+    implicit def ec = someEc()
+  }
+}
+```
 
 ## Usage
 + Eg, for ExplicitNonNullaryApply rule:

scalafix/input/src/main/scala/fix/ExplicitImplicitTypesTest.scala

@@ -13,7 +13,7 @@ abstract class ExplicitImplicitTypesTest {
     def ec: ExecutionContextExecutor
   }
   trait T
-  
+
   def f(e: E) = new T {
     private implicit def ec = e.ec
     final implicit val s1 = Seq(1)

scalafix/input/src/main/scala/test/explicitResultTypes/SkipLocalImplicits.scala

@@ -0,0 +1,19 @@
+/*
+rule = ExplicitImplicitTypes
+*/
+package test.explicitResultTypes
+
+class SkipLocalImplicits {
+  trait T
+  def f(): T = new T {
+    implicit val i = 1
+  }
+  def g(): Unit = {
+    class C {
+      implicit val i = 2
+    }
+  }
+  def h(): Unit = {
+    implicit val i = 3
+  }
+}

scalafix/output/src/main/scala/fix/ExplicitImplicitTypesTest.scala

@@ -8,15 +8,15 @@ abstract class ExplicitImplicitTypesTest {
     def ec: ExecutionContextExecutor
   }
   trait T
-  
+
   def f(e: E) = new T {
     private implicit def ec: ExecutionContext = e.ec
     final implicit val s1: Seq[Int] = Seq(1)
     implicit var nil_ : Seq[AnJavaInterface] = Nil
     implicit var i: Int = 10 // keep
     def g() = {
-      implicit val l: Long = 1L
-      implicit var none: Option[String] = None
+      implicit val l = 1L
+      implicit var none = Option.empty[String]
     }
   }
 }

scalafix/output/src/main/scala/test/explicitResultTypes/SkipLocalImplicits.scala

@@ -0,0 +1,16 @@
+package test.explicitResultTypes
+
+class SkipLocalImplicits {
+  trait T
+  def f(): T = new T {
+    implicit val i: Int = 1
+  }
+  def g(): Unit = {
+    class C {
+      implicit val i: Int = 2
+    }
+  }
+  def h(): Unit = {
+    implicit val i = 3
+  }
+}

scalafix/rules/src/main/scala/fix/ExplicitImplicitTypes.scala

@@ -57,19 +57,52 @@ final class ExplicitImplicitTypes(global: LazyValue[ScalafixGlobal]) extends Sem
     lazy val types = new CompilerTypePrinter(global.value)
     doc.tree.collect {
       case t @ Defn.Val(mods, Pat.Var(name) :: Nil, None, body)
-        if mods.exists(_.isInstanceOf[Mod.Implicit]) => // t.hasMod(mod"implicit")
+        if isRuleCandidate(t, name, mods, body) =>
         fixDefinition(t, body, name, types)
 
       case t @ Defn.Var(mods, Pat.Var(name) :: Nil, None, Some(body))
-        if mods.exists(_.isInstanceOf[Mod.Implicit]) =>
+        if isRuleCandidate(t, name, mods, body) =>
         fixDefinition(t, body, name, types)
 
       case t @ Defn.Def(mods, name, _, _, None, body)
-        if mods.exists(_.isInstanceOf[Mod.Implicit]) =>
+        if isRuleCandidate(t, name, mods, body) =>
         fixDefinition(t, body, name, types)
     }.asPatch
   }
 
+  // Don't explicitly annotate vals when the right-hand body is a single call
+  // to `implicitly`. Prevents ambiguous implicit. Not annotating in such cases,
+  // this a common trick employed implicit-heavy code to workaround SI-2712.
+  // Context: https://gitter.im/typelevel/cats?at=584573151eb3d648695b4a50
+  private def isImplicitly(term: Term): Boolean = term match {
+    case Term.ApplyType(Term.Name("implicitly"), _) => true
+    case _ => false
+  }
+
+  def isRuleCandidate(
+     defn: Defn,
+     nm: Name,
+     mods: Iterable[Mod],
+     body: Term
+   )(implicit ctx: SemanticDocument): Boolean = {
+
+    def isFinalLiteralVal: Boolean =
+      defn.is[Defn.Val] &&
+        mods.exists(_.is[Mod.Final]) &&
+        body.is[Lit]
+
+    def isImplicit: Boolean =
+      mods.exists(_.is[Mod.Implicit]) && !isImplicitly(body)
+
+    def hasParentWihTemplate: Boolean =
+      defn.parent.exists(_.is[Template])
+
+    isImplicit &&
+      !isFinalLiteralVal &&
+      nm.symbol.isLocal && // use ExplicitResultTypes for non-local implicits
+      hasParentWihTemplate // use this Rule for local implicits in Template only
+  }
+
   def fixDefinition(defn: Defn, body: Term, name: Term.Name, types: TypePrinter)(
       implicit ctx: SemanticDocument
   ): Patch = {
@@ -135,11 +168,14 @@ class CompilerTypePrinter(g: ScalafixGlobal)(
       defn: m.Defn,
       space: String
   ): Option[v1.Patch] = {
-    if (sym.isGlobal) return None // pls use rule ExplicitResultTypes
-
     val gpos = unit.position(pos.start)
-    val t = GlobalProxy.typedTreeAt(g, gpos)
-    val inverseSemanticdbSymbol = t.symbol
+    val tpe = GlobalProxy.typedTreeAt(g, gpos)
+    val inverseSemanticdbSymbol =
+      if (sym.isLocal) tpe.symbol
+      else
+        g.inverseSemanticdbSymbols(sym.value)
+          .find(s => g.semanticdbSymbol(s) == sym.value)
+          .getOrElse(g.NoSymbol)
     val hasNothing = inverseSemanticdbSymbol.info.exists {
       case g.definitions.NothingTpe => true
       case _ => false
@@ -307,6 +343,7 @@ class CompilerTypePrinter(g: ScalafixGlobal)(
 
   private def isPossibleSyntheticParent(tpe: Type): Boolean = {
     definitions.isPossibleSyntheticParent(tpe.typeSymbol) ||
+    CompilerCompat.serializableClass(g).toSet[Symbol](tpe.typeSymbol) ||
     definitions.AnyRefTpe == tpe ||
     definitions.ObjectTpe == tpe
   }
@@ -329,9 +366,19 @@ class CompilerTypePrinter(g: ScalafixGlobal)(
         case t: MethodType => loop(t.resultType)
         case RefinedType(parents, _) =>
           // Remove redundant `Product with Serializable`, if possible.
-          val strippedParents = parents.filterNot { tpe =>
-            definitions.isPossibleSyntheticParent(tpe.typeSymbol)
-          }
+          val productRootClass = definitions.ProductClass.seq
+            .toSet[Symbol] + definitions.ProductRootClass
+          val serializableClass =
+            CompilerCompat.serializableClass(g).toSet[Symbol]
+          val parentSymbols = parents.map(_.typeSymbol).toSet
+          val strippedParents =
+            if (parentSymbols
+                .intersect(productRootClass)
+                .nonEmpty && parentSymbols
+                .intersect(serializableClass)
+                .nonEmpty) {
+              parents.filterNot(isPossibleSyntheticParent)
+            } else parents
           val newParents =
             if (strippedParents.nonEmpty) strippedParents
             else parents
@@ -395,6 +442,12 @@ class CompilerTypePrinter(g: ScalafixGlobal)(
     g.rootMirror.RootPackage
   )
 }
+
+object CompilerCompat {
+  // support scala 2.13 only
+  def serializableClass(g: ScalafixGlobal): Set[g.ClassSymbol] =
+    Set(g.definitions.SerializableClass)
+}
 // end inlining fix/impl/CompilerTypePrinter.scala
 // begin inlining fix/impl/PatchEmptyBody.scala
 

scalafix/rules/src/main/scala/fix/ExplicitImplicitTypesImpl.scala

@@ -57,19 +57,52 @@ final class ExplicitImplicitTypesImpl(global: LazyValue[ScalafixGlobal]) extends
     lazy val types = new CompilerTypePrinter(global.value)
     doc.tree.collect {
       case t @ Defn.Val(mods, Pat.Var(name) :: Nil, None, body)
-        if mods.exists(_.isInstanceOf[Mod.Implicit]) => // t.hasMod(mod"implicit")
+        if isRuleCandidate(t, name, mods, body) =>
         fixDefinition(t, body, name, types)
 
       case t @ Defn.Var(mods, Pat.Var(name) :: Nil, None, Some(body))
-        if mods.exists(_.isInstanceOf[Mod.Implicit]) =>
+        if isRuleCandidate(t, name, mods, body) =>
         fixDefinition(t, body, name, types)
 
       case t @ Defn.Def(mods, name, _, _, None, body)
-        if mods.exists(_.isInstanceOf[Mod.Implicit]) =>
+        if isRuleCandidate(t, name, mods, body) =>
         fixDefinition(t, body, name, types)
     }.asPatch
   }
 
+  // Don't explicitly annotate vals when the right-hand body is a single call
+  // to `implicitly`. Prevents ambiguous implicit. Not annotating in such cases,
+  // this a common trick employed implicit-heavy code to workaround SI-2712.
+  // Context: https://gitter.im/typelevel/cats?at=584573151eb3d648695b4a50
+  private def isImplicitly(term: Term): Boolean = term match {
+    case Term.ApplyType(Term.Name("implicitly"), _) => true
+    case _ => false
+  }
+
+  def isRuleCandidate(
+     defn: Defn,
+     nm: Name,
+     mods: Iterable[Mod],
+     body: Term
+   )(implicit ctx: SemanticDocument): Boolean = {
+
+    def isFinalLiteralVal: Boolean =
+      defn.is[Defn.Val] &&
+        mods.exists(_.is[Mod.Final]) &&
+        body.is[Lit]
+
+    def isImplicit: Boolean =
+      mods.exists(_.is[Mod.Implicit]) && !isImplicitly(body)
+
+    def hasParentWihTemplate: Boolean =
+      defn.parent.exists(_.is[Template])
+
+    isImplicit &&
+      !isFinalLiteralVal &&
+      nm.symbol.isLocal && // use ExplicitResultTypes for non-local implicits
+      hasParentWihTemplate // use this Rule for local implicits in Template only
+  }
+
   def fixDefinition(defn: Defn, body: Term, name: Term.Name, types: TypePrinter)(
       implicit ctx: SemanticDocument
   ): Patch = {