From e379d469c8fe7425656e735241016988660ab915 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 | 3 + .../com/facebook/ktfmt/format/Formatter.kt | 2 + .../facebook/ktfmt/format/ModifierSorter.kt | 210 ++++++++++++++++++ .../facebook/ktfmt/format/FormatterTest.kt | 184 ++++++++++++++- 4 files changed, 387 insertions(+), 12 deletions(-) create mode 100644 core/src/main/java/com/facebook/ktfmt/format/ModifierSorter.kt diff --git a/CHANGELOG.md b/CHANGELOG.md index 38d10a8a..c6bb96da 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,9 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/). ### Changed +- Sort declaration modifiers according to Kotlin conventions while preserving unsupported modifier + positions (https://github.com/facebook/ktfmt/issues/293) + ### Fixed * Fix non-idempotent formatting when a managed trailing comma pushes a line over MAX_WIDTH (e.g. a long qualified expression as the last argument of a call). The comma is now accounted for by re-running the layout, so the line is broken correctly on the first pass. (https://github.com/facebook/ktfmt/pull/636) diff --git a/core/src/main/java/com/facebook/ktfmt/format/Formatter.kt b/core/src/main/java/com/facebook/ktfmt/format/Formatter.kt index 5e9f33f9..4379cdd7 100644 --- a/core/src/main/java/com/facebook/ktfmt/format/Formatter.kt +++ b/core/src/main/java/com/facebook/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/java/com/facebook/ktfmt/format/ModifierSorter.kt b/core/src/main/java/com/facebook/ktfmt/format/ModifierSorter.kt new file mode 100644 index 00000000..cb81f8e6 --- /dev/null +++ b/core/src/main/java/com/facebook/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 com.facebook.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/java/com/facebook/ktfmt/format/FormatterTest.kt b/core/src/test/java/com/facebook/ktfmt/format/FormatterTest.kt index f986044b..bccc1db9 100644 --- a/core/src/test/java/com/facebook/ktfmt/format/FormatterTest.kt +++ b/core/src/test/java/com/facebook/ktfmt/format/FormatterTest.kt @@ -20,6 +20,8 @@ import com.facebook.ktfmt.format.Formatter.META_FORMAT import com.facebook.ktfmt.testutil.assertFormatted import com.facebook.ktfmt.testutil.assertThatFormatting import com.facebook.ktfmt.testutil.defaultTestFormattingOptions +import com.google.common.collect.Range +import com.google.common.collect.TreeRangeSet import com.google.common.truth.Truth.assertThat import org.junit.Assert.fail import org.junit.BeforeClass @@ -2278,7 +2280,7 @@ class FormatterTest { fun `method modifiers`() = assertFormatted( """ - |override internal fun f() {} + |internal override fun f() {} |""" .trimMargin() ) @@ -5987,17 +5989,175 @@ class FormatterTest { ) @Test - fun `handle annotations mixed with keywords since we cannot reorder them for now`() = - assertFormatted( - """ - |public @Magic final class Foo - | - |public @Magic(1) final class Foo - | - |@Magic(1) public final class Foo - |""" - .trimMargin() - ) + 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) {} + |""" + .trimMargin() + + 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) {} + |""" + .trimMargin() + + assertThatFormatting(code).isEqualTo(expected) + } + + @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() {} + |""" + .trimMargin() + + 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() {} + |""" + .trimMargin() + + assertThatFormatting(code).isEqualTo(expected) + } + + @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) + |""" + .trimMargin() + + 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) + |""" + .trimMargin() + + assertThatFormatting(code).isEqualTo(expected) + } + + @Test + fun `modifier sorting is a whole-file cleanup for every style`() { + val code = + """ + |override public fun outsideSelection() {} + |fun insideSelection() { println( 1 ) } + |""" + .trimMargin() + val expected = + """ + |public override fun outsideSelection() {} + | + |fun insideSelection() { + | println(1) + |} + |""" + .trimMargin() + val selectedSecondLine = TreeRangeSet.create().apply { add(Range.closedOpen(1, 2)) } + + for (options in listOf(Formatter.META_FORMAT, Formatter.GOOGLE_FORMAT)) { + assertThat(Formatter.format(options, code, lineRanges = selectedSecondLine)) + .isEqualTo(expected) + } + + val kotlinlangExpected = expected.replace(" println", " println") + assertThat( + Formatter.format( + Formatter.KOTLINLANG_FORMAT, + code, + lineRanges = selectedSecondLine, + ) + ) + .isEqualTo(kotlinlangExpected) + } + + @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" + + assertThat(Formatter.format(code)).isEqualTo(expected) + } @Test fun `handle annotations more`() = assertFormatted(