New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add `formatCount = 1/2/fixpoint` option for guaranteed idempotent formatting #1055

Closed
djspiewak opened this Issue Sep 27, 2017 · 6 comments

Comments

2 participants
@djspiewak
Contributor

djspiewak commented Sep 27, 2017

I wish I had a better title for this…

What is happening here is I run scalafmt to format the source files in the quasar project, and it returns successfully (with a few files blowing search space, but not this one). I run it a second time and no files are reformatted. My next step was to commit the changes and check Travis, where I am testing formatting by running scalafmt again (on Travis) and then checking git status -s to see if anything is changed. The following file is listed as changed, despite not formatting any further on my machine.

I have similar issues if I use the scalafmt::test task in neo-scalafmt (which, as I understand it, does the same thing). Basically, I double-format the sources, then run scalafmt::test, and the test fails despite scalafmt not actually reformatting anything when I run it directly.

I'm including the configuration as well as the source file post-formatting (i.e. the state which scalafmt::test and scalafmt on Travis believes needs to be reformatted, but scalafmt on my laptop will not actually change).

The only thing that even seems slightly suspicious to me is the )( line. Dangling parentheses are disabled in the configuration. Obviously this configuration doesn't outright-prevent scalafmt from producing files with dangling parentheses, but I wonder if it's enough to make the results less deterministic. Not sure.

Exemplar (after formatting):

/*
 * Copyright 2014–2017 SlamData Inc.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

package quasar.main

import quasar.fp._
import quasar.fp.free._
import quasar.fs._
import quasar.fs.mount._, BackendDef.DefinitionResult
import quasar.metastore._

import doobie.imports._
import eu.timepit.refined.auto._
import scalaz.{Failure => _, Lens => _, _}
import scalaz.concurrent.Task

object metastore {

  def jdbcMounter[S[_]](
      hfsRef: TaskRef[BackendEffect ~> HierarchicalFsEffM],
      mntdRef: TaskRef[Mounts[DefinitionResult[PhysFsEffM]]]
  )(
      implicit
      S0: ConnectionIO :<: S,
      S1: PhysErr :<: S,
      S2: FsAsk :<: S): Mounting ~> Free[S, ?] = {
    type M[A] = Free[MountEff, A]
    type G[A] = Coproduct[ConnectionIO, M, A]
    type T[A] = Coproduct[Task, S, A]

    val t: T ~> S =
      S0.compose(taskToConnectionIO) :+: reflNT[S]

    val g: G ~> Free[S, ?] =
      injectFT[ConnectionIO, S] :+:
        foldMapNT(mapSNT(t) compose MountEff.interpreter[T](hfsRef, mntdRef))

    def mounter(handler: MountRequestHandler[PhysFsEffM, HierarchicalFsEff]) = {
      MetaStoreMounter[M, G](handler.mount[MountEff](_), handler.unmount[MountEff](_))
    }

    λ[Mounting ~> Free[S, ?]] { mounting =>
      mountHandler[S].flatMap(handler => (foldMapNT(g) compose mounter(handler))(mounting))
    }
  }
}

Configuration:

# this roughly corresponds to half the screen of a MacBook Pro at default resolution with editor chrome
maxColumn = 96

includeCurlyBraceInSelectChains = false

optIn.breakChainOnFirstMethodDot = false

binPack.literalArgumentLists = true

continuationIndent.callSite = 2

newlines {
  afterImplicitKWInVerticalMultiline = true
  beforeImplicitKWInVerticalMultiline = true
  sometimesBeforeColonInMethodReturnType = true
}

align = none

docstrings = JavaDoc

project.git = false

rewrite {
  rules = [PreferCurlyFors, RedundantBraces, RedundantParens, SortImports]
  redundantBraces.maxLines = 1
}

rewriteTokens {
  "⇒": "=>"
  "←": "<-"
}

I'm using neo-scalafmt, if it matters, but I believe it just delegates to the CLI tool. Scalafmt version 1.2.0.

If you're interested in the full state of this experiment, you can look at this branch

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Sep 29, 2017

Member

Thank you for reporting! It's been awhile since I saw an idempotency report.

I'm unable to reproduce the non-idempotency with your example in the description. However, I was able to reproduce non-idempotency on the second run of scalafmt on the entire quasar codebase

diff --git a/core/src/main/scala/quasar/fs/mount/FileSystemMountHandler.scala b/core/src/main/scala/quasar/fs/mount/FileSystemMountHandler.scala
index b8aac6c..5a58e39 100644
--- a/core/src/main/scala/quasar/fs/mount/FileSystemMountHandler.scala
+++ b/core/src/main/scala/quasar/fs/mount/FileSystemMountHandler.scala
@@ -83,10 +83,8 @@ final class FileSystemMountHandler[F[_]](fsDef: BackendDef[F]) {
   ////
 
   private def cleanup[S[_]](mnts: Mounts[DefinitionResult[F]], loc: ADir)(
-      implicit S: F :<: S): Free[S, Unit] = {
-
+      implicit S: F :<: S): Free[S, Unit] =
     mnts.lookup(loc).map(_.close).fold(().point[Free[S, ?]])(free.lift(_).into[S])
-  }
 }
 
 object FileSystemMountHandler {
diff --git a/interface/src/main/scala/quasar/main/metastore.scala b/interface/src/main/scala/quasar/main/metastore.scala
index 25cb8d2..760d47f 100644
--- a/interface/src/main/scala/quasar/main/metastore.scala
+++ b/interface/src/main/scala/quasar/main/metastore.scala
@@ -48,9 +48,8 @@ object metastore {
       injectFT[ConnectionIO, S] :+:
         foldMapNT(mapSNT(t) compose MountEff.interpreter[T](hfsRef, mntdRef))
 
-    def mounter(handler: MountRequestHandler[PhysFsEffM, HierarchicalFsEff]) = {
+    def mounter(handler: MountRequestHandler[PhysFsEffM, HierarchicalFsEff]) =
       MetaStoreMounter[M, G](handler.mount[MountEff](_), handler.unmount[MountEff](_))
-    }
 
     λ[Mounting ~> Free[S, ?]] { mounting =>
       mountHandler[S].flatMap(handler => (foldMapNT(g) compose mounter(handler))(mounting))
diff --git a/niflheim/src/test/scala/quasar/niflheim/CookedBlockSpecs.scala b/niflheim/src/test/scala/quasar/niflheim/CookedBlockSpecs.scala
index 5aa8346..27573d5 100644
--- a/niflheim/src/test/scala/quasar/niflheim/CookedBlockSpecs.scala
+++ b/niflheim/src/test/scala/quasar/niflheim/CookedBlockSpecs.scala
@@ -49,10 +49,12 @@ trait CookedBlockFormatSpecs extends Specification with ScalaCheck with SegmentF
     }
 
     "round trip simple segments" in {
-      surviveRoundTrip(format)(CookedBlockMetadata(
-        999L,
-        1,
-        Array(SegmentId(1234L, CPath("a.b.c"), CLong) -> new File("/hello/there/abc.cooked"))))
+      surviveRoundTrip(format)(
+        CookedBlockMetadata(
+          999L,
+          1,
+          Array(
+            SegmentId(1234L, CPath("a.b.c"), CLong) -> new File("/hello/there/abc.cooked"))))
     }
 
     // this test seems to run forever?
diff --git a/rdbms/src/main/scala/quasar/physical/rdbms/fs/RdbmsManageFile.scala b/rdbms/src/main/scala/quasar/physical/rdbms/fs/RdbmsManageFile.scala
index 3f59683..32b7665 100644
--- a/rdbms/src/main/scala/quasar/physical/rdbms/fs/RdbmsManageFile.scala
+++ b/rdbms/src/main/scala/quasar/physical/rdbms/fs/RdbmsManageFile.scala
@@ -57,9 +57,8 @@ trait RdbmsManageFile extends RdbmsDescribeTable with RdbmsCreate with RdbmsMove
 
   override def ManageFileModule = new ManageFileModule {
 
-    def dropSchemaWithChildren(parent: CustomSchema): ConnectionIO[Unit] = {
+    def dropSchemaWithChildren(parent: CustomSchema): ConnectionIO[Unit] =
       findChildSchemas(parent).flatMap(cs => (cs :+ parent).foldMap(dropSchema))
-    }
 
     def tempSchema: M[CustomSchema] = {
       MonotonicSeq

Two of those cases seem to be caused by the fact that scalafmt made those braces redundant in run 1 making them picked up by RedundantBraces in run 2. The surviveRoundTrip case is worse IMO, I've seen similar cases where curried functions format differently in run 2 when run 1 produced a significant diff. Run 3 does not introduce a diff.

Quoting the docs on non-idempotency http://scalameta.org/scalafmt/#Non-idempotent

Pro tip. As awkward as I feel recommending this, you may want to run scalafmt twice on your codebase for the first time you reformat it.

Sadly I don't have a better solution. If I were to implement scalafmt today, I would use a totally different design (see #917) that I believe would make it easier to avoid these kinds of problems.

🙈
screen shot 2017-09-29 at 11 18 12

Member

olafurpg commented Sep 29, 2017

Thank you for reporting! It's been awhile since I saw an idempotency report.

I'm unable to reproduce the non-idempotency with your example in the description. However, I was able to reproduce non-idempotency on the second run of scalafmt on the entire quasar codebase

diff --git a/core/src/main/scala/quasar/fs/mount/FileSystemMountHandler.scala b/core/src/main/scala/quasar/fs/mount/FileSystemMountHandler.scala
index b8aac6c..5a58e39 100644
--- a/core/src/main/scala/quasar/fs/mount/FileSystemMountHandler.scala
+++ b/core/src/main/scala/quasar/fs/mount/FileSystemMountHandler.scala
@@ -83,10 +83,8 @@ final class FileSystemMountHandler[F[_]](fsDef: BackendDef[F]) {
   ////
 
   private def cleanup[S[_]](mnts: Mounts[DefinitionResult[F]], loc: ADir)(
-      implicit S: F :<: S): Free[S, Unit] = {
-
+      implicit S: F :<: S): Free[S, Unit] =
     mnts.lookup(loc).map(_.close).fold(().point[Free[S, ?]])(free.lift(_).into[S])
-  }
 }
 
 object FileSystemMountHandler {
diff --git a/interface/src/main/scala/quasar/main/metastore.scala b/interface/src/main/scala/quasar/main/metastore.scala
index 25cb8d2..760d47f 100644
--- a/interface/src/main/scala/quasar/main/metastore.scala
+++ b/interface/src/main/scala/quasar/main/metastore.scala
@@ -48,9 +48,8 @@ object metastore {
       injectFT[ConnectionIO, S] :+:
         foldMapNT(mapSNT(t) compose MountEff.interpreter[T](hfsRef, mntdRef))
 
-    def mounter(handler: MountRequestHandler[PhysFsEffM, HierarchicalFsEff]) = {
+    def mounter(handler: MountRequestHandler[PhysFsEffM, HierarchicalFsEff]) =
       MetaStoreMounter[M, G](handler.mount[MountEff](_), handler.unmount[MountEff](_))
-    }
 
     λ[Mounting ~> Free[S, ?]] { mounting =>
       mountHandler[S].flatMap(handler => (foldMapNT(g) compose mounter(handler))(mounting))
diff --git a/niflheim/src/test/scala/quasar/niflheim/CookedBlockSpecs.scala b/niflheim/src/test/scala/quasar/niflheim/CookedBlockSpecs.scala
index 5aa8346..27573d5 100644
--- a/niflheim/src/test/scala/quasar/niflheim/CookedBlockSpecs.scala
+++ b/niflheim/src/test/scala/quasar/niflheim/CookedBlockSpecs.scala
@@ -49,10 +49,12 @@ trait CookedBlockFormatSpecs extends Specification with ScalaCheck with SegmentF
     }
 
     "round trip simple segments" in {
-      surviveRoundTrip(format)(CookedBlockMetadata(
-        999L,
-        1,
-        Array(SegmentId(1234L, CPath("a.b.c"), CLong) -> new File("/hello/there/abc.cooked"))))
+      surviveRoundTrip(format)(
+        CookedBlockMetadata(
+          999L,
+          1,
+          Array(
+            SegmentId(1234L, CPath("a.b.c"), CLong) -> new File("/hello/there/abc.cooked"))))
     }
 
     // this test seems to run forever?
diff --git a/rdbms/src/main/scala/quasar/physical/rdbms/fs/RdbmsManageFile.scala b/rdbms/src/main/scala/quasar/physical/rdbms/fs/RdbmsManageFile.scala
index 3f59683..32b7665 100644
--- a/rdbms/src/main/scala/quasar/physical/rdbms/fs/RdbmsManageFile.scala
+++ b/rdbms/src/main/scala/quasar/physical/rdbms/fs/RdbmsManageFile.scala
@@ -57,9 +57,8 @@ trait RdbmsManageFile extends RdbmsDescribeTable with RdbmsCreate with RdbmsMove
 
   override def ManageFileModule = new ManageFileModule {
 
-    def dropSchemaWithChildren(parent: CustomSchema): ConnectionIO[Unit] = {
+    def dropSchemaWithChildren(parent: CustomSchema): ConnectionIO[Unit] =
       findChildSchemas(parent).flatMap(cs => (cs :+ parent).foldMap(dropSchema))
-    }
 
     def tempSchema: M[CustomSchema] = {
       MonotonicSeq

Two of those cases seem to be caused by the fact that scalafmt made those braces redundant in run 1 making them picked up by RedundantBraces in run 2. The surviveRoundTrip case is worse IMO, I've seen similar cases where curried functions format differently in run 2 when run 1 produced a significant diff. Run 3 does not introduce a diff.

Quoting the docs on non-idempotency http://scalameta.org/scalafmt/#Non-idempotent

Pro tip. As awkward as I feel recommending this, you may want to run scalafmt twice on your codebase for the first time you reformat it.

Sadly I don't have a better solution. If I were to implement scalafmt today, I would use a totally different design (see #917) that I believe would make it easier to avoid these kinds of problems.

🙈
screen shot 2017-09-29 at 11 18 12

@djspiewak

This comment has been minimized.

Show comment
Hide comment
@djspiewak

djspiewak Sep 29, 2017

Contributor

Even running scalafmt twice on the codebase, I haven't been able to avoid the issue. I could try running it three times though, since that's when you were able to reproduce it. :-P Maybe a better thing to do would be to remove the RedundantBraces rewrite, since that seems to be the underlying issue.

🙈

Hey, don't hate! 😜 At least we're fixing it.

Contributor

djspiewak commented Sep 29, 2017

Even running scalafmt twice on the codebase, I haven't been able to avoid the issue. I could try running it three times though, since that's when you were able to reproduce it. :-P Maybe a better thing to do would be to remove the RedundantBraces rewrite, since that seems to be the underlying issue.

🙈

Hey, don't hate! 😜 At least we're fixing it.

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Sep 29, 2017

Member

I have two actionable suggestions

  • document that RedundantBraces can cause non-idempotent formatting, #1057
  • provide some formatCount = 1/2/fixpoint (name up for debate) option to tell scalafmt to run twice or as many times as necessary to reach a fixpoint (with upper bound at count = 5 to cyclic output). I'm ashamed to provide such an option, scalafmt is already terribly slow, but it's at least a workaround that we can add now.

Hey, don't hate! 😜 At least we're fixing it.

I always become stressed when I see such massive diffs from scalafmt 😄

Member

olafurpg commented Sep 29, 2017

I have two actionable suggestions

  • document that RedundantBraces can cause non-idempotent formatting, #1057
  • provide some formatCount = 1/2/fixpoint (name up for debate) option to tell scalafmt to run twice or as many times as necessary to reach a fixpoint (with upper bound at count = 5 to cyclic output). I'm ashamed to provide such an option, scalafmt is already terribly slow, but it's at least a workaround that we can add now.

Hey, don't hate! 😜 At least we're fixing it.

I always become stressed when I see such massive diffs from scalafmt 😄

@djspiewak

This comment has been minimized.

Show comment
Hide comment
@djspiewak

djspiewak Sep 29, 2017

Contributor

document that RedundantBraces can cause non-idempotent formatting

👍

provide some formatCount = 1/2/fixpoint (name up for debate) option to tell scalafmt to run twice or as many times as necessary to reach a fixpoint

This is a terrifying option, but I think it's the right thing to do for now. Regarding performance, scalafmt is already slow enough that running it on compile (and often even on save) is prohibitive, so running it multiple times isn't really going to change that workflow: we've already crossed that threshold. Obviously, if the algorithm is rewritten to be faster, that whole calculus changes, since format-on-save/compile becomes viable again.

I always become stressed when I see such massive diffs from scalafmt

I think it's mostly the fault of our own radically inconsistent formatting.

Contributor

djspiewak commented Sep 29, 2017

document that RedundantBraces can cause non-idempotent formatting

👍

provide some formatCount = 1/2/fixpoint (name up for debate) option to tell scalafmt to run twice or as many times as necessary to reach a fixpoint

This is a terrifying option, but I think it's the right thing to do for now. Regarding performance, scalafmt is already slow enough that running it on compile (and often even on save) is prohibitive, so running it multiple times isn't really going to change that workflow: we've already crossed that threshold. Obviously, if the algorithm is rewritten to be faster, that whole calculus changes, since format-on-save/compile becomes viable again.

I always become stressed when I see such massive diffs from scalafmt

I think it's mostly the fault of our own radically inconsistent formatting.

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Oct 1, 2017

Member

Cool, I'm happy to merge this option if someone's interested in sending a PR. I have personally never hit on non-idempotency issues with scalafmt and I use it for all of my projects. The limited time I have for scalafmt these days is mostly spent reviewing PRs and work towards #917.

Member

olafurpg commented Oct 1, 2017

Cool, I'm happy to merge this option if someone's interested in sending a PR. I have personally never hit on non-idempotency issues with scalafmt and I use it for all of my projects. The limited time I have for scalafmt these days is mostly spent reviewing PRs and work towards #917.

@olafurpg olafurpg changed the title from Hard-to-reproduce anti-fixpoint non-idempotency to Add `formatCount = 1/2/fixpoint` option for guaranteed idempotent formatting Oct 1, 2017

@olafurpg olafurpg added the revisit label May 13, 2018

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg May 13, 2018

Member

Closing due to inactivity, please reopen if you'd like to take a stab at this.

Member

olafurpg commented May 13, 2018

Closing due to inactivity, please reopen if you'd like to take a stab at this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment