Refactor VectorSet to use Vector+Set instead of VectorMap

library/src/scala/collection/immutable/VectorSet.scala

@@ -18,14 +18,15 @@ import scala.language.`2.13`
 import language.experimental.captureChecking
 
 import scala.collection.generic.DefaultSerializable
-import scala.collection.mutable.Builder
+import scala.collection.mutable.{Builder, LinkedHashSet}
 
-/** This class implements immutable sets using a vector/map-based data
+/** This class implements immutable sets using a vector-backed data
  *  structure, which preserves insertion order.
  *
- *  Unlike `ListSet`, `VectorSet` has amortized effectively constant lookup,
- *  inclusion and exclusion at the expense of using extra memory, which makes
- *  it suitable beyond a small number of elements.
+ *  Unlike `ListSet`, `VectorSet` has amortized effectively constant lookup
+ *  and inclusion at the expense of using extra memory. Exclusion is linear in
+ *  the number of elements, which still makes it suitable beyond a small
+ *  number of elements.
  *
  *  Adding an element that is already present keeps its original position;
  *  removing and re-adding an element moves it to the end.
@@ -35,7 +36,9 @@ import scala.collection.mutable.Builder
  *  @define coll immutable vector set
  *  @define Coll `immutable.VectorSet`
  */
-final class VectorSet[A] private (private val underlying: VectorMap[A, Unit])
+final class VectorSet[A] private (
+    private val elements: Vector[A],
+    private val membership: Set[A])
   extends AbstractSet[A]
     with SeqSet[A]
     with StrictOptimizedSetOps[A, VectorSet, VectorSet[A]]
@@ -44,62 +47,68 @@ final class VectorSet[A] private (private val underlying: VectorMap[A, Unit])
 
   override protected def className: String = "VectorSet"
 
-  override def size: Int = underlying.size
+  override def size: Int = elements.size
 
-  override def knownSize: Int = underlying.knownSize
+  override def knownSize: Int = size
 
-  override def isEmpty: Boolean = underlying.isEmpty
+  override def isEmpty: Boolean = elements.isEmpty
 
-  def contains(elem: A): Boolean = underlying.contains(elem)
+  def contains(elem: A): Boolean = membership.contains(elem)
 
   def incl(elem: A): VectorSet[A] =
-    if (underlying.contains(elem)) this
-    else new VectorSet(underlying.updated(elem, ()))
+    if (membership.contains(elem)) this
+    else new VectorSet(elements :+ elem, membership + elem)
 
   def excl(elem: A): VectorSet[A] = {
-    val newUnderlying = underlying.removed(elem)
-    if (newUnderlying eq underlying) this
-    else new VectorSet(newUnderlying)
+    if (!membership.contains(elem)) this
+    else new VectorSet(elements.filterNot(_ == elem), membership - elem)
   }
 
-  def iterator: Iterator[A] = underlying.keysIterator
+  def iterator: Iterator[A] = elements.iterator
 
-  override def head: A = underlying.head._1
+  override def head: A = elements.head
 
-  override def last: A = underlying.last._1
+  override def last: A = elements.last
 
-  override def tail: VectorSet[A] = new VectorSet(underlying.tail)
+  override def tail: VectorSet[A] = {
+    if (isEmpty) throw new UnsupportedOperationException("empty.tail")
+    new VectorSet(elements.tail, membership - elements.head)
+  }
 
-  override def init: VectorSet[A] = new VectorSet(underlying.init)
+  override def init: VectorSet[A] = {
+    if (isEmpty) throw new UnsupportedOperationException("empty.init")
+    new VectorSet(elements.init, membership - elements.last)
+  }
 
   override def iterableFactory: IterableFactory[VectorSet] = VectorSet
 }
 
 object VectorSet extends IterableFactory[VectorSet] {
 
   private val EmptySet: VectorSet[Nothing] =
-    new VectorSet[Nothing](VectorMap.empty[Nothing, Unit])
+    new VectorSet[Nothing](Vector.empty, Set.empty)
 
   def empty[A]: VectorSet[A] = EmptySet.asInstanceOf[VectorSet[A]]
 
   def from[A](it: collection.IterableOnce[A]^): VectorSet[A] =
     it match {
       case vs: VectorSet[A @unchecked] => vs
+      case it: Iterable[?] if it.isEmpty => empty[A]
       case _ => (newBuilder[A] ++= it).result()
     }
 
   def newBuilder[A]: Builder[A, VectorSet[A]] = new Builder[A, VectorSet[A]] {
-    private val mapBuilder = new VectorMapBuilder[A, Unit]
+    private val elems = LinkedHashSet.empty[A]
 
-    override def clear(): Unit = mapBuilder.clear()
+    override def clear(): Unit = elems.clear()
 
     override def result(): VectorSet[A] = {
-      val m = mapBuilder.result()
-      if (m.isEmpty) empty else new VectorSet(m)
+      if (elems.isEmpty) empty
+      else new VectorSet(elems.toVector, elems.toSet)
     }
 
     def addOne(elem: A): this.type = {
-      mapBuilder.addOne(elem, ())
+      elems += elem
       this
     }
   }

tests/run/seq-set.scala

@@ -10,6 +10,8 @@ import scala.collection.immutable.{SeqSet, VectorSet}
   // incl appends new elements, keeps the position of existing ones
   assert((s + 7).toList == List(3, 1, 4, 5, 9, 2, 6, 7))
   assert((s + 4).toList == s.toList)
+  // incl on empty set
+  assert((VectorSet.empty[Int] + 1).toList == List(1))
 
   // excl preserves the order of the remaining elements
   assert((s - 4).toList == List(3, 1, 5, 9, 2, 6))
@@ -36,6 +38,10 @@ import scala.collection.immutable.{SeqSet, VectorSet}
   assert(ss.isInstanceOf[VectorSet[?]])
   assert(SeqSet.empty[Int].isEmpty)
   assert(SeqSet.from(List(2, 1)).toList == List(2, 1))
+  // SeqSet.from with a SeqSet returns the same instance
+  val ss2: SeqSet[Int] = SeqSet(1, 2)
+  val ss3 = SeqSet.from(ss2)
+  assert(ss2 eq ss3, "SeqSet.from(SeqSet) should return the same instance")
 
   // builder deduplicates while preserving first-seen order
   val b = VectorSet.newBuilder[Int]
@@ -44,8 +50,37 @@ import scala.collection.immutable.{SeqSet, VectorSet}
 
   // empty cases
   assert(VectorSet.empty[Int].toList == Nil)
+  assert(VectorSet.empty[Int].knownSize == 0)
   assert((VectorSet(1) - 1).isEmpty)
 
+  // tail/init on empty set throw UnsupportedOperationException
+  assert {
+    var caught = false
+    try { VectorSet.empty[Int].tail; assert(false) }
+    catch case _: UnsupportedOperationException => caught = true
+    caught
+  }
+  assert {
+    var caught = false
+    try { VectorSet.empty[Int].init; assert(false) }
+    catch case _: UnsupportedOperationException => caught = true
+    caught
+  }
+
+  // from when given a VectorSet directly returns the same instance
+  val vs1 = VectorSet(1, 2, 3)
+  val vs2 = VectorSet.from(vs1)
+  assert(vs1 eq vs2, "VectorSet.from(VectorSet) should return the same instance")
+
+  // from with empty Iterable returns empty
+  val vs3 = VectorSet.from(List.empty[Int])
+  assert(vs3.isEmpty)
+
+  // builder clear and result on empty builder
+  val b2 = VectorSet.newBuilder[Int]
+  b2.clear()
+  assert(b2.result().isEmpty)
+
   // larger set exercises the underlying vector/tombstone machinery
   val big = VectorSet.from(1 to 100)
   assert(big.toList == (1 to 100).toList)