Add missing IArray operations #11354

nicolasstucki · 2021-02-09T10:35:45Z

Add missing operations
Adapt some existing operations
Remove some ClassTag from existing operations that did not need them
Added IArrayOps.WithFilter and IArrayBuilder

TODO

Solve Cannot fully construct extension method for IArray #11358 and add missing overloads that receive IArrays
Fix double definition of startsWith (removed default parameters)
Should operations with implicit conversion keep those conversions? See flatMap, WithFilter.flatMap, flatten, unzip, unzip3, transpose.
Where should we place the WithFilter and IArrayBuilder?

Left for later

Refactor collective extensions and add documentation

library/src/scala/IArray.scala

nicolasstucki · 2021-02-10T11:02:44Z

library/src/scala/IArray.scala

@@ -102,7 +100,7 @@ object opaques:

  /** Flattens a two-dimensional array by concatenating all its rows
    * into a single array. */
-  extension [T](arr: IArray[T]) def flatten[U: ClassTag](using T => Iterable[U]): IArray[U] =
+  extension [T](arr: IArray[T]) def flatten[U: ClassTag](using asIterable: T => Iterable[U]): IArray[U] =


If we are against adding new implicit conversions should we simplify the definitions of flatMap, WithFilter.flatMap, flatten, unzip, unzip3, transpose?

For this one the alternative is to define it as

extension [T](arr: IArray[Iterable[T]]) def flatten: IArray[T] = ...

We could also add an overload for

extension [T](arr: IArray[IArray[T]]) def flatten: IArray[T] = ...

@odersky @sjrd WDYT?

We'll need an implicit conversion from IArray[T] to Iterable[T] anyway, because I can to be able to give IArrays in the flatMap methods of List and other collections.

Should the conversion be to Iterable[T] or directly to immutable.ArraySeq[T]?

immutable.ArraySeq sounds reasonable to me.

This allowed me to remove many redundant operations. See the second commit.

The problem with using Conversion instances is that you get a feature warning.

there were 1 feature warning(s); re-run with -feature for details

If you run with -feature you get something like this

|Use of implicit conversion given instance genericWrapAnyRefIArray in object IArray should be enabled |by adding the import clause 'import scala.language.implicitConversions' |or by setting the compiler option -language:implicitConversions. |See the Scala docs for value scala.language.implicitConversions for a discussion |why the feature should be explicitly enabled.

To prevent this you can use an old-style implicit def. Or, alternatively, change checkImplicitConversionUseOK to make a special case for conversions coming from IArray. But the second alternative requires a discussion what the policy wrt implicit conversions should be. So I think it's easiest for now to simply revert to implicit defs for the conversions here.

You mean flatMap as in the following example?

for x <- List(1, 2) y <- IArray(1, 2) yield ...

~~For this, we do not need the implicit conversion.~~ Actually it did not work without the implicit conversion.

I used implicit defs and aligned the implementation with the one for Arrays

library/src/scala/IArray.scala

sjrd · 2021-02-11T10:25:35Z

library/src/scala/IArray.scala


  /** Conversion from IArray to immutable.ArraySeq */
-  extension [T](arr: IArray[T]) def toSeq: immutable.ArraySeq[T] =
-    immutable.ArraySeq.unsafeWrapArray(arr.asInstanceOf[Array[T]])
+  given genericWrapIArray[T](using DummyImplicit): Conversion[IArray[T], scala.collection.immutable.ArraySeq[T]] =


Why the DummyImplicits, here and below?

If I don't I get an error at call site

[error] -- [E007] Type Mismatch Error: library/src/scala/runtime/TupleXXL.scala:26:18 [error] 26 | while i < es.length do [error] | ^^ [error] |Found: (TupleXXL.this.es : IArray[Object]) [error] |Required: ?{ length: ? } [error] |Note that implicit extension methods cannot be applied because they are ambiguous; [error] |both object IArray in package scala and given instance genericWrapIArray in object IArray provide an extension method `length` on (TupleXXL.this.es : IArray[Object])

Maybe they should be placed in Predef as are the ones for Arrays. But adding them to stdLibPatches/Predef.scala crashed the compiler.

Avoided this with implicit def

library/src/scala/IArray.scala

sjrd · 2021-02-11T10:35:06Z

library/src/scala/IArray.scala

+  */
+  @SerialVersionUID(3L)
+  sealed abstract class IArrayBuilder[T]


Hum, now that I see it here, I don't think it's the right place for this one, in fact. ArrayBuilder is a top-level class in scala.collection.mutable, even though Array is in scala.*. By analogy, IArrayBuilder should also be a top-level class in scala.collection.mutable (yes, mutable, like StringBuilder).

Moved to scala.collection.mutable

odersky · 2021-02-11T10:41:25Z

library/src/scala/IArray.scala

+  /** A companion object for array builders.
+  */
+  object IArrayBuilder {


There should be a way to re-use array builders here. I believe there is an operation where you create a builder by using an existing builder and adding a post-processing step. In this case the post-processing step would be simply asInstanceOf[IArray[T]]. Can we use that one?

We could still have specialized versions for each primitive type, but we would not need to manually re-implement all the methods.

Indeed! This should probably be as easy as

@inline def makeIArrayBuilder[T: ClassTag]: Builder[T, IArray[T]] = mutable.ArrayBuilder.make[T].mapResult(r => r: IArray[T])

I reimplemeted IArrayBuilder and moved it to scala.collection.mutable.

sjrd

LGTM other than the dangerous import, and possibly using mapResult for the builder.

sjrd · 2021-02-11T13:44:08Z

library/src/scala/collection/mutable/IArrayBuilder.scala

+  *  @tparam T     type of the elements for the array builder, with a `ClassTag` context bound.
+  *  @return       a new empty array builder.
+  */
+  @inline def make[T: ClassTag]: IArrayBuilder[T] = {


I take it that using mapResult didn't work?

It works, but the signature must be Builder[T, IArray[T]] and it adds an extra indirection.

This would mean that we do not have the efficient addAll(xs: IArray[T]) and (xs: IArray[T], offset: Int, length: Int) on that builder.

I guess we could also have addAll(xs: Array[T])

We have 2 options:

Return IArrayBuilder[T]

Larger code

Efficient add for IArray/Array

Possible to create a specialized IArrayBuilder[T] for primitive types

Return Builder[T, IArray[T]]

Trivial code

In-efficient add for IArray/Array

Impossible to create a specialized Builder[T, IArray[T]] for primitive types

I'm inclined to have the IArrayBuilder[T] for performance reasons. Otherwise, we might see applications that use the ArrayBuilder and then cast it to IArray (or use IArray.unsafeFromArray)

Impossible to create a specialized Builder[T, IArray[T]] for primitive types

Hum, that's not the case. If you go through ArrayBuilder.make[T] followed by a mapResult, you will receive the specialized ArrayBuilder, which will create a specialized Array[T], and the only thing is that is going to be cast to an IArray[T] at the end of the day, which even when not specialized is a no-op. So we would definitely have the specialized builders for IArrays.

Now we couldn't have addAll specialized for IArray and Array, that's true. However addAll is virtually never used on Builders.

The issue was alluding to is that the result of mapResult does not have an overload of addAll that receives Arrays.

But if it is almost never used I will remove this.

These call use `java.lang.System.arraycopy` to add the elements

bishabosha · 2021-02-11T22:58:17Z

library/src/scala/IArray.scala

+   *  @param  it the iterable collection
+   *  @return    an array consisting of elements of the iterable collection
+   */
+  def from[A : ClassTag](it: IterableOnce[A]): Array[A] =


is this intended to return an Array?

Ouch. Nope.

I'll fix it

nicolasstucki self-assigned this Feb 9, 2021

nicolasstucki changed the title ~~Add missing i array operations~~ Add missing IArray operations Feb 9, 2021

nicolasstucki force-pushed the add-missing-IArray-operations branch 4 times, most recently from e86d821 to 79743c7 Compare February 9, 2021 14:35

nicolasstucki added this to the 3.0.0-RC1 milestone Feb 9, 2021

nicolasstucki force-pushed the add-missing-IArray-operations branch from 79743c7 to d5dcb7b Compare February 9, 2021 15:14

nicolasstucki mentioned this pull request Feb 9, 2021

Cannot fully construct extension method for IArray #11358

Closed

nicolasstucki added the RC1:high-prio label Feb 9, 2021

nicolasstucki force-pushed the add-missing-IArray-operations branch 8 times, most recently from fabcb58 to 16b2ed7 Compare February 10, 2021 10:45

nicolasstucki commented Feb 10, 2021

View reviewed changes

nicolasstucki force-pushed the add-missing-IArray-operations branch 8 times, most recently from 001b436 to a61aeea Compare February 11, 2021 07:55

Add missing IArray operations

85b699e

nicolasstucki force-pushed the add-missing-IArray-operations branch from a61aeea to 85b699e Compare February 11, 2021 08:24

Add converion from IArray to immutable.ArraySeq

04ea027

nicolasstucki marked this pull request as ready for review February 11, 2021 09:53

sjrd requested changes Feb 11, 2021

View reviewed changes

odersky requested changes Feb 11, 2021

View reviewed changes

nicolasstucki added 3 commits February 11, 2021 11:52

Add comments

8136b5c

Delegate implementation of IArrayBuilder to ArrayBuilder

a55b18c

Move IArrayBuilder to scala.collection.mutable

b26da40

nicolasstucki linked an issue Feb 11, 2021 that may be closed by this pull request

Review/fix definition of IArray #11298

Closed

nicolasstucki force-pushed the add-missing-IArray-operations branch from c687c65 to b26da40 Compare February 11, 2021 13:32

sjrd previously approved these changes Feb 11, 2021

View reviewed changes

Use implicit defs for conversions to avoid Dummy param

ad7ab97

nicolasstucki dismissed sjrd’s stale review via ad7ab97 February 11, 2021 13:50

nicolasstucki force-pushed the add-missing-IArray-operations branch from ee9f4aa to ad7ab97 Compare February 11, 2021 13:50

nicolasstucki added 2 commits February 11, 2021 15:03

Align converions with Array conversions in Predef

9039341

Cleanup imports

46c0540

nicolasstucki force-pushed the add-missing-IArray-operations branch from b6899a5 to 46c0540 Compare February 11, 2021 14:38

Make add all also recive Arrays

0b9c017

These call use `java.lang.System.arraycopy` to add the elements

nicolasstucki requested review from sjrd and odersky February 11, 2021 15:23

Remove IArrayBuilder

c265957

sjrd approved these changes Feb 11, 2021

View reviewed changes

nicolasstucki enabled auto-merge February 11, 2021 17:21

nicolasstucki unassigned sjrd Feb 11, 2021

odersky approved these changes Feb 11, 2021

View reviewed changes

nicolasstucki merged commit ae7bbd8 into scala:master Feb 11, 2021

nicolasstucki deleted the add-missing-IArray-operations branch February 11, 2021 22:23

bishabosha reviewed Feb 11, 2021

View reviewed changes

nicolasstucki mentioned this pull request Feb 12, 2021

Fix signature of IArray.from #11387

Merged

bishabosha mentioned this pull request Feb 12, 2021

REPL: chained extension methods on opaque types can only be called once #10044

Closed

Kordyjan modified the milestones: 3.0.0-RC1, 3.0.0 Aug 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing IArray operations #11354

Add missing IArray operations #11354

nicolasstucki commented Feb 9, 2021 •

edited

Loading

nicolasstucki Feb 10, 2021

sjrd Feb 10, 2021

nicolasstucki Feb 10, 2021 •

edited

Loading

sjrd Feb 10, 2021

nicolasstucki Feb 11, 2021

odersky Feb 11, 2021

nicolasstucki Feb 11, 2021 •

edited

Loading

nicolasstucki Feb 11, 2021

sjrd Feb 11, 2021

nicolasstucki Feb 11, 2021

nicolasstucki Feb 11, 2021

sjrd Feb 11, 2021

nicolasstucki Feb 11, 2021

odersky Feb 11, 2021 •

edited

Loading

odersky Feb 11, 2021 •

edited

Loading

sjrd Feb 11, 2021

nicolasstucki Feb 11, 2021

sjrd left a comment

sjrd Feb 11, 2021

nicolasstucki Feb 11, 2021

nicolasstucki Feb 11, 2021

nicolasstucki Feb 11, 2021 •

edited

Loading

sjrd Feb 11, 2021

nicolasstucki Feb 11, 2021

bishabosha Feb 11, 2021 •

edited

Loading

sjrd Feb 11, 2021

nicolasstucki Feb 12, 2021

nicolasstucki Feb 12, 2021

Add missing IArray operations #11354

Add missing IArray operations #11354

Conversation

nicolasstucki commented Feb 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolasstucki Feb 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolasstucki Feb 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odersky Feb 11, 2021 • edited Loading

Choose a reason for hiding this comment

odersky Feb 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjrd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolasstucki Feb 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bishabosha Feb 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolasstucki commented Feb 9, 2021 •

edited

Loading

nicolasstucki Feb 10, 2021 •

edited

Loading

nicolasstucki Feb 11, 2021 •

edited

Loading

odersky Feb 11, 2021 •

edited

Loading

odersky Feb 11, 2021 •

edited

Loading

nicolasstucki Feb 11, 2021 •

edited

Loading

bishabosha Feb 11, 2021 •

edited

Loading