matthewadams
commented
1 year ago @tnorbye Do you think you might be able to have a look at this, please?
Open matthewadams opened 1 year ago
matthewadams
commented
1 year ago @tnorbye Do you think you might be able to have a look at this, please?
tnorbye
commented
1 year ago Sorry, I somehow missed this bug when it was filed in March.
I just took a look -- and I can't repro this, and I've never seen it happen. And from the thread dump, this is the formatting processor which is called from IntelliJ's own formatting hook. The formatting hook does use PSI to manipulate the document -- and the error message is "Attempt to modify PSI for non-committed Document!". So in theory, the fix would be for the plugin to call PsiDocumentManager.getInstance(project).commitDocument(document) before performing the PSI manipulation.
But -- this should be done before formatting begins, not in the callback for an individual PSI element -- so I would think the formatting action itself should do it, not a formatting processor. And I looked at a handful of other formatting processors; all of them are doing PSI manipulation, and none of them are trying to do document manipulation.
Do you have any other third party plugins installed? I wonder if one of them is doing document manipulation without committing it back to PSI, leaving the document dirty and triggering the above error on the next PSI manipulation.
matthewadams
commented
1 year ago Sorry, took me a while to get back to this. It looks like the culprit is the ktlint plugin. I'll see about filing an issue there & referencing this one.
tnorbye
commented
1 year ago Great, thanks for chasing this down!
matthewadams
commented
1 year ago Here's the issue I filed: https://github.com/nbadal/ktlint-intellij-plugin/issues/323
paul-dingemans
commented
11 months ago Hi Tor,
I have been investigating this issue from the ktlint plugin perspective. Your suggestion about using PsiDocumentManager.getInstance(project).commitDocument(document) is indeed key to solving the issue. But I do believe that it needs to be fixed in the KDoc Formatter. I have created a reproducable scenario with two dummy formatters. One formatter modifies the PSI like KDoc Formatter does. The other formatter modifies the document by replacing the entire text of the document like the Ktlint plugin does.
The extensions are defined like this:
<extensions defaultExtensionNs="com.intellij">
<postFormatProcessor implementation="plugins.DummyKtlintPostFormatProcessor" />
<postFormatProcessor implementation="plugins.DummyKdocPostFormatProcessor1" />
</extensions>The order of the postFormatterProcessors is important for reproducing the problem. Only when DummyKdocPostFormatProcessor1 runs after DummyKtlintPostFormatProcessor the problem with Attempt to modify PSI for non-committed Document! occurs. In real live it is out of control of the developer which of the plugins in invoked before the others. So it is also important to test the scenario with the plugins in different order.
The DummyKtlintPostFormatProcessor prepends the existing content of the document with a comment // Formatted with DummyKtlintPostFormatProcessor and is defined as follows:
package plugins
import com.intellij.openapi.command.WriteCommandAction
import com.intellij.openapi.util.TextRange
import com.intellij.psi.PsiDocumentManager
import com.intellij.psi.PsiElement
import com.intellij.psi.PsiFile
import com.intellij.psi.codeStyle.CodeStyleSettings
import com.intellij.psi.impl.source.codeStyle.PostFormatProcessor
const val KTLINT_FORMATTER_COMMENT = "// Formatted with DummyKtlintPostFormatProcessor"
/**
* Like the ktlint-intellij-plugin this post format processor replaces the entire content of the document instead of
* manipulating the PSI.
*/
class DummyKtlintPostFormatProcessor : PostFormatProcessor {
val className = this::class.simpleName
override fun processElement(
source: PsiElement,
settings: CodeStyleSettings,
) = source
override fun processText(
psiFile: PsiFile,
rangeToReformat: TextRange,
settings: CodeStyleSettings,
): TextRange {
val document = psiFile.viewProvider.document
PsiDocumentManager
.getInstance(psiFile.project)
.doPostponedOperationsAndUnblockDocument(document)
WriteCommandAction.runWriteCommandAction(psiFile.project) {
println("$className start")
document.setText("$KTLINT_FORMATTER_COMMENT\n${psiFile.text}")
println("$className finished")
}
return rangeToReformat
}
}The DummyKdocPostFormatProcessor1 looks for a comment with text // Formatted with DummyKtlintPostFormatProcessor and alters that text. It is defined as follows and is comparable with current implementation of KDoc Formatter:
package plugins
import com.intellij.openapi.util.TextRange
import com.intellij.psi.PsiComment
import com.intellij.psi.PsiElement
import com.intellij.psi.PsiFile
import com.intellij.psi.codeStyle.CodeStyleSettings
import com.intellij.psi.impl.source.codeStyle.PostFormatProcessor
import com.intellij.psi.util.PsiTreeUtil
import org.jetbrains.kotlin.psi.KtPsiFactory
/**
* Like the current Kdoc Formatter this post format processor manipulates the PSI, but it does not commit document
* changes and does not use the WriteCommandAction while manipulating the PSI. As a result this post formatter throws an
* exception if it runs after [DummyKtlintPostFormatProcessor].
*/
class DummyKdocPostFormatProcessor1 : PostFormatProcessor {
val className = this::class.simpleName
override fun processElement(
source: PsiElement,
settings: CodeStyleSettings,
): PsiElement {
return if (source is PsiComment && source.text == KTLINT_FORMATTER_COMMENT) {
val newComment =
KtPsiFactory(source.project)
.createComment("$KTLINT_FORMATTER_COMMENT (altered by $className)")
source.replace(newComment)
} else {
source
}
}
override fun processText(
psiFile: PsiFile,
rangeToReformat: TextRange,
settings: CodeStyleSettings,
): TextRange {
println("$className start")
// Format all top-level comments in this range
for (element in PsiTreeUtil.findChildrenOfType(psiFile, PsiComment::class.java)) {
if (rangeToReformat.intersects(element.textRange)) {
processElement(element, settings)
}
}
println("$className start")
return rangeToReformat
}
}Now run the plugin with setup above and start with a simple document containing text:
class FooAfter the first invocation of reformatting the document, it will be changed to:
// Formatted with DummyKtlintPostFormatProcessor
class FooAt the second invocation of reformatting the document both the post formatters processors will be run and the Attempt to modify PSI for non-committed Document! exception is thrown.
When DummyKdocPostFormatProcessor1 runs after DummyKtlintPostFormatProcessor it has to ensure that the document does not contain any uncommitted changes before starting to change the PSI. Next to committing the document, it should also execute the PSI change within a WriteActionCommand. Replace DummyKdocPostFormatProcessor1 with DummyKdocPostFormatProcessor2 below:
package plugins
import com.intellij.openapi.command.WriteCommandAction
import com.intellij.openapi.util.TextRange
import com.intellij.psi.PsiComment
import com.intellij.psi.PsiDocumentManager
import com.intellij.psi.PsiElement
import com.intellij.psi.PsiFile
import com.intellij.psi.codeStyle.CodeStyleSettings
import com.intellij.psi.impl.source.codeStyle.PostFormatProcessor
import com.intellij.psi.util.PsiTreeUtil
import org.jetbrains.kotlin.psi.KtPsiFactory
/**
* Like the current Kdoc Formatter this post format processor manipulates the PSI, but it does only so after committing
* the document changes and uses the WriteCommandAction while manipulating the PSI. As a result this post formatter
* no longer throws an exception if it runs after [DummyKtlintPostFormatProcessor].
*/
class DummyKdocPostFormatProcessor2 : PostFormatProcessor {
val className = this::class.simpleName
override fun processElement(
source: PsiElement,
settings: CodeStyleSettings,
): PsiElement {
return if (source is PsiComment && source.text == KTLINT_FORMATTER_COMMENT) {
val newComment =
KtPsiFactory(source.project)
.createComment("$KTLINT_FORMATTER_COMMENT (altered by $className)")
source.replace(newComment)
} else {
source
}
}
override fun processText(
psiFile: PsiFile,
rangeToReformat: TextRange,
settings: CodeStyleSettings,
): TextRange {
PsiDocumentManager
.getInstance(psiFile.project)
.commitDocument(psiFile.viewProvider.document)
WriteCommandAction.runWriteCommandAction(psiFile.project) {
println("$className start")
// Format all top-level comments in this range
for (element in PsiTreeUtil.findChildrenOfType(psiFile, PsiComment::class.java)) {
if (rangeToReformat.intersects(element.textRange)) {
processElement(element, settings)
}
}
println("$className finished")
}
return rangeToReformat
}
}Rerun the plugin and start again with file:
class FooAfter the first run of reformatting the document, it is changed to:
// Formatted with DummyKtlintPostFormatProcessor (altered by DummyKdocPostFormatProcessor2)
class FooBtw, ktlint plugin has been changed as well to prevent problems in case the KDoc Formatter runs before Ktlint formatter.
paul-dingemans
commented
9 months ago @matthewadams Did you have any chance to look into this?
tnorbye
commented
8 months ago No, apologies still haven't had a chance -- very busy these days and it sounds like nobody is actually blocked since it sounds like you changed ktlint to work around this?
rmcmk
commented
6 months ago Our team is encountering this issue in a large multi-module project, which is quite frustrating. We've tried the latest versions of ktlint, but to no avail. If I can find some spare time this weekend, I plan to examine the reproduction provided by @paul-dingemans to gain a better understanding of the problem. Maybe I'll even submit a PR if I'm feeling real frisky.
I suspect this issue does not have more traction as this only happens if you have two Intellij formatters enabled and used in the same workspace. Most projects, including many of the other Kotlin projects we have internally, use a build system plugin or CI for linting/format corrections.
Ever since updating to the latest IntelliJ version, we've been getting the exception with root cause message
Attempt to modify PSI for non-committed Document!.IntelliJ version info:
Plugin version:
1.5.6Getting exception: