[lihaoyi]

As discussed in https://contributors.scala-lang.org/t/standard-library-now-open-for-improvements-and-suggestions/7337/62:

Can we merge mutable.ArrayDeque into mutable.Buffer?

As far as I know, ArrayDeque introduced in Scala 2.13 is basically a superset of Buffer’s functionality, and there is no reason that anyone should use Buffer anymore. ArrayDeque supports all the same APIs efficiently, and more due to allowing efficient operations on the other side.

It should be straightforward to implement all the mutable.Buffer operations using ArrayDeque, and add in the other ArrayDeque operations as well. Since the name mutable.Buffer is the “default” mutable buffer semantically, we should make it do what most people would want, rather than forcing them to reach for the more verbose ArrayDeque sometimes but not other times.

This would streamline usage of mutable.Buffer/mutable.ArrayDeque, and remove a stumbling block of having to choose which one to use when there really is one right answer and the obvious answer is wrong

This PR adds the following convenience methods to mutable.Buffer

  /** Optionally removes and returns the first element.
   *
   *  @return  `Some(firstElement)` if the buffer is non-empty, `None` otherwise
   */
  def removeHeadOption(): Option[A]

  /** Removes and returns the first element.
   *
   *  @return  the removed first element
   *  @throws NoSuchElementException if the buffer is empty
   */
  def removeHead(): A

  /** Optionally removes and returns the last element.
   *
   *  @return  `Some(lastElement)` if the buffer is non-empty, `None` otherwise
   */
  def removeLastOption(): Option[A]

  /** Removes and returns the last element.
   *
   *  @return  the removed last element
   *  @throws NoSuchElementException if the buffer is empty
   */
  def removeLast(): A

  /** Removes all elements from this $coll and returns them.
   *
   *  @return a sequence containing all removed elements
   */
  def removeAll(): scala.collection.immutable.Seq[A]
  
  /** Removes all elements from this $coll and returns them in reverse order.
   *
   *  @return a sequence containing all removed elements in reverse order
   */
  def removeAllReverse(): scala.collection.immutable.Seq[A]

  /** Removes and returns elements from the head of this $coll while they satisfy the predicate.
   *
   *  @param f  the predicate used for choosing elements
   *  @return   a sequence containing all removed elements
   */
  def removeHeadWhile(f: A => Boolean): scala.collection.immutable.Seq[A]

  /** Removes and returns elements from the tail of this $coll while they satisfy the predicate.
   *
   *  @param f  the predicate used for choosing elements
   *  @return   a sequence containing all removed elements
   */
  def removeLastWhile(f: A => Boolean): scala.collection.immutable.Seq[A]
  
  /** Finds and removes the first element satisfying a predicate.
   *
   *  @param p    the predicate used for choosing the first element
   *  @param from the start index
   *  @return the first element for which p yields true, or None if no such element exists
   */
  def removeFirst(p: A => Boolean, from: Int = 0): Option[A]
  
  /** Removes and returns all elements satisfying the given predicate.
   *
   *  @param p  the predicate used for choosing elements
   *  @return   a sequence of all removed elements
   */
  def removeAll(p: A => Boolean): scala.collection.immutable.Seq[A]

This PR basically swaps out ArrayBuffer in the Buffer factory objects with ArrayDeque, and copies the unique ArrayDeque convenience methods over while forwarding them to the existing methods like Buffer#remove. You could actually already do all these workflows with Buffer already, just that the API sucked (e.g. buffer.remove(buffer.length - 1)) and the asymptotic complexity of some operations sucked (e.g. buffer.remove(0)).

This PR solves both those problems, so after this for most intents and purposes people reaching for the obvious thing mutable.Buffer() will generally get everything they need without needing to separately reach for mutable.ArrayDeque. There is still some im-precision in the types, since technically a Buffer could be a ListBuffer or some other weird data structure, but that's no worse than where we are today so it's still a strict improvement

Regarding compatibility, we're only adding things to the API, which should be backwards binary and source compatible. And the change from ArrayBuffer to ArrayDeque only improve the asymptotic complexity of some operations while leaving others unchanged

I have updated the Scala.js implementation as well which previously used js.WrappedArray to also use ArrayDeque

[lihaoyi]

the community build failure is benign and expected given the nature of the change

[123-3] X test.pprint.HorizontalTests.Horizontal.collections.Buffer 6ms 
[123-3]   utest.AssertionError: expected.map(_.trim).contains(pprinted)
[123-3]   expected: Seq[String] = ArraySeq(ArrayBuffer("omg", "wtf", "bbq"), WrappedArray("omg", "wtf", "bbq"))
[123-3]   pprinted: String = ArrayDeque("omg", "wtf", "bbq")
[123-3]     utest.asserts.Asserts$.assertImpl(Asserts.scala:30)
[123-3]     test.pprint.Check.apply$$anonfun$1(Check.scala:22)
[123-3]     scala.runtime.function.JProcedure1.apply(JProcedure1.java:15)
[123-3]     scala.runtime.function.JProcedure1.apply(JProcedure1.java:10)
[123-3]     scala.collection.immutable.List.foreach(List.scala:327)
[123-3]     test.pprint.Check.apply(Check.scala:19)
[123-3]     test.pprint.HorizontalTests$.$init$$$anonfun$1$$anonfun$1$$anonfun$3$$anonfun$7(HorizontalTests.scala:115)

[sjrd]

For now I only change the default on the JVM since that's what I personally care about, and JS is left unchanged. We could conceivably change the default on JS as well to improve the asymptotic complexity there, though we may want benchmarks to check its effect on the performance constant factors

Keeping complexity guarantees in sync between platforms is more important than keeping better constant factors. If we're going to improve complexity guarantees in the JVM, we must also change the JS version.

[lihaoyi]

I have updated the Scala.js implementation as well which previously used js.WrappedArray to also use ArrayDeque

The mima changes should go away once we tweak mima to allow standard library evolution

[Gedochao]

@lihaoyi this might be worth a rebase, feature freeze has been lifted on main.

[lihaoyi]

@Gedochao I have rebased it and it should be ready for review I think

[Gedochao]

the community build failure is benign and expected given the nature of the change

[123-3] X test.pprint.HorizontalTests.Horizontal.collections.Buffer 6ms 
[123-3]   utest.AssertionError: expected.map(_.trim).contains(pprinted)
[123-3]   expected: Seq[String] = ArraySeq(ArrayBuffer("omg", "wtf", "bbq"), WrappedArray("omg", "wtf", "bbq"))
[123-3]   pprinted: String = ArrayDeque("omg", "wtf", "bbq")
[123-3]     utest.asserts.Asserts$.assertImpl(Asserts.scala:30)
[123-3]     test.pprint.Check.apply$$anonfun$1(Check.scala:22)
[123-3]     scala.runtime.function.JProcedure1.apply(JProcedure1.java:15)
[123-3]     scala.runtime.function.JProcedure1.apply(JProcedure1.java:10)
[123-3]     scala.collection.immutable.List.foreach(List.scala:327)
[123-3]     test.pprint.Check.apply(Check.scala:19)
[123-3]     test.pprint.HorizontalTests$.$init$$$anonfun$1$$anonfun$1$$anonfun$3$$anonfun$7(HorizontalTests.scala:115)

I guess we'd need an adjustment in community_build_c?
cc @WojciechMazur

[lihaoyi]

@Gedochao yeah that's an expected change and I can update the pprint test suite if necessary

[Gedochao]

Please do, we need the CI green, I guess.
Also note that this will get reviewed after it gets considered by Scala Core, as per SLC.
wait for the slc:approved label.

[lihaoyi]

@Gedochao community build should be fixed by com-lihaoyi/PPrint#166

[WojciechMazur]

It should have been a JUnit test in library/test/scala/collection/* - it might not have been present when PR was originally drafted, but that's the correct place to put standard library unit tests

[WojciechMazur]

Should be removed, .check files used to see expected output, not the progress of executing of test. Redundant when converting to dedicated unit test in stdlib project

[tgodzik]

Please do, we need the CI green, I guess. Also note that this will get reviewed after it gets considered by Scala Core, as per SLC. wait for the slc:approved label.

We also require it to be mentioned on the contributors forum for people to raise potential issues with the proposal.

[lihaoyi]

@tgodzik contributors thread https://contributors.scala-lang.org/t/slc-merge-mutable-arraydeque-into-mutable-buffer-on-the-jvm/7469

[SethTisue]

These new methods on Buffer, should they also be available on all mutable collections? Why only Buffer?

[lihaoyi]

@SethTisue I want these added to buffer because it is in many ways the "default" mutable sequence constructor.

[tgodzik]

We discussed it during meeting today I the biggest problem we saw is that it's very much disjoint from other collections. Maybe some of these method actually correspond to existing ones? Or at least we should follow similar conventions instead of having completely new names. ArrayDEqueue is itself an outlier in the collections.

[SethTisue]

sorry, I should really have commented on the forum thread instead. I will comment there.

[lihaoyi]

@tgodzik what's so disjoint about removeHead or removeLast or removeAll? I thought the names are pretty straightforward and look just like any other Scala collection APIs

[SethTisue]

@lihaoyi @tgodzik let's centralize design discussion (as opposed to implementation details) on the forum thread. I have responded there about the naming issue

[bishabosha]

@tgodzik @SethTisue well the discussion has had enough time on the forum (and i added a summary post this morning), so is it time to consider at core?

[SethTisue]

As I wrote on the forum thread, core briefly discussed and broadly approves, but core hasn't necessarily studied all the details, e.g. naming of individual methods, and the requests for improved documentation that came up on the forum.

So Jamie, if you and Haoyi are able to agree on and finalize those details, then I expect we'll be good.

Marking as draft for now.

[lihaoyi]

@bishabosha anything we need to do here? It feels like the discussion did not result in any requested changes

[lihaoyi]

Also feel free to just patch the PR yourself, if you have straightforward changes no need to go through me if you know what you want

[bishabosha]

@lihaoyi im just starting a vacation until August 16 so may be a bit slow

[lihaoyi]

@bishabosha this has been dragging on for close to a year now without any changes or updates, should I just close it?

[bishabosha]

I'm in favour of the change, so i'll put some time toward it tomorrow

[bishabosha]

I added documentation per method, and now on ArrayBuffer and ListBuffer the cost of the default implementation is minimised.

[tgodzik]

We discussed this today at the core meeting and this has green light. We just need to have it reviewed and the community build issue investigated.

[bishabosha]

So the pprint had a test suite that checked the toString output of a buffer expression - which changed. This is now rebased on a commit upstream that changes the test to avoid the specific data structure

• of course this PR (Buffer(...) now returns ArrayDeque) comes with all the same considerations that replacing List with Vector has, except maybe its more rare?