-
Notifications
You must be signed in to change notification settings - Fork 116
Sort modifiers according to Kotlin conventions #641
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
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
| @@ -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"), | ||
|
Collaborator
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.
Or, if we also want to tackle all modifiers, then it makes sense to include noinline and other parameter modifiers as well
Member
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. Yes, I think current implementation requires having all modifiers. Currently implementation fails with a parsing error on this example: Modifier sorter does not consider |
||
| 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<Replacement>() | ||
| 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)) | ||
|
Member
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 suspect that this sorter can be implemented in one pass (i.e. requiring only one re-parsing). I'm not 100% sure if we need it though, because it can complicate the code. So we can merge this version and improve it later |
||
| } | ||
|
|
||
| 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<Part>() | ||
| val sortableSegment = mutableListOf<PsiElement>() | ||
|
|
||
| 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 } | ||
|
Member
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. One potential problem of this implementation is that we don't preserve the line breaks between modifiers. So we will trigger replacement/parsing even if the original code just contained line breaks like
Member
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 would suggest modifying the code to detect if the modifiers were actually sorted |
||
| } | ||
|
|
||
| private fun sortSegment(list: KtModifierList, elements: List<PsiElement>): String { | ||
| val units = mutableListOf<SortableUnit>() | ||
| val leadingComments = mutableListOf<PsiComment>() | ||
|
|
||
| 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) | ||
|
Member
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. To be honest I was not able to come up with a test case that covers this condition (except for test cases with currently unsupported modifiers). If we handle all modifiers properly --- would we even need that? |
||
| } | ||
|
|
||
| val sortedUnits = | ||
| units.filter { it.isAnnotation } + units.filterNot { it.isAnnotation }.sortedBy { it.rank } | ||
|
Member
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. Having a rank for annotations will simplify this code |
||
| 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 | ||
|
Member
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. We can give annotations rank |
||
|
|
||
| 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<String>.joinWithSpaces(): String = buildString { | ||
|
Member
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. There are several cases in code where you use This will be similar to |
||
| 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<PsiComment>, | ||
| val trailingComments: MutableList<PsiComment> = mutableListOf(), | ||
| val followingComments: MutableList<PsiComment> = mutableListOf(), | ||
| ) | ||
|
|
||
| private sealed interface Part { | ||
|
Member
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 feels like this abstraction is not necessary. Both subclasses are only created in one place in code and no distinction is made between them after: Lets remove this abstraction. Or add documentation explaining why its necessary |
||
| val text: String | ||
|
|
||
| data class Sortable(override val text: String) : Part | ||
|
|
||
| data class Fixed(override val text: String) : Part | ||
| } | ||
| } | ||
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.
This comment doesn't add much value, let's remove it