Skip to content
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.

Sign up for GitHub

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

Remove usages of getPsi() #2901

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

Remove usages of getPsi() #2901

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
7f8984c
implement ASTNode.isPartOfComment without psi
mgroth0 Dec 8, 2024
2562354
remove psi from existingSuppressionsFromNamedArgumentOrNull
mgroth0 Dec 10, 2024
a05595f
remove psi from visitKtlintSuppressionInAnnotation
mgroth0 Dec 10, 2024
1b76046
remove psi from getAnnotationUseSiteTarget
mgroth0 Dec 10, 2024
546dfca
remove psi from isOnSameLineAsControlFlowKeyword
mgroth0 Dec 10, 2024
5f0f4a8
reduce psi in NoUnusedImportsRule
mgroth0 Dec 10, 2024
7e2f282
remove psi from SpacingAroundColonRule
mgroth0 Dec 10, 2024
bd1596b
remove psi from SpacingAroundDoubleColonRule
mgroth0 Dec 10, 2024
d0c798e
remove psi from StatementWrappingRule
mgroth0 Dec 10, 2024
0058330
reduce psi in TrailingCommaOnDeclarationSiteRule
mgroth0 Dec 10, 2024
a10fd38
deprecate and replace usages of "isPartOf(klass: KClass<out PsiElemen…
mgroth0 Dec 10, 2024
94624ba
remove psi from SuppressionLocator
mgroth0 Dec 10, 2024
e962b64
simple psi removals
mgroth0 Dec 10, 2024
c8d1a54
refactor token sets
mgroth0 Dec 10, 2024
efd32be
remove psi from BlankLineBeforeDeclarationRule
mgroth0 Dec 10, 2024
6094819
remove psi from EnumEntryNameCaseRule
mgroth0 Dec 10, 2024
e2846c1
changs from review
mgroth0 Dec 20, 2024
fe71a48
add kdoc for findChildByTypeRecursively and endOffset
mgroth0 Dec 29, 2024
d92140d
remove a getPsi() from TrailingCommaOnDeclarationSiteRule
mgroth0 Dec 29, 2024
ffda19b
add contract to isWhiteSpace
mgroth0 Dec 29, 2024
3efbf7a
make token sets more maintainable
mgroth0 Dec 29, 2024
cb757a9
alternative solution to token set
mgroth0 Dec 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions ktlint-rule-engine-core/api/ktlint-rule-engine-core.api
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ public final class com/pinterest/ktlint/rule/engine/core/api/ASTNodeExtensionKt
public static final fun findCompositeParentElementOfType (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;Lorg/jetbrains/kotlin/com/intellij/psi/tree/IElementType;)Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;
public static final fun firstChildLeafOrSelf (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;)Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;
public static final fun getColumn (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;)I
public static final fun getPrevSiblingIgnoringWhitespaceAndComments (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;)Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;
paul-dingemans marked this conversation as resolved.
Show resolved Hide resolved
public static final fun hasModifier (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;Lorg/jetbrains/kotlin/com/intellij/psi/tree/IElementType;)Z
public static final fun hasNewLineInClosedRange (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;)Z
public static final fun indent (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;Z)Ljava/lang/String;
Expand All @@ -14,6 +15,7 @@ public final class com/pinterest/ktlint/rule/engine/core/api/ASTNodeExtensionKt
public static final fun isLeaf (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;)Z
public static final fun isPartOf (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;Lkotlin/reflect/KClass;)Z
public static final fun isPartOf (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;Lorg/jetbrains/kotlin/com/intellij/psi/tree/IElementType;)Z
public static final fun isPartOf (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;Lorg/jetbrains/kotlin/com/intellij/psi/tree/TokenSet;)Z
public static final fun isPartOfComment (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;)Z
public static final fun isPartOfCompositeElementOfType (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;Lorg/jetbrains/kotlin/com/intellij/psi/tree/IElementType;)Z
public static final fun isPartOfString (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;)Z
Expand Down Expand Up @@ -56,6 +58,8 @@ public final class com/pinterest/ktlint/rule/engine/core/api/ASTNodeExtensionKt
public static synthetic fun prevLeaf$default (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;ZILjava/lang/Object;)Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;
public static final fun prevSibling (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;Lkotlin/jvm/functions/Function1;)Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;
public static synthetic fun prevSibling$default (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;Lkotlin/jvm/functions/Function1;ILjava/lang/Object;)Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;
public static final fun recursiveChildren (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;Z)Lkotlin/sequences/Sequence;
public static synthetic fun recursiveChildren$default (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;ZILjava/lang/Object;)Lkotlin/sequences/Sequence;
public static final fun remove (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;)V
public static final fun replaceWith (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;)V
public static final fun upsertWhitespaceAfterMe (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;Ljava/lang/String;)V
Expand Down Expand Up @@ -524,6 +528,12 @@ public final class com/pinterest/ktlint/rule/engine/core/api/SinceKtlint$Status
public static fun values ()[Lcom/pinterest/ktlint/rule/engine/core/api/SinceKtlint$Status;
}

public final class com/pinterest/ktlint/rule/engine/core/api/TokenSets {
public static final field INSTANCE Lcom/pinterest/ktlint/rule/engine/core/api/TokenSets;
public final fun getCOMMENTS ()Lorg/jetbrains/kotlin/com/intellij/psi/tree/TokenSet;
public final fun getEXPRESSIONS ()Lorg/jetbrains/kotlin/com/intellij/psi/tree/TokenSet;
}

public final class com/pinterest/ktlint/rule/engine/core/api/editorconfig/CodeStyleEditorConfigPropertyKt {
public static final fun getCODE_STYLE_PROPERTY ()Lcom/pinterest/ktlint/rule/engine/core/api/editorconfig/EditorConfigProperty;
public static final fun getCODE_STYLE_PROPERTY_TYPE ()Lorg/ec4j/core/model/PropertyType$LowerCasingPropertyType;
Expand Down
28 changes: 26 additions & 2 deletions ...engine-core/src/main/kotlin/com/pinterest/ktlint/rule/engine/core/api/ASTNodeExtension.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,14 @@ import com.pinterest.ktlint.rule.engine.core.api.ElementType.VARARG_KEYWORD
import com.pinterest.ktlint.rule.engine.core.api.ElementType.VAR_KEYWORD
import com.pinterest.ktlint.rule.engine.core.api.ElementType.WHITE_SPACE
import org.jetbrains.kotlin.com.intellij.lang.ASTNode
import org.jetbrains.kotlin.com.intellij.psi.PsiComment
import org.jetbrains.kotlin.com.intellij.psi.PsiElement
import org.jetbrains.kotlin.com.intellij.psi.impl.source.tree.CompositeElement
import org.jetbrains.kotlin.com.intellij.psi.impl.source.tree.LeafElement
import org.jetbrains.kotlin.com.intellij.psi.impl.source.tree.PsiWhiteSpaceImpl
import org.jetbrains.kotlin.com.intellij.psi.tree.IElementType
import org.jetbrains.kotlin.com.intellij.psi.tree.TokenSet
import org.jetbrains.kotlin.psi.psiUtil.leaves
import org.jetbrains.kotlin.psi.psiUtil.siblings
import org.jetbrains.kotlin.util.prefixIfNot
import org.jetbrains.kotlin.utils.addToStdlib.applyIf
import kotlin.reflect.KClass
Expand Down Expand Up @@ -183,11 +184,17 @@ public fun ASTNode.parent(
return null
}

public fun ASTNode.isPartOf(tokenSet: TokenSet): Boolean = parent(predicate = { tokenSet.contains(it.elementType) }, strict = false) != null
paul-dingemans marked this conversation as resolved.
Show resolved Hide resolved

/**
* @param elementType [ElementType].*
*/
public fun ASTNode.isPartOf(elementType: IElementType): Boolean = parent(elementType, strict = false) != null

@Deprecated(
"psi is a performance issue, see https://github.com/pinterest/ktlint/pull/2901",
replaceWith = ReplaceWith("ASTNode.isPartOf(elementType: IElementType) or ASTNode.isPartOf(tokenSet: TokenSet)"),
)
paul-dingemans marked this conversation as resolved.
Show resolved Hide resolved
public fun ASTNode.isPartOf(klass: KClass<out PsiElement>): Boolean {
var n: ASTNode? = this
while (n != null) {
Expand Down Expand Up @@ -223,10 +230,21 @@ public fun ASTNode.isLeaf(): Boolean = firstChildNode == null
*/
public fun ASTNode.isCodeLeaf(): Boolean = isLeaf() && !isWhiteSpace() && !isPartOfComment()

public fun ASTNode.isPartOfComment(): Boolean = parent(strict = false) { it.psi is PsiComment } != null
public fun ASTNode.isPartOfComment(): Boolean = isPartOf(TokenSets.COMMENTS)
paul-dingemans marked this conversation as resolved.
Show resolved Hide resolved

public fun ASTNode.children(): Sequence<ASTNode> = generateSequence(firstChildNode) { node -> node.treeNext }

public fun ASTNode.recursiveChildren(includeSelf: Boolean = false): Sequence<ASTNode> =
sequence {
if (includeSelf) {
yield(this@recursiveChildren)
}
children().forEach {
yield(it)
yieldAll(it.recursiveChildren())
}
paul-dingemans marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Updates or inserts a new whitespace element with [text] before the given node. If the node itself is a whitespace
* then its contents is replaced with [text]. If the node is a (nested) composite element, the whitespace element is
Expand Down Expand Up @@ -555,3 +573,9 @@ public fun ASTNode.replaceWith(node: ASTNode) {
public fun ASTNode.remove() {
treeParent.removeChild(this)
}

public fun ASTNode.getPrevSiblingIgnoringWhitespaceAndComments(): ASTNode? =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function getPrevSiblingIgnoringWhitespaceAndComments is duplicate of prevCodeSibling. Note that Psi is a java interface, and as of that is using the get prefix. For the ASTNodeExtensions we should prefer kotlin naming style without get prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Deleted getPrevSiblingIgnoringWhitespaceAndComments.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In SpacingBetweenDeclarationsWithCommentsRule the PSI version of getPrevSiblingIgnoringWhitespaceAndComments is used. I was trying to see whether I could refactor the code below, to not use psi:

            val declaration = node.parent as? KtDeclaration ?: return
            val isTailComment = node.startOffset > declaration.startOffset
            if (isTailComment || declaration.getPrevSiblingIgnoringWhitespaceAndComments() !is KtDeclaration) return

I could not (easily) find an angle how to replace KtDeclaration with an ASTNode implementation. Can you give me a pointer how to approach that? You don't need to implement it, but I would learn to know for myself how I could do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all very trial and error for me, but I was able to implement a solution for SpacingBetweenDeclarationsWithCommentsRule. It involves KtTokenSets.DECLARATION_TYPES, which is probably the key for getting rid of the is checking of KtDeclaration.

I won't commit the implementation since you asked to try it yourself. But here it is for your reference:

/**
 * @see https://youtrack.jetbrains.com/issue/KT-35088
 */
@SinceKtlint("0.37", EXPERIMENTAL)
@SinceKtlint("0.46", STABLE)
public class SpacingBetweenDeclarationsWithCommentsRule : StandardRule("spacing-between-declarations-with-comments") {
    override fun beforeVisitChildNodes(
        node: ASTNode,
        emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> AutocorrectDecision,
    ) {
        if (node.elementType in TokenSets.COMMENTS) {
            val declaration = node.parent { it.elementType in KtTokenSets.DECLARATION_TYPES } ?: return
            val isTailComment = node.startOffset > declaration.startOffset
            if (
                isTailComment ||
                declaration.prevSibling { !it.isWhiteSpace() && !it.isPartOfComment() }?.elementType !in KtTokenSets.DECLARATION_TYPES
            ) {
                return
            }

            val prevSibling = declaration.prevSibling { it.elementType != WHITE_SPACE }
            if (prevSibling != null &&
                prevSibling.elementType != FILE &&
                !prevSibling.isPartOfComment()
            ) {
                val directPrevSibling = declaration.prevSibling()
                if (directPrevSibling.isWhiteSpace() && directPrevSibling.text.count { it == '\n' } < 2) {
                    emit(
                        node.startOffset,
                        "Declarations and declarations with comments should have an empty space between.",
                        true,
                    ).ifAutocorrectAllowed {
                        val indent = node.prevLeaf()?.text?.trim('\n') ?: ""
                        (directPrevSibling as LeafPsiElement).rawReplaceWithText("\n\n$indent")
                    }
                }
            }
        }
    }
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question was indeed, how should I have gotten the idea to use KtTokenSets.DECLARATION_TYPES instead of is KtDeclaration? Could you derive that somehow from the Psi library, or did you find this accidentally while exploring the lib?

Copy link
Contributor Author

@mgroth0 mgroth0 Dec 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I essentially found it accidentally. I either searched for the term "declaration" in the kotlin repository or I found it while reading through the members of KtTokenSets. I am making the assumption that the word "declaration" is used consistently in the Psi class hierarchy and in KtTokenSets to define the same set of node types. I then checked all the ktlint tests still passed.

If we wanted to be even more sure, I have at least one idea:

  1. For each member of KtTokenSets.DECLARATION_TYPES, navigate to its associated Psi type. For example, looking at KtStubElementTypes.CLASS, we see it is a KtClassElementType. Looking at the KtClassElementType.kt we can see it is assocaited with the Psi type KtClass, which we can figure out implements KtDeclaration.
  2. For each subclass of KtDeclaration, check that all of the tokens associcated with it are contained in KtTokenSets.DECLARATION_TYPES.

This would obviously be a lot of work. Here are my follow up thoughts about it.

  • Such a test could be automated at least somewhat through reflection. For example, ClassGraph could be used in a test to easily get all of the subclasses of KtDeclaration.
  • In the end I defer to your judgement on when this PR is safe enough to release, but personally I lean towards not doing any of these sort of intense validations yet. If the kotlin compiler has an inconsistent definition on what a "declaration" is between KtTokenSets.DECLARATION_TYPES and KtDeclaration, then I suppose we will find out when someone makes a bug report and we could react accordingly.

siblings(forward = false)
.filter {
!it.isWhiteSpace() && !TokenSets.COMMENTS.contains(it.elementType)
}.firstOrNull()
38 changes: 38 additions & 0 deletions ...t-rule-engine-core/src/main/kotlin/com/pinterest/ktlint/rule/engine/core/api/TokenSets.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
package com.pinterest.ktlint.rule.engine.core.api

import org.jetbrains.kotlin.com.intellij.psi.tree.TokenSet

public object TokenSets {
public val COMMENTS: TokenSet = TokenSet.create(ElementType.BLOCK_COMMENT, ElementType.EOL_COMMENT, ElementType.KDOC)
public val EXPRESSIONS: TokenSet =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are those token sets based on information from the Psi Interface? If so, please add comment with references/links. If tokes are added or removed in the future from the Psi Interface, this makes it easier to keep our token sets in sync with Psi.

Also, I prefer to keep the elements in such collection sorted alphabetically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I didn't get this from a specific source unfortunately. When I mentioned that there might be bugs, these token sets are one of my biggest worries. Like EXPRESSIONS, I'm afraid it could be missing something that didn't get covered by a test.

I alphabetized the lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The kotlin compiler has some like KtTokenSets but I am not sure if I've seen one with all expressions.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I didn't get this from a specific source unfortunately. When I mentioned that there might be bugs, these token sets are one of my biggest worries. Like EXPRESSIONS, I'm afraid it could be missing something that didn't get covered by a test.

This is rather concerning.

The kotlin compiler has some like KtTokenSets but I am not sure if I've seen one with all expressions.

https://kotlinlang.org/docs/reference/grammar.html#expression gives some insights, but it can not be simply match to the tokens either.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some more searching, and found a lot more examples of public TokenSet instances in the kotlin repository. I regret not looking harder before. I'm going to try to see if I can replace my token sets with established ones from the kotlin repistory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is what I did:

  • For COMMENTS, I was able to use one from the kotlin compiler directly
  • For CONTROL_FLOW_KEYWORDS, I was able to copy and paste a subset of KotlinExpressionParsing.EXPRESSION_FIRST. This added a few, but still all tests pass. Should be more maintainable.
  • For EXPRESSIONS, I could not find anything even close that could serve as a reference. So instead, I asked "why do we need this anyway if aparently nobody else has ever needed it?". I found the one usage (in NoUnusedImportsRule), removed it, checked all tests pass, and then removed TokenSets.EXPRESSIONS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I was editing NoUnusedImportsRule I tried to pick up a conceptual understanding of why this token set was being used in parentCallExpressionOrNull, but I could not figure it out.

TokenSet.create(
ElementType.LAMBDA_EXPRESSION,
ElementType.FUNCTION_LITERAL,
ElementType.ANNOTATED_EXPRESSION,
ElementType.REFERENCE_EXPRESSION,
ElementType.ENUM_ENTRY_SUPERCLASS_REFERENCE_EXPRESSION,
ElementType.OPERATION_REFERENCE,
ElementType.LABEL,
ElementType.LABEL_QUALIFIER,
ElementType.THIS_EXPRESSION,
ElementType.SUPER_EXPRESSION,
ElementType.BINARY_EXPRESSION,
ElementType.BINARY_WITH_TYPE,
ElementType.IS_EXPRESSION,
ElementType.PREFIX_EXPRESSION,
ElementType.POSTFIX_EXPRESSION,
ElementType.LABELED_EXPRESSION,
ElementType.CALL_EXPRESSION,
ElementType.ARRAY_ACCESS_EXPRESSION,
ElementType.INDICES,
ElementType.DOT_QUALIFIED_EXPRESSION,
ElementType.CALLABLE_REFERENCE_EXPRESSION,
ElementType.CLASS_LITERAL_EXPRESSION,
ElementType.SAFE_ACCESS_EXPRESSION,
ElementType.OBJECT_LITERAL,
ElementType.WHEN,
ElementType.COLLECTION_LITERAL_EXPRESSION,
ElementType.TYPE_CODE_FRAGMENT,
ElementType.EXPRESSION_CODE_FRAGMENT,
)
}
18 changes: 10 additions & 8 deletions ...ule-engine/src/main/kotlin/com/pinterest/ktlint/rule/engine/internal/KtlintSuppression.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import com.pinterest.ktlint.rule.engine.core.api.isPartOfComment
import com.pinterest.ktlint.rule.engine.core.api.isRoot
import com.pinterest.ktlint.rule.engine.core.api.isWhiteSpace
import com.pinterest.ktlint.rule.engine.core.api.nextCodeSibling
import com.pinterest.ktlint.rule.engine.core.api.recursiveChildren
import com.pinterest.ktlint.rule.engine.core.api.replaceWith
import org.jetbrains.kotlin.com.intellij.lang.ASTNode
import org.jetbrains.kotlin.com.intellij.psi.PsiElement
Expand All @@ -30,7 +31,6 @@ import org.jetbrains.kotlin.psi.KtBinaryExpression
import org.jetbrains.kotlin.psi.KtBlockExpression
import org.jetbrains.kotlin.psi.KtClass
import org.jetbrains.kotlin.psi.KtClassInitializer
import org.jetbrains.kotlin.psi.KtCollectionLiteralExpression
import org.jetbrains.kotlin.psi.KtDeclaration
import org.jetbrains.kotlin.psi.KtDeclarationModifierList
import org.jetbrains.kotlin.psi.KtExpression
Expand All @@ -47,7 +47,6 @@ import org.jetbrains.kotlin.psi.KtScript
import org.jetbrains.kotlin.psi.KtScriptInitializer
import org.jetbrains.kotlin.psi.KtStringTemplateExpression
import org.jetbrains.kotlin.psi.psiUtil.children
import org.jetbrains.kotlin.psi.psiUtil.findDescendantOfType
import org.jetbrains.kotlin.psi.psiUtil.getChildOfType
import org.jetbrains.kotlin.util.prefixIfNot

Expand Down Expand Up @@ -202,12 +201,15 @@ private fun ASTNode.existingSuppressions() =
existingSuppressionsFromNamedArgumentOrNull()
?: getValueArguments()

private fun ASTNode.existingSuppressionsFromNamedArgumentOrNull() =
psi
.findDescendantOfType<KtCollectionLiteralExpression>()
?.children
?.map { it.text }
?.toSet()
private fun ASTNode.existingSuppressionsFromNamedArgumentOrNull(): Set<String>? =
recursiveChildren()
.firstOrNull { it.elementType == ElementType.COLLECTION_LITERAL_EXPRESSION }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of adding:

public fun ASTNode.findChildByTypeRecursively(
    elementType: IElementType,
    includeSelf: Boolean,
): ASTNode? = recursiveChildren(includeSelf).firstOrNull { it.elementType == elementType }

so that this can be written as:

        findChildByTypeRecursively(ElementType.COLLECTION_LITERAL_EXPRESSION)

which nicely aligns with the findChildByType(ElementType) function of the embedded Kotlin compiler?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I was thinking of adding a function like this.

Can we leave it without a kdoc since the name is self explanatory? I feel like a kdoc would say "Finds a child with type elementType recursively", which is pretty redundant with the name of the function.

I notice a bunch of other public functions in this file don't have a kdoc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test class

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we leave it without a kdoc since the name is self explanatory? I feel like a kdoc would say "Finds a child with type elementType recursively", which is pretty redundant with the name of the function.

I notice a bunch of other public functions in this file don't have a kdoc.

True, I was aware of the latter. For Ktlint contributors it would suffice, to not document. But those extensions are also meant to be used by external rule providers which might not be familiar with ktlint that much. By providing the API docs, we make it slightly better for them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is now a kdoc for findChildByTypeRecursively.

?.run {
children()
.filter { it.elementType == ElementType.STRING_TEMPLATE }
.map { it.text }
.toSet()
}
paul-dingemans marked this conversation as resolved.
Show resolved Hide resolved

private fun ASTNode.findSuppressionAnnotations(): Map<SuppressAnnotationType, ASTNode> =
if (this.isRoot()) {
Expand Down
115 changes: 61 additions & 54 deletions ...le-engine/src/main/kotlin/com/pinterest/ktlint/rule/engine/internal/SuppressionLocator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,19 @@
package com.pinterest.ktlint.rule.engine.internal

import com.pinterest.ktlint.rule.engine.core.api.ElementType
import com.pinterest.ktlint.rule.engine.core.api.ElementType.RBRACE
import com.pinterest.ktlint.rule.engine.core.api.IgnoreKtlintSuppressions
import com.pinterest.ktlint.rule.engine.core.api.Rule
import com.pinterest.ktlint.rule.engine.core.api.RuleId
import com.pinterest.ktlint.rule.engine.core.api.TokenSets
import com.pinterest.ktlint.rule.engine.core.api.editorconfig.EditorConfig
import com.pinterest.ktlint.rule.engine.core.api.nextSibling
import com.pinterest.ktlint.rule.engine.core.api.parent
import com.pinterest.ktlint.rule.engine.core.api.recursiveChildren
import com.pinterest.ktlint.rule.engine.internal.SuppressionLocator.CommentSuppressionHint.Type.BLOCK_END
import com.pinterest.ktlint.rule.engine.internal.SuppressionLocator.CommentSuppressionHint.Type.BLOCK_START
import org.jetbrains.kotlin.com.intellij.lang.ASTNode
import org.jetbrains.kotlin.com.intellij.psi.PsiComment
import org.jetbrains.kotlin.psi.KtAnnotated
import org.jetbrains.kotlin.psi.KtAnnotationEntry
import org.jetbrains.kotlin.psi.ValueArgument
import org.jetbrains.kotlin.psi.psiUtil.endOffset
import org.jetbrains.kotlin.psi.psiUtil.startOffset
import org.jetbrains.kotlin.com.intellij.psi.tree.TokenSet

internal class SuppressionLocator(
editorConfig: EditorConfig,
Expand Down Expand Up @@ -57,34 +56,27 @@ internal class SuppressionLocator(
private fun findSuppressionHints(rootNode: ASTNode): List<SuppressionHint> {
val suppressionHints = ArrayList<SuppressionHint>()
val commentSuppressionsHints = mutableListOf<CommentSuppressionHint>()
rootNode.findSuppressionHints { node ->
when (val psi = node.psi) {
is PsiComment -> {
rootNode.recursiveChildren(includeSelf = true).forEach { node ->
val eType = node.elementType
when {
TokenSets.COMMENTS.contains(eType) -> {
node
.createSuppressionHintFromComment()
?.let { commentSuppressionsHints.add(it) }
}

is KtAnnotated -> {
psi
.createSuppressionHintFromAnnotations()
?.let { suppressionHints.add(it) }
eType == ElementType.ANNOTATION_ENTRY -> {
createSuppressionHintFromAnnotations(
annotation = node,
)?.let { suppressionHints.add(it) }
}
}
paul-dingemans marked this conversation as resolved.
Show resolved Hide resolved
}

return suppressionHints.plus(
commentSuppressionsHints.toSuppressionHints(),
)
}

private fun ASTNode.findSuppressionHints(block: (node: ASTNode) -> Unit) {
block(this)
this
.getChildren(null)
.forEach { it.findSuppressionHints(block) }
}

private fun ASTNode.createSuppressionHintFromComment(): CommentSuppressionHint? =
text
.removePrefix("//")
Expand Down Expand Up @@ -178,42 +170,41 @@ internal class SuppressionLocator(

private fun <T> List<T>.tail() = this.subList(1, this.size)

private fun KtAnnotated.createSuppressionHintFromAnnotations(): SuppressionHint? =
annotationEntries
.filter {
it
.calleeExpression
?.constructorReferenceExpression
?.getReferencedName() in SUPPRESS_ANNOTATIONS
}.flatMap(KtAnnotationEntry::getValueArguments)
.flatMap { it.findRuleSuppressionIds() }
.let { suppressedRuleIds ->
when {
suppressedRuleIds.isEmpty() -> {
null
}

suppressedRuleIds.contains(ALL_KTLINT_RULES_SUPPRESSION_ID) -> {
SuppressionHint(
IntRange(startOffset, endOffset - 1),
emptySet(),
)
}
private fun createSuppressionHintFromAnnotations(annotation: ASTNode): SuppressionHint? {
paul-dingemans marked this conversation as resolved.
Show resolved Hide resolved
if (annotation
.findChildByType(ElementType.CONSTRUCTOR_CALLEE)
?.findChildByType(ElementType.TYPE_REFERENCE)
?.text !in SUPPRESS_ANNOTATIONS
) {
return null
}
paul-dingemans marked this conversation as resolved.
Show resolved Hide resolved

else -> {
SuppressionHint(
IntRange(startOffset, endOffset - 1),
suppressedRuleIds.toSet(),
)
}
val suppressedRuleIds =
annotation
.recursiveChildren()
.filter { it.elementType == ElementType.VALUE_ARGUMENT }
.flatMapTo(mutableListOf()) {
it.text.findRuleSuppressionIds()
}
}

private fun ValueArgument.findRuleSuppressionIds(): List<String> =
getArgumentExpression()
?.text
?.removeSurrounding("\"")
?.let { argumentExpressionText ->
if (suppressedRuleIds.isEmpty()) return null

val owner =
annotation.parent {
ANNOTATED_ELEMENT_TYPES.contains(it.elementType)
} ?: return null
paul-dingemans marked this conversation as resolved.
Show resolved Hide resolved

val tr = owner.textRange
paul-dingemans marked this conversation as resolved.
Show resolved Hide resolved

return SuppressionHint(
IntRange(tr.startOffset, tr.endOffset - 1),
if (suppressedRuleIds.contains(ALL_KTLINT_RULES_SUPPRESSION_ID)) emptySet() else suppressedRuleIds.toSet(),
paul-dingemans marked this conversation as resolved.
Show resolved Hide resolved
)
}

private fun String.findRuleSuppressionIds(): List<String> =
removeSurrounding("\"")
.let { argumentExpressionText ->
paul-dingemans marked this conversation as resolved.
Show resolved Hide resolved
when {
argumentExpressionText == "ktlint" -> {
// Disable all rules
Expand Down Expand Up @@ -280,5 +271,21 @@ internal class SuppressionLocator(
)
val SUPPRESS_ANNOTATIONS = setOf("Suppress", "SuppressWarnings")
const val ALL_KTLINT_RULES_SUPPRESSION_ID = "ktlint:suppress-all-rules"
val ANNOTATED_ELEMENT_TYPES =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this token set based on information from the Psi Interface? If so, please add comment with references/links. If tokes are added or removed in the future from the Psi Interface, this makes it easier to keep our token sets in sync with Psi.

What would be your policy for creating such token sets inside the TokenSets.kt or within a class as long as there is only one usage? When this token set is derived from Psi, I am inclined to define it inside TokenSets.kt.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, as with the other token sets I created I do not think that this was based on another source such as psi. I basically figured it out by adding element types until all tests passed. Sorry I did not say this earlier, as this is the biggest weak spot of this PR in terms of possible bugs and maintainability.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be your policy for creating such token sets inside the TokenSets.kt or within a class as long as there is only one usage? When this token set is derived from Psi, I am inclined to define it inside TokenSets.kt.

Personally, I would lean towards centralizing all token sets into TokenSets.kt. Based on the times I have tried to skim Psi for token sets, I don't think I found much of use. So I am not sure if "being derived from psi" would be a realistic standard as many or even most of ktlint's token sets might have to be original.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we may need to give psi a closer look. I only skimmed it lightly and could be wrong.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, as with the other token sets I created I do not think that this was based on another source such as psi. I basically figured it out by adding element types until all tests passed. Sorry I did not say this earlier, as this is the biggest weak spot of this PR in terms of possible bugs and maintainability.

As said before, this is indeed concerning. In order to test this we would need some big code bases that do not use ktlint. Next we could format both code bases with ktlint version without your changeset, and once more with another version including your changeset. Both versions should provide the same results. Doing this with code bases which are already formatted with ktlint might not reveal all problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to test this we would need some big code bases that do not use ktlint. Next we could format both code bases with ktlint version without your changeset, and once more with another version including your changeset. Both versions should provide the same results

This is an interesting idea. Maybe this should be integrated into the project as a test in general? A single "integration" test that does what you describe might be heavy but probably worthwhile so that refactorings like this are safer.

I'm imagining a full large, unformatted kotlin project sitting inside of a test resources folder. Another file adjacent to the root of that folder could be a csv where one column is the relative path of each file and another file is the hash of the expected file contents after formatting. I think you might see where I'm going with this?

Copy link
Contributor Author

@mgroth0 mgroth0 Dec 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd personally like to avoid having to imlpement such a test though, only because it might take an uncomfortably long time to develop (though I think it could be a great addition to this project, if you feel you have the time). Instead, I have explored a different solution to making this safer and more maintainble. See my commit "alternative to token set" where I do something a bit hacky, but that actually might be more safe and maintainable. That is, to create dummy psi elements from ASTNode and then using is checking as before (like is KtAnnotated). I'm not 100% sure this is performant, but I would think it likely is since the dummy elements can be cached. If you like this strategy, it might be generalizable to other places as well as an alternative to making custom TokenSet instances.

TokenSet.create(
ElementType.CLASS,
ElementType.OBJECT_DECLARATION,
ElementType.ENUM_ENTRY,
ElementType.FUN,
ElementType.PROPERTY,
ElementType.PROPERTY_ACCESSOR,
ElementType.TYPE_PARAMETER,
ElementType.TYPEALIAS,
ElementType.PRIMARY_CONSTRUCTOR,
ElementType.SECONDARY_CONSTRUCTOR,
ElementType.ANNOTATED_EXPRESSION,
ElementType.EXPRESSION_CODE_FRAGMENT,
ElementType.FILE,
)
}
}
11 changes: 6 additions & 5 deletions .../src/main/kotlin/com/pinterest/ktlint/rule/engine/internal/rules/KtlintSuppressionRule.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import com.pinterest.ktlint.rule.engine.core.api.nextLeaf
import com.pinterest.ktlint.rule.engine.core.api.nextSibling
import com.pinterest.ktlint.rule.engine.core.api.parent
import com.pinterest.ktlint.rule.engine.core.api.prevLeaf
import com.pinterest.ktlint.rule.engine.core.api.recursiveChildren
import com.pinterest.ktlint.rule.engine.core.api.remove
import com.pinterest.ktlint.rule.engine.core.api.replaceWith
import com.pinterest.ktlint.rule.engine.internal.KTLINT_SUPPRESSION_ID_ALL_RULES
Expand All @@ -42,9 +43,9 @@ import org.jetbrains.kotlin.psi.KtScriptInitializer
import org.jetbrains.kotlin.psi.KtStringTemplateExpression
import org.jetbrains.kotlin.psi.KtValueArgument
import org.jetbrains.kotlin.psi.KtValueArgumentList
import org.jetbrains.kotlin.psi.psiUtil.findDescendantOfType
import org.jetbrains.kotlin.psi.psiUtil.getChildOfType
import org.jetbrains.kotlin.psi.psiUtil.siblings
import org.jetbrains.kotlin.psi.psiUtil.startOffset
import org.jetbrains.kotlin.utils.addToStdlib.applyIf

/**
Expand Down Expand Up @@ -105,10 +106,10 @@ public class KtlintSuppressionRule(
emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> AutocorrectDecision,
) {
node
.psi
.findDescendantOfType<KtLiteralStringTemplateEntry>()
?.node
?.let { literalStringTemplateEntry ->
.recursiveChildren(includeSelf = true)
.firstOrNull {
it.elementType == ElementType.LITERAL_STRING_TEMPLATE_ENTRY
}?.let { literalStringTemplateEntry ->
paul-dingemans marked this conversation as resolved.
Show resolved Hide resolved
val prefixedSuppression =
literalStringTemplateEntry
.text
Expand Down
Loading
Loading