Upgrade to Scalameta v4.0.0-M4 #756

olafurpg · 2018-06-22T10:08:16Z

Fixes #749
Fixes #757

move scala.meta.{Symbol,Signature} to scalafix.v0 package
improve code quality in the type printer:
- much cleaner pattern matching thanks to new sealed oneof encoding
- existential types and universal types no longer require hacks because of Scope, thanks @xeno-by ❤️
sbt-scalafix is removed in this PR because it will be moved into a separate repo and I prefer to handle the upgrade after the split.

The plugin will be moved to a separate repo.

xeno-by · 2018-06-22T11:00:56Z

project/ScalafixBuild.scala

-        "-Xplugin-require:semanticdb"
+        "-Xplugin-require:semanticdb",
+        "-P:semanticdb:synthetics:on",
+        "-P:semanticdb:text:on"


Is -P:semanticdb:text still required?

Opened #757 to track this. This PR is already a lot, prefer to delegate this to a another PR.

xeno-by · 2018-06-22T11:01:12Z

project/ScalafixBuild.scala

@@ -74,7 +74,9 @@ object ScalafixBuild extends AutoPlugin with GhpagesKeys {
    lazy val semanticdbSettings = Seq(
      scalacOptions ++= List(
        "-Yrangepos",
-        "-Xplugin-require:semanticdb"
+        "-Xplugin-require:semanticdb",
+        "-P:semanticdb:synthetics:on",


What functionality requires -P:semanticdb:synthetics?

The NoInfer rule requires the setting, I've updated the docs to tell users they need this setting for this rule.

xeno-by · 2018-06-22T11:05:19Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/PrettyType.scala

+        case _ =>
+          throw new IllegalArgumentException(info.toProtoString)
+      }
+    }


I wonder where this comes into play?

I renamed the method to .valueType, it's a helper method to get the type of a ValueSignature or fail.

xeno-by · 2018-06-22T11:06:53Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/PrettyType.scala

+  private implicit class XtensionScopeHardlinks(scope: s.Scope) {
+    def infos: List[s.SymbolInformation] =
+      if (scope.hardlinks.isEmpty) scope.symlinks.iterator.map(info).toList
+      else scope.hardlinks.toList


Don't we already have a method like that in semanticdb?

There is a method like this but it requires access to a symbol table to resolve the symlinks.

Ah, I missed the .map(info) part.

xeno-by · 2018-06-22T11:08:07Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/PrettyType.scala

-              !sym.contains("$anon"))
-          val typeParameters = classInfo.typeParameters
-          val parents = classInfo.parents
+            decls.infos.filter(info => !info.symbol.contains("$anon"))


Is this filter still necessary after all the fixes that we did around 4.0.0-M2?

Good question. Doesn't seem required anymore, I removed it.

xeno-by · 2018-06-22T11:14:27Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/PrettyType.scala

              if (shorten.isName) {
                name
              } else {
                toTypeRef(info(symbol))
              }
+            case _: s.SingleType | _: s.ThisType | _: s.SuperType |
+                _: s.ConstantType =>
+              Type.Select(toTermRef(prefix), name)


Can a constant type be a prefix to anything?

I don't think so, removing it seems to be OK. This was a direct refactoring of the old implementation. Hurray for breaking down SingletonType 🎉

xeno-by · 2018-06-22T11:15:00Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/PrettyType.scala

+        Type.Name(this.info(symbol).name)
+      )
+    case s.SuperType(prefix @ _, symbol) =>
+      // TODO: prefix


FIXME with a ticket?

xeno-by · 2018-06-22T11:15:18Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/PrettyType.scala

+    case s.ConstantType(_) =>
+      toType(tpe.widen)
+    case s.ExistentialType(underlying, Some(declarations)) =>
+      withHardlinks(declarations.hardlinks) { () =>


How did you find dealing with hardlinks?

It's been great so far 😊 Removed a lot of ???/WTFs in the old implementation. I currently keep a mutable scope for hardlink lookups, I could make it immutable but that would require adding a (hardlinks: Map[String, SymbolInformation) parameter everywhere for questionable benefit.

xeno-by · 2018-06-22T11:16:35Z

scalafix-core/shared/src/main/scala/scalafix/v0/Symbol.scala

+import scala.meta.internal.semanticdb.Scala._
+import scalafix.internal.util.SymbolOps.Root
+
+sealed trait Symbol extends Product {


Oh hey. Long time no see :)

xeno-by · 2018-06-22T11:17:23Z

scalafix-tests/unit/src/test/scala/scalafix/tests/core/PrettyTypeSuite.scala

@@ -47,13 +46,13 @@ class PrettyTypeSuite extends BasePrettyTypeSuite {
 }

 // This test is slow, no need to run it on every PR
-@Ignore
+//@Ignore


Is this comment accidental?

Removed the comment.

- Fix TODOs: - Print Defn.Object types - Print all s.Accessibility tags - Print Pkg.Object - Add `fatalErrors: Boolean` option to optionally fail on errors. In the fuzz tests we ignore fatal errors like NoSuchElementException because they are bugs that need to be fixed upstream. - Remove irrelevant TODOs - Remove unnecessary guards - Add link to GitHub issue for relevant TODOs - Remove workarounds for bugs that have been fixed upstream - Document when users need to enable -P:semanticdb:synthetics:on

olafurpg

Thank you for the detailed review @xeno-by I've addressed your comments and made several more improvements.

olafurpg · 2018-06-22T11:35:10Z

project/ScalafixBuild.scala

@@ -74,7 +74,9 @@ object ScalafixBuild extends AutoPlugin with GhpagesKeys {
    lazy val semanticdbSettings = Seq(
      scalacOptions ++= List(
        "-Yrangepos",
-        "-Xplugin-require:semanticdb"
+        "-Xplugin-require:semanticdb",
+        "-P:semanticdb:synthetics:on",


The NoInfer rule requires the setting, I've updated the docs to tell users they need this setting for this rule.

olafurpg · 2018-06-22T11:39:52Z

project/ScalafixBuild.scala

-        "-Xplugin-require:semanticdb"
+        "-Xplugin-require:semanticdb",
+        "-P:semanticdb:synthetics:on",
+        "-P:semanticdb:text:on"


Opened #757 to track this. This PR is already a lot, prefer to delegate this to a another PR.

olafurpg · 2018-06-22T11:40:44Z

scalafix-tests/unit/src/test/scala/scalafix/tests/core/PrettyTypeSuite.scala

@@ -47,13 +46,13 @@ class PrettyTypeSuite extends BasePrettyTypeSuite {
 }

 // This test is slow, no need to run it on every PR
-@Ignore
+//@Ignore


Removed the comment.

olafurpg · 2018-06-22T11:45:32Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/PrettyType.scala

+        case _ =>
+          throw new IllegalArgumentException(info.toProtoString)
+      }
+    }


I renamed the method to .valueType, it's a helper method to get the type of a ValueSignature or fail.

olafurpg · 2018-06-22T11:45:56Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/PrettyType.scala

+  private implicit class XtensionScopeHardlinks(scope: s.Scope) {
+    def infos: List[s.SymbolInformation] =
+      if (scope.hardlinks.isEmpty) scope.symlinks.iterator.map(info).toList
+      else scope.hardlinks.toList


There is a method like this but it requires access to a symbol table to resolve the symlinks.

olafurpg · 2018-06-22T12:37:41Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/PrettyType.scala

+      )
+    case targ =>
+      targ
+  }


Yup. RepeatedType can appear in both Init position and also for Defn.Def.decltpe

case class A(a: AnyRef*)

currently produces a val a: AnyRef* method symbol

olafurpg · 2018-06-22T12:47:12Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/PrettyType.scala

          None
        case NonFatal(e) =>
+          e.setStackTrace(e.getStackTrace.filter { e =>
+            e.getClassName.startsWith("scalafix")
+          })


I've replaced this with a fatalErrors: Boolean option to control if exceptions are caught or not. It's possible to set ExplicitResultTypes.fatalWarnings = true to control the option. There are still too many NoSuchElementException, for example

Metacp emits reference to non-existent term symbol for val forwarders scalameta/scalameta#1625

Scalacp emits wrong symbols for Java references scalameta/scalameta#1491

Metac should emit global symbol definitions even if they don't have occurrences scalameta/scalameta#1504

olafurpg · 2018-06-22T12:49:04Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/PrettyType.scala

              if (shorten.isName) {
                name
              } else {
                toTypeRef(info(symbol))
              }
+            case _: s.SingleType | _: s.ThisType | _: s.SuperType |
+                _: s.ConstantType =>
+              Type.Select(toTermRef(prefix), name)


I don't think so, removing it seems to be OK. This was a direct refactoring of the old implementation. Hurray for breaking down SingletonType 🎉

olafurpg · 2018-06-22T12:49:17Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/PrettyType.scala

+        Type.Name(this.info(symbol).name)
+      )
+    case s.SuperType(prefix @ _, symbol) =>
+      // TODO: prefix


olafurpg · 2018-06-22T12:51:30Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/PrettyType.scala

+    case s.ConstantType(_) =>
+      toType(tpe.widen)
+    case s.ExistentialType(underlying, Some(declarations)) =>
+      withHardlinks(declarations.hardlinks) { () =>


It's been great so far 😊 Removed a lot of ???/WTFs in the old implementation. I currently keep a mutable scope for hardlink lookups, I could make it immutable but that would require adding a (hardlinks: Map[String, SymbolInformation) parameter everywhere for questionable benefit.

Our build upgraded coursier to 1.1.0-M4 to synchronzie the directories-jvm upgrade with metacp. This problem will go away once we publish scalafix-big with shaded dependencies and move sbt-scalafix to an external repo scalacenter#751 ``` java.lang.NoSuchMethodError: coursier.Cache$.fetch(Ljava/io/File;Lcoursier/CachePolicy;Lscala/collection/Seq;Lscala/Option;Ljava/util/concurrent/ExecutorService;Lscala/Option;)Lscala/Function1; ```

olafurpg · 2018-06-22T14:25:54Z

I am working on #757 right now, the recent refactorings of the cli made it easier than I expected. Let's wait merging this until #757 is fixed.

xeno-by · 2018-06-22T17:52:45Z

LGTM.

olafurpg added 14 commits June 21, 2018 19:46

Add scala.meta.{Symbol,Signature}

42636e7

Sed

ec06d04

s/semanticdb3/semanticdb/

c311561

s/semanticdb.Symbol/v0.Symbol/

8410add

Manual low-hanging fixes

fa45226

Make PrettyTypeSuite pass

458a091

Upgrade to coursier 1.1.0

204e76b

Fix remaining failing tests \o/

f67d571

Handle existential types

260bc29

Add NOTICE for LazySymbolTable

9288d83

Update universal types to use hardlinks too

83fe3a8

Remove sbt-scalafix.

a60f840

The plugin will be moved to a separate repo.

Re-enable synchronous tests

6b0019f

Format

fd91c86

xeno-by reviewed Jun 22, 2018

View reviewed changes

Fix compilation error in the website project

82b0163

olafurpg mentioned this pull request Jun 22, 2018

Support -P:semanticdb:text:off #757

Closed

olafurpg commented Jun 22, 2018

View reviewed changes

olafurpg mentioned this pull request Jun 22, 2018

Move sbt-scalafix into a separate repository #751

Closed

olafurpg added 2 commits June 23, 2018 01:38

Support TextDocument.md5 to detect stale SemanticDBs

ebf42d0

Reuse logic for reading TextDocuments from AbsolutePath

bdb2d60

olafurpg force-pushed the M4 branch from cfa204d to bdb2d60 Compare June 22, 2018 23:44

Assert that TextDocument.schema matches expected value

ce591f6

olafurpg merged commit cbadb2b into scalacenter:master Jun 23, 2018

olafurpg deleted the M4 branch June 23, 2018 09:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to Scalameta v4.0.0-M4 #756

Upgrade to Scalameta v4.0.0-M4 #756

olafurpg commented Jun 22, 2018 •

edited

Loading

xeno-by Jun 22, 2018

olafurpg Jun 22, 2018

xeno-by Jun 22, 2018

olafurpg Jun 22, 2018

xeno-by Jun 22, 2018

olafurpg Jun 22, 2018

xeno-by Jun 22, 2018

olafurpg Jun 22, 2018

xeno-by Jun 22, 2018

xeno-by Jun 22, 2018

olafurpg Jun 22, 2018

xeno-by Jun 22, 2018

olafurpg Jun 22, 2018

xeno-by Jun 22, 2018

olafurpg Jun 22, 2018

xeno-by Jun 22, 2018

olafurpg Jun 22, 2018

xeno-by Jun 22, 2018

xeno-by Jun 22, 2018

olafurpg Jun 22, 2018

olafurpg left a comment

olafurpg Jun 22, 2018

olafurpg Jun 22, 2018

olafurpg Jun 22, 2018

olafurpg Jun 22, 2018

olafurpg Jun 22, 2018

olafurpg Jun 22, 2018

olafurpg Jun 22, 2018

olafurpg Jun 22, 2018

olafurpg Jun 22, 2018

olafurpg Jun 22, 2018

olafurpg commented Jun 22, 2018

xeno-by commented Jun 22, 2018

Upgrade to Scalameta v4.0.0-M4 #756

Upgrade to Scalameta v4.0.0-M4 #756

Conversation

olafurpg commented Jun 22, 2018 • 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

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

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

olafurpg commented Jun 22, 2018

xeno-by commented Jun 22, 2018

olafurpg commented Jun 22, 2018 •

edited

Loading