saveourtool / diktat

Strict coding standard for Kotlin and a custom set of rules for detecting code smells, code style issues and bugs
https://diktat.saveourtool.com
MIT License
521 stars 39 forks source link

Investigate replaceWithText function #882

Closed kgevorkyan closed 11 months ago

kgevorkyan commented 3 years ago

Describe the bug

There is not the first time, when public LeafElement replaceWithText() from LeafElement.java causes the exceptions in diKTat

Seems, that it can break the structure of AST tree in some cases, therefore we can met exceptions. But this behavior should be investigated, and probably, we must refuse to use this function in diKTat.

Steps to Reproduce

E.g.: In function insertNewlinesBetweenBlocks from FileStructureRule change line with replaceChild to the

(this as LeafPsiElement).replaceWithText("\n\n${text.replace("\n", "")}")
petertrr commented 3 years ago

Another evidence: when diktat is executed with ktlint like ktlint -R diktat-0.6.1.jar -F smoke/src/main/kotlin/Example1Test.kt we also get this error, because ktlint mutates the tree:


[ERROR] 2021-06-23 11:52:49 Internal error has occurred in block-structure. Please make an issue on this bug at https://github.com/cqfn/diKTat/.
java.lang.NullPointerException
        at org.jetbrains.kotlin.com.intellij.psi.impl.source.codeStyle.CodeEditUtil.saveWhitespacesInfo(CodeEditUtil.java:122)
        at org.jetbrains.kotlin.com.intellij.psi.impl.source.tree.ChangeUtil.saveIndentationToCopy(ChangeUtil.java:106)
        at org.jetbrains.kotlin.com.intellij.psi.impl.source.tree.ChangeUtil.copyLeafWithText(ChangeUtil.java:82)
        at org.jetbrains.kotlin.com.intellij.psi.impl.source.tree.LeafElement.replaceWithText(LeafElement.java:154)
        at org.cqfn.diktat.ruleset.rules.chapter3.BlockStructureBraces$checkCloseBrace$2.invoke(BlockStructureBraces.kt:248)
        at org.cqfn.diktat.ruleset.rules.chapter3.BlockStructureBraces$checkCloseBrace$2.invoke(BlockStructureBraces.kt:244)

Possibe reason:

    PsiFile file = psiElement.getContainingFile();
    setOldIndentation((TreeElement)first, IndentHelper.getInstance().getIndent(file.getProject(), file.getFileType(), first));

where file is null

orchestr7 commented 3 years ago

The issue was in the replaceWithText method. It requires a special Indenting Service that saves original indenting in the node (useful in cases when you want just to change the functional part of the node without thinking about newlines and spaces).

But! if you will not provide this service during parsing (in ktlint for example), you will get an NPE (during the loading of the Service):

    public static void saveWhitespacesInfo(ASTNode first) {
        // if indentation was changed HERE <<<<
        if (first != null && !isNodeGenerated(first) && getOldIndentation(first) < 0) {
            PsiElement psiElement = first.getPsi();
            if (psiElement != null) {
                PsiFile file = psiElement.getContainingFile();
                // IndentHelper.getInstance() will be null here if we will not pass this service to kotlin compiler
                // and we will get NPE
                setOldIndentation((TreeElement)first, IndentHelper.getInstance().getIndent(file, first));
            }
        }
    }

Ktlint has already faced it in: https://github.com/pinterest/ktlint/commit/7fb5885c551ceea0329983dc1c15ce0993f1f6f9

orchestr7 commented 2 years ago

Need to remove this method from diktat and replace it with some more reliable logic.

nulls commented 11 months ago

already done, we use rawReplaceWithText