Fix spurious reserved-reformat and enum-alias-removal diffs

tools/src/main/scala/proteus/Change.scala

@@ -90,7 +90,7 @@ enum Change {
       case EnumRemoved(_, name)                                  => s"enum '$name' removed"
       case EnumRenamed(_, oldName, newName)                      => s"enum renamed from '$oldName' to '$newName'"
       case EnumValueAdded(_, name, _)                            => s"enum value '$name' added"
-      case EnumValueRemoved(_, name, _, reserved)                => s"enum value '$name' removed${if (reserved) " (number reserved)" else ""}"
+      case EnumValueRemoved(_, name, _, reserved)                => s"enum value '$name' removed${if (reserved) " (number retained)" else ""}"
       case EnumValueNumberChanged(_, name, oldNum, newNum)       => s"enum value '$name' number changed from $oldNum to $newNum"
       case EnumValueRenamed(_, _, oldName, newName)              => s"enum value '$oldName' renamed to '$newName'"
       case ReservedAdded(_, reserved)                            => s"reserved ${Renderer.renderReservedValue(reserved)} added"

tools/src/main/scala/proteus/ProtoDiff.scala

@@ -604,8 +604,10 @@ object ProtoDiff {
     val rem3New  = rem2New.filterNot(v => numKeys.contains(v.intValue))
 
     // Pass 4: unmatched
-    val removed = rem3Old.map(v => EnumValueRemoved(path, v.name, v.intValue, isNumberReserved(v.intValue, newReserved)))
-    val added   = rem3New.map(v => EnumValueAdded(path, v.name, v.intValue))
+    val newNumbers = newValues.map(_.intValue).toSet
+    val removed    =
+      rem3Old.map(v => EnumValueRemoved(path, v.name, v.intValue, isNumberReserved(v.intValue, newReserved) || newNumbers.contains(v.intValue)))
+    val added      = rem3New.map(v => EnumValueAdded(path, v.name, v.intValue))
 
     exactDiffs ++ nameDiffs ++ numDiffs ++ removed ++ added
   }
@@ -630,12 +632,54 @@ object ProtoDiff {
     (changes, oldItems.filterNot(renamedOld.contains), newItems.filterNot(renamedNew.contains))
   }
 
+  private def reservedNumberIntervals(res: List[Reserved]): List[(Int, Int)] = {
+    val ranges = res
+      .collect {
+        case Reserved.Number(n)   => (n, n)
+        case Reserved.Range(s, e) => (s, e)
+      }
+      .sortBy(_._1)
+    ranges
+      .foldLeft(List.empty[(Int, Int)]) { case (acc, (s, e)) =>
+        acc match {
+          case (ps, pe) :: tail if s.toLong <= pe.toLong + 1 => (ps, math.max(pe, e)) :: tail
+          case _                                             => (s, e) :: acc
+        }
+      }
+      .reverse
+  }
+
+  /**
+    * Coverage of `minuend` intervals not covered by `subtrahend`, as canonical Int intervals.
+    */
+  private def subtractIntervals(minuend: List[(Int, Int)], subtrahend: List[(Int, Int)]): List[(Int, Int)] =
+    minuend.flatMap { case (s, e) =>
+      subtrahend
+        .foldLeft(List((s.toLong, e.toLong))) { (segments, b) =>
+          val (bs, be) = (b._1.toLong, b._2.toLong)
+          segments.flatMap { case (ls, le) =>
+            if (be < ls || bs > le) List((ls, le))
+            else (if (ls < bs) List((ls, bs - 1)) else Nil) ++ (if (le > be) List((be + 1, le)) else Nil)
+          }
+        }
+        .map { case (ls, le) => (ls.toInt, le.toInt) }
+    }
+
+  private def intervalToReserved(iv: (Int, Int)): Reserved =
+    if (iv._1 == iv._2) Reserved.Number(iv._1) else Reserved.Range(iv._1, iv._2)
+
   private def diffReserved(oldRes: List[Reserved], newRes: List[Reserved], path: List[String]): List[Change] = {
-    val oldSet  = oldRes.toSet
-    val newSet  = newRes.toSet
-    val removed = (oldSet -- newSet).toList.map(r => ReservedRemoved(path, r))
-    val added   = (newSet -- oldSet).toList.map(r => ReservedAdded(path, r))
-    removed ++ added
+    val oldIv         = reservedNumberIntervals(oldRes)
+    val newIv         = reservedNumberIntervals(newRes)
+    val numberChanges =
+      subtractIntervals(oldIv, newIv).map(iv => ReservedRemoved(path, intervalToReserved(iv))) ++
+        subtractIntervals(newIv, oldIv).map(iv => ReservedAdded(path, intervalToReserved(iv)))
+    val oldNames      = oldRes.collect { case r: Reserved.Name => r }.toSet
+    val newNames      = newRes.collect { case r: Reserved.Name => r }.toSet
+    val nameChanges   =
+      (oldNames -- newNames).toList.map(r => ReservedRemoved(path, r)) ++
+        (newNames -- oldNames).toList.map(r => ReservedAdded(path, r))
+    numberChanges ++ nameChanges
   }
 
   private def diffServices(oldSvcs: List[Service], newSvcs: List[Service], path: List[String]): List[Change] = {

tools/src/test/scala/proteus/ProtoDiffSpec.scala

@@ -333,6 +333,15 @@ object ProtoDiffSpec extends ZIOSpecDefault {
         val old = parse("""syntax = "proto3"; enum Status { UNKNOWN = 0; ACTIVE = 1; }""")
         val nw  = parse("""syntax = "proto3"; enum Status { UNKNOWN = 0; ENABLED = 1; }""")
         assertTrue(ProtoDiff.diff(old, nw) == List(EnumValueRenamed(List("Status"), 1, "ACTIVE", "ENABLED")))
+      },
+      test("removing an enum alias whose number is still used is wire-compatible") {
+        val old     = parse("""syntax = "proto3"; enum E { option allow_alias = true; A = 0; B = 1; C = 1; }""")
+        val nw      = parse("""syntax = "proto3"; enum E { option allow_alias = true; A = 0; B = 1; }""")
+        val changes = ProtoDiff.diff(old, nw)
+        assertTrue(
+          changes == List(EnumValueRemoved(List("E"), "C", 1, numberReserved = true)),
+          changes.forall(c => ProtoDiff.severity(c, CompatMode.Wire) == Severity.Info)
+        )
       }
     ),
     suite("Nested messages")(
@@ -440,6 +449,26 @@ object ProtoDiffSpec extends ZIOSpecDefault {
         val old = parse("""syntax = "proto3"; message Foo { string name = 1; reserved 2; }""")
         val nw  = parse("""syntax = "proto3"; message Foo { string name = 1; }""")
         assertTrue(ProtoDiff.diff(old, nw) == List(ReservedRemoved(List("Foo"), Reserved.Number(2))))
+      },
+      test("reformatting a reserved range as individual numbers is not a change") {
+        val old = parse("""syntax = "proto3"; message Foo { reserved 2 to 4; }""")
+        val nw  = parse("""syntax = "proto3"; message Foo { reserved 2, 3, 4; }""")
+        assertTrue(ProtoDiff.diff(old, nw) == Nil)
+      },
+      test("a reserved-name change alongside a range reformat only reports the name change") {
+        val old = parse("""syntax = "proto3"; message Foo { reserved 2 to 4; }""")
+        val nw  = parse("""syntax = "proto3"; message Foo { reserved 2, 3, 4; reserved "foo"; }""")
+        assertTrue(ProtoDiff.diff(old, nw) == List(ReservedAdded(List("Foo"), Reserved.Name("foo"))))
+      },
+      test("extending a reserved range while reformatting reports only the added number") {
+        val old = parse("""syntax = "proto3"; message Foo { reserved 2 to 4; }""")
+        val nw  = parse("""syntax = "proto3"; message Foo { reserved 2, 3, 4, 5; }""")
+        assertTrue(ProtoDiff.diff(old, nw) == List(ReservedAdded(List("Foo"), Reserved.Number(5))))
+      },
+      test("dropping one number from a reserved range reports only that number") {
+        val old = parse("""syntax = "proto3"; message Foo { reserved 2 to 4; }""")
+        val nw  = parse("""syntax = "proto3"; message Foo { reserved 2, 4; }""")
+        assertTrue(ProtoDiff.diff(old, nw) == List(ReservedRemoved(List("Foo"), Reserved.Number(3))))
       }
     ),
     suite("Option changes")(