Skip to content
This repository has been archived by the owner on Dec 22, 2021. It is now read-only.

Remove the boilerplate associated with the new design (and more) #468

Merged
merged 1 commit into from Feb 20, 2018

Conversation

szeiger
Copy link
Member

@szeiger szeiger commented Feb 20, 2018

  • The respective CC types are exposed (to protected[this]) through
    aliases IterableCC, SortedIterableCC, MapCC and SortedMapCC
    in the respective Ops traits, plus a BitSetC for the shared
    BitSetOps trait.

  • This allows defining fromSpecificIterable, newSpecificBuilder
    and empty (for all but Iterable) in the respective collection
    traits Iterable, SortedSet, Map and SortedMap (not in their
    Ops traits) where the CC type is visible and set to a concrete
    type but can still be refined in subtypes. This gives us a valid
    implementation until a concrete collection type with a refined CC
    gets defined. In this case the inherited implementations have the
    wrong type, so the user is forced to override them.

  • Implementations of fromSpecificIterable, newSpecificBuilder and
    empty can be removed from almost all collection implementations
    except the ones where they need to be refined even further than what
    a factory can provide (e.g. IntMap, PriorityQueue).

  • Factories are generally overridden in abstract collection types that
    refine a CC (e.g. Iterable -> Seq -> immutable.Set ->
    immutable.IndexedSeq) so that concrete implementations without
    further refinement do not need to override them.

  • DefaultMap is deprecated because there is no more boilerplate left
    that it could implement.

  • sortedFromIterable, mapFromIterable and sortedMapFromIterable
    are treated the same way as fromIterable. They are implemented as
    inline final calls to the respective factory method.

Fixes #335

Copy link
Contributor

@julienrf julienrf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting! It goes beyond what I thought we could achieve :)

I’m a little bit worried of introducing new abstract types. Even though they are defined as protected, they leak to the public API via iterableFactory: IterableFactory[IterableCC], etc.

def sortedIterableFactory: SortedIterableFactory[SortedIterableCC]

/** Similar to `fromSpecificIterable`, but for a (possibly) different type of element.
* Note that the return type is know `CC[E]`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: know => now. Also CC[E] => CC[B]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll update these comments to make more sense for the new design.


/** The wrapped mutable `Array` that backs this `ImmutableArray`. Any changes to this array will break
* the expected immutability. */
def unsafeArray: Array[A @uncheckedVariance]
// uncheckedVariance should be safe: Array[A] for reference types A is covariant at the JVM level. Array[A] for
// primitive types A can only be widened to Array[Any] which erases to Object.

protected[this] def fromSpecificIterable(coll: strawman.collection.Iterable[A]): ImmutableArray[A] = fromIterable(coll)
override protected[this] def fromSpecificIterable(coll: strawman.collection.Iterable[A]): ImmutableArray[A] = fromIterable(coll)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here what’s the difference with the base implementation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong and was equally wrong before. We need to build using elemTag here.

@julienrf
Copy link
Contributor

You will have to update collections-contrib as well.

@lrytz
Copy link
Member

lrytz commented Feb 20, 2018

This is really great!!

@szeiger szeiger force-pushed the wip/default-iterable-factory branch 2 times, most recently from 803ea97 to 0cbb677 Compare February 20, 2018 12:44
@szeiger
Copy link
Member Author

szeiger commented Feb 20, 2018

The new type members are unavoidable. They are merely aliases for existing type parameters, not abstract types. They ensure that the collection traits can declare the correct return types depending on some CC even though CC is fixed at that point (but can be covariantly refined). This allows us to provide concrete implementations even though the types can be changed in incompatible ways in subclasses.

This started as a plan B after running into scala/bug#10725 but even without that problem my original approach would not have worked because concrete definitions win over abstract ones. It might still work in Dotty if it has gotten rid of that rule in favor of proper type unification.

- The respective `CC` types are exposed (to `protected[this]`) through
  aliases `IterableCC`, `SortedIterableCC`, `MapCC` and `SortedMapCC`
  in the respective `Ops` traits, plus a `BitSetC` for the shared
  `BitSetOps` trait.

- This allows defining `fromSpecificIterable`, `newSpecificBuilder`
  and `empty` (for all but `Iterable`) in the respective collection
  traits `Iterable`, `SortedSet`, `Map` and `SortedMap` (*not* in their
  `Ops` traits) where the `CC` type is visible and set to a concrete
  type but can still be refined in subtypes. This gives us a valid
  implementation until a concrete collection type with a refined `CC`
  gets defined. In this case the inherited implementations have the
  wrong type, so the user is forced to override them.

- Implementations of `fromSpecificIterable`, `newSpecificBuilder` and
  `empty` can be removed from almost all collection implementations
  except the ones where they need to be refined even further than what
  a factory can provide (e.g. `IntMap`, `PriorityQueue`).

- Factories are generally overridden in abstract collection types that
  refine a `CC` (e.g. `Iterable` -> `Seq` -> `immutable.Set` ->
  `immutable.IndexedSeq`) so that concrete implementations without
  further refinement do not need to override them.

- DefaultMap is deprecated because there is no more boilerplate left
  that it could implement.

- `sortedFromIterable`, `mapFromIterable` and `sortedMapFromIterable`
  are treated the same way as `fromIterable`. They are implemented as
  `inline final` calls to the respective factory method.

Fixes #335
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants