Fix vcr

jvm/CHANGELOG.md

@@ -11,6 +11,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## [Unreleased]
+### Fixed
+- Selfie VCR is now out of beta, no opt-in required. ([#525](https://github.com/diffplug/selfie/pull/525))
+  - ArrayMap now sorts strings with multi-digit numbers as `1 2 ... 9 10 11` instead of `1 11 2 ...`.
+  - Improved VCR-specific error messages for determining why `//selfieonce` might not be working for a test rule.
+  - Fixed some bugs in VCR data storage (specifically concurrency and multiple frames).
 
 ## [2.5.0] - 2025-02-21
 ### Added

jvm/selfie-lib/src/commonMain/kotlin/com/diffplug/selfie/ArrayMap.kt

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2023-2024 DiffPlug
+ * Copyright (C) 2023-2025 DiffPlug
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -23,16 +23,48 @@ private val STRING_SLASHFIRST =
         val charA = a[i]
         val charB = b[i]
         if (charA != charB) {
-          return@Comparator ( // treat
-          if (charA == '/') -1 // slash as
-          else if (charB == '/') 1 // the lowest
-          else charA.compareTo(charB) // character
-          )
+          // Check for slash first as it's special
+          if (charA == '/') return@Comparator -1 // treat slash as the lowest character
+          if (charB == '/') return@Comparator 1
+
+          // Check for embedded numbers
+          if (charA.isDigit() && charB.isDigit()) {
+            // Extract the complete numbers from both strings
+            val numA = extractNumber(a, i)
+            val numB = extractNumber(b, i)
+
+            // Compare the numeric values
+            val numCompare = numA.first.compareTo(numB.first)
+            if (numCompare != 0) return@Comparator numCompare
+
+            // If the numbers are equal, adjust index to after the numbers
+            // and continue comparing the rest of the string
+            i =
+                maxOf(numA.second, numB.second) -
+                    1 // -1 because we increment i at the end of the loop
+          } else {
+            // Regular character comparison
+            return@Comparator charA.compareTo(charB)
+          }
         }
         i++
       }
       a.length.compareTo(b.length)
     }
+
+/**
+ * Extracts a numeric substring starting at the given index.
+ *
+ * @return Pair of (numeric value, index after the last digit)
+ */
+private fun extractNumber(s: String, startIndex: Int): Pair<Int, Int> {
+  var endIndex = startIndex
+  while (endIndex < s.length && s[endIndex].isDigit()) {
+    endIndex++
+  }
+  val number = s.substring(startIndex, endIndex).toInt()
+  return Pair(number, endIndex)
+}
 private val PAIR_STRING_SLASHFIRST =
     Comparator<Pair<String, Any>> { a, b -> STRING_SLASHFIRST.compare(a.first, b.first) }
 

jvm/selfie-lib/src/commonMain/kotlin/com/diffplug/selfie/Mode.kt

@@ -47,13 +47,15 @@ enum class Mode {
       msg("Snapshot " + SnapshotNotEqualErrorMsg.forUnequalStrings(expected, actual))
   internal fun msgSnapshotMismatchBinary(expected: ByteArray, actual: ByteArray) =
       msgSnapshotMismatch(expected.toQuotedPrintable(), actual.toQuotedPrintable())
-  internal fun msgVcrMismatch(key: String, expected: String, actual: String) =
-      msg("VCR frame $key " + SnapshotNotEqualErrorMsg.forUnequalStrings(expected, actual))
-  internal fun msgVcrUnread(expected: Int, actual: Int) =
-      msg("VCR frames unread - only $actual were read out of $expected")
-  internal fun msgVcrUnderflow(expected: Int) =
-      msg(
-          "VCR frames exhausted - only $expected are available but you tried to read ${expected + 1}")
+  internal fun msgVcrSnapshotNotFound(call: CallStack) = msgVcr("VCR snapshot not found", call)
+  internal fun msgVcrMismatch(key: String, expected: String, actual: String, call: CallStack) =
+      msgVcr("VCR frame $key " + SnapshotNotEqualErrorMsg.forUnequalStrings(expected, actual), call)
+  internal fun msgVcrUnread(expected: Int, actual: Int, call: CallStack) =
+      msgVcr("VCR frames unread - only $actual were read out of $expected", call)
+  internal fun msgVcrUnderflow(expected: Int, call: CallStack) =
+      msgVcr(
+          "VCR frames exhausted - only $expected are available but you tried to read ${expected + 1}",
+          call)
   private fun ByteArray.toQuotedPrintable(): String {
     val sb = StringBuilder()
     for (byte in this) {
@@ -70,11 +72,19 @@ enum class Mode {
       when (this) {
         interactive ->
             "$headline\n" +
-                (if (headline.startsWith("Snapshot "))
-                    "‣ update this snapshot by adding `_TODO` to the function name\n"
-                else "") +
+                "‣ update this snapshot by adding `_TODO` to the function name\n" +
                 "‣ update all snapshots in this file by adding `//selfieonce` or `//SELFIEWRITE`"
         readonly -> headline
         overwrite -> "$headline\n(didn't expect this to ever happen in overwrite mode)"
       }
+  private fun msgVcr(headline: String, call: CallStack) =
+      when (this) {
+        interactive ->
+            "$headline\n" +
+                "‣ update all snapshots in this file by adding `//selfieonce` or `//SELFIEWRITE`\n" +
+                "‣ could not find control comment in ${call.location.ideLink(Selfie.system.layout)}\n" +
+                "‣ remember to call `Selfie.vcrTestLocator()` in the test itself, or put the file above into the `selfie` package to mark that it is not the test file"
+        readonly -> headline
+        overwrite -> "$headline\n(didn't expect this to ever happen in overwrite mode)"
+      }
 }

jvm/selfie-lib/src/commonMain/kotlin/com/diffplug/selfie/Selfie.kt

@@ -94,14 +94,5 @@ object Selfie {
    * control whether the VCR is writing or reading. If the caller lives in a package called
    * `selfie.*` it will keep looking up the stack trace until a caller is not inside `selfie.*`.
    */
-  @JvmStatic
-  @ExperimentalSelfieVcr
-  fun vcrTestLocator(sub: String = "") = VcrSelfie.TestLocator(sub, deferredDiskStorage)
+  @JvmStatic fun vcrTestLocator(sub: String = "") = VcrSelfie.TestLocator(sub, deferredDiskStorage)
 }
-
-@RequiresOptIn(
-    level = RequiresOptIn.Level.WARNING,
-    message = "This API is in beta and may change in the future.")
-@Retention(AnnotationRetention.BINARY)
-@Target(AnnotationTarget.CLASS, AnnotationTarget.FUNCTION, AnnotationTarget.PROPERTY)
-annotation class ExperimentalSelfieVcr

jvm/selfie-lib/src/commonMain/kotlin/com/diffplug/selfie/VcrSelfie.kt

@@ -17,7 +17,10 @@ package com.diffplug.selfie
 
 import com.diffplug.selfie.guts.CallStack
 import com.diffplug.selfie.guts.DiskStorage
+import com.diffplug.selfie.guts.atomic
 import com.diffplug.selfie.guts.recordCall
+import com.diffplug.selfie.guts.reentrantLock
+import com.diffplug.selfie.guts.withLock
 
 private const val OPEN = "«"
 private const val CLOSE = "»"
@@ -28,73 +31,98 @@ internal constructor(
     private val call: CallStack,
     private val disk: DiskStorage,
 ) : AutoCloseable {
+  /** Creates VCRs who store their data based on where this TestLocator was created. */
   class TestLocator internal constructor(private val sub: String, private val disk: DiskStorage) {
     private val call = recordCall(false)
     fun createVcr() = VcrSelfie(sub, call, disk)
   }
+  private val state: State
+
+  internal sealed class State {
+    class Read(val frames: List<Pair<String, SnapshotValue>>) : State() {
+      fun currentFrameThenAdvance(): Int = cf.getAndUpdate { it + 1 }
+      fun framesReadSoFar(): Int = cf.get()
+      private val cf = atomic(0)
+    }
 
-  private class State(val readMode: Boolean) {
-    var currentFrame = 0
-    val frames = mutableListOf<Pair<String, SnapshotValue>>()
+    class Write : State() {
+      private val lock = reentrantLock()
+      private var frames: MutableList<Map.Entry<String, SnapshotValue>>? =
+          mutableListOf<Map.Entry<String, SnapshotValue>>()
+      fun add(key: String, value: SnapshotValue) {
+        lock.withLock {
+          frames?.apply {
+            val idx = size + 1
+            add(entry("$OPEN$idx$CLOSE$key", value))
+          } ?: throw IllegalStateException("This VCR was already closed.")
+        }
+      }
+      fun closeAndGetSnapshot(): Snapshot =
+          Snapshot.ofEntries(
+              lock.withLock {
+                val frozen = frames ?: throw IllegalStateException("This VCR was already closed.")
+                frames = null
+                frozen
+              })
+    }
   }
-  private val state: State
 
   init {
     val canWrite = Selfie.system.mode.canWrite(isTodo = false, call, Selfie.system)
-    state = State(readMode = !canWrite)
-    if (state.readMode) {
+    if (canWrite) {
+      state = State.Write()
+    } else {
       val snapshot =
           disk.readDisk(sub, call)
-              ?: throw Selfie.system.fs.assertFailed(Selfie.system.mode.msgSnapshotNotFound())
+              ?: throw Selfie.system.fs.assertFailed(
+                  Selfie.system.mode.msgVcrSnapshotNotFound(call))
       var idx = 1
+      val frames = mutableListOf<Pair<String, SnapshotValue>>()
       for ((key, value) in snapshot.facets) {
         check(key.startsWith(OPEN))
         val nextClose = key.indexOf(CLOSE)
         check(nextClose != -1)
         val num = key.substring(OPEN.length, nextClose).toInt()
-        check(num == idx)
+        check(num == idx) { "expected $idx in $key" }
         ++idx
         val keyAfterNum = key.substring(nextClose + 1)
-        state.frames.add(keyAfterNum to value)
+        frames.add(keyAfterNum to value)
       }
+      state = State.Read(frames)
     }
   }
   override fun close() {
-    if (state.readMode) {
-      if (state.frames.size != state.currentFrame) {
+    if (state is State.Read) {
+      if (state.frames.size != state.framesReadSoFar()) {
         throw Selfie.system.fs.assertFailed(
-            Selfie.system.mode.msgVcrUnread(state.frames.size, state.currentFrame))
+            Selfie.system.mode.msgVcrUnread(state.frames.size, state.framesReadSoFar(), call))
       }
     } else {
-      var snapshot = Snapshot.of("")
-      var idx = 1
-      for ((key, value) in state.frames) {
-        snapshot = snapshot.plusFacet("$OPEN$idx$CLOSE$key", value)
-      }
-      disk.writeDisk(snapshot, sub, call)
+      disk.writeDisk((state as State.Write).closeAndGetSnapshot(), sub, call)
     }
   }
-  private fun nextFrameValue(key: String): SnapshotValue {
+  private fun nextFrameValue(state: State.Read, key: String): SnapshotValue {
     val mode = Selfie.system.mode
     val fs = Selfie.system.fs
-    if (state.frames.size <= state.currentFrame) {
-      throw fs.assertFailed(mode.msgVcrUnderflow(state.frames.size))
+    val currentFrame = state.currentFrameThenAdvance()
+    if (state.frames.size <= currentFrame) {
+      throw fs.assertFailed(mode.msgVcrUnderflow(state.frames.size, call), call)
     }
-    val expected = state.frames[state.currentFrame++]
+    val expected = state.frames[currentFrame]
     if (expected.first != key) {
       throw fs.assertFailed(
-          mode.msgVcrMismatch("$sub[$OPEN${state.currentFrame}$CLOSE]", expected.first, key),
+          mode.msgVcrMismatch("$sub[$OPEN${currentFrame}$CLOSE]", expected.first, key, call),
           expected.first,
           key)
     }
     return expected.second
   }
   fun <V> nextFrame(key: String, roundtripValue: Roundtrip<V, String>, value: Cacheable<V>): V {
-    if (state.readMode) {
-      return roundtripValue.parse(nextFrameValue(key).valueString())
+    if (state is State.Read) {
+      return roundtripValue.parse(nextFrameValue(state, key).valueString())
     } else {
       val value = value.get()
-      state.frames.add(key to SnapshotValue.of(roundtripValue.serialize(value)))
+      (state as State.Write).add(key, SnapshotValue.of(roundtripValue.serialize(value)))
       return value
     }
   }
@@ -107,11 +135,11 @@ internal constructor(
       roundtripValue: Roundtrip<V, ByteArray>,
       value: Cacheable<V>
   ): V {
-    if (state.readMode) {
-      return roundtripValue.parse(nextFrameValue(key).valueBinary())
+    if (state is State.Read) {
+      return roundtripValue.parse(nextFrameValue(state, key).valueBinary())
     } else {
       val value = value.get()
-      state.frames.add(key to SnapshotValue.of(roundtripValue.serialize(value)))
+      (state as State.Write).add(key, SnapshotValue.of(roundtripValue.serialize(value)))
       return value
     }
   }

jvm/selfie-lib/src/commonMain/kotlin/com/diffplug/selfie/guts/AtomicFuBroken.kt

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2024 DiffPlug
+ * Copyright (C) 2024-2025 DiffPlug
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -24,12 +24,8 @@ expect class AtomicRef<T> {
 /** Replace with atomicfu when stable. */
 expect fun <T> atomic(initial: T): AtomicRef<T>
 
-expect fun reentrantLock(): ReentrantLock
+expect class ReentrantLock
 
-expect class ReentrantLock {
-  fun lock(): Unit
-  fun tryLock(): Boolean
-  fun unlock(): Unit
-}
+expect fun reentrantLock(): ReentrantLock
 
 expect inline fun <T> ReentrantLock.withLock(block: () -> T): T

jvm/selfie-lib/src/jsMain/kotlin/com/diffplug/selfie/guts/AtomicFuBroken.js.kt

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2024 DiffPlug
+ * Copyright (C) 2024-2025 DiffPlug
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -29,12 +29,7 @@ actual class AtomicRef<T>(private var value: T) {
   }
 }
 val Lock = ReentrantLock()
-actual inline fun reentrantLock() = Lock
+actual fun reentrantLock() = Lock
 
-@Suppress("NOTHING_TO_INLINE")
-actual class ReentrantLock {
-  actual inline fun lock(): Unit {}
-  actual inline fun tryLock() = true
-  actual inline fun unlock(): Unit {}
-}
+@Suppress("NOTHING_TO_INLINE") actual class ReentrantLock
 actual inline fun <T> ReentrantLock.withLock(block: () -> T) = block()

jvm/selfie-lib/src/jvmMain/kotlin/com/diffplug/selfie/guts/AtomicFuBroken.jvm.kt

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2024 DiffPlug
+ * Copyright (C) 2024-2025 DiffPlug
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -24,7 +24,7 @@ actual class AtomicRef<T>(value: T) {
   actual fun updateAndGet(update: (T) -> T): T = ref.updateAndGet(update)
   actual fun getAndUpdate(update: (T) -> T) = ref.getAndUpdate(update)
 }
-actual inline fun reentrantLock() = ReentrantLock()
+actual fun reentrantLock() = ReentrantLock()
 
 actual typealias ReentrantLock = java.util.concurrent.locks.ReentrantLock
 actual inline fun <T> ReentrantLock.withLock(block: () -> T): T {