From 1e2c35be92851f9957fbec481567a3c6aabe2e87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Kondratek?= Date: Tue, 10 Sep 2024 10:13:37 +0200 Subject: [PATCH] Fix IndexOutOfBounds from the highlight pass (#2252) ## Test plan Try these examples: - https://github.com/sourcegraph/jetbrains/issues/2110 - https://github.com/sourcegraph/jetbrains/issues/1988 --- .../agent/intellij_extensions/Document.kt | 8 ++ .../cody/inspections/CodyFixHighlightPass.kt | 116 +++++++++--------- .../com/sourcegraph/utils/CodyFormatter.kt | 6 +- .../com/sourcegraph/utils/ThreadingUtil.kt | 19 +++ 4 files changed, 89 insertions(+), 60 deletions(-) create mode 100644 src/main/kotlin/com/sourcegraph/utils/ThreadingUtil.kt diff --git a/src/main/kotlin/com/sourcegraph/cody/agent/intellij_extensions/Document.kt b/src/main/kotlin/com/sourcegraph/cody/agent/intellij_extensions/Document.kt index 279619aaf3..f2baaadcae 100644 --- a/src/main/kotlin/com/sourcegraph/cody/agent/intellij_extensions/Document.kt +++ b/src/main/kotlin/com/sourcegraph/cody/agent/intellij_extensions/Document.kt @@ -13,6 +13,14 @@ fun Document.codyPosition(offset: Int): Position { } fun Document.codyRange(startOffset: Int, endOffset: Int): Range { + if (startOffset < 0 || + startOffset > this.textLength || + endOffset > this.textLength || + startOffset > endOffset) { + throw IllegalArgumentException( + "codyRange error - startOffset: $startOffset, endOffset: $endOffset, textLength: ${this.textLength}") + } + val startLine = this.getLineNumber(startOffset) val lineStartOffset1 = this.getLineStartOffset(startLine) val startCharacter = startOffset - lineStartOffset1 diff --git a/src/main/kotlin/com/sourcegraph/cody/inspections/CodyFixHighlightPass.kt b/src/main/kotlin/com/sourcegraph/cody/inspections/CodyFixHighlightPass.kt index 911c63ec4f..2fb136c6d0 100644 --- a/src/main/kotlin/com/sourcegraph/cody/inspections/CodyFixHighlightPass.kt +++ b/src/main/kotlin/com/sourcegraph/cody/inspections/CodyFixHighlightPass.kt @@ -9,26 +9,23 @@ import com.intellij.codeInsight.daemon.DaemonCodeAnalyzer import com.intellij.codeInsight.daemon.impl.DaemonCodeAnalyzerImpl import com.intellij.codeInsight.daemon.impl.HighlightInfo import com.intellij.lang.annotation.HighlightSeverity -import com.intellij.openapi.application.ApplicationManager import com.intellij.openapi.diagnostic.Logger import com.intellij.openapi.editor.Editor import com.intellij.openapi.progress.ProgressIndicator import com.intellij.openapi.progress.util.ProgressIndicatorUtils import com.intellij.openapi.project.Project import com.intellij.psi.PsiFile -import com.intellij.util.concurrency.AppExecutorUtil +import com.intellij.util.concurrency.annotations.RequiresEdt import com.sourcegraph.cody.agent.CodyAgentService import com.sourcegraph.cody.agent.intellij_extensions.codyRange -import com.sourcegraph.cody.agent.protocol.ProtocolTextDocument +import com.sourcegraph.cody.agent.protocol.ProtocolTextDocument.Companion.uriFor import com.sourcegraph.cody.agent.protocol_generated.CodeActions_ProvideParams import com.sourcegraph.cody.agent.protocol_generated.Diagnostics_PublishParams import com.sourcegraph.cody.agent.protocol_generated.ProtocolDiagnostic import com.sourcegraph.cody.agent.protocol_generated.ProtocolLocation import com.sourcegraph.cody.agent.protocol_generated.Range +import com.sourcegraph.utils.ThreadingUtil import java.util.concurrent.CompletableFuture -import java.util.concurrent.TimeUnit -import java.util.concurrent.TimeoutException -import java.util.concurrent.atomic.AtomicReference class CodyFixHighlightPass(val file: PsiFile, val editor: Editor) : TextEditorHighlightingPass(file.project, editor.document, false) { @@ -37,35 +34,13 @@ class CodyFixHighlightPass(val file: PsiFile, val editor: Editor) : private var myHighlights = emptyList() private val myRangeActions = mutableMapOf>() - // We are invoked by the superclass on a daemon/worker thread, but we need the EDT - // for this and perhaps other things (e.g. Document.codyRange). So we set things up - // to block the caller and fetch what we need on the EDT. - private val protocolTextDocumentFuture: CompletableFuture = - CompletableFuture.supplyAsync( - { - val result = AtomicReference() - ApplicationManager.getApplication().invokeAndWait { - result.set(ProtocolTextDocument.fromVirtualFile(file.virtualFile)) - } - result.get() - }, - AppExecutorUtil.getAppExecutorService()) - override fun doCollectInformation(progress: ProgressIndicator) { - // Fetch the protocol text document on the EDT. - val protocolTextDocument = - try { - protocolTextDocumentFuture.get(5, TimeUnit.SECONDS) - } catch (e: TimeoutException) { - logger.warn("Timeout fetching protocol text document") - return - } - if (!DaemonCodeAnalyzer.getInstance(file.project).isHighlightingAvailable(file) || progress.isCanceled) { // wait until after code-analysis is completed return } + val uri = uriFor(file.virtualFile) myRangeActions.clear() @@ -73,48 +48,68 @@ class CodyFixHighlightPass(val file: PsiFile, val editor: Editor) : DaemonCodeAnalyzerImpl.getHighlights(editor.document, HighlightSeverity.ERROR, file.project) val protocolDiagnostics = - myHighlights - // TODO: We need to check how Enum comparison works to check if we can do things like >= - // HighlightSeverity.INFO - .filter { it.severity == HighlightSeverity.ERROR } - .mapNotNull { - try { - ProtocolDiagnostic( - message = it.description, - severity = - "error", // TODO: Wait for CODY-2882. This isn't currently used by the - // agent, - // so we just keep our lives simple. - location = - // TODO: Rik Nauta -- Got incorrect range; see QA report Aug 6 2024. - ProtocolLocation( - uri = protocolTextDocument.uri, - range = document.codyRange(it.startOffset, it.endOffset)), - code = it.problemGroup?.problemName) - } catch (x: Exception) { - // Don't allow range errors to throw user-visible exceptions (QA found this). - logger.warn("Failed to convert highlight to protocol diagnostic", x) - null + ThreadingUtil.runInEdtAndGet { + myHighlights + // TODO: We need to check how Enum comparison works to check if we can do things like + // >= + // HighlightSeverity.INFO + .asSequence() + .filter { it.severity == HighlightSeverity.ERROR } + .filter { it.startOffset <= document.textLength } + .filter { it.endOffset <= document.textLength } + .filter { it.startOffset <= it.endOffset } + .mapNotNull { + try { + ProtocolDiagnostic( + message = it.description, + severity = + "error", // TODO: Wait for CODY-2882. This isn't currently used by the + // agent, + // so we just keep our lives simple. + location = + // TODO: Rik Nauta -- Got incorrect range; see QA report Aug 6 2024. + ProtocolLocation( + uri = uri, range = document.codyRange(it.startOffset, it.endOffset)), + code = it.problemGroup?.problemName) + } catch (x: Exception) { + // Don't allow range errors to throw user-visible exceptions (QA found this). + logger.warn("Failed to convert highlight to protocol diagnostic", x) + null + } } - } + .toList() + } + + if (protocolDiagnostics.isEmpty()) { + return + } + val done = CompletableFuture() CodyAgentService.withAgentRestartIfNeeded(file.project) { agent -> try { agent.server.diagnostics_publish( Diagnostics_PublishParams(diagnostics = protocolDiagnostics)) + for (highlight in myHighlights) { if (progress.isCanceled) { break } - val range = document.codyRange(highlight.startOffset, highlight.endOffset) + + val range = + ThreadingUtil.runInEdtAndGet { + if (highlight.startOffset > document.textLength || + highlight.endOffset > document.textLength || + highlight.startOffset > highlight.endOffset) { + return@runInEdtAndGet null + } + + return@runInEdtAndGet document.codyRange(highlight.startOffset, highlight.endOffset) + } ?: break if (myRangeActions.containsKey(range)) { continue } - val location = - ProtocolLocation( - uri = protocolTextDocument.uri, - range = document.codyRange(highlight.startOffset, highlight.endOffset)) + val location = ProtocolLocation(uri = uri, range = range) val provideResponse = agent.server .codeActions_provide( @@ -133,10 +128,17 @@ class CodyFixHighlightPass(val file: PsiFile, val editor: Editor) : ProgressIndicatorUtils.awaitWithCheckCanceled(done, progress) } + @RequiresEdt override fun doApplyInformationToEditor() { for (highlight in myHighlights) { highlight.unregisterQuickFix { it.familyName == CodeActionQuickFix.FAMILY_NAME } + if (highlight.startOffset > document.textLength || + highlight.endOffset > document.textLength || + highlight.startOffset > highlight.endOffset) { + break + } + val range = document.codyRange(highlight.startOffset, highlight.endOffset) for (action in myRangeActions[range].orEmpty()) { highlight.registerFix( diff --git a/src/main/kotlin/com/sourcegraph/utils/CodyFormatter.kt b/src/main/kotlin/com/sourcegraph/utils/CodyFormatter.kt index e7f4a912ac..c41e9204cb 100644 --- a/src/main/kotlin/com/sourcegraph/utils/CodyFormatter.kt +++ b/src/main/kotlin/com/sourcegraph/utils/CodyFormatter.kt @@ -7,6 +7,7 @@ import com.intellij.openapi.project.Project import com.intellij.openapi.util.TextRange import com.intellij.psi.PsiFileFactory import com.intellij.psi.codeStyle.CodeStyleManager +import kotlin.math.max import kotlin.math.min class CodyFormatter { @@ -35,9 +36,8 @@ class CodyFormatter { .createFileFromText("TEMP", file.fileType, appendedString) val codeStyleManager = CodeStyleManager.getInstance(project) - // This will fail if cursor < range.startOffset + completionText.length - // TODO change the signature of this method or at least validate the arguments - codeStyleManager.reformatText(psiFile, cursor, range.startOffset + completionText.length) + val endOffset = max(cursor, range.startOffset + completionText.length) + codeStyleManager.reformatText(psiFile, cursor, endOffset) // Fix for the IJ formatting bug which removes spaces even before the given formatting // range. diff --git a/src/main/kotlin/com/sourcegraph/utils/ThreadingUtil.kt b/src/main/kotlin/com/sourcegraph/utils/ThreadingUtil.kt new file mode 100644 index 0000000000..6a2d427901 --- /dev/null +++ b/src/main/kotlin/com/sourcegraph/utils/ThreadingUtil.kt @@ -0,0 +1,19 @@ +package com.sourcegraph.utils + +import java.util.concurrent.CompletableFuture +import javax.swing.SwingUtilities.invokeAndWait + +object ThreadingUtil { + + fun runInEdtAndGet(task: () -> T): T { + val future = CompletableFuture() + invokeAndWait { + try { + future.complete(task()) + } catch (exception: Exception) { + future.completeExceptionally(exception) + } + } + return future.get() + } +}