`remove` operation in `mutable.Set` now returns an `Option`, whereas it used to return a `Boolean`

[julienrf]

This is a source of incompatibility. Do we want to keep returning Option? mutable.Map does return an Option, btw, so maybe this will be more consistent to return an option as well in mutable sets…

To make existing code bases compile with the new collections we can replace calls to xs.remove(x) with xs.remove(x).nonEmpty, when it appears in expression position. However, it’s not clear to me how we can support cross-building. We could implicitly enrich Boolean with a nonEmpty operation, so that the above code will also compile with old Scala versions, but this looks super hacky and error prone: what if users mistakenly call nonEmpty on a Boolean that doesn’t come from a remove operation?

Another solution would consist in keeping remove(elem: A): Boolean in the new collections, but also adding a delete(elem: A): Option[A] (and we could implement remove in terms of delete). But this is not really elegant either…

[sjrd]

What's the use case for Set.remove to remove an Option? Isn't it always the case that xs.remove(x) == Some(x) in cases where it succeeds? In that case, the Option does not convey more information than Boolean.

The consistency with Map argument is void because m.remove(key) returns an Option of value, not of key.

I suggest going back to Boolean.

[julienrf]

Isn't it always the case that xs.remove(x) == Some(x) in cases where it succeeds?

Yes, but xs.remove(x).forall(removed => x eq removed) might not be true, though. That’s the only use case I can think of.

[sjrd]

I don't think such an obscure use case is worth breaking compatibility.

[julienrf]

I agree with you.

@odersky, I think you introduced this change, can you tell us more on the motivation behind it?

[szeiger]

The new Set.remove is more consistent with Map.remove and Set.get. Both return an Option of the element. I'm on the fence on this one. It may be worth breaking compatibility to improve consistency

[sjrd]

@szeiger See my earlier comment where I argue that the consistency argument is moot.

[sjrd]

Also, apparently this is what's needed to make call sites cross-compile:

  implicit class BooleanNonEmpty(val boolean: Boolean) extends AnyVal {
    def nonEmpty: Boolean = boolean
  }

And that is seriously bad. Pimping nonEmpty on Boolean!

Please, please, please, revert this incompatibility. This is nonsense.

[NthPortal]

The new Set.remove is more consistent with Map.remove and Set.get

It is not, actually. Map[K, V].remove returns an Option[V]; Set[K] (which is kind of like a Map[K, Unit]) has no type V to return (or would return an Option[Unit]), so it makes more sense to return a Boolean.

If you want to have Set[A].remove return an Option[A], Map[K, V].remove should return an Option[(K, V)] to be consistent, which doesn't seem that reasonable to me.