ADFA-5231: Publish document version and content together

editor/src/main/java/com/itsaky/androidide/editor/ui/IDEEditor.kt

@@ -104,12 +104,15 @@ import kotlinx.coroutines.cancel
 import kotlinx.coroutines.isActive
 import kotlinx.coroutines.launch
 import kotlinx.coroutines.suspendCancellableCoroutine
+import kotlinx.coroutines.sync.Mutex
+import kotlinx.coroutines.sync.withLock
 import kotlinx.coroutines.withContext
 import org.greenrobot.eventbus.EventBus
 import org.greenrobot.eventbus.Subscribe
 import org.greenrobot.eventbus.ThreadMode
 import org.slf4j.LoggerFactory
 import java.io.File
+import java.util.concurrent.atomic.AtomicInteger
 import kotlin.coroutines.resume
 
 fun interface OnEditorLongPressListener {
@@ -141,7 +144,17 @@ open class IDEEditor
 		private var actionsMenu: EditorActionsMenu? = null
 		private var _signatureHelpWindow: SignatureHelpWindow? = null
 		private var _diagnosticWindow: DiagnosticWindow? = null
-		private var fileVersion = 0
+
+		/**
+		 * [documentChangeMutex] only serialises change dispatches against each other; the resets in
+		 * [release] and [dispatchDocumentOpenEvent] run outside it, so a reset can race an in-flight
+		 * [dispatchDocumentChangeEvent]'s `incrementAndGet()` and stamp a low version right after a
+		 * newly-opened file's counter is zeroed. This is tolerated: it is bounded (self-heals on the
+		 * next edit) and distinct from the same-document backwards-version bug this ticket fixes,
+		 * which `ActiveDocument.update` now guards regardless of how `fileVersion` got there.
+		 */
+		private val fileVersion = AtomicInteger(0)
+		private val documentChangeMutex = Mutex()
 		internal var isModified = false
 
 		// Length and content hash of the content the last time the file was loaded or saved.
@@ -570,7 +583,7 @@ open class IDEEditor
 			languageClient = null
 
 			_file = null
-			fileVersion = 0
+			fileVersion.set(0)
 			markUnmodified()
 
 			editorFeatures.editor = null
@@ -960,7 +973,9 @@ open class IDEEditor
 				file ?: return@subscribeEvent
 
 				editorScope.launch {
-					dispatchDocumentChangeEvent(event)
+					// Serialised so the version a change is stamped with is never older than the text
+					// snapshot taken with it: two edits in one frame land here as two coroutines.
+					documentChangeMutex.withLock { dispatchDocumentChangeEvent(event) }
 					checkForSignatureHelp(event)
 					handleCustomTextReplacement(event)
 				}
@@ -1242,9 +1257,9 @@ open class IDEEditor
 
 			val file = this.file ?: return
 
-			this.fileVersion = 0
+			this.fileVersion.set(0)
 
-			val openEvent = DocumentOpenEvent(file.toPath(), text.toString(), fileVersion)
+			val openEvent = DocumentOpenEvent(file.toPath(), text.toString(), fileVersion.get())
 
 			eventDispatcher.dispatch(openEvent)
 		}
@@ -1278,7 +1293,7 @@ open class IDEEditor
 					file,
 					changedText,
 					text.toString(),
-					++fileVersion,
+					fileVersion.incrementAndGet(),
 					type,
 					changeDelta,
 					changeRange,

subprojects/projects/src/main/java/com/itsaky/androidide/projects/FileManager.kt

@@ -43,28 +43,19 @@ import java.util.concurrent.ConcurrentHashMap
  * @author Akash Yadav
  */
 object FileManager {
-
 	private val log = LoggerFactory.getLogger(FileManager::class.java)
 	private val _activeDocuments = ConcurrentHashMap<Path, ActiveDocument>()
 
 	val activeDocuments: Collection<ActiveDocument>
 		get() = _activeDocuments.values.toSet()
 
-	fun isActive(uri: URI): Boolean {
-		return isActive(Paths.get(uri))
-	}
+	fun isActive(uri: URI): Boolean = isActive(Paths.get(uri))
 
-	fun isActive(file: Path): Boolean {
-		return this._activeDocuments.containsKey(file.normalize())
-	}
+	fun isActive(file: Path): Boolean = this._activeDocuments.containsKey(file.normalize())
 
-	fun getActiveDocument(file: Path): ActiveDocument? {
-		return this._activeDocuments[file.normalize()]
-	}
+	fun getActiveDocument(file: Path): ActiveDocument? = this._activeDocuments[file.normalize()]
 
-	fun getActiveDocumentCount(): Int {
-		return this._activeDocuments.size
-	}
+	fun getActiveDocumentCount(): Int = this._activeDocuments.size
 
 	fun getDocumentContents(file: Path): String {
 		val document = getActiveDocument(file)
@@ -115,14 +106,19 @@ object FileManager {
 			_activeDocuments[event.changedFile.normalize()] = createDocument(event)
 			log.warn(
 				"Document change event received before open event for file {}",
-				event.changedFile
+				event.changedFile,
 			)
 			return
 		}
 
-		document.version = event.version
-		document.modified = Instant.now()
-		document.content = event.newText!!
+		if (!document.update(event.version, event.newText!!)) {
+			log.debug(
+				"Ignoring out-of-order change for {}: event version {} is older than {}",
+				event.changedFile,
+				event.version,
+				document.version,
+			)
+		}
 		event.newText = null
 	}
 
@@ -142,52 +138,46 @@ object FileManager {
 		_activeDocuments.remove(event.file.toPath().normalize())
 	}
 
-	private fun createDocument(event: DocumentOpenEvent): ActiveDocument {
-		return ActiveDocument(
+	private fun createDocument(event: DocumentOpenEvent): ActiveDocument =
+		ActiveDocument(
 			file = event.openedFile,
 			version = event.version,
 			modified = Instant.now(),
-			content = event.text
+			content = event.text,
 		)
-	}
 
-	private fun createDocument(event: DocumentChangeEvent): ActiveDocument {
-		return ActiveDocument(
+	private fun createDocument(event: DocumentChangeEvent): ActiveDocument =
+		ActiveDocument(
 			file = event.changedFile,
 			version = event.version,
 			modified = Instant.now(),
-			content = event.changedText
+			content = event.changedText,
 		)
-	}
 
-	private fun createFileReader(file: Path): BufferedReader {
-		return try {
+	private fun createFileReader(file: Path): BufferedReader =
+		try {
 			Files.newBufferedReader(file)
 		} catch (noFile: java.nio.file.NoSuchFileException) {
 			log.warn("No such file", noFile)
 			"".reader().buffered()
 		} catch (cancelled: CancellationException) {
 			"".reader().buffered()
 		}
-	}
 
-	private fun createFileInputStream(file: Path): InputStream {
-		return try {
+	private fun createFileInputStream(file: Path): InputStream =
+		try {
 			Files.newInputStream(file)
 		} catch (noFile: java.nio.file.NoSuchFileException) {
 			log.warn("No such file", noFile)
 			"".byteInputStream()
 		} catch (cancelled: CancellationException) {
 			"".byteInputStream()
 		}
-	}
 
-	private fun getLastModifiedFromDisk(file: Path): Instant {
-		return Files.getLastModifiedTime(file).toInstant()
-	}
+	private fun getLastModifiedFromDisk(file: Path): Instant = Files.getLastModifiedTime(file).toInstant()
 
-	private fun getFileContents(file: Path): String {
-		return try {
+	private fun getFileContents(file: Path): String =
+		try {
 			ProgressManager.abortIfCancelled()
 			FileUtils.readFileToString(file.toFile(), Charset.defaultCharset())
 		} catch (noFile: java.nio.file.NoSuchFileException) {
@@ -196,5 +186,4 @@ object FileManager {
 		} catch (cancelled: CancellationException) {
 			""
 		}
-	}
 }

subprojects/projects/src/main/java/com/itsaky/androidide/projects/models/ActiveDocument.kt

@@ -29,19 +29,60 @@ import java.time.Instant
  */
 open class ActiveDocument(
 	val file: Path,
-	var version: Int,
-	var modified: Instant,
-	content: String = ""
+	version: Int,
+	modified: Instant,
+	content: String = "",
 ) {
+	private data class Snapshot(
+		val version: Int,
+		val modified: Instant,
+		val content: String,
+	)
 
-	var content: String = content
-		internal set
+	/*
+	 * One volatile reference, so a reader can never pair a new version with the old content. The editor
+	 * dispatches change events from a background coroutine per edit, so two edits in one frame do reach
+	 * this concurrently.
+	 */
+	@Volatile
+	private var snapshot = Snapshot(version, modified, content)
 
-	fun inputStream(): BufferedInputStream {
-		return content.byteInputStream().buffered()
-	}
+	/** The version last published via [update]. Always consistent with [content] and [modified]. */
+	val version: Int
+		get() = snapshot.version
+
+	/** The timestamp of the last [update]. Always consistent with [version] and [content]. */
+	val modified: Instant
+		get() = snapshot.modified
+
+	/** The content last published via [update]. Always consistent with [version] and [modified]. */
+	val content: String
+		get() = snapshot.content
 
-	fun reader(): BufferedReader {
-		return content.reader().buffered()
+	/**
+	 * Publishes [content] at [version], or returns false if [version] is older than what is already
+	 * published.
+	 *
+	 * A version that moves backwards makes the Kotlin index mint a second `KtFile` for text that never
+	 * changed, which is what surfaced as redeclaration errors across a whole file (ADFA-5231).
+	 *
+	 * An equal version is accepted and overwrites, rather than being rejected like an older one. The
+	 * only writer, `IDEEditor`, stamps versions from a single serialised `AtomicInteger.incrementAndGet()`
+	 * per document, so distinct edits never share a version - an equal version is a re-delivery of the
+	 * same edit, and taking its (identical) content is harmless.
+	 */
+	internal fun update(
+		version: Int,
+		content: String,
+	): Boolean {
+		synchronized(this) {
+			if (version < snapshot.version) return false
+			snapshot = Snapshot(version, Instant.now(), content)
+			return true
+		}
 	}
+
+	fun inputStream(): BufferedInputStream = content.byteInputStream().buffered()
+
+	fun reader(): BufferedReader = content.reader().buffered()
 }

subprojects/projects/src/test/java/com/itsaky/androidide/projects/ActiveDocumentVersionTest.kt

@@ -0,0 +1,47 @@
+package com.itsaky.androidide.projects
+
+import com.google.common.truth.Truth.assertThat
+import com.itsaky.androidide.eventbus.events.editor.ChangeType
+import com.itsaky.androidide.eventbus.events.editor.DocumentChangeEvent
+import com.itsaky.androidide.eventbus.events.editor.DocumentCloseEvent
+import com.itsaky.androidide.eventbus.events.editor.DocumentOpenEvent
+import com.itsaky.androidide.models.Range
+import org.junit.After
+import org.junit.Test
+import java.nio.file.Paths
+
+/** A document's version and content always move forward together. */
+class ActiveDocumentVersionTest {
+	private val path = Paths.get("/tmp/adfa5231/Main.kt")
+
+	@After
+	fun close() {
+		FileManager.onDocumentClose(DocumentCloseEvent(path))
+	}
+
+	private fun change(
+		text: String,
+		version: Int,
+	) = DocumentChangeEvent(path, text, text, version, ChangeType.NEW_TEXT, 0, Range.NONE)
+
+	@Test
+	fun `a backwards version is rejected and leaves the newer content in place`() {
+		FileManager.onDocumentOpen(DocumentOpenEvent(path, "v1", 1))
+		FileManager.onDocumentContentChange(change("v3", 3))
+
+		FileManager.onDocumentContentChange(change("v2", 2))
+
+		val document = FileManager.getActiveDocument(path)!!
+		assertThat(document.version).isEqualTo(3)
+		assertThat(document.content).isEqualTo("v3")
+	}
+
+	@Test
+	fun `a version and its content are never observed apart`() {
+		FileManager.onDocumentOpen(DocumentOpenEvent(path, "v1", 1))
+		FileManager.onDocumentContentChange(change("v2", 2))
+
+		val document = FileManager.getActiveDocument(path)!!
+		assertThat(document.version to document.content).isEqualTo(2 to "v2")
+	}
+}