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

Implement SortModifiers rewrite rule #1140

Merged
merged 10 commits into from Apr 23, 2018

Conversation

3 participants
@lorandszakacs
Contributor

lorandszakacs commented Apr 19, 2018

Closes #1138
Later edit:
I decided to add docs even before feedback.

TODO:

  • If the implementation/behavior/defaults are deemed OK, then I will add the appropriate documentation as well

This rewrite rule will sort the modifiers applied to the following language constructs:

  • Defn.Def
  • Defn.Val
  • Defn.Var
  • Defn.Type
  • Defn.Class
  • Defn.Object
  • Defn.Trait
  • Term.Param

Example config:

rewrite {
  rules = [SortModifiers]
  sortModifiers {
    # also the default value, the user has to specify all 8. The message if they fail
    # to do so is very instructive, and the docs will be clear, so hopefully it's user friendly
    order = ["implicit", "final", "sealed", "abstract", "override", "private", "protected", "lazy"]
  }
}

Later edit — example with config:

maxColumn = 200
rewrite {
  rules = [SortModifiers]
}

Before:

package test

sealed private[test] trait Test {
  protected def name: String
}

final private[test] object Tests {

  private final type T1 = Int
  final private type T2 = String

  final private implicit case object Test1 extends Test { def name: String = "Test1" }
  implicit private final case object Test2 extends Test { def name: String = "Test2" }
  final implicit private case object Test3 extends Test { def name: String = "Test3" }

  sealed abstract class TestSup(protected override val name: String) extends Test

  final class Test4(implicit final private val impl: String) extends Test {
    final protected implicit override lazy val name: String = impl
  }
}

After:

package test

sealed private[test] trait Test {
  protected def name: String
}

final private[test] object Tests {

  final private type T1 = Int
  final private type T2 = String

  implicit final private case object Test1 extends Test { def name: String = "Test1" }
  implicit final private case object Test2 extends Test { def name: String = "Test2" }
  implicit final private case object Test3 extends Test { def name: String = "Test3" }

  sealed abstract class TestSup(override protected val name: String) extends Test

  final class Test4(implicit final private val impl: String) extends Test {
    implicit final override protected lazy val name: String = impl
  }
}

N.B:
There is some weird behavior that shows up when formatting SortModifiers_Mod_With_No_Token.source. The tokens of a Mod are empty (which makes very little sense to me). This case is explicitly handled and no patches are applied to the code. And lo' and behold everything formats as expected anyway. Very, very weird.

Supporting changes:

  • added a method org.scalafmt.config.ReaderUtil.oneOfIgnoreBackticks
    to easily define a reader for the config that ignores backticks in
    case object names. So I factored out the implementation of oneOf
Implement SortModifiers rewrite rule
This rewrite rule will sort the modifiers applied to the following language constructs:
  case d: Defn.Def
  case v: Defn.Val
  case v: Defn.Var
  case t: Defn.Type
  case c: Defn.Class
  case o: Defn.Object
  case t: Defn.Trait
  case p: Term.Param

Example config:
rewrite {
  rules = [SortModifiers]
  sortModifiers {
    order = ["private", "protected" , "abstract", "final", "sealed", "implicit", "override", "lazy"]
  }
}

With default value for 'sortModifiers.order':
["implicit", "final", "sealed", "abstract", "override", "private", "protected", "lazy"]

N.B:
There is some weird behavior that shows up when formatting SortModifiers_Mod_With_No_Token.source.
The tokens of a Mod are empty (which makes very little sense to me). This case is explicitely handled
and no patches are applied to the code. And lo' and behold everything formats as expected anyway.
Very, very weird.

Supporting changes:
 - added a method org.scalafmt.config.ReaderUtil.oneOfIgnoreBackticks
   to easily define a reader for the config that ignores backticks in
   case object names. So I factored out the implementation of oneOf

@lorandszakacs lorandszakacs changed the title from WIP: Implement SortModifiers rewrite rule to Implement SortModifiers rewrite rule Apr 22, 2018

@olafurpg

Thanks for this contribution! Overall looks great, I left a few stylistic comments. Great job with the test suite 👍

* are considered Mods, instead of being similar to `Defn.Val`, or `Defn.Var`.
*/
val patchesOfPatches = code.collect {
case d: Defn.Def => patchMods(sortMods, d.mods)

This comment has been minimized.

@olafurpg

olafurpg Apr 22, 2018

Member

Missing Decl.{Def,Val,Var}, those are the abstract variants (declarations)

implicit val x: Int
def x: Int
var x: Int
sortedMods.zip(oldMods).flatMap {
case (next, old) =>
if (old.tokens.isEmpty) {
//Not sure why this happens, how can you have a mod with no tokens?

This comment has been minimized.

@olafurpg

olafurpg Apr 22, 2018

Member

Yes, there is a bug for example for def foo(implicit x: Int), the implicit has no tokens. The user can't do anything about it so it's fine to ignore it.

This comment has been minimized.

@olafurpg

olafurpg Apr 22, 2018

Member

Please remove the comment, this guard is totally fine.

def default: SortSettings =
SortSettings(defaultOrder)
sealed trait ModKey

This comment has been minimized.

@olafurpg

olafurpg Apr 22, 2018

Member

extends Product allows you to use .productPrefix from the docs instead of the getClass hack

private val isValOrVar: Mod => Boolean = m =>
m.is[Mod.ValParam] || m.is[Mod.VarParam]
private val isCase: Mod => Boolean = m => m.is[Mod.Case]

This comment has been minimized.

@olafurpg

olafurpg Apr 22, 2018

Member

Inline, it's clearer to read filterNot(_.is[Mod.Case])

This comment has been minimized.

@lorandszakacs

lorandszakacs Apr 22, 2018

Contributor

@olafurpg I did not inline because I didn't want to leave it to JIT to not do extra allocations for this function (if it can even do that).

This comment has been minimized.

@olafurpg

olafurpg Apr 22, 2018

Member

I see, I would be surprised if this was a performance issue. It is better to optimize for readability.

}
private val isValOrVar: Mod => Boolean = m =>
m.is[Mod.ValParam] || m.is[Mod.VarParam]

This comment has been minimized.

@olafurpg

olafurpg Apr 22, 2018

Member

Ditto, better to inline.

override def rewrite(code: Tree, ctx: RewriteCtx): Seq[Patch] = {
val order = ctx.style.rewrite.sortModifiers.order
val sortMods: Seq[Mod] => Seq[Mod] = { mods =>

This comment has been minimized.

@olafurpg

olafurpg Apr 22, 2018

Member

It's clearer to use methods

def sortMods(mods: Seq[Mod]): Seq[Mod] = mods.sortWith(orderModsBy(order))
private val isCase: Mod => Boolean = m => m.is[Mod.Case]
private def patchMods(
sortMods: Seq[Mod] => Seq[Mod],

This comment has been minimized.

@olafurpg

olafurpg Apr 22, 2018

Member

Why pass in the function as a parameter instead of sortedMods: Seq[Mod]? Alternatively, it will always be mods.sortWith(orderModsBy(order)) so that can be inlined at line 51 below

@lorandszakacs

This comment has been minimized.

Contributor

lorandszakacs commented Apr 22, 2018

@olafurpg. Thanks for the comments. I'll get on these asap.

@lorandszakacs

This comment has been minimized.

Contributor

lorandszakacs commented Apr 23, 2018

@olafurpg applied all suggestions, and added the missing cases 😃

@olafurpg

Thanks for the update! I added a few more comments while doing another round

options: sourcecode.Text[T]*): ConfDecoder[T] =
oneOfImpl(lowerCaseNoBackticks, options)
private val lowerCase: String => String = s => s.toLowerCase

This comment has been minimized.

@olafurpg

olafurpg Apr 23, 2018

Member

Why not def lowerCase(s: String): String = s.toLowercase?

There are many reasons to use methods instead of functions. Function declarations are harder to read, produce more cryptic stack traces, val in objects are evaluated during global initialization potentially causing NullPointerException, val in object creates a JVM field + method while methods creates only one JVM method.

@@ -21,4 +22,12 @@ case class RewriteSettings(
)
}
if (sortModifiers.order.distinct.length != 8)

This comment has been minimized.

@olafurpg

olafurpg Apr 23, 2018

Member

It's better to write a decoder by hand in the SortSettings companion, the values are encoded from Conf.Lst(values) where values should be only Conf.Str.

The Rewrite.validateRewrites(rules) validation above is a hack that should not be repeated, I'm cleaning up configuration in #1145.

This comment has been minimized.

@olafurpg

olafurpg Apr 23, 2018

Member

Actually, this is fine, I can take care of it in #1145 once this PR is merged :)

import metaconfig._
@DeriveConfDecoder

This comment has been minimized.

@olafurpg

olafurpg Apr 23, 2018

Member

Best to replace this with a hand-written decoder that validates that all modifiers are defined, and produces a helpful error message saying which modifier is missing in case something is wrong.

This comment has been minimized.

@olafurpg

olafurpg Apr 23, 2018

Member

Nevermind this comment.

@lorandszakacs

This comment has been minimized.

Contributor

lorandszakacs commented Apr 23, 2018

@olafurpg ok, changed that function vals to defs. Ignored the comments about the config 😃

@olafurpg

LGTM 👍 Thank you @lorandszakacs !

@olafurpg olafurpg merged commit 367170b into scalameta:master Apr 23, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@lorandszakacs lorandszakacs deleted the lorandszakacs:feature/add_rewrite_sorting_modifiers branch Apr 24, 2018

`lazy`,
)
val defaultOrder: Vector[ModKey] =

This comment has been minimized.

@kitbellew

kitbellew May 12, 2018

is there any reason why the default order doesn't follow standard scala style:

https://docs.scala-lang.org/style/declarations.html#modifiers

Specifically, final should precede lazy.

This comment has been minimized.

@olafurpg

olafurpg May 12, 2018

Member

I have no strong preference, defaulting to the scala style guide sounds good 👍

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