Make the breakOut rewrite rule cross compatible (fix #80) (fix #93) #102

MasseGuillaume · 2018-07-18T13:25:32Z

No description provided.

MasseGuillaume · 2018-07-18T13:28:37Z

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

+
+      val patchSpecificCollection =
+        toCollection match {
+          case "scala.collection.immutable.Map" => ctx.addRight(ap0, ".toMap")


@julienrf do you have more cases in mind where xs.to(C) is not cross compatible?

Any type constructor which has a kind different from * -> *. For instance, SortedMap, TreeMap, HashMap.

However, there are no toSortedMap or toTreeMap methods that we can use…

is it safe to cast to the target collection?

also: IntMap

MasseGuillaume · 2018-07-18T13:33:10Z

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

+      patchSpecificCollection
+    }
+
+    def extractColFromBreakout(breakout: Tree): Term = {


This is the trick I found.

import scala.reflect.runtime.universe._ reify { List(1).map(identity)(scala.collection.breakOut): Set[Int] } List.apply(1).map(identity)(breakOut(Set.canBuildFrom)): Set[Int] // ^^^ available in synthetics

MasseGuillaume · 2018-07-18T14:20:18Z

CI should be green, we are getting the same Scala.js runner error: https://travis-ci.org/scala/scala-collection-compat/jobs/405370378#L637

@sjrd does this stack trace ring a bell?

[info] Fast optimizing /home/travis/build/scala/scala-collection-compat/compat/.js/target/scala-2.11/scala-collection-compat-test-fastopt.js
java.lang.NullPointerException
	at org.scalajs.jsenv.nodejs.AbstractNodeJSEnv$NodeComJSRunner$class.send(AbstractNodeJSEnv.scala:247)
	at org.scalajs.jsenv.nodejs.NodeJSEnv$ComNodeRunner.send(NodeJSEnv.scala:70)
	at org.scalajs.testadapter.ComJSEnvRPC.send(ComJSEnvRPC.scala:67)
	at org.scalajs.testcommon.RPCCore.call(RPCCore.scala:137)
	at org.scalajs.testadapter.TestAdapter.loadFrameworks(TestAdapter.scala:53)
	at org.scalajs.sbtplugin.ScalaJSPluginInternal$$anonfun$79.apply(ScalaJSPluginInternal.scala:1012)
	at org.scalajs.sbtplugin.ScalaJSPluginInternal$$anonfun$79.apply(ScalaJSPluginInternal.scala:986)
	at scala.Function1$$anonfun$compose$1.apply(Function1.scala:47)
	at sbt.$tilde$greater$$anonfun$$u2219$1.apply(TypeFunctions.scala:40)
	at sbt.std.Transform$$anon$4.work(System.scala:63)
	at sbt.Execute$$anonfun$submit$1$$anonfun$apply$1.apply(Execute.scala:228)
	at sbt.Execute$$anonfun$submit$1$$anonfun$apply$1.apply(Execute.scala:228)
	at sbt.ErrorHandling$.wideConvert(ErrorHandling.scala:17)
	at sbt.Execute.work(Execute.scala:237)
	at sbt.Execute$$anonfun$submit$1.apply(Execute.scala:228)
	at sbt.Execute$$anonfun$submit$1.apply(Execute.scala:228)
	at sbt.ConcurrentRestrictions$$anon$4$$anonfun$1.apply(ConcurrentRestrictions.scala:159)
	at sbt.CompletionService$$anon$2.call(CompletionService.scala:28)
	at java.util.concurrent.FutureTask.run(FutureTask.java:262)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:473)
	at java.util.concurrent.FutureTask.run(FutureTask.java:262)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1152)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:622)
	at java.lang.Thread.run(Thread.java:748)
[error] (compatJS/test:loadedTestFrameworks) java.lang.NullPointerException
[error] Total time: 35 s, completed Jul 18, 2018 2:05:32 PM

sjrd · 2018-07-18T14:38:47Z

Looks like scala-js/scala-js#3408
Try restarting the build, as it is a race condition which only happens every once in a while.

MasseGuillaume · 2018-07-19T11:29:36Z

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

+
+    val `TraversableLike ++`  = Symbol("_root_.scala.collection.TraversableLike#`++`(Lscala/collection/GenTraversableOnce;Lscala/collection/generic/CanBuildFrom;)Ljava/lang/Object;.")
+    val `Vector ++`           = Symbol("_root_.scala.collection.immutable.Vector#`++`(Lscala/collection/GenTraversableOnce;Lscala/collection/generic/CanBuildFrom;)Ljava/lang/Object;.")
+    val `List ++`             = Symbol("_root_.scala.collection.immutable.List#`++`(Lscala/collection/GenTraversableOnce;Lscala/collection/generic/CanBuildFrom;)Ljava/lang/Object;.")


This should only be TraversableLike. The way symbols work in scalafix right now is similar to go-to-definition in an IDE, It will hit the most concrete class first. @olafurpg Is there a way only to hit the base class?

MasseGuillaume · 2018-07-19T17:01:28Z

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

+  // == rule ==
+  def apply(ctx: RuleCtx): Patch = {
+
+    var requiresCompatImport = false


if we only use .toMap we don't need the compat import.

uses scala/scala-collection-compat#102

MasseGuillaume · 2018-07-19T17:26:17Z

I just successfully removed breakout from Akka: akka/akka@3be0535

MasseGuillaume · 2018-07-20T13:30:38Z

@julienrf this one is good to go. CI is green, works on akka codebase.

julienrf · 2018-07-20T13:46:42Z

compat/src/main/scala-2.11/scala/collection/compat/TraversableOnceTupleExtensions.scala

+import scala.collection.{immutable => i, mutable => m}
+
+class TraversableOnceKVExtensionMethods[K, V](private val self: TraversableOnce[(K, V)]) extends AnyVal {
+  def toImmutableSortedMap(implicit ordering: Ordering[K]): i.SortedMap[K, V] = {


I’m on the fence. I think we should only try to provide the 2.13 syntax on 2.11 and 2.12, but we shouldn’t try to introduce new operations. Why can’t to(immutable.SortedMap) cross-compile?

Because to returns Coll[A], in the case of SortedMap it returns a Iterable[(K, V)].

Ah, I remember this one. Then I guess we have to write immutable.SortedMap.from(List(1 -> "1").map(x => x))

This is not cross-compatible.

It should be, with import scala.collection.compat._.

Some ToCBF are missing, I will implement them and fix: #103

There should be no CBF instance involved in that code. We should just enrich the immutable.SortedMap object with a from operation, like we previously did: https://github.com/scala/scala-collection-compat/blob/master/compat/src/main/scala-2.11_2.12/scala/collection/compat/package.scala#L36

I see this is a good idea!

This does not create an intermediate collection?

implicit class IterableFactoryExtensionMethods[CC[X] <: GenTraversable[X]](private val fact: GenericCompanion[CC]) { def from[A](source: TraversableOnce[A]): CC[A] = fact.apply(source.toSeq: _*) }

Yes, the .toSeq call does create an intermediate collection :-/

julienrf · 2018-07-20T13:47:33Z

compat/src/test/scala/test/scala/collection/Playground.scala

+  @Test
+  def t(): Unit = {
+
+  }


What’s the purpose of this test?

It's an empty test you can use as a sandbox. I keep finding myself to add it back locally, It's useful to have it here.

I find this a bit weird. Can’t you just have it locally if you only use it during development?

Can we keep it, it would save me some time when I want to test something quick or if I need to create another unit test.

I don’t understand why you can’t keep it locally only.

julienrf · 2018-07-20T13:51:31Z

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

+
+object Playground {
+
+}


I guess this is an oversight?

Ditto, it's useful.

What is this file testing?

There is no testOnly in scalafix-testkit. I mimic this with: https://github.com/scala/scala-collection-compat/blob/master/scalafix/tests/src/test/scala/fix/ScalafixTests.scala#L20

…cala#93)

MasseGuillaume · 2018-07-23T09:27:44Z

@julienrf ready for round 2.

julienrf

Awesome, thanks!

julienrf · 2018-07-23T10:13:51Z

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

@@ -1,25 +1,108 @@
 /*
-rule = "scala:fix.NewCollections"
+rule = "scala:fix.CrossCompat"


MasseGuillaume changed the title ~~Cross breakout~~ Make the breakOut rewrite rule cross compatible (fix #80) Jul 18, 2018

MasseGuillaume force-pushed the cross-breakout branch from a082870 to 765943b Compare July 18, 2018 13:27

MasseGuillaume commented Jul 18, 2018

View reviewed changes

MasseGuillaume force-pushed the cross-breakout branch from 765943b to 67511b2 Compare July 18, 2018 14:02

MasseGuillaume force-pushed the cross-breakout branch from 67511b2 to 3c382a5 Compare July 19, 2018 11:27

MasseGuillaume changed the title ~~Make the breakOut rewrite rule cross compatible (fix #80)~~ Make the breakOut rewrite rule cross compatible (fix #80) (fix #93) Jul 19, 2018

MasseGuillaume commented Jul 19, 2018

View reviewed changes

MasseGuillaume force-pushed the cross-breakout branch from 3c382a5 to 7312b59 Compare July 19, 2018 16:59

MasseGuillaume commented Jul 19, 2018

View reviewed changes

MasseGuillaume force-pushed the cross-breakout branch 2 times, most recently from ab8f965 to b8c613a Compare July 19, 2018 17:23

MasseGuillaume added a commit to MasseGuillaume/akka that referenced this pull request Jul 19, 2018

Remove breakout

3be0535

uses scala/scala-collection-compat#102

julienrf reviewed Jul 20, 2018

View reviewed changes

MasseGuillaume force-pushed the cross-breakout branch 3 times, most recently from cd3f48e to df2f4a2 Compare July 23, 2018 08:44

Make the breakOut rewrite rule cross compatible (fix scala#80) (fix s…

4f8d74b

…cala#93)

MasseGuillaume force-pushed the cross-breakout branch from df2f4a2 to 4f8d74b Compare July 23, 2018 08:59

julienrf approved these changes Jul 23, 2018

View reviewed changes

julienrf merged commit c2bbd2d into scala:master Jul 23, 2018

MasseGuillaume deleted the cross-breakout branch July 23, 2018 11:34

MasseGuillaume mentioned this pull request Jul 23, 2018

Fix broken .travis.yml #112

Merged

martijnhoekstra pushed a commit to martijnhoekstra/scala-collection-compat that referenced this pull request Nov 9, 2022

Update jol-core to 0.13 (scala#102)

29f481d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the breakOut rewrite rule cross compatible (fix #80) (fix #93) #102

Make the breakOut rewrite rule cross compatible (fix #80) (fix #93) #102

MasseGuillaume commented Jul 18, 2018

MasseGuillaume Jul 18, 2018

julienrf Jul 18, 2018

MasseGuillaume Jul 18, 2018

julienrf Jul 18, 2018

MasseGuillaume Jul 19, 2018

MasseGuillaume Jul 18, 2018

MasseGuillaume commented Jul 18, 2018

sjrd commented Jul 18, 2018

MasseGuillaume Jul 19, 2018

MasseGuillaume Jul 19, 2018

MasseGuillaume commented Jul 19, 2018

MasseGuillaume commented Jul 20, 2018

julienrf Jul 20, 2018

MasseGuillaume Jul 20, 2018

julienrf Jul 20, 2018

MasseGuillaume Jul 21, 2018

julienrf Jul 21, 2018

MasseGuillaume Jul 21, 2018

julienrf Jul 21, 2018

MasseGuillaume Jul 21, 2018

MasseGuillaume Jul 21, 2018 •

edited

Loading

julienrf Jul 21, 2018

julienrf Jul 20, 2018

MasseGuillaume Jul 20, 2018

julienrf Jul 20, 2018

MasseGuillaume Jul 20, 2018

julienrf Jul 20, 2018

julienrf Jul 20, 2018

MasseGuillaume Jul 20, 2018

julienrf Jul 20, 2018

MasseGuillaume Jul 20, 2018

MasseGuillaume commented Jul 23, 2018

julienrf left a comment

julienrf Jul 23, 2018

Make the breakOut rewrite rule cross compatible (fix #80) (fix #93) #102

Make the breakOut rewrite rule cross compatible (fix #80) (fix #93) #102

Conversation

MasseGuillaume commented Jul 18, 2018

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

MasseGuillaume commented Jul 18, 2018

sjrd commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MasseGuillaume commented Jul 19, 2018

MasseGuillaume commented Jul 20, 2018

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

MasseGuillaume Jul 21, 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

MasseGuillaume commented Jul 23, 2018

julienrf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MasseGuillaume Jul 21, 2018 •

edited

Loading