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
Changes from 9 commits
e07c07f
771439a
327a9b3
68f1475
c432f2e
3e8f1b9
548393b
7a3120b
aa4dd13
6f8c4b7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,8 @@ case class RewriteSettings( | |
rules: Seq[Rewrite] = Nil, | ||
@Recurse redundantBraces: RedundantBracesSettings = | ||
RedundantBracesSettings(), | ||
@Recurse neverInfix: Pattern = Pattern.neverInfix | ||
@Recurse neverInfix: Pattern = Pattern.neverInfix, | ||
@Recurse sortModifiers: SortSettings = SortSettings.default | ||
) { | ||
Rewrite.validateRewrites(rules) match { | ||
case Nil => // OK | ||
|
@@ -21,4 +22,12 @@ case class RewriteSettings( | |
) | ||
} | ||
|
||
if (sortModifiers.order.distinct.length != 8) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's better to write a decoder by hand in the The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, this is fine, I can take care of it in #1145 once this PR is merged :) |
||
throw InvalidScalafmtConfiguration( | ||
new IllegalArgumentException( | ||
"'sortModifiers.order', if specified, it has to contain all of the following values in the order you wish them sorted:" + | ||
"""["private", "protected" , "abstract", "final", "sealed", "implicit", "override", "lazy"]""" | ||
) | ||
) | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
package org.scalafmt.config | ||
|
||
import metaconfig._ | ||
|
||
@DeriveConfDecoder | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nevermind this comment. |
||
case class SortSettings( | ||
order: Vector[SortSettings.ModKey] | ||
) | ||
|
||
object SortSettings { | ||
|
||
implicit val SortSettingsModKeyReader: ConfDecoder[ModKey] = | ||
ReaderUtil.oneOfIgnoreBackticks[ModKey]( | ||
`implicit`, | ||
`final`, | ||
`sealed`, | ||
`abstract`, | ||
`override`, | ||
`private`, | ||
`protected`, | ||
`lazy` | ||
) | ||
|
||
val defaultOrder: Vector[ModKey] = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there any reason why the default order doesn't follow standard scala style: https://docs.scala-lang.org/style/declarations.html#modifiers Specifically, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have no strong preference, defaulting to the scala style guide sounds good 👍 |
||
Vector( | ||
`implicit`, | ||
// | ||
`final`, | ||
`sealed`, | ||
`abstract`, | ||
// | ||
`override`, | ||
// | ||
`private`, | ||
`protected`, | ||
// | ||
`lazy` | ||
) | ||
|
||
def default: SortSettings = | ||
SortSettings(defaultOrder) | ||
|
||
sealed trait ModKey extends Product | ||
|
||
case object `private` extends ModKey | ||
case object `protected` extends ModKey | ||
case object `final` extends ModKey | ||
case object `sealed` extends ModKey | ||
case object `abstract` extends ModKey | ||
case object `implicit` extends ModKey | ||
case object `override` extends ModKey | ||
case object `lazy` extends ModKey | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
package org.scalafmt.rewrite | ||
|
||
import org.scalafmt.config.SortSettings._ | ||
|
||
import scala.meta.Tree | ||
import scala.meta._ | ||
|
||
object SortModifiers extends Rewrite { | ||
|
||
override def rewrite(code: Tree, ctx: RewriteCtx): Seq[Patch] = { | ||
implicit val order = ctx.style.rewrite.sortModifiers.order | ||
|
||
/* | ||
* in the case of Class, Object, and of class constructor parameters | ||
* some Mods are immovable, e.g. 'case' in "case class X". | ||
* | ||
* The case of parameters is a bit more curious because there the | ||
* "val" or "var" in, say: | ||
* {{{ | ||
* class Test(private final val x: Int) | ||
* }}} | ||
* are considered Mods, instead of being similar to `Defn.Val`, or `Defn.Var`. | ||
*/ | ||
val patchesOfPatches = code.collect { | ||
case d: Decl.Def => sortMods(d.mods) | ||
case v: Decl.Val => sortMods(v.mods) | ||
case v: Decl.Var => sortMods(v.mods) | ||
case t: Decl.Type => sortMods(t.mods) | ||
case d: Defn.Def => sortMods(d.mods) | ||
case v: Defn.Val => sortMods(v.mods) | ||
case v: Defn.Var => sortMods(v.mods) | ||
case t: Defn.Type => sortMods(t.mods) | ||
case c: Defn.Class => sortMods(c.mods.filterNot(_.is[Mod.Case])) | ||
case o: Defn.Object => sortMods(o.mods.filterNot(_.is[Mod.Case])) | ||
case t: Defn.Trait => sortMods(t.mods) | ||
case p: Term.Param => | ||
sortMods( | ||
p.mods.filterNot(m => m.is[Mod.ValParam] || m.is[Mod.VarParam])) | ||
} | ||
patchesOfPatches.flatten | ||
} | ||
|
||
private def sortMods( | ||
oldMods: Seq[Mod] | ||
)(implicit order: Vector[ModKey]): Seq[Patch] = { | ||
if (oldMods.isEmpty) Nil | ||
else { | ||
val sortedMods: Seq[Mod] = oldMods.sortWith(orderModsBy(order)) | ||
sortedMods.zip(oldMods).flatMap { | ||
case (next, old) => | ||
if (old.tokens.isEmpty) { | ||
//required for cases like: def foo(implicit x: Int) | ||
Nil | ||
} else { | ||
val removeOld = old.tokens.map(t => TokenPatch.Remove(t)) | ||
val addNext = TokenPatch.AddRight(old.tokens.head, next.syntax) | ||
removeOld :+ addNext | ||
} | ||
} | ||
} | ||
} | ||
|
||
/** | ||
* @return | ||
* m1 < m2; according to the order given by the Vector | ||
*/ | ||
private def orderModsBy(order: Vector[ModKey])(m1: Mod, m2: Mod): Boolean = { | ||
val idx1 = order.indexWhere(modCorrespondsToSettingKey(m1)) | ||
val idx2 = order.indexWhere(modCorrespondsToSettingKey(m2)) | ||
idx1 < idx2 | ||
} | ||
|
||
private def modCorrespondsToSettingKey(m: Mod)(p: ModKey): Boolean = { | ||
p == `private` && m.is[Mod.Private] || | ||
p == `protected` && m.is[Mod.Protected] || | ||
p == `final` && m.is[Mod.Final] || | ||
p == `sealed` && m.is[Mod.Sealed] || | ||
p == `abstract` && m.is[Mod.Abstract] || | ||
p == `lazy` && m.is[Mod.Lazy] || | ||
p == `implicit` && m.is[Mod.Implicit] || | ||
p == `override` && m.is[Mod.Override] | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.