Scalafix rewrite for lazyZip #291

marcelocenerine · 2017-11-07T01:08:27Z

Enhancing the scalafix rule to rewrite (xs, ys).zipped to xs.lazyZip(ys) (likewise for triples).

There is one caveat here: LazyZipN.filter is not equivalent to TupleNZipped.filter. This will likely require manual intervention to be fully fixed. Choosing not to rewrite (xs, ys).zipped.filter(p) would leave it in a broken state anyway because a TupleNZipped cannot be created with iterables from strawman.

/cc @olafurpg

marcelocenerine · 2017-11-07T01:12:44Z

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

+        }.asPatch
+        removeTokensPatch + addTokensPatch
+    }.asPatch
+


I had to do this at the token level as suggested by @olafurpg due to a limitation in scalafix that is explained here: https://gitter.im/scalacenter/scalafix?at=59ff60b35a1758ed0f90981b

marcelocenerine · 2017-11-07T01:18:36Z

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

+    xs.lazyZip(ys).lazyZip(zs)
+    coll(1).lazyZip(coll(2)).lazyZip(coll(3))
+    List(1, 2, 3).lazyZip(Set(1, 2, 3)).lazyZip(LazyList.from(1))
+  }


Apparently there isn't a consensus on the syntax for lazyZip in the discussions in #223. I chose xs.lazyZip(ys).xxx(f) over (xs lazyZip ys).xxx(f). Please let me know if you prefer the second option ;)

I would go for a.lazyZip(b), it's harder to get the parentheses right for infix operators. For example, (a, (b, c)).zipped becomes a lazyZip ((b, c)) under -Yno-adapted-args. You might also have to add wrapping parentheses depending on the parent tree node (a lazyZip b).c

olafurpg

Nice work!

olafurpg · 2017-11-07T08:12:29Z

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

+    xs.lazyZip(ys).lazyZip(zs)
+    coll(1).lazyZip(coll(2)).lazyZip(coll(3))
+    List(1, 2, 3).lazyZip(Set(1, 2, 3)).lazyZip(LazyList.from(1))
+  }


I would go for a.lazyZip(b), it's harder to get the parentheses right for infix operators. For example, (a, (b, c)).zipped becomes a lazyZip ((b, c)) under -Yno-adapted-args. You might also have to add wrapping parentheses depending on the parent tree node (a lazyZip b).c

olafurpg · 2017-11-07T08:14:22Z

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

+  def replaceTupleZipped(ctx: RuleCtx) =
+    ctx.tree.collect {
+      case tupleZipped(t @ Term.Select(Term.Tuple(args), _)) =>
+        val argTokens = args.flatMap(_.tokens).toSet


tupleElementTokens is more descriptive than argTokens

olafurpg · 2017-11-07T08:15:04Z

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

+    ctx.tree.collect {
+      case tupleZipped(t @ Term.Select(Term.Tuple(args), _)) =>
+        val argTokens = args.flatMap(_.tokens).toSet
+        val removeTokensPatch = t.tokens.filter(!argTokens.contains(_)).map(ctx.removeToken).asPatch


nonTupleElementTokens

t.tokens.filterNot(tupleElementTokens)

PS, you might be removing comments here, I would keep comments

The only tokens that need to be removed a leading (, trailing ), commas , and .zipped.

@olafurpg yeah, you are right. Everything that is not a tuple element gets removed. I'm changing the approach to remove only the necessary tokens.

Also, I think it would be nice to get rid of empty spaces between the comma and the first non-empty space token that follows it. Otherwise the output will be something like:
xs.lazyZip( ys)

olafurpg · 2017-11-07T08:20:24Z

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

+        val removeTokensPatch = t.tokens.filter(!argTokens.contains(_)).map(ctx.removeToken).asPatch
+        val addTokensPatch = args.tail.map { arg =>
+          val tokens = arg.tokens
+          ctx.addLeft(tokens.head, ".lazyZip(") + ctx.addRight(tokens.last, ")")


There are addRight/addLeft overloads for trees: ctx.addLeft(arg, ".lazyZip(") + ctx.addRight(arg, ")")

cool, I missed that

olafurpg · 2017-11-07T08:25:27Z

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

+object Collectionstrawman_v0_Tuple2Zipped {
+  def zipped(xs: List[Int], ys: List[Int]): Unit = {
+    (xs, ys).zipped
+    (coll(1), coll(2)).zipped


I recommend you add at least one test with comments inside of the tuple, the biggest reason scalafix exposes low-level token APIs is so that trivia like comments and formatting can be preserved.

marcelocenerine · 2017-11-09T01:21:51Z

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

+          case _ => Patch.empty
+        }
+
+        removeTokensPatch + replaceCommasPatch
    }.asPatch



Handling comments makes the rewrite a lot more complicated compared to the previous version 😅.
Note that in lines 153-155 I'm preserving the original closing ) in order to allow comments on the right side of the last tuple element to be enclosed by the correct parenthesis. Similar care needs to be taken in regards to comments on the right side of a middle triple element (that's the reason for the additional lines in line 174-175).

marcelocenerine · 2017-11-09T01:26:21Z

@olafurpg thank you for your awesome suggestions! I updated the PR addressing all the comments. Could you take a look at it again?

olafurpg

Manually handling comments is indeed tricky, and this PR does a great job at it LGTM 👍

Things may change soon with a new tree printer I'm working on that has the ability to attach comments to synthetic tree nodes https://github.com/olafurpg/scala-syntax/pull/10/files#diff-e7e24ee6fee3b6a67316ebd60f6d7fd8R7 that opens possibilities for syntax preserving tree transforms.

olafurpg · 2017-11-09T08:42:31Z

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

+              ctx.removeToken(zipped)
+          }).asPatch
+
+        def removeSurroundingWhiteSpaces(tk: Token) =


Utilities for trimming whitespace are severely missing in scalafix. I'd love to hear if you have idea how this can be generalized

it would definitely be handy to have such utility in the api. Maybe something like ctx.trimLeft(token) / ctx.trimRight(token) or ctx.removeTokens(tokenList.spacesBefore(token)) / ctx.removeTokens(tokenList.spacesAfter(token)). I can put more thoughts on this and create a PR in scalafix with a proposal.

olafurpg · 2017-11-09T08:43:11Z

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

+            zipped <- name.tokens.headOption
+            closeTuple <- ctx.tokenList.leading(zipped).find(_.is[Token.RightParen])
+            openTuple <- ctx.matchingParens.open(closeTuple.asInstanceOf[Token.RightParen])
+            maybeDot = ctx.tokenList.slice(closeTuple, zipped).find(_.is[Token.Dot])


Utilities to find dots is missing in scalafix, I've written something similar to this many times 😅

marcelocenerine · 2017-11-09T09:11:09Z

One thing I realized that this PR is missing is replacing the old types in case someone assigned the zipped result to a explicitly typed variable or parameter before invoking operations on it (not sure if this really happens in practice as the result would be rather verbose):
val zipped: Tuple3Zipped[A, Col1[A], B, Col2[B], C, Col3[C]] = (xs, ys, zs).zipped

Replacing the symbols wouldn't be enough I guess. The new types have fewer type parameters.
If you think this is a scenario worth covering, I can address it before the PR is merged.

julienrf · 2017-11-09T09:50:56Z

I don’t think lots of people have explicitly used the TupleNZipped type. In my opinion, this rewrite is great in its current state.

olafurpg · 2017-11-09T10:22:52Z

I agree with @julienrf I would not worry about it. If you want you can report a warning if Tuple3Zipped is inferred

marcelocenerine · 2017-11-09T22:25:21Z

ok, I'm going to keep the PR as is. @julienrf you can merge the PR if you are happy with it.
Thank you both for all the feedback!

julienrf · 2017-11-10T07:59:57Z

Thanks a lot @marcelocenerine !

marcelocenerine commented Nov 7, 2017

View reviewed changes

olafurpg reviewed Nov 7, 2017

View reviewed changes

Scalafix rewrite for lazyZip

91ff009

marcelocenerine force-pushed the scalafix_lazyZip branch from c9f04b0 to 91ff009 Compare November 9, 2017 01:09

marcelocenerine commented Nov 9, 2017

View reviewed changes

olafurpg approved these changes Nov 9, 2017

View reviewed changes

julienrf merged commit c5fe683 into scala:master Nov 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scalafix rewrite for lazyZip #291

Scalafix rewrite for lazyZip #291

marcelocenerine commented Nov 7, 2017 •

edited

Loading

marcelocenerine Nov 7, 2017 •

edited

Loading

marcelocenerine Nov 7, 2017 •

edited

Loading

olafurpg Nov 7, 2017

olafurpg left a comment

olafurpg Nov 7, 2017

olafurpg Nov 7, 2017

olafurpg Nov 7, 2017

olafurpg Nov 7, 2017

olafurpg Nov 7, 2017

marcelocenerine Nov 8, 2017 •

edited

Loading

olafurpg Nov 7, 2017

marcelocenerine Nov 8, 2017

olafurpg Nov 7, 2017

marcelocenerine Nov 9, 2017

marcelocenerine Nov 9, 2017 •

edited

Loading

marcelocenerine commented Nov 9, 2017 •

edited

Loading

olafurpg left a comment

olafurpg Nov 9, 2017

marcelocenerine Nov 9, 2017

olafurpg Nov 9, 2017

marcelocenerine commented Nov 9, 2017 •

edited

Loading

julienrf commented Nov 9, 2017

olafurpg commented Nov 9, 2017

marcelocenerine commented Nov 9, 2017 •

edited

Loading

julienrf commented Nov 10, 2017

Scalafix rewrite for lazyZip #291

Scalafix rewrite for lazyZip #291

Conversation

marcelocenerine commented Nov 7, 2017 • edited Loading

marcelocenerine Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

marcelocenerine Nov 7, 2017 • edited Loading

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

marcelocenerine Nov 8, 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

marcelocenerine Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

marcelocenerine commented Nov 9, 2017 • edited Loading

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

marcelocenerine commented Nov 9, 2017 • edited Loading

julienrf commented Nov 9, 2017

olafurpg commented Nov 9, 2017

marcelocenerine commented Nov 9, 2017 • edited Loading

julienrf commented Nov 10, 2017

marcelocenerine commented Nov 7, 2017 •

edited

Loading

marcelocenerine Nov 7, 2017 •

edited

Loading

marcelocenerine Nov 7, 2017 •

edited

Loading

marcelocenerine Nov 8, 2017 •

edited

Loading

marcelocenerine Nov 9, 2017 •

edited

Loading

marcelocenerine commented Nov 9, 2017 •

edited

Loading

marcelocenerine commented Nov 9, 2017 •

edited

Loading

marcelocenerine commented Nov 9, 2017 •

edited

Loading