Improving lintting/diagnostic reporting - ability to cancel the progress, ensure stale data are not recorded SourceFile(s), always start linting on change.

jlahoda · jlahoda · commit c1f4a1d3ef2e · 2020-01-05T23:25:21.000+01:00
diff --git a/server/src/main/kotlin/org/javacs/kt/KotlinTextDocumentService.kt b/server/src/main/kotlin/org/javacs/kt/KotlinTextDocumentService.kt
@@ -42,7 +42,6 @@ class KotlinTextDocumentService(
 ) : TextDocumentService, Closeable {
     private lateinit var client: LanguageClient
     private val async = AsyncExecutor()
-    private var linting = false
 
     var debounceLint = Debouncer(Duration.ofMillis(config.linting.debounceTime))
     val lintTodo = mutableSetOf<URI>()
@@ -65,7 +64,7 @@ class KotlinTextDocumentService(
         get() = uriContentProvider.contentOf(parseURI(uri))
 
     private enum class Recompile {
-        ALWAYS, AFTER_DOT, WAIT_FOR_LINT, NEVER
+        ALWAYS, AFTER_DOT, NEVER
     }
 
     private fun recover(position: TextDocumentPositionParams, recompile: Recompile): Pair<CompiledFile, Int> {
@@ -75,10 +74,6 @@ class KotlinTextDocumentService(
         val shouldRecompile = when (recompile) {
             Recompile.ALWAYS -> true
             Recompile.AFTER_DOT -> offset > 0 && content[offset - 1] == '.'
-            Recompile.WAIT_FOR_LINT -> {
-                debounceLint.waitForPendingTask()
-                false
-            }
             Recompile.NEVER -> false
         }
         val compiled = if (shouldRecompile) sp.currentVersion(uri) else sp.latestCompiledVersion(uri)
@@ -254,27 +249,22 @@ class KotlinTextDocumentService(
 
     private fun lintLater(uri: URI) {
         lintTodo.add(uri)
-        if (!linting) {
-            debounceLint.submit(::doLint)
-        }
+        debounceLint.submit(::doLint)
     }
 
     private fun lintNow(file: URI) {
         lintTodo.add(file)
         debounceLint.submitImmediately(::doLint)
     }
 
-    private fun doLint() {
-        linting = true
-        try {
-            LOG.info("Linting {}", describeURIs(lintTodo))
-            val files = clearLint()
-            val context = sp.compileFiles(files)
+    private fun doLint(cancelCallback : () -> Boolean) {
+        LOG.info("Linting {}", describeURIs(lintTodo))
+        val files = clearLint()
+        val context = sp.compileFiles(files)
+        if (!cancelCallback.invoke()) {
             reportDiagnostics(files, context.diagnostics)
-            lintCount++
-        } finally {
-            linting = false
         }
+        lintCount++
     }
 
     private fun reportDiagnostics(compiled: Collection<URI>, kotlinDiagnostics: Diagnostics) {
@@ -312,6 +302,10 @@ class KotlinTextDocumentService(
     override fun close() {
         shutdownExecutors(awaitTermination = true)
     }
+
+    fun setTestLintRecompilationCallback(callback: () -> Unit) {
+        sp.beforeCompileCallback = callback;
+    }
 }
 
 private inline fun<T> reportTime(block: () -> T): T {
diff --git a/server/src/main/kotlin/org/javacs/kt/SourcePath.kt b/server/src/main/kotlin/org/javacs/kt/SourcePath.kt
@@ -6,15 +6,18 @@ import org.jetbrains.kotlin.container.ComponentProvider
 import org.jetbrains.kotlin.psi.KtFile
 import org.jetbrains.kotlin.resolve.BindingContext
 import org.jetbrains.kotlin.resolve.CompositeBindingContext
+import kotlin.concurrent.withLock
 import java.nio.file.Path
 import java.nio.file.Paths
 import java.net.URI
+import java.util.concurrent.locks.ReentrantLock
 
 class SourcePath(
     private val cp: CompilerClassPath,
     private val contentProvider: URIContentProvider
 ) {
     private val files = mutableMapOf<URI, SourceFile>()
+    private val parseDataWriteLock = ReentrantLock()
 
     private inner class SourceFile(
         val uri: URI,
@@ -60,9 +63,11 @@ class SourcePath(
                 LOG.debug("Compiling {}", path?.fileName)
 
                 val (context, container) = cp.compiler.compileFile(parsed!!, allIncludingThis(), kind)
-                compiledContext = context
-                compiledContainer = container
-                compiledFile = parsed
+                parseDataWriteLock.withLock {
+                    compiledContext = context
+                    compiledContainer = container
+                    compiledFile = parsed
+                }
             }
 
             return this
@@ -136,6 +141,8 @@ class SourcePath(
     fun latestCompiledVersion(uri: URI): CompiledFile =
             sourceFile(uri).prepareCompiledFile()
 
+    var beforeCompileCallback: () -> Unit = { };
+
     /**
      * Compile changed files
      */
@@ -148,14 +155,21 @@ class SourcePath(
         // Compile changed files
         fun compileAndUpdate(changed: List<SourceFile>, kind: CompilationKind): BindingContext? {
             if (changed.isEmpty()) return null
-            val parse = changed.map { it.parseIfChanged().parsed!! }
-            val (context, container) = cp.compiler.compileFiles(parse, all(), kind)
+            val parse = changed.associateWith { it.parseIfChanged().parsed!! }
+            val all = all()
+            beforeCompileCallback.invoke()
+            val (context, container) = cp.compiler.compileFiles(parse.values, all, kind)
 
             // Update cache
-            for (f in changed) {
-                f.compiledFile = f.parsed
-                f.compiledContext = context
-                f.compiledContainer = container
+            for ((f, parsed) in parse) {
+                parseDataWriteLock.withLock {
+                    if (f.parsed == parsed) {
+                        //only updated if the parsed file didn't change:
+                        f.compiledFile = parsed
+                        f.compiledContext = context
+                        f.compiledContainer = container
+                    }
+                }
             }
 
             return context
diff --git a/server/src/test/kotlin/org/javacs/kt/DebouncerTest.kt b/server/src/test/kotlin/org/javacs/kt/DebouncerTest.kt
@@ -12,7 +12,7 @@ class DebouncerTest {
 
     @Test fun callQuickly() {
         for (i in 1..10) {
-            debounce.submit {
+            debounce.submit { ->
                 counter++
             }
         }
diff --git a/server/src/test/kotlin/org/javacs/kt/LintTest.kt b/server/src/test/kotlin/org/javacs/kt/LintTest.kt
@@ -1,6 +1,10 @@
 package org.javacs.kt
 
+import java.util.concurrent.CancellationException
 import org.eclipse.lsp4j.Diagnostic
+import org.eclipse.lsp4j.DocumentSymbolParams
+import org.eclipse.lsp4j.PublishDiagnosticsParams
+import org.eclipse.lsp4j.TextDocumentIdentifier
 import org.hamcrest.Matchers.*
 import org.junit.Assert.assertThat
 import org.junit.Test
@@ -26,4 +30,27 @@ class LintTest : SingleFileTestFixture("lint", "LintErrors.kt") {
         assertThat(diagnostics, not(empty<Diagnostic>()))
         assertThat(languageServer.textDocumentService.lintCount, lessThan(5))
     }
+
+    @Test fun `lintting should not be dropped if another lintting is running`() {
+        var callbackCount = 0;
+        languageServer.textDocumentService.debounceLint.waitForPendingTask()
+        languageServer.textDocumentService.setTestLintRecompilationCallback({
+            if (callbackCount++ == 0) {
+                diagnostics.clear()
+                replace(file, 3, 9, "return 11", "")
+                languageServer.textDocumentService.documentSymbol(DocumentSymbolParams(TextDocumentIdentifier(uri(file).toString()))).get()
+            }
+        })
+        replace(file, 3, 16, "", "1")
+
+        while (callbackCount < 2) {
+            try {
+                languageServer.textDocumentService.debounceLint.waitForPendingTask()
+            } catch (ex: CancellationException) {}
+        }
+
+        languageServer.textDocumentService.debounceLint.waitForPendingTask()
+        assertThat(diagnostics, empty<Diagnostic>())
+        languageServer.textDocumentService.setTestLintRecompilationCallback({})
+    }
 }
diff --git a/shared/src/main/kotlin/org/javacs/kt/util/Debouncer.kt b/shared/src/main/kotlin/org/javacs/kt/util/Debouncer.kt
@@ -3,6 +3,7 @@ package org.javacs.kt.util
 import org.javacs.kt.LOG
 import java.time.Duration
 import java.util.function.Supplier
+import java.util.concurrent.atomic.AtomicReference
 import java.util.concurrent.ScheduledExecutorService
 import java.util.concurrent.Executors
 import java.util.concurrent.TimeUnit
@@ -25,11 +26,27 @@ class Debouncer(
         pendingTask = executor.submit(task)
     }
 
+    fun submitImmediately(task: (cancelCallback : () -> Boolean) -> Unit) {
+        pendingTask?.cancel(false)
+        val currentTaskRef = AtomicReference<Future<*>>()
+        val currentTask = executor.submit({ -> task.invoke({ -> val f = currentTaskRef.get(); f?.isCancelled()?:false})})
+        currentTaskRef.set(currentTask);
+        pendingTask = currentTask
+    }
+
     fun submit(task: () -> Unit) {
         pendingTask?.cancel(false)
         pendingTask = executor.schedule(task, delayMs, TimeUnit.MILLISECONDS)
     }
 
+    fun submit(task: (cancelCallback : () -> Boolean) -> Unit) {
+        pendingTask?.cancel(false)
+        val currentTaskRef = AtomicReference<Future<*>>()
+        val currentTask = executor.schedule({ -> task.invoke({ -> val f = currentTaskRef.get(); f?.isCancelled()?:false})}, delayMs, TimeUnit.MILLISECONDS)
+        currentTaskRef.set(currentTask);
+        pendingTask = currentTask
+    }
+
     fun waitForPendingTask() {
         pendingTask?.get()
     }

Original file line number	Diff line number	Diff line change
`@@ -12,7 +12,7 @@ class DebouncerTest {`
`12`	`12`
`13`	`13`	`@Test fun callQuickly() {`
`14`	`14`	`for (i in 1..10) {`
`15`		`- debounce.submit {`
	`15`	`+ debounce.submit { ->`
`16`	`16`	`counter++`
`17`	`17`	`}`
`18`	`18`	`}`