WIP - adding example to cross-rewrite #466

ShaneDelmore · 2017-03-28T18:24:24Z

This is not even close to ready, just making a pr to see if I am on the right track.

CLAassistant · 2017-03-28T18:25:25Z

All committers have signed the CLA.

ShaneDelmore · 2017-03-28T18:42:57Z

examples/cross-rewrite-example/build/build.scala

-class Build(val context: Context) extends BaseBuild{ outer =>
-  override def defaultScalaVersion = "2.11.8"
+class Build(val context: Context) extends BaseBuild { outer =>
+  override def defaultScalaVersion = "2.12.1"


I changed the default because I thought people might like to write code in the latest version and backwards compile.

ShaneDelmore · 2017-03-28T18:43:52Z

examples/cross-rewrite-example/build/build.scala

+  override def test: Dependency = {
+    new BasicBuild(context) with ScalaTest {
+      override def dependencies = outer +: super.dependencies
+      override def defaultScalaVersion = "2.12.1"


Is this correct that I need to override version for test build (why would I ever want to test on a different version?)
Or should my test be extending my main project somehow to have access to the code in it?

you overwrote defautScalaVersion is class Build, but here you are instantiating class BasicBuild. That's fine, but it doesn't magically pick up on what you overwrote in Build. So if you want this as 2.12.1 you need to provide that info here somehow.

ShaneDelmore · 2017-03-28T18:45:13Z

examples/cross-rewrite-example/build/build.scala

+              val fromTo = lib.autoRelative(outer.sources).collect {
+                case (location, relative) if location.isFile =>
+                  location -> projectDirectory / "src" / relative
+              }


scalafmt must have changed all of this slightly. Would you be interested in a pr adding a .scalafmt config to the project?

Not completely warmed up to scalafmt yet. I played around with it a bit, but it currently does not allow me to produce code that I find as readable as scalariform. So for now I have settled on Scalariform for CBT and there is a preconfigured plugins.scalariform plugin you can add as a dependency to your BuildBuild, allowing you to mix in Scalariform into your build, which adds a preconfigured task scalariform, which by default is not run on compile, but can be easily added (in which case one has to call scalariform.apply there, because it is lazy).

(had a longer discussion with olaf around the limitations of scalafmt for my personal preferred style a few weeks ago)

No problem. I don't mind using scalariform but I may take a shot at making a .scalafmt.conf that matches your scalariform preferences just to see if I can.

Scalafmt does not have any settings which mimic scalariform, they're very different formatters.

ShaneDelmore · 2017-03-28T18:46:59Z

examples/cross-rewrite-example/src/Main.scala

+object Main {
+  def main(args: Array[String]): Unit = {
+    //For 2.11.8 rewrite to case match
+    Disjunction.intOrString.map(println)


I could either rewrite this to a case statement for scalaz 2.11.8 (I don't believe they have an either enrichment for map) or I could change the type to /. Either one would require the semantic mirror which I am not clear is actually available based on @olafurpg comment about ExplicitImplicit being available. I will experiment more once I get tests to pass.

I think a semantic mirror is available, just not the Scalafix Mirror, which ExplicitImplicit is currently coded against.

ShaneDelmore · 2017-03-28T18:48:29Z

plugins/scalatest/ScalaTest.scala

@@ -9,7 +9,7 @@ trait ScalaTest extends BaseBuild{
    runSuites( suiteNames.map( loadSuite( _, classLoader ) ) )
    ExitCode.Success
  }
-  override def dependencies = super.dependencies ++ Resolver( mavenCentral ).bind( ScalaDependency("org.scalatest","scalatest","2.2.4") )
+  override def dependencies = super.dependencies ++ Resolver( mavenCentral ).bind( ScalaDependency("org.scalatest","scalatest","3.0.1") )


There is no 2.x version of Scalatest for scala 2.12 so I bumped to scalatest 3.0.1.

ShaneDelmore · 2017-03-28T18:49:27Z

plugins/scalatest/build/build.scala

@@ -5,6 +5,6 @@ class Build(val context: Context) extends BaseBuild{
    super.dependencies
    :+ context.cbtDependency
  ) ++ Resolver( mavenCentral ).bind(
-    ScalaDependency("org.scalatest","scalatest","2.2.4")
+    ScalaDependency("org.scalatest","scalatest","3.0.1")


I haven't looked into this yet but don't like that there are these duplicated strings in multiple files. Maybe I need to create a Dependencies.scala file to share them.

cvogt · 2017-03-28T20:24:48Z

examples/cross-rewrite-example/build/build.scala

+              val fromTo = lib.autoRelative(outer.sources).collect {
+                case (location, relative) if location.isFile =>
+                  location -> projectDirectory / "src" / relative
+              }


(had a longer discussion with olaf around the limitations of scalafmt for my personal preferred style a few weeks ago)

cvogt · 2017-03-28T20:25:48Z

examples/cross-rewrite-example/src/Main.scala

+object Main {
+  def main(args: Array[String]): Unit = {
+    //For 2.11.8 rewrite to case match
+    Disjunction.intOrString.map(println)


I think a semantic mirror is available, just not the Scalafix Mirror, which ExplicitImplicit is currently coded against.

cvogt · 2017-03-29T14:57:34Z

examples/cross-rewrite-example/build/build.scala

+  override def defaultScalaVersion = "2.12.1"
+
+  override def test: Dependency = {
+    new BasicBuild(context) with ScalaTest {


you need to add override def projectDirectory = outer.projectDirectory / "test" here, so it can find your source files. Otherwise it only looks in context.workingDirectory and context.workingDirectory / "src". context.workingDirectory is the default for BaseBuild.projectDirectory and the directory one up from the build.scala file. I'd suggest you look start looking into the source code a little bit. CBT makes that quite easy :).

cvogt · 2017-03-31T02:38:12Z

the tests check for the existence of a cats AND scalaz crossbuilds, which is why this still fails ;).

ShaneDelmore · 2017-03-31T02:56:10Z

Ah, that makes sense. That should be easy enough for me to resolve Saturday.

ShaneDelmore · 2017-03-31T03:56:09Z

examples/cross-rewrite-example/build/build.scala

+        Replace(Symbol("_root_.Disjunction."), q"Test"),
+        Rename(q"intOrString", q"xor"),
+        //And this works again.  What is different about import rewrites vs the others?
+        RemoveGlobalImport(importer"scala.concurrent.Future")


I am not sure if I am missing something or if there is a bug in scalafix, but import releated rewrites work and others do not. I can't tell what the difference is yet. I looked back through scalafix code and these are all instances of TreePatch. Once the patching works this should be as simple as adding scalaz./, replacing Either with / and Right with /-

olafurpg · 2017-03-31T08:32:51Z

examples/cross-rewrite-example/build/build.scala

+        //None of these work, why?
+        Replace(Symbol("_root_.scala.util.Either."), q"\/"),
+        Replace(Symbol("_root_.Disjunction."), q"Test"),
+        Rename(q"intOrString", q"xor"),


Rename should probably be called RenameThisInstance (or similar) because it's reference based, I opened scalacenter/scalafix#109 to track that change. To replace call-sites of a specific definition you want to use Replace, which is symbol based.

olafurpg · 2017-03-31T08:34:52Z

examples/cross-rewrite-example/build/build.scala

+        //This works
+        AddGlobalImport(importer"scalaz._"),
+        //None of these work, why?
+        Replace(Symbol("_root_.scala.util.Either."), q"\/"),


Have you tried Mirror.database.toString to double check that this symbol matches the symbol you want to rewrite? Otherwise, please open an issue in scalafix and I can take a look at it next week.

The type of scala.util.Either is actually scala.package.Either. Printing out the mirror.database helped, thanks @olafurpg

ShaneDelmore · 2017-04-01T22:15:41Z

plugins/scalafix/Scalafix.scala

+            println("printing mirror database messages")
+            println(ctx.mirror.database)
+            println("end printing")
+            assert(ctx.mirror.database.toString.trim != "", "I do not believe the database should be empty here")


@olafurpg The semantic db appears to be empty here even though sources is not. Do you think the semantic db is being constructed incorrectly, or that I should open a scalameta issue on this?

The project source files need to be compiled with the scalahost compiler plugin for the database to be built, so the Scalameta plugin needs to be mixed in the root build. The mirror is composed of a classpath (pointing to at least one directory containing semanticdb files) and a sourcepath (pointing to scala source files). In this case, the classpath does not contain any semanticdb files, maybe the Mirror constructor should fail in that case.

I do believe fail fast would be better here, but this should be enough to advice to get me back on track. 👍

olafurpg · 2017-04-03T09:39:54Z

plugins/scalafix/Scalafix.scala

        Mirror(
          classpath.string,
          files.map(_._1).mkString(File.pathSeparator)
        )
+      }

      def context(file: File): ( RewriteCtx[Mirror], RewriteCtx[ScalafixMirror] ) = (


RewriteCtx[ScalafixMirror] is not necessary here, ScalafixMirror will go away in favor of Mirror.

olafurpg · 2017-04-03T09:42:58Z

examples/cross-rewrite-example/build/build.scala

@@ -6,67 +6,101 @@ import scalafix.util._
 import scalafix.util.TreePatch._
 import scalafix.util.TokenPatch._

-class Build(val context: Context) extends BaseBuild{ outer =>
+class Build(val context: Context) extends BaseBuild { outer =>


There's missing a with Scalameta here to inject the scalahost compiler plugin, which generates semanticdb files under target/.

Doh! Thanks for catching that.

olafurpg · 2017-04-03T09:43:43Z

examples/cross-rewrite-example/build/build.scala

+              val fromTo = lib.autoRelative(outer.sources).collect {
+                case (location, relative) if location.isFile =>
+                  location -> projectDirectory / "src" / relative
+              }


Scalafmt does not have any settings which mimic scalariform, they're very different formatters.

olafurpg · 2017-04-03T09:47:42Z

plugins/scalafix/Scalafix.scala

+            println("printing mirror database messages")
+            println(ctx.mirror.database)
+            println("end printing")
+            assert(ctx.mirror.database.toString.trim != "", "I do not believe the database should be empty here")


The project source files need to be compiled with the scalahost compiler plugin for the database to be built, so the Scalameta plugin needs to be mixed in the root build. The mirror is composed of a classpath (pointing to at least one directory containing semanticdb files) and a sourcepath (pointing to scala source files). In this case, the classpath does not contain any semanticdb files, maybe the Mirror constructor should fail in that case.

ShaneDelmore · 2017-04-08T21:19:56Z

examples/cross-rewrite-example/build/build.scala

+    //Over time I would hope to build up a community libraryof rewrites so many of the transforms would be pulled from
+    //a library instead of declared in the build
+  }
+


Ignore everything below here, I didn't mean to commit it.

would contain wrong classLoader when switching scalaVersions)

otherwise those libraries wouldn’t find the classes we might have to reconsider this eventually

running something, not the one that may end up in a cache

rather than the more obscure context object

(there is still a test failure, where the code hasn’t been rewritten properly, but that’s ok. One step at a time.)

ShaneDelmore · 2017-05-01T03:56:59Z

I forgot all about this PR. Will try to take a look at it in a few days and see if I can figure out why tests fail.

ShaneDelmore commented Mar 28, 2017

View reviewed changes

ShaneDelmore force-pushed the meta branch from 36b7803 to 80d4865 Compare March 30, 2017 20:01

cvogt reviewed Mar 30, 2017

View reviewed changes

ShaneDelmore commented Mar 31, 2017

View reviewed changes

olafurpg reviewed Mar 31, 2017

View reviewed changes

ShaneDelmore commented Apr 1, 2017

View reviewed changes

olafurpg reviewed Apr 3, 2017

View reviewed changes

ShaneDelmore force-pushed the meta branch from b42ea27 to 6b3097c Compare April 8, 2017 17:32

ShaneDelmore commented Apr 8, 2017

View reviewed changes

ShaneDelmore force-pushed the meta branch from de96ead to 4a6814d Compare April 9, 2017 17:47

ShaneDelmore and others added 12 commits April 12, 2017 18:34

Adding rewrite and test examples from 2.12 to 2.11 with cats and scalaz

c8ac544

Demonstration of build api I have in my head.

2a7e7d7

fix moduleKey cache keys to respect scalaVersion (otherwise cache

43cb700

would contain wrong classLoader when switching scalaVersions)

treat provided and runtime dependencies as mandatory ones for now

db730e3

otherwise those libraries wouldn’t find the classes we might have to reconsider this eventually

flatClassLoader should probably only affect the classLoader used when

efa8606

running something, not the one that may end up in a cache

disable DayDreaming

f07d01b

name default build self name more clearly and refer to it’s fields

7c36795

rather than the more obscure context object

fixing cross-build-example tests to run across versions

5d7fe52

(there is still a test failure, where the code hasn’t been rewritten properly, but that’s ok. One step at a time.)

All tests pass.

50add61

factor logic out into helpers

fc317ce

clean up cross-rewrite-example (more uniform)

251e9f7

format code (breaks rewrites!)

0d3e56d

ShaneDelmore force-pushed the meta branch from 10ef4c9 to 0d3e56d Compare April 13, 2017 01:35

Add example of semantic rewrite

2251bd7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP - adding example to cross-rewrite #466

WIP - adding example to cross-rewrite #466

ShaneDelmore commented Mar 28, 2017

CLAassistant commented Mar 28, 2017 •

edited

ShaneDelmore Mar 28, 2017

cvogt Mar 28, 2017

ShaneDelmore Mar 28, 2017

cvogt Mar 28, 2017

ShaneDelmore Mar 28, 2017

cvogt Mar 28, 2017

cvogt Mar 28, 2017

ShaneDelmore Mar 28, 2017

olafurpg Apr 3, 2017

ShaneDelmore Mar 28, 2017

cvogt Mar 28, 2017

ShaneDelmore Mar 28, 2017

ShaneDelmore Mar 28, 2017

cvogt Mar 28, 2017

cvogt Mar 28, 2017

cvogt Mar 29, 2017

cvogt commented Mar 31, 2017

ShaneDelmore commented Mar 31, 2017

ShaneDelmore Mar 31, 2017

olafurpg Mar 31, 2017

olafurpg Mar 31, 2017

ShaneDelmore Apr 1, 2017

ShaneDelmore Apr 1, 2017

olafurpg Apr 3, 2017

ShaneDelmore Apr 3, 2017

olafurpg Apr 3, 2017

olafurpg Apr 3, 2017

ShaneDelmore Apr 3, 2017

olafurpg Apr 3, 2017

olafurpg Apr 3, 2017

ShaneDelmore Apr 8, 2017

ShaneDelmore commented May 1, 2017

WIP - adding example to cross-rewrite #466

Are you sure you want to change the base?

WIP - adding example to cross-rewrite #466

Conversation

ShaneDelmore commented Mar 28, 2017

CLAassistant commented Mar 28, 2017 • edited

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

cvogt commented Mar 31, 2017

ShaneDelmore commented Mar 31, 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

ShaneDelmore commented May 1, 2017

CLAassistant commented Mar 28, 2017 •

edited