From d8741f240b0363167404a3c1507c1ff9a386612e Mon Sep 17 00:00:00 2001 From: Mario Date: Tue, 14 Jul 2026 18:02:15 +0200 Subject: [PATCH] Sort modifiers by Kotlin conventions --- CHANGELOG.md | 2 + .../org/jetbrains/ktfmt/format/Formatter.kt | 2 + .../jetbrains/ktfmt/format/ModifierSorter.kt | 210 ++++++++++++++++++ .../ktfmt/format/ModifierSorterTest.kt | 197 ++++++++++++++++ .../annotationsWithModifiers.output | 5 + .../cases/format/function/modifiers.output | 1 + .../DeclarationAnnotations.new.output | 9 +- .../annotation/DeclarationAnnotations.output | 6 +- 8 files changed, 426 insertions(+), 6 deletions(-) create mode 100644 core/src/main/kotlin/org/jetbrains/ktfmt/format/ModifierSorter.kt create mode 100644 core/src/test/kotlin/org/jetbrains/ktfmt/format/ModifierSorterTest.kt create mode 100644 core/src/test/resources/cases/format/annotation/annotationsWithModifiers.output create mode 100644 core/src/test/resources/cases/format/function/modifiers.output diff --git a/CHANGELOG.md b/CHANGELOG.md index e67d71ff2..0e344d50d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,8 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/). - Trailing comma is now added when a single parameter is formatted onto its own line (https://github.com/facebook/ktfmt/issues/461) - Migrated from `com.facebook.ktfmt` package to `org.jetbrains.ktfmt` +- Sort declaration modifiers according to Kotlin conventions while preserving unsupported modifier + positions (https://github.com/facebook/ktfmt/issues/293) ### Fixed diff --git a/core/src/main/kotlin/org/jetbrains/ktfmt/format/Formatter.kt b/core/src/main/kotlin/org/jetbrains/ktfmt/format/Formatter.kt index b5f5f7f98..09c58896c 100644 --- a/core/src/main/kotlin/org/jetbrains/ktfmt/format/Formatter.kt +++ b/core/src/main/kotlin/org/jetbrains/ktfmt/format/Formatter.kt @@ -119,6 +119,7 @@ object Formatter { if (lineRanges == null && characterRanges == null) { FormatterContext(normalizedKotlinCode) .transform { sortedAndDistinctImports(it) } + .transform { ModifierSorter.sort(it) } .transform { dropRedundantElements(it, options) } .transform { addRedundantElements(it, options) } .let { prettyPrintAndManageTrailingCommas(it, options, lineSeparator = "\n") } @@ -147,6 +148,7 @@ object Formatter { .code } FormatterContext(partiallyFormattedCode) + .transform { ModifierSorter.sort(it) } .transform { dropRedundantElements(it, options) } .transform { sortedAndDistinctImports(it, trimLeadingWhitespace = true) } .transform { addRedundantElements(it, options) } diff --git a/core/src/main/kotlin/org/jetbrains/ktfmt/format/ModifierSorter.kt b/core/src/main/kotlin/org/jetbrains/ktfmt/format/ModifierSorter.kt new file mode 100644 index 000000000..c2df217b2 --- /dev/null +++ b/core/src/main/kotlin/org/jetbrains/ktfmt/format/ModifierSorter.kt @@ -0,0 +1,210 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * 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 org.jetbrains.ktfmt.format + +import org.jetbrains.kotlin.com.intellij.psi.PsiComment +import org.jetbrains.kotlin.com.intellij.psi.PsiElement +import org.jetbrains.kotlin.com.intellij.psi.PsiWhiteSpace +import org.jetbrains.kotlin.lexer.KtModifierKeywordToken +import org.jetbrains.kotlin.psi.KtAnnotation +import org.jetbrains.kotlin.psi.KtAnnotationEntry +import org.jetbrains.kotlin.psi.KtFile +import org.jetbrains.kotlin.psi.KtModifierList +import org.jetbrains.kotlin.psi.KtTreeVisitorVoid +import org.jetbrains.kotlin.psi.psiUtil.endOffset +import org.jetbrains.kotlin.psi.psiUtil.startOffset + +/** Sorts declaration modifiers according to the Kotlin coding conventions. */ +internal object ModifierSorter { + private val modifierRanks = listOf( + setOf("public", "protected", "private", "internal"), + setOf("expect", "actual"), + setOf("final", "open", "abstract", "sealed"), + setOf("const"), + setOf("external"), + setOf("override"), + setOf("lateinit"), + setOf("tailrec"), + setOf("vararg"), + setOf("suspend"), + setOf("inner"), + setOf("enum", "annotation", "fun"), + setOf("companion"), + setOf("inline", "value"), + setOf("infix"), + setOf("operator"), + setOf("data"), + ) + .flatMapIndexed { rank, modifiers -> modifiers.map { it to rank } } + .toMap() + + internal fun sort(file: KtFile): String { + val replacements = mutableListOf() + file.accept( + object : KtTreeVisitorVoid() { + override fun visitModifierList(list: KtModifierList) { + sortedText(list)?.let { replacements.add(Replacement(list, it)) } + super.visitModifierList(list) + } + }, + ) + + if (replacements.isEmpty()) return file.text + val innermostReplacements = replacements.filter { candidate -> + replacements.none { other -> + other !== candidate && + other.element.startOffset >= candidate.element.startOffset && + other.element.endOffset <= candidate.element.endOffset + } + } + val result = StringBuilder(file.text) + for (replacement in innermostReplacements.sortedByDescending { it.element.endOffset }) { + result.replace( + replacement.element.startOffset, + replacement.element.endOffset, + replacement.text, + ) + } + val sortedCode = result.toString() + return if (innermostReplacements.size == replacements.size) sortedCode + else sort(Parser.parse(sortedCode)) + } + + private fun sortedText(list: KtModifierList): String? { + val significantChildren = + generateSequence(list.node.firstChildNode) { it.treeNext } + .map { it.psi } + .filterNot { it is PsiWhiteSpace } + .toList() + val parts = mutableListOf() + val sortableSegment = mutableListOf() + + fun flushSortableSegment() { + if (sortableSegment.isEmpty()) return + parts.add(Part.Sortable(sortSegment(list, sortableSegment))) + sortableSegment.clear() + } + + for (child in significantChildren) { + if (child is PsiComment || child.isAnnotation() || child.sortRank() != null) { + sortableSegment.add(child) + } else { + flushSortableSegment() + parts.add(Part.Fixed(child.text)) + } + } + flushSortableSegment() + + val sorted = parts.map { it.text }.joinWithSpaces() + return sorted.takeIf { it != list.text } + } + + private fun sortSegment(list: KtModifierList, elements: List): String { + val units = mutableListOf() + val leadingComments = mutableListOf() + + for (element in elements) { + if (element is PsiComment) { + val previous = units.lastOrNull() + if (previous != null && list.hasNoNewlineBetween(previous.base, element)) { + previous.trailingComments.add(element) + } else { + leadingComments.add(element) + } + continue + } + + units.add( + SortableUnit( + base = element, + rank = element.sortRank(), + isAnnotation = element.isAnnotation(), + leadingComments = leadingComments.toMutableList(), + ), + ) + leadingComments.clear() + } + + if (leadingComments.isNotEmpty()) { + units.lastOrNull()?.followingComments?.addAll(leadingComments) + } + + val sortedUnits = + units.filter { it.isAnnotation } + units.filterNot { it.isAnnotation }.sortedBy { it.rank } + if (sortedUnits.map { it.base } == units.map { it.base }) { + return list.text.substring( + elements.first().startOffset - list.startOffset, + elements.last().endOffset - list.startOffset, + ) + } + return sortedUnits.map { it.render() }.joinWithSpaces() + } + + private fun PsiElement.isAnnotation(): Boolean = this is KtAnnotation || this is KtAnnotationEntry + + private fun PsiElement.sortRank(): Int? = + if (node.elementType is KtModifierKeywordToken) modifierRanks[text] else null + + private fun KtModifierList.hasNoNewlineBetween(first: PsiElement, second: PsiElement): Boolean = + text.substring(first.endOffset - startOffset, second.startOffset - startOffset).none { + it == '\n' || it == '\r' + } + + private fun SortableUnit.render(): String = buildString { + for (comment in leadingComments) { + append(comment.text) + append('\n') + } + append(base.text) + for (comment in trailingComments) { + append(' ') + append(comment.text) + if (comment.text.startsWith("//")) append('\n') + } + for (comment in followingComments) { + append('\n') + append(comment.text) + append('\n') + } + } + + private fun Iterable.joinWithSpaces(): String = buildString { + for (text in this@joinWithSpaces) { + if (isNotEmpty() && !endsWith('\n') && !endsWith('\r')) append(' ') + append(text) + } + } + + private data class Replacement(val element: PsiElement, val text: String) + + private data class SortableUnit( + val base: PsiElement, + val rank: Int?, + val isAnnotation: Boolean, + val leadingComments: MutableList, + val trailingComments: MutableList = mutableListOf(), + val followingComments: MutableList = mutableListOf(), + ) + + private sealed interface Part { + val text: String + + data class Sortable(override val text: String) : Part + + data class Fixed(override val text: String) : Part + } +} diff --git a/core/src/test/kotlin/org/jetbrains/ktfmt/format/ModifierSorterTest.kt b/core/src/test/kotlin/org/jetbrains/ktfmt/format/ModifierSorterTest.kt new file mode 100644 index 000000000..868b50bd0 --- /dev/null +++ b/core/src/test/kotlin/org/jetbrains/ktfmt/format/ModifierSorterTest.kt @@ -0,0 +1,197 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * 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 org.jetbrains.ktfmt.format + +import com.google.common.collect.Range +import com.google.common.collect.TreeRangeSet +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Test + +class ModifierSorterTest { + @Test + fun `sort modifiers and move annotations before them`() { + val code = + """ + final @Magic public class Foo + + data operator infix inline @Magic(1) suspend override internal class Foo + + fun interface Foo + + final @get:Rule @field:[Inject Named("WEB_VIEW")] private val property = 1 + + inline fun consume(noinline @Magic block: () -> Unit) {} + """ + .trimIndent() + + val expected = + """ + @Magic public final class Foo + + @Magic(1) internal override suspend inline infix operator data class Foo + + fun interface Foo + + @get:Rule + @field:[Inject Named("WEB_VIEW")] + private final val property = 1 + + inline fun consume(noinline @Magic block: () -> Unit) {} + """ + .trimIndent() + .plus("\n") + + assertEquals(expected, Formatter.format(code)) + } + + @Test + fun `comments stay attached while modifiers are sorted`() { + val code = + """ + override /* override explanation */ public fun one() {} + + override + // public explanation + public fun two() {} + + override // override explanation + public fun three() {} + + override public + /* declaration explanation */ fun four() {} + """ + .trimIndent() + + val expected = + """ + public override /* override explanation */ fun one() {} + + // public explanation + public override fun two() {} + + public override // override explanation + fun three() {} + + public override /* declaration explanation */ fun four() {} + """ + .trimIndent() + .plus("\n") + + assertEquals(expected, Formatter.format(code)) + } + + @Test + fun `sort every modifier convention group`() { + val code = + """ + actual public typealias ActualName = String + sealed internal class SealedClass + const private val constant = 1 + external public fun externalFunction() + suspend override protected fun overriddenFunction() + lateinit internal var property: String + tailrec private fun recursiveFunction() {} + inner protected class InnerClass + enum public class EnumClass + class Container { companion private object } + value public class ValueClass(val value: Int) + operator inline fun plus(other: ValueClass) = this + infix inline fun combine(other: ValueClass) = this + data internal class DataClass(val value: Int) + """ + .trimIndent() + + val expected = + """ + public actual typealias ActualName = String + + internal sealed class SealedClass + + private const val constant = 1 + + public external fun externalFunction() + + protected override suspend fun overriddenFunction() + + internal lateinit var property: String + + private tailrec fun recursiveFunction() {} + + protected inner class InnerClass + + public enum class EnumClass + + class Container { + private companion object + } + + public value class ValueClass(val value: Int) + + inline operator fun plus(other: ValueClass) = this + + inline infix fun combine(other: ValueClass) = this + + internal data class DataClass(val value: Int) + """ + .trimIndent() + .plus("\n") + + assertEquals(expected, Formatter.format(code)) + } + + @Test + fun `modifier sorting is a whole-file cleanup for every style`() { + val code = + """ + override public fun outsideSelection() {} + fun insideSelection() { println( 1 ) } + """ + .trimIndent() + val expected = + """ + public override fun outsideSelection() {} + + fun insideSelection() { + println(1) + } + """ + .trimIndent() + .plus("\n") + val selectedSecondLine = TreeRangeSet.create().apply { add(Range.closedOpen(1, 2)) } + + for (options in listOf(Formatter.META_FORMAT, Formatter.GOOGLE_FORMAT)) { + assertEquals(expected, Formatter.format(options, code, lineRanges = selectedSecondLine)) + } + + assertEquals( + expected.replace(" println", " println"), + Formatter.format( + Formatter.KOTLINLANG_FORMAT, + code, + lineRanges = selectedSecondLine, + ), + ) + } + + @Test + fun `modifier sorting preserves line separators and context receivers`() { + val code = "context(Something)\r\noverride public fun f() {}\r\n" + val expected = "context(Something)\r\npublic override fun f() {}\r\n" + + assertEquals(expected, Formatter.format(code)) + } +} diff --git a/core/src/test/resources/cases/format/annotation/annotationsWithModifiers.output b/core/src/test/resources/cases/format/annotation/annotationsWithModifiers.output new file mode 100644 index 000000000..2f178588a --- /dev/null +++ b/core/src/test/resources/cases/format/annotation/annotationsWithModifiers.output @@ -0,0 +1,5 @@ +@Magic public final class Foo + +@Magic(1) public final class Foo + +@Magic(1) public final class Foo diff --git a/core/src/test/resources/cases/format/function/modifiers.output b/core/src/test/resources/cases/format/function/modifiers.output new file mode 100644 index 000000000..1b4d7f8e6 --- /dev/null +++ b/core/src/test/resources/cases/format/function/modifiers.output @@ -0,0 +1 @@ +internal override fun f() {} diff --git a/core/src/test/resources/cases/new_codestyle/annotation/DeclarationAnnotations.new.output b/core/src/test/resources/cases/new_codestyle/annotation/DeclarationAnnotations.new.output index d4cd74f11..da9a1a4d8 100644 --- a/core/src/test/resources/cases/new_codestyle/annotation/DeclarationAnnotations.new.output +++ b/core/src/test/resources/cases/new_codestyle/annotation/DeclarationAnnotations.new.output @@ -13,11 +13,14 @@ annotation class OnAnnotationClass @A private val beforeModifier = 1 -private @Deprecated("some reason") val afterModifier = 1 +@Deprecated("some reason") +private val afterModifier = 1 -public @Magic final class Interleaved +@Magic +public final class Interleaved -public @Magic(1, "argument") final class InterleavedWithArgs +@Magic(1, "argument") +public final class InterleavedWithArgs class PrimaryCtor @Inject constructor(val a: Int) diff --git a/core/src/test/resources/cases/new_codestyle/annotation/DeclarationAnnotations.output b/core/src/test/resources/cases/new_codestyle/annotation/DeclarationAnnotations.output index 1fa54528d..230a49215 100644 --- a/core/src/test/resources/cases/new_codestyle/annotation/DeclarationAnnotations.output +++ b/core/src/test/resources/cases/new_codestyle/annotation/DeclarationAnnotations.output @@ -8,11 +8,11 @@ @A private val beforeModifier = 1 -private @Deprecated("some reason") val afterModifier = 1 +@Deprecated("some reason") private val afterModifier = 1 -public @Magic final class Interleaved +@Magic public final class Interleaved -public @Magic(1, "argument") final class InterleavedWithArgs +@Magic(1, "argument") public final class InterleavedWithArgs class PrimaryCtor @Inject constructor(val a: Int)