Escape hatch on Patch (fix #368) #503

MasseGuillaume · 2017-12-13T15:19:45Z

This is a really raw first pass.

MasseGuillaume · 2017-12-13T15:21:30Z

scalafix-core/shared/src/main/scala/scalafix/patch/Patch.scala

 }

 //////////////////////////////
 // Low-level patches
 //////////////////////////////
 trait LowLevelPatch
+


this fucks up sublime syntax highlighting.

awww, maybe get a better editor 😜

MasseGuillaume · 2017-12-13T17:14:21Z

scalafix-tests/output/src/main/scala/test/escapeHatch/EscapeHatchPatches.scala

+
+import scala.collection.immutable // scalafix:ok RemoveUnusedImports
+
+import scala.collection.mutable.{ // scalafix:ok RemoveUnusedImports


@olafurpg we expected this to fail. It looks like it works.

That's likely because removing Map or Set independently removes the { which is escaped. If the comment only applies to import scala I suspect it won't work

This is good then, it makes it really difficult to fall on the non-supported case

olafurpg

Implementation looks great, only left a few minor comments otherwise I think this is good to merge 💯

olafurpg · 2017-12-14T12:05:05Z

scalafix-tests/output/src/main/scala/test/escapeHatch/EscapeHatchPatches.scala

+
+import scala.collection.immutable // scalafix:ok RemoveUnusedImports
+
+import scala.collection.mutable.{ // scalafix:ok RemoveUnusedImports


That's likely because removing Map or Set independently removes the { which is escaped. If the comment only applies to import scala I suspect it won't work

olafurpg · 2017-12-14T12:05:34Z

scalafix-core/shared/src/main/scala/scalafix/internal/rule/RemoveUnusedImports.scala

@@ -28,6 +28,6 @@ case class RemoveUnusedImports(index: SemanticdbIndex)
  }
  override def fix(ctx: RuleCtx): Patch =
    ctx.tree.collect {
-      case i: Importee if isUnused(i) => ctx.removeImportee(i)
+      case i: Importee if isUnused(i) => ctx.removeImportee(i).atomic


Are there any other scalafix rules that could benefit from .atomic?

Yep, this is what I am doing right now. I'm making sure all the Scalafix rules uses atomic.

olafurpg · 2017-12-14T12:06:57Z

scalafix-core/shared/src/main/scala/scalafix/patch/Patch.scala

@@ -48,12 +48,14 @@ sealed abstract class Patch {
  def ++(other: Iterable[Patch]): Patch = other.foldLeft(this)(_ + _)
  def isEmpty: Boolean = this == EmptyPatch
  def nonEmpty: Boolean = !isEmpty
+  def atomic: Patch = AtomicPatch(this)


Add docstring. The other methods have no docstring since I think they're quite self explanatory but atomic is not as obvious

I will write user documentation with examples and limitation.

We should still have a docstring despite docs on the website, a single sentence like

Skip this entire patch if a part of it is disabled with // scalafix:off

will do.

olafurpg

Looking forward to the docs!

olafurpg · 2017-12-14T16:45:18Z

scalafix-testkit/src/main/scala/scalafix/testkit/SemanticRuleSuite.scala

+      val result = compareContents(obtained, expected)
+      if (result.nonEmpty) {
+        println("###########> Diff       <###########")
+        println(error2message(obtained, expected))


Is the motivation for this change to copy-paste the terminal output? If so, we should include the filename since println is detached from the scalatest failure report.

no it's because it was failing too early with the unreported lint message. I want to see both the lint asserts and the diff for the fix.

MasseGuillaume · 2017-12-14T17:23:59Z

scalafix-core/shared/src/main/scala/scalafix/internal/rule/RemoveXmlLiterals.scala

@@ -123,7 +123,6 @@ case object RemoveXmlLiterals extends Rule("RemoveXmlLiterals") {
        patchXml(xml, tripleQuoted).atomic
    }.asPatch

-    if (patch.nonEmpty) (patch + importXmlQuote)
-    else patch
+    importXmlQuote.dependsOn(patch)


@olafurpg what do you think about this?

MasseGuillaume · 2017-12-14T17:42:13Z

I just added atomics for other rules like DottyVolatileLazyVal. However, I don't think we should allow escaping certain types of fix. For example, DottyVarArgPattern since it would result in a compilation error on the target side.

MasseGuillaume · 2017-12-14T17:53:01Z

Ready for round 2.

olafurpg

dependsOn is unrelated to atomic, can you please open an issue or separate pr?

MasseGuillaume · 2017-12-15T14:14:14Z

Ready for round 3.

olafurpg

Great to see .atomic work for the existing rules!

olafurpg · 2017-12-15T14:21:31Z

scalafix-core/shared/src/main/scala/scalafix/internal/rule/RemoveUnusedTerms.scala

@@ -40,6 +40,7 @@ case class RemoveUnusedTerms(index: SemanticdbIndex)
  override def fix(ctx: RuleCtx): Patch =
    ctx.tree.collect {
      case i: Defn if isUnused(i) =>
-        tokensToRemove(i).fold(Patch.empty)(ctx.removeTokens)


Could have added .atomic on the whole thing 😉

olafurpg · 2017-12-15T14:21:49Z

scalafix-core/shared/src/main/scala/scalafix/internal/rule/RemoveXmlLiterals.scala

    }.asPatch

+    // NB if all patches are not yet escaped here


olafurpg · 2017-12-15T14:24:08Z

scalafix-core/shared/src/main/scala/scalafix/patch/Patch.scala

@@ -48,12 +48,14 @@ sealed abstract class Patch {
  def ++(other: Iterable[Patch]): Patch = other.foldLeft(this)(_ + _)
  def isEmpty: Boolean = this == EmptyPatch
  def nonEmpty: Boolean = !isEmpty
+  def atomic: Patch = AtomicPatch(this)


We should still have a docstring despite docs on the website, a single sentence like

Skip this entire patch if a part of it is disabled with // scalafix:off

will do.

olafurpg · 2017-12-15T14:27:46Z

website/src/main/tut/docs/rule-authors/glossary.md

@@ -42,6 +42,9 @@ utility methods to accomplish common tasks.
 If you experience that it's difficult to implement something that
 seems simple then don't hesitate to ask on {% gitter %}.

+It's possible to escape parts of the Patch with `// scalafix:ok`. If you want to treat


olafurpg · 2017-12-15T14:28:43Z

website/src/main/tut/docs/rule-authors/glossary.md

@@ -42,6 +42,9 @@ utility methods to accomplish common tasks.
 If you experience that it's difficult to implement something that
 seems simple then don't hesitate to ask on {% gitter %}.

+It's possible to escape parts of the Patch with `// scalafix:ok`. If you want to treat
+the Patch as a transaction use `.atomic`.


MasseGuillaume · 2017-12-15T14:35:27Z

Ready for round 4.

olafurpg

LGTM 👍 Awesome work @MasseGuillaume 💯

olafurpg · 2017-12-15T14:44:59Z

scalafix-core/shared/src/main/scala/scalafix/patch/Patch.scala

@@ -48,12 +48,16 @@ sealed abstract class Patch {
  def ++(other: Iterable[Patch]): Patch = other.foldLeft(this)(_ + _)
  def isEmpty: Boolean = this == EmptyPatch
  def nonEmpty: Boolean = !isEmpty
+  /** Skip this entire patch if a part of it is disabled with // scalafix:off


Nitpick, single line docstrings should be /** Blah blah */, I should make scalafmt enforce this 😅

gabro · 2017-12-15T14:49:38Z

This is so cool! 🎉 Thank you @MasseGuillaume, looking forward to use it!

MasseGuillaume · 2017-12-15T14:55:36Z

@gabro hehe, don't forget to set .atomic properly.

I found one counter example in RemoveXmlLiterals: ``` // scalafix-tests/input/src/main/scala/test/RemoveXmlLiteralsAtomic.scala /* rules = RemoveXmlLiterals */ package test class RemoveXmlLiteralsAtomic { <div></div>// scalafix:ok RemoveXmlLiterals } // scalafix-tests/output/src/main/scala/test/RemoveXmlLiteralsAtomic.scala package test import scala.xml.quote._ // <<< fail class RemoveXmlLiteralsAtomic { <div></div>// scalafix:ok RemoveXmlLiterals } ``` This happens because of: ```` if (patch.nonEmpty) patch + importXmlQuote else patch ``` At this point, patch is not empty but it will be removed subsequently by the escape hatch. The solution would be to add a `dependsOn` on Patch ``` case class DependsOn(parent: Patch, child: Patch) extends Patch class Patch { // ... def dependsOn(other: Patch): Patch = DependsOn(this, other) } object Patch { private def foreach // ... case DependsOn(parent, child) => loop(parent) loop(child) // ... } } class EscapeHatch { def filter // ... case DependsOn(parent, child) => val childs = loop(name, child) if (childs == EmptyPatch) EmptyPatch else loop(name, Concat(parent, childs)) } ```

``` > scalafix --diff-base <tab> HEAD ag/gh-pages ag/issue-260 ag/issue-319 ag/master ... v0.5.5 v0.5.6 |00| 51abefa -- wip improve sbt completion (13 hours ago) |01| 3f63b5b -- Merge pull request scalacenter#474 from marcelocenerine/trim_includeNewLine (13 hours ago) |02| 47b94c0 -- Add leadingSpaces/trailingSpaces methods to TokenList (13 hours ago) |03| 8bd026d -- Merge pull request scalacenter#503 from MasseGuillaume/feature/368-escape-patches (13 hours ago) |04| 20e445f -- Escape hatch on Patch (fix scalacenter#368) (13 hours ago) |05| a2c5d70 -- Merge pull request scalacenter#500 from MasseGuillaume/feature/495-custom-id (13 hours ago) |06| 59efe7d -- Add CustomMessage to the public api (13 hours ago) |07| 9ae6071 -- Add id for CustomMessage (fix scalacenter#495) (13 hours ago) |08| e4a5c35 -- Merge pull request scalacenter#494 from MasseGuillaume/disable-regex (13 hours ago) |09| a422860 -- Merge pull request scalacenter#497 from olafurpg/disable-signatures (13 hours ago) |10| 7930947 -- DisableSyntax add regex (13 hours ago) |11| 5dbdd6b -- IntervalSet test for empty and add toString (13 hours ago) |12| b022fbd -- DisableSyntax don't repeat DisableSyntax.keyword in message (13 hours ago) |13| a992b02 -- Assert instead of scalafix:ok (13 hours ago) |14| 7896ccd -- Refactor Disable to use views. (13 hours ago) |15| 58acdbe -- Fix scalacenter#493, handle synthetics and symbol signatures in Disable. (13 hours ago) |16| b48d7f0 -- Merge pull request scalacenter#490 from olafurpg/unmanagedSources (13 hours ago) |17| e9b2b0a -- s/canFormat/canFix/ (13 hours ago) |18| 26be6fa -- Use unmanagedSources instead of unmanagedSourceDirectories. (13 hours ago) |19| 4d46001 -- Merge pull request scalacenter#488 from olafurpg/master (13 hours ago) ```

MasseGuillaume changed the title ~~Escape hatches on fix (fix #368)~~ Escape hatche on Patch (fix #368) Dec 13, 2017

MasseGuillaume requested a review from olafurpg December 13, 2017 15:19

MasseGuillaume commented Dec 13, 2017

View reviewed changes

This was referenced Dec 13, 2017

Rule.apply: List[Patch] #368

Closed

Benchmarks #504

Closed

MasseGuillaume changed the title ~~Escape hatche on Patch (fix #368)~~ Escape hatch on Patch (fix #368) Dec 13, 2017

MasseGuillaume force-pushed the feature/368-escape-patches branch 2 times, most recently from 32b0e5c to ad04c99 Compare December 13, 2017 16:51

MasseGuillaume commented Dec 13, 2017

View reviewed changes

olafurpg suggested changes Dec 14, 2017

View reviewed changes

MasseGuillaume force-pushed the feature/368-escape-patches branch 2 times, most recently from ded5ddf to 4feb337 Compare December 14, 2017 16:25

olafurpg reviewed Dec 14, 2017

View reviewed changes

MasseGuillaume commented Dec 14, 2017

View reviewed changes

MasseGuillaume added the ready label Dec 14, 2017

olafurpg suggested changes Dec 14, 2017

View reviewed changes

olafurpg mentioned this pull request Dec 15, 2017

Support --diff for .atomic rewrites #508

Closed

MasseGuillaume force-pushed the feature/368-escape-patches branch from 9d8f66e to 30037d1 Compare December 15, 2017 14:13

olafurpg suggested changes Dec 15, 2017

View reviewed changes

MasseGuillaume force-pushed the feature/368-escape-patches branch 2 times, most recently from a5e9596 to 4726fcd Compare December 15, 2017 14:35

olafurpg approved these changes Dec 15, 2017

View reviewed changes

MasseGuillaume force-pushed the feature/368-escape-patches branch from 4726fcd to 64b94b3 Compare December 15, 2017 14:54

MasseGuillaume force-pushed the feature/368-escape-patches branch from 64b94b3 to 20e445f Compare December 15, 2017 15:05

MasseGuillaume merged commit 8bd026d into scalacenter:master Dec 15, 2017

MasseGuillaume deleted the feature/368-escape-patches branch December 15, 2017 15:44

MasseGuillaume added this to the v0.5.8 milestone Jan 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Escape hatch on Patch (fix #368) #503

Escape hatch on Patch (fix #368) #503

MasseGuillaume commented Dec 13, 2017 •

edited

Loading

MasseGuillaume Dec 13, 2017

olafurpg Dec 13, 2017

MasseGuillaume Dec 13, 2017

olafurpg Dec 14, 2017

MasseGuillaume Dec 14, 2017

olafurpg left a comment

olafurpg Dec 14, 2017

olafurpg Dec 14, 2017

MasseGuillaume Dec 14, 2017

olafurpg Dec 14, 2017

MasseGuillaume Dec 14, 2017

olafurpg Dec 15, 2017

olafurpg left a comment

olafurpg Dec 14, 2017

MasseGuillaume Dec 14, 2017

MasseGuillaume Dec 14, 2017

MasseGuillaume commented Dec 14, 2017

MasseGuillaume commented Dec 14, 2017

olafurpg left a comment

MasseGuillaume commented Dec 15, 2017

olafurpg left a comment

olafurpg Dec 15, 2017

olafurpg Dec 15, 2017

olafurpg Dec 15, 2017

olafurpg Dec 15, 2017

olafurpg Dec 15, 2017

MasseGuillaume commented Dec 15, 2017

olafurpg left a comment

olafurpg Dec 15, 2017

gabro commented Dec 15, 2017

MasseGuillaume commented Dec 15, 2017


		import scala.collection.immutable // scalafix:ok RemoveUnusedImports

		import scala.collection.mutable.{ // scalafix:ok RemoveUnusedImports

Escape hatch on Patch (fix #368) #503

Escape hatch on Patch (fix #368) #503

Conversation

MasseGuillaume commented Dec 13, 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

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

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

MasseGuillaume commented Dec 14, 2017

MasseGuillaume commented Dec 14, 2017

olafurpg left a comment

Choose a reason for hiding this comment

MasseGuillaume commented Dec 15, 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

MasseGuillaume commented Dec 15, 2017

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabro commented Dec 15, 2017

MasseGuillaume commented Dec 15, 2017

MasseGuillaume commented Dec 13, 2017 •

edited

Loading