Add symbol-based type shortener #42

jvican · 2017-01-23T10:31:24Z

This PR introduces a symbol-based type shortener to replace the string-based one. The benefits of this approach are:

Provides more consistent behaviour with the Scala compiler
Allows fine-grained control over what's in scope and what's not
Helps future customization (as the whole information about scopes and symbols is available)

I think that there are some small details to flesh out before we nail down the desired behaviour. I'm commenting on some lines to make it clearer. I ran this on cats and I received pretty diffs, but for some reasons I was not successful at running it in scalajs.

jvican · 2017-01-23T10:32:34Z

core/src/main/scala/scalafix/util/Patch.scala

-    token.input.eq(from.input) &&
-      token.end <= to.end &&
-      token.start >= from.start
+sealed abstract class Patch {


These changes were imported from previous @olafurpg's commits in other branch.

I have bigger pending changes on imports. But I'm hoping to get this in first if that's possible.

Are these changes necessary now? If not remove

jvican · 2017-01-23T10:33:12Z

scalafix-nsc/src/main/scala/scalafix/nsc/NscSemanticApi.scala

-        case _ =>
-      }
+  /** The compiler sets different symbols for `PackageDef`s
+		* and term names pointing to that package. Get the


For some reason this is not aligned, sorry about that.

what?! can you fix it manually?

Reword suggestion: returns canonical symbol of a package symbol for equality comparison.

It's typically good to start docstrings with "Returns ..."

jvican · 2017-01-23T10:34:39Z

scalafix-nsc/src/main/scala/scalafix/nsc/NscSemanticApi.scala

+      * we need to check both names to guarantee whether a name is in scope.
+      */
+    @inline
+    def lookupBothNames(name: String,


I'm extending this method to return both the term symbol and type names if both exist. This can happen if types with same names are defined in a trait and its companion object, for example.

Explain that in docstring instead of here?

jvican · 2017-01-23T10:36:07Z

scalafix-nsc/src/main/scala/scalafix/nsc/NscSemanticApi.scala

+        }
+
+      // Disambiguate overloaded symbols caused by name shadowing
+      if (symbol.isOverloaded) {


We try to disambiguate the overloaded symbol with the owner. If there's not luck, we fallback to a string-based approach: get the symbol whose full name is the longest common subexpression of our tpe.

mention this in code comment?

jvican · 2017-01-23T10:36:23Z

scalafix-nsc/src/main/scala/scalafix/nsc/NscSemanticApi.scala

+      val wholeScope = mixScopes(inScope, realRootScope)
+      val disambiguatingSyntax = ref.syntax
+      val (_, reversedSymbols) = {
+        names.iterator.foldLeft(wholeScope -> List.empty[g.Symbol]) {


The iterator here can be removed.

why do you have it?

jvican · 2017-01-23T10:37:13Z

scalafix-nsc/src/main/scala/scalafix/nsc/NscSemanticApi.scala

+          // Return shortened type for names already in scope
+          val onlyNames = onlyPaths.map(_._1)
+          val removed = removePrefixes(refNoThis, onlyNames)
+          val shortenedAndRenamedType = removed.transform {


The renaming logic can go into an independent method for easing readability.

I think that is a good idea. I'm not religious about it, but I think the rule of 30 is nice to keep in mind https://github.com/databricks/scala-style-guide#rule_of_30

jvican · 2017-01-23T10:38:09Z

scalafix-nsc/src/main/scala/scalafix/nsc/NscSemanticApi.scala

+            else paths.tail.map(_._1)
+          }
+
+          // Shortened names must be just one if no PDT


What's the idiomatic way of failing in scalafix? Here we're failing fast since these cases should never happen. I wonder if this is ok or we should go for another solution.

Are these exceptional/unexpected cases? If so, then throw exception with clear error message about what assumption was broken. If not exceptional or unexpected, then encode in type signature of shortenType

btw, I suspect this will eventually become a part of the scala.meta semantic api, once we have gained some experience with it in scalafix.

jvican · 2017-01-23T10:41:11Z

So far, imports inside blocks are not recognised (and we should):

val foo = {
  import scala.concurrent.mutable.List
  List.empty[Int]
}

It could be done by changing the ScopeTraverser to visit vals and defs and then changing the logic in the shortenType method to get the enclosing scope. I'm happy to take a look at it in another PR, since these cases are rare and should not block any progress on scalafix's front.

olafurpg

Amazing work @jvican! A lot of comments, but mostly small things.

I am not as deep into the reflect api, so some parts I couldn't review thoroughly. Do you have anything to add @xeno-by?

olafurpg · 2017-01-23T12:25:01Z

core/src/main/scala/scalafix/rewrite/ExplicitImplicit.scala

+    }
+  }
+  /** Get the last sequential top level package from a source file. */
+  def getLastTopLevelPkg(potentialPkg: Stat): Stat = {


Is this an equivalent implementation?

def getLastTopLevelPkg(potPkg: Stat): Stat = potPkg match { case Pkg(_, head +: Nil) => getLastTopLevelPkg(head) case Pkg(_, head +: _) => head case _ => potPkg }

In what test is this method needed?

olafurpg · 2017-01-23T12:28:47Z

core/src/main/scala/scalafix/rewrite/SemanticApi.scala

@@ -18,4 +17,7 @@ trait SemanticApi {

  /** Returns the type annotation for given val/def. */
  def typeSignature(defn: Defn): Option[Type]
+
+  /**  */


Suggestion: Returns the same type with fully qualified names shortened and missing imports.

What do you think about returning a case class instead of tuple?

tpe should be named toShorten and t should be atLocation.

What happens on failure? This is method is used for aestetics reasons and can safely be skipped so it could either return an Option[(a, b)] if shortening failed or default to toShorten -> Nil. WDYT?

olafurpg · 2017-01-23T12:29:38Z

core/src/main/scala/scalafix/util/Patch.scala

-    token.input.eq(from.input) &&
-      token.end <= to.end &&
-      token.start >= from.start
+sealed abstract class Patch {


I have bigger pending changes on imports. But I'm hoping to get this in first if that's possible.

olafurpg · 2017-01-23T12:30:24Z

core/src/test/resources/ExplicitImplicit/basic.source

+>>>
+import scala.collection.immutable.{Map => MyMap}
+class A {
+  implicit val x: MyMap[Int, String] =


Impressive! 😍

Have a look at the other two tests checking renames.

olafurpg · 2017-01-23T12:32:25Z

core/src/test/resources/ExplicitImplicit/basic.source

+    MyMap(1 -> "STRING")
+}
+<<< removed map
+import scala.collection.immutable.{Map => _}


This isn't testing anything, since Map is a val in Predef. Maybe use a different collection that's not in Predef?

olafurpg · 2017-01-23T13:10:35Z

scalafix-nsc/src/main/scala/scalafix/nsc/NscSemanticApi.scala

+            else paths.tail.map(_._1)
+          }
+
+          // Shortened names must be just one if no PDT


btw, I suspect this will eventually become a part of the scala.meta semantic api, once we have gained some experience with it in scalafix.

olafurpg · 2017-01-23T13:11:09Z

scalafix-nsc/src/main/scala/scalafix/nsc/NscSemanticApi.scala

+          }
+
+          // Shortened names must be just one if no PDT
+          assert(shortenedNames.size == 1)


Include at least a clarifying message.

olafurpg · 2017-01-23T13:12:32Z

scalafix-nsc/src/main/scala/scalafix/nsc/NscSemanticApi.scala

    new SemanticApi {
+      override def shortenType(tpe: m.Type,
+                               owner: m.Tree): (m.Type, Seq[m.Ref]) = {
+        logger.elem(tpe)


olafurpg · 2017-01-23T13:14:06Z

scalafix-nsc/src/main/scala/scalafix/nsc/NscSemanticApi.scala

+        val typeRefsAndRewrites = typeRefs.zip(typeRewrites)
+        val mappedRewrites = typeRefsAndRewrites.map {
+          case (typeRef, missingOrHit) =>
+            typeRef -> missingOrHit.fold(_._2, identity)


._.2, identity is a sign that a case class might be a better encoding.

olafurpg · 2017-01-23T13:23:25Z

scalafix-tests/src/test/scala/scalafix/tests/IntegrationPropertyTest.scala

@@ -190,7 +190,7 @@ class ScalaJs
          Command("examples/test:compile")
        )
      ),
-      skip = true // GenJsCode is hard: import renames + dependent types
+      skip = false // GenJsCode is hard: import renames + dependent types


Did you run clean/compile after disabling scalafix? I think I disabled it for some reason while debugging.

jvican · 2017-01-23T13:34:20Z

Diff from cats:

diff --git a/.jvmopts b/.jvmopts
deleted file mode 100644
index 83ed6fb4..00000000
--- a/.jvmopts
+++ /dev/null
@@ -1,12 +0,0 @@
-# see https://weblogs.java.net/blog/kcpeppe/archive/2013/12/11/case-study-jvm-hotspot-flags
--Dfile.encoding=UTF8
--Xms1G
--Xmx6G
--XX:MaxPermSize=512M
--XX:ReservedCodeCacheSize=250M
--XX:+TieredCompilation
--XX:-UseGCOverheadLimit
-# effectively adds GC to Perm space
--XX:+CMSClassUnloadingEnabled
-# must be enabled for CMSClassUnloadingEnabled to work
--XX:+UseConcMarkSweepGC
diff --git a/js/src/test/scala/cats/tests/FutureTests.scala b/js/src/test/scala/cats/tests/FutureTests.scala
index e248189d..d81a4746 100644
--- a/js/src/test/scala/cats/tests/FutureTests.scala
+++ b/js/src/test/scala/cats/tests/FutureTests.scala
@@ -2,6 +2,7 @@ package cats
 package js
 package tests
 
+import scala.concurrent.ExecutionContextExecutor
 import cats.laws.discipline._
 import cats.js.instances.Await
 import cats.js.instances.future.futureComonad
@@ -17,7 +18,7 @@ import cats.laws.discipline.arbitrary._
 // https://issues.scala-lang.org/browse/SI-7934
 @deprecated("", "")
 class DeprecatedForwarder {
-  implicit def runNow = scala.scalajs.concurrent.JSExecutionContext.Implicits.runNow
+  implicit def runNow: ExecutionContextExecutor = scala.scalajs.concurrent.JSExecutionContext.Implicits.runNow
 }
 object DeprecatedForwarder extends DeprecatedForwarder
 import DeprecatedForwarder.runNow
diff --git a/kernel-laws/src/test/scala/cats/kernel/laws/LawTests.scala b/kernel-laws/src/test/scala/cats/kernel/laws/LawTests.scala
index 769970c6..addac01e 100644
--- a/kernel-laws/src/test/scala/cats/kernel/laws/LawTests.scala
+++ b/kernel-laws/src/test/scala/cats/kernel/laws/LawTests.scala
@@ -228,7 +228,7 @@ class LawTests extends FunSuite with Discipline {
           .forall { case (x, y) => a.eqv(x, y) == b.eqv(x, y) }
     }
 
-    implicit val monoidOrderN = Order.whenEqualMonoid[N]
+    implicit val monoidOrderN: Monoid[Order[N]] with Band[Order[N]] = Order.whenEqualMonoid[N]
     laws[GroupLaws, Order[N]].check(_.monoid)
     laws[GroupLaws, Order[N]].check(_.band)
 
diff --git a/kernel/src/main/scala/cats/kernel/instances/char.scala b/kernel/src/main/scala/cats/kernel/instances/char.scala
index 4238fa84..456afbe4 100644
--- a/kernel/src/main/scala/cats/kernel/instances/char.scala
+++ b/kernel/src/main/scala/cats/kernel/instances/char.scala
@@ -4,7 +4,7 @@ package instances
 package object char extends CharInstances
 
 trait CharInstances {
-  implicit val catsKernelStdOrderForChar = new CharOrder
+  implicit val catsKernelStdOrderForChar: CharOrder = new CharOrder
 }
 
 class CharOrder extends Order[Char] {
diff --git a/project/plugins.sbt b/project/plugins.sbt
index 10bb5da4..e8db97b3 100644
--- a/project/plugins.sbt
+++ b/project/plugins.sbt
@@ -11,3 +11,5 @@ addSbtPlugin("org.scala-js"         % "sbt-scalajs"           % "0.6.13")
 addSbtPlugin("com.github.tkawachi"  % "sbt-doctest"           % "0.4.1")
 addSbtPlugin("org.xerial.sbt"       % "sbt-sonatype"          % "1.1")
 addSbtPlugin("com.fortysevendeg"    % "sbt-microsites"        % "0.3.2")
+
+addSbtPlugin("ch.epfl.scala" % "sbt-scalafix" % "0.2.2")
diff --git a/tests/src/test/scala/cats/tests/EvalTests.scala b/tests/src/test/scala/cats/tests/EvalTests.scala
index d7c41002..9a9af542 100644
--- a/tests/src/test/scala/cats/tests/EvalTests.scala
+++ b/tests/src/test/scala/cats/tests/EvalTests.scala
@@ -1,6 +1,11 @@
 package cats
 package tests
 
+import cats.Monoid
+import cats.Semigroup
+import cats.Order
+import cats.PartialOrder
+import cats.Eq
 import scala.math.min
 import cats.laws.ComonadLaws
 import cats.laws.discipline.{BimonadTests, CartesianTests, MonadTests, SerializableTests}
@@ -92,7 +97,7 @@ class EvalTests extends CatsSuite {
   }
 
   {
-    implicit val iso = CartesianTests.Isomorphisms.invariant[Eval]
+    implicit val iso: CartesianTests.Isomorphisms[Eval] = CartesianTests.Isomorphisms.invariant[Eval]
     checkAll("Eval[Int]", BimonadTests[Eval].bimonad[Int, Int, Int])
     checkAll("Eval[Int]", MonadTests[Eval].monad[Int, Int, Int])
   }
@@ -102,27 +107,27 @@ class EvalTests extends CatsSuite {
   checkAll("Eval[Int]", GroupLaws[Eval[Int]].group)
 
   {
-    implicit val A = ListWrapper.monoid[Int]
+    implicit val A: Monoid[ListWrapper[Int]] = ListWrapper.monoid[Int]
     checkAll("Eval[ListWrapper[Int]]", GroupLaws[Eval[ListWrapper[Int]]].monoid)
   }
 
   {
-    implicit val A = ListWrapper.semigroup[Int]
+    implicit val A: Semigroup[ListWrapper[Int]] = ListWrapper.semigroup[Int]
     checkAll("Eval[ListWrapper[Int]]", GroupLaws[Eval[ListWrapper[Int]]].semigroup)
   }
 
   {
-    implicit val A = ListWrapper.order[Int]
+    implicit val A: Order[ListWrapper[Int]] = ListWrapper.order[Int]
     checkAll("Eval[ListWrapper[Int]]", OrderLaws[Eval[ListWrapper[Int]]].order)
   }
 
   {
-    implicit val A = ListWrapper.partialOrder[Int]
+    implicit val A: PartialOrder[ListWrapper[Int]] = ListWrapper.partialOrder[Int]
     checkAll("Eval[ListWrapper[Int]]", OrderLaws[Eval[ListWrapper[Int]]].partialOrder)
   }
 
   {
-    implicit val A = ListWrapper.eqv[Int]
+    implicit val A: Eq[ListWrapper[Int]] = ListWrapper.eqv[Int]
     checkAll("Eval[ListWrapper[Int]]", OrderLaws[Eval[ListWrapper[Int]]].eqv)
   }
 
diff --git a/tests/src/test/scala/cats/tests/IdTests.scala b/tests/src/test/scala/cats/tests/IdTests.scala
index 0465dd34..a7b12751 100644
--- a/tests/src/test/scala/cats/tests/IdTests.scala
+++ b/tests/src/test/scala/cats/tests/IdTests.scala
@@ -1,10 +1,11 @@
 package cats
 package tests
 
+import cats.Id
 import cats.laws.discipline._
 
 class IdTests extends CatsSuite {
-  implicit val iso = CartesianTests.Isomorphisms.invariant[Id]
+  implicit val iso: CartesianTests.Isomorphisms[Id] = CartesianTests.Isomorphisms.invariant[Id]
 
   checkAll("Id[Int]", BimonadTests[Id].bimonad[Int, Int, Int])
   checkAll("Bimonad[Id]", SerializableTests.serializable(Bimonad[Id]))
diff --git a/tests/src/test/scala/cats/tests/NonEmptyListTests.scala b/tests/src/test/scala/cats/tests/NonEmptyListTests.scala
index c913a8f8..c59a86e5 100644
--- a/tests/src/test/scala/cats/tests/NonEmptyListTests.scala
+++ b/tests/src/test/scala/cats/tests/NonEmptyListTests.scala
@@ -1,6 +1,8 @@
 package cats
 package tests
 
+import cats.PartialOrder
+import cats.Order
 import cats.kernel.laws.{GroupLaws, OrderLaws}
 
 import cats.data.NonEmptyList
@@ -36,7 +38,7 @@ class NonEmptyListTests extends CatsSuite {
   checkAll("Eq[NonEmptyList[ListWrapper[Int]]]", SerializableTests.serializable(Eq[NonEmptyList[ListWrapper[Int]]]))
 
   {
-    implicit val A = ListWrapper.partialOrder[Int]
+    implicit val A: PartialOrder[ListWrapper[Int]] = ListWrapper.partialOrder[Int]
     checkAll("NonEmptyList[ListWrapper[Int]]", OrderLaws[NonEmptyList[ListWrapper[Int]]].partialOrder)
     checkAll("PartialOrder[NonEmptyList[ListWrapper[Int]]]", SerializableTests.serializable(PartialOrder[NonEmptyList[ListWrapper[Int]]]))
 
@@ -44,7 +46,7 @@ class NonEmptyListTests extends CatsSuite {
   }
 
   {
-    implicit val A = ListWrapper.order[Int]
+    implicit val A: Order[ListWrapper[Int]] = ListWrapper.order[Int]
     checkAll("NonEmptyList[ListWrapper[Int]]", OrderLaws[NonEmptyList[ListWrapper[Int]]].order)
     checkAll("Order[NonEmptyList[ListWrapper[Int]]]", SerializableTests.serializable(Order[NonEmptyList[ListWrapper[Int]]]))

olafurpg · 2017-01-23T14:06:58Z

scalafix-nsc/src/main/scala/scalafix/nsc/NscSemanticApi.scala

+    traverser.traverse(unit.body)
+
+    def toMetaType(tp: g.Tree) =
+      config.dialect(tp.toString).parse[m.Type].get


This is causing the errors.

olafurpg · 2017-01-24T17:28:42Z

core/src/main/scala/scalafix/rewrite/ExplicitImplicit.scala

@@ -13,6 +15,14 @@ case object ExplicitImplicit extends Rewrite {
    case m.Term.ApplyType(m.Term.Name("implicitly"), _) => true
    case _ => false
  }
+  @scala.annotation.tailrec
+  final def parents(tree: Tree,


olafurpg · 2017-01-24T17:29:52Z

core/src/test/resources/ExplicitImplicit/basic.source

@@ -8,14 +8,37 @@ class A {
  implicit val x: List[Int] = List(1)
 }
 <<< map
+import scala.concurrent.Future


olafurpg · 2017-01-24T17:30:12Z

core/src/main/scala/scalafix/util/Patch.scala

-    token.input.eq(from.input) &&
-      token.end <= to.end &&
-      token.start >= from.start
+sealed abstract class Patch {


Are these changes necessary now? If not remove

olafurpg · 2017-01-24T17:32:09Z

core/src/test/resources/ExplicitImplicit/basic.source

@@ -44,14 +67,124 @@ class A {
 class A {
  class B { class C }
  implicit val x = new B
-  implicit val y = new x.C
+  implicit val y: x.C = new x.C


This is the input, remove?

Adding and organizing imports is delegated to a future rewrite. `shortenType` will only perform a type shortening at a given location. This greatly simplifies the implementation, which has to be still cleaned up for readability purposes.

Given the ambiguity of term selects in Scala meta, we have to try luck with every name and look it up with both type and term names. The previous traversal assumed that once we found a symbol, we took the right path, but this is not true: ```scala object A class A { type B } ``` Looking up `A.B` would fail if the algorithm start looking term names. `object A` would be found, but `type B` wouldn't since it's not defined in the companion object but the type. The new algorithm allow us to backtrack once we find these lookup failures (as we're browsing through the RootPackage members, we know that we will find at least one solution). Therefore, we perform a full DFS pre-order traversal in the binary tree that represents all the possibilities.

jvican

I've gone through the remaining set of comments. Current version has more documentation, a simplified implementation product of several iterations, and higher test covering (with added tests).

I have one question:

Where should I define exceptions for scalafix? Do you have the infrastructure for error handling yet? Perhaps it's better to use sys.error until you get to it?

And some remarks on the current implementation:

The errors in cats cannot be fixed with -Yrangepos. The type in the implicit val is inferred and cannot be fetched from the sources. Therefore, implicit val types whose types depend on the Kind projector cannot be fixed in a principled way. I suggest that we think about a low-level fix to still print the string representation of the types (remember that being an inferred type, its original tree is also an empty TypeTree).
We're still not handling imports inside DefTrees. I'll have a look at that tomorrow.
Implicit vals that depend on type alias will get the dealiased type.

type Foo = String
implicit val foo = new Foo

will be rewritten as:

implicit val foo: String = new Foo

I'm still thinking of a solution to this (if there's one, it's not clear to me we can be consistent with type aliases without introducing misbehaviours--scalac only give us the expanded type).

jvican · 2017-01-24T22:23:27Z

core/src/test/resources/ExplicitImplicit/basic.source

@@ -8,14 +8,37 @@ class A {
  implicit val x: List[Int] = List(1)
 }
 <<< map
+import scala.concurrent.Future


jvican · 2017-01-24T22:23:49Z

core/src/test/resources/ExplicitImplicit/basic.source

+>>>
+import scala.collection.immutable.{Map => MyMap}
+class A {
+  implicit val x: MyMap[Int, String] =


Have a look at the other two tests checking renames.

jvican · 2017-01-24T22:24:37Z

core/src/test/resources/ExplicitImplicit/basic.source

 }
 >>>
 class A {
  class B { class C }
  implicit val x: B = new B
  implicit val y: x.C = new x.C
 }
+<<< NOWRAP higher kinded cats
+package cats {


This is just a reproduction of an intricate scenario. Even though it has the names cats, I would leave it here. If you disagree, please point to concrete file you'd like to have it.

jvican · 2017-01-24T22:25:03Z

core/src/test/resources/ExplicitImplicit/basic.source

+  }
+}
+<<< higher kinded
+import scala.concurrent.Future


No, this line is needed. If you have a look at the shortened Future in the implicit val type result, you'll see that the import oracle correctly shortens the type because it finds it in the context.

jvican · 2017-01-24T22:25:12Z

core/src/test/resources/ExplicitImplicit/basic.source

+  trait C {
+    class B
+  }
+  implicit val x: D.c.B = new D.c.B


xeno-by

I didn't have time to look at the algorithm in details, but went through the rest of the code.

xeno-by · 2017-01-24T20:56:39Z

core/src/test/resources/ExplicitImplicit/basic.source

+>>>
+import scala.collection.immutable.{Map => MyMap}
+class A {
+  implicit val x: MyMap[Int, String] =


xeno-by · 2017-01-24T21:01:17Z

core/src/test/resources/ExplicitImplicit/basic.source

 object slick {
  case class Supplier(id: Int, name: String)
  implicit val supplierGetter = (arg: (Int, String)) => Supplier(arg._1, arg._2)
 }
 >>>
 object slick {
  case class Supplier(id: Int, name: String)
-  implicit val supplierGetter: ((Int, String)) => slick.Supplier = (arg: (Int, String)) => Supplier(arg._1, arg._2)
+  implicit val supplierGetter: ((Int, String)) => Supplier = (arg: (Int, String)) => Supplier(arg._1, arg._2)
 }
 <<< NOWRAP package import
 package scala.concurrent {


Some tricky examples that come to mind:

package a { class C } package b { class C } package c { object M { import a._ import b._ implicit val c = new b.C } }

trait T { class C } package object a extends T { } object M { import a._ implicit val c = new C }

package a { class C } package b { class C } package c { object M { import a._ locally { import b._ implicit val c = new C } } }

Thanks for the code snippets, will add to tests. Forgot about our beloved locally 😄.

Awesome tests! Very nice 👍

xeno-by · 2017-01-24T22:26:29Z

scalafix-nsc/src/main/scala/scalafix/nsc/NscSemanticApi.scala

+  /** The compiler sets different symbols for `PackageDef`s
+    * and term names pointing to that package. Get the
+    * underlying symbol of `moduleClass` for them to be equal. */
+  @inline


Why @inline here and below?

They can be removed.

xeno-by · 2017-01-24T22:32:28Z

scalafix-nsc/src/main/scala/scalafix/nsc/NscSemanticApi.scala

+    val mixedScope = sc.cloneScope
+    sc2.foreach(s => mixedScope.enterIfNew(s))
+    mixedScope
+  }


I'd probably use a dedicated data structure to manipulate scopes. Things like enterIfNew may have hidden performance costs.

There is indeed a performance cost attached to it. I'll study a better data structure to manipulate scopes in the future, right now we're focusing more on correctness. I've also realised that I should not be using enterIfNew in some concrete parts of the code. Fixing tomorrow.

Anything trivial, e.g. case class Scope(syms: List[Symbol]), would do. The implementation of Scope is quite intricate, so it'll be just an unnecessary complication.

We should optimize for correctness and simplicity for now. We can worry about performance later.

xeno-by · 2017-01-24T22:35:40Z

scalafix-nsc/src/main/scala/scalafix/nsc/NscSemanticApi.scala

+    // Handle packages to import from user-defined compiler flags
+    val packagesToImportFrom = {
+      if (g.settings.noimports) Nil
+      else if (unit.isJava) RootImports.javaImports


Can unit.isJava ever be true?

I got this logic from nsc Context just in case it is sometimes true. I would expect it to be false almost always, but better safe than sorry?

scalafix-nsc guards against unit.isJava, so this will always be false. I don't mind keeping it though for consistency with the compiler impl.

xeno-by · 2017-01-24T22:48:29Z

scalafix-nsc/src/main/scala/scalafix/nsc/NscSemanticApi.scala

+    traverser.traverse(unit.body)
+
+    def toMetaType(tp: g.Tree) = {
+      config.dialect(tp.toString).parse[m.Type].get


There's no guarantee that parse is going to succeed here. Do you plan to make this more robust?

This is actually the cause of a problem with cats. We have no general solution so far, tp may not have a tree representation (inferred) and it's not fetchable from sources. Any suggestion @xeno-by?

Wait for a Type converter that is coming to 2.x along with nirvana and friends.

I'm not sure if the type converter will solve the problem either. Kind-projector synthesizes unique type names for each appearance of Type.Name(?). The synthesized types look like this

cats.laws.discipline.CartesianTests.Isomorphisms[[β$0$]cats.free.Free[Option,β$0$]]

I'm not 100% sure, but I think those should be pretty-printed as ?. Maybe we'll need to special case kind-projector since it's a very popular compiler plugin.

The best solution for now is to gracefully handle the parse error and not shorten that type signature. The type signature of shortenType indicates that it's always a safe operation, and indeed, you can always choose not to shorten the type.

We can specialcase kind-projector, yeah.

The [T]Foo[T] type doesn't have surface syntax in Scala, so it'll have to be emulated - either with kind-projector or with the { type λ[T] = Foo[T] }#λ trick. This can be done automatically by the type converter.

xeno-by · 2017-01-24T22:52:47Z

scalafix-nsc/src/main/scala/scalafix/nsc/NscSemanticApi.scala

    new SemanticApi {
+      override def shortenType(tpe: m.Type, owner: m.Tree): m.Type = {


Should owner be renamed to atLocation to be consistent with the API?

If owner is only used for its position, why not pass a position here in the first place?

Yes and yes. We decided to pass owner just in case we could need it afterwards, but it's unnecessary IIRC.

This signature is tricky because the current impl only works for defn.{val,var,def}, so the owner should really be a Defn, if anything. With a position, it wouldn't be clear which position you should pass in e.g. the defn's name or the defn itself.

IIRC, the owner needs to map to some tree with an ownerchain.

Can the disambiguation between defn and defn's name be done automatically?

xeno-by · 2017-01-24T23:11:42Z

scalafix-nsc/src/main/scala/scalafix/nsc/NscSemanticApi.scala

+        val bottomUpScope = interestingOwners.iterator
+          .map(_.info.members.filter(keepRelevantMembers))
+          .reduce(mixScopes _)
+        val globalScope = mixScopes(bottomUpScope, userImportsScope)


Does this take import priority into account?

It doesn't yet, thanks for pointing out.

xeno-by · 2017-01-24T23:14:03Z

scalafix-nsc/src/main/scala/scalafix/nsc/NscSemanticApi.scala

+                        inScope: g.Scope,
+                        enclosingTerm: g.Symbol): m.Ref = {
+
+      val refNoThis = stripThis(toShorten)


Will this strip things like C.this.foo or only some.pkg.this.foo?

Both, actually. They will be converted to C.foo and some.pkg.foo.

What if C is an enclosing class? Then C.foo doesn't make sense.

If I understand correctly, Foo.this.bar can be stripped to bar in all cases except the following?

trait Foo { def bar trait Kas { def bar Foo.this.bar } }

I think so, yes.

olafurpg · 2017-01-25T09:12:12Z

BTW. Maybe move the new test cases to a ShortenType.source. We've gone waay beyond basic.source :D

jvican · 2017-02-13T17:01:31Z

I'll rebase this on top of the semantic API soon.

olafurpg · 2017-04-03T10:35:46Z

With the introduction of scalahost and the scala.meta semantic API, I believe context aware scope would be a great feature to have in scala.meta instead of scalafix. Also, scalafix-nsc is being phased out in favor of the offline Mirror so I think that is the best way forward.

jvican · 2017-04-03T11:32:40Z

@xeno-by I'm planning to follow up on this next weekend. Will you accept a PR fully implementing a symbol-based type shortener with the new semantic API?

olafurpg · 2017-04-03T11:35:31Z

I think a good start would be to open a ticket in scalameta listing the details of this feature, what methods/signatures it would have etc.

jvican added the enhancement label Jan 23, 2017

jvican self-assigned this Jan 23, 2017

jvican requested a review from olafurpg January 23, 2017 10:31

jvican commented Jan 23, 2017

View reviewed changes

olafurpg reviewed Jan 23, 2017

View reviewed changes

olafurpg reviewed Jan 24, 2017

View reviewed changes

jvican force-pushed the imports branch from 8a45325 to 7556d08 Compare January 24, 2017 18:18

jvican added 13 commits January 24, 2017 19:22

Add very rough prototype of import oracle

9f891dc

Increase test success rate of prototype

8f8270a

Improve scope and symbol handling rules

9c41f1d

Add better scope handling with symbols

7d35b92

Apply type rewrites in type args and add tests

373b67b

Improve API, remove stale bits and add comments

6cbba37

Pick last top level package to insert imports

19ba2d5

Handle renames and n => _ in the Scope traverser

e6b7df2

Improve handling of shadowed symbols in scope

36a4120

Replace 'removed map' test by local map definition

a5c07a2

Extract core tasks into independent functions

b52e51b

Don't add imports, just shorten type

fafc9fc

Adding and organizing imports is delegated to a future rewrite. `shortenType` will only perform a type shortening at a given location. This greatly simplifies the implementation, which has to be still cleaned up for readability purposes.

Refactor root imports and add some of the feedback

25f4949

jvican added 4 commits January 24, 2017 19:22

Use principled approach to deal with imports

d3a7a68

Try to make CI pass by disabling scalajs

71f922d

WIP simplification

4745229

Address @olafurpg's comments

29f6dd6

jvican force-pushed the imports branch from 7556d08 to 29f6dd6 Compare January 24, 2017 18:23

jvican added 2 commits January 24, 2017 22:39

Make sure that user-defined renames are used

b6fee12

jvican commented Jan 24, 2017

View reviewed changes

Remove logger and ONLY in tests

bf341f7

jvican force-pushed the imports branch from 680086b to bf341f7 Compare January 24, 2017 22:38

xeno-by reviewed Jan 24, 2017

View reviewed changes

xeno-by mentioned this pull request Jan 30, 2017

Tree.lexicalScope scalameta/scalameta#614

Closed

2 tasks

olafurpg removed the enhancement label Feb 24, 2017

olafurpg mentioned this pull request Mar 6, 2017

Import shadowing #65

Closed

olafurpg closed this Apr 3, 2017

olafurpg mentioned this pull request Aug 21, 2017

Ability to query names in scope scalameta/scalameta#1075

Closed

olafurpg mentioned this pull request Sep 6, 2017

ExplicitReturnTypes with unsafeShortenNames is adding current file class as import #329

Closed

bjaglin pushed a commit to liancheng/scalafix that referenced this pull request May 23, 2023

Provide an IntelliJ compatible mode (scalacenter#42)

5c3e261

		new SemanticApi {
		override def shortenType(tpe: m.Type, owner: m.Tree): m.Type = {

Add symbol-based type shortener #42

Add symbol-based type shortener #42

Conversation

jvican commented Jan 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican commented Jan 23, 2017

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican commented Jan 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican Jan 24, 2017 • edited Loading

Choose a reason for hiding this comment

jvican Jan 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xeno-by left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican Jan 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xeno-by Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican commented Jan 23, 2017 •

edited

Loading

jvican left a comment •

edited

Loading

jvican Jan 24, 2017 •

edited

Loading

jvican Jan 24, 2017 •

edited

Loading

jvican Jan 24, 2017 •

edited

Loading

xeno-by Jan 25, 2017 •

edited

Loading