Skip to content

Fix IndexOutOfBounds from the highlight pass #2252

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

Merged
merged 8 commits into from
Sep 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially I wanted to bound the values here. But maybe we should not do that? We should be aware of what arguments we pass here.

"codyRange error - startOffset: $startOffset, endOffset: $endOffset, textLength: ${this.textLength}")
}

val startLine = this.getLineNumber(startOffset)
val lineStartOffset1 = this.getLineStartOffset(startLine)
val startCharacter = startOffset - lineStartOffset1
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -37,84 +34,82 @@ class CodyFixHighlightPass(val file: PsiFile, val editor: Editor) :
private var myHighlights = emptyList<HighlightInfo>()
private val myRangeActions = mutableMapOf<Range, List<CodeActionQuickFixParams>>()

// 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<ProtocolTextDocument> =
CompletableFuture.supplyAsync(
{
val result = AtomicReference<ProtocolTextDocument>()
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()

myHighlights =
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()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was possible that we have been sending an empty list 😅 that was completely redundant

return
}

val done = CompletableFuture<Unit>()
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(
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that running in the EDT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It runs in EDT. I added @RequiresEdt on it.

for (action in myRangeActions[range].orEmpty()) {
highlight.registerFix(
Expand Down
6 changes: 3 additions & 3 deletions src/main/kotlin/com/sourcegraph/utils/CodyFormatter.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Comment on lines +39 to +40
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated to highlight pass but I managed to get it once


// Fix for the IJ formatting bug which removes spaces even before the given formatting
// range.
Expand Down
19 changes: 19 additions & 0 deletions src/main/kotlin/com/sourcegraph/utils/ThreadingUtil.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package com.sourcegraph.utils

import java.util.concurrent.CompletableFuture
import javax.swing.SwingUtilities.invokeAndWait

object ThreadingUtil {

fun <T> runInEdtAndGet(task: () -> T): T {
val future = CompletableFuture<T>()
invokeAndWait {
try {
future.complete(task())
} catch (exception: Exception) {
future.completeExceptionally(exception)
}
}
return future.get()
}
}
Loading