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

Remove operations of Iterator #7

Closed
wants to merge 2 commits into from
Closed

Remove operations of Iterator #7

wants to merge 2 commits into from

Conversation

julienrf
Copy link
Contributor

  • Remove all the operations of Iterator. The rationale is that most of these operations were leaving the underlying Iterator in a stale state.
  • Introduce a class Iterating, which provides the exact same operations as Iterator used to provide, but takes care of instantiating a fresh Iterator each time, so that there is no risk of manipulating a stale Iterator.
  • Remove IterableOnce: this type was confusing since an IterableOnce was iterable more than once as long as we use a fresh Iterator each time.

@odersky
Copy link
Contributor

odersky commented Dec 27, 2016

I am skeptical about this one. I think if the contract of Iterator is understood, it's quite useful.
The point is, even next on an iterator does not maintain the old iterator. So the only repeatable operation is hasNext. In essence, iterators are inherently linear. An iterator stops being defined after any operation is applied to it. And I would argue there is a place for such things!

Besides, this probably falls under the category of "too much change". How would you migrate a program that uses full collection ops on iterators?

@odersky
Copy link
Contributor

odersky commented Dec 27, 2016

I believe an interesting possibility would be to add an immutable Iterator. Instead of next, such an iterator would have head and tail. Essentially that would be a LazyList without caching.

@odersky
Copy link
Contributor

odersky commented Dec 27, 2016

Another thought: How about we extend this notation of linearity to mutable collections? @lihaoyi has once remarked that he finds most operations on mutable collections useless whereas the variants that are useful are often missing, If I remember correctly, he mentioned operations like map and filter. I think the default should still be that these operations return new collections, because that's the cleanest semantics and for small collections performance does not matter. Remember, a good guideline is that most collections are small.

But what about adding a subclass of linear collections that do have mutation behavior?. There should be an operator - call it for now linear until we find a better name - that turns any mutable collection into a linear collection (without copying it). Then one could write:

 xs.linear.filter(p).map(f) ++ elems

and the result would be the collection xs, with all elements that do not satisfy p removed, with the remaining elements mapped via f and with elems added. f must be a function from the collection's element type to itself.

We could probably achieve that by adding the following to scala.collection.mutable.Iterable

def linear: this.type & Linear

and defining the necessary operations in trait Linear. These operations would be based on replace, remove and add which in turn need to be defined by all mutable collections.

WDYT?

@lihaoyi
Copy link

lihaoyi commented Dec 27, 2016

My main complaint with mutable collections was that the immutable operations are generally not useful. This is an empirical rather than a theoretical argument, but if I have a mutable.Buffer, it is always because I want to perform operations in place due to performance or whatever. That means map, filter, flatMap, and basically every other immutable transformation isn't commonly useful. They may be useful occasionally, but are uncommon enough that they should be namespaced and accessed via e.g. myBuffer.transform.map(...).filter(...) or something similar. These operations are used infrequently enough that they should not be "top level" methods available on every mutable collection.

The "aggregation" operations like sum, fold, reduce etc. are still as commonly useful on mutable collections as immutable ones, at least in my experience.

Apart from that @odersky is right that I believe commonly needed operations are not provided. For example, mutable.ArrayBuffer has .append but does not have a .pop operation that is the reverse of append (remove the last element and return it), meaning I've written boilerplate snippets like myBuffer.remove(myBuffer.length-1) hundreds of times. Other missing useful operations are pop(n: Int) (basically the inverse of appendAll), in-place versions of the various set operations on mutable.Set (intersect, union, ...), and probably others I can't recite off the top of my head.

Having a set of in-place operations on the mutable collections that mirror the copying-transformation operations on the immutable collections would be something I could get behind. For example, Java 8 now has operations on such as

replaceAll(UnaryOperator<E> operator)
Replaces each element of this list with the result of applying the operator to that element.

removeIf(Predicate<? super E> filter)
Removes all of the elements of this collection that satisfy the given predicate.

on java.util.ArrayList, and I think they would be just as useful on Scala's mutable collections. I'm not sure exactly what set of the transformations on immutable could be provided in-place on the mutable collections - clearly something like .map can't work because the type needs to be the same - but presumably some useful subset could be chosen.

If we do this, I believe that the symmetry between e.g. myImmutableList.filter(p), myMutableBuffer.transform.filter(p) myMutableBuffer.linear.filter(p) would be quite a nice thing to have: there would be no ambiguity whether an operation on myMutableBuffer is in-place or copying, and someone exploring the API looking for useful methods like append/remove/pop/etc. would not be flooded with a mess of theoretically-simple but practically-useless API methods that will only get called once in a blue moon

@odersky
Copy link
Contributor

odersky commented Dec 27, 2016

For example, Java 8 now has operations on such as

replaceAll(UnaryOperator<E> operator)
Replaces each element of this list with the result of applying the operator to that element.

removeIf(Predicate<? super E> filter)
Removes all of the elements of this collection that satisfy the given predicate.

My main problem with this approach is that it further increases the already large number of different names for collection operations. That's why I think in the end

xs.inPlace.filter(p).map(f) ++ elems

is easier to remember than

xs.keepOnlyMatching(p).transform(f).appendAll(elems)

or something like that. (I now think that inPlace is a better name than linear.)

clearly something like .map can't work because the type needs to be the same - but presumably some useful subset could be chosen.

Yes, map on InPlace (formerly known as Linear) needs to have type

def map(f: Elem => Elem): this.type

where Elem is the element type of the collection.

@odersky
Copy link
Contributor

odersky commented Dec 27, 2016

This also means that linear collections would have two maps. One is defined like this:

trait InPlace[Elem] {
  def map(f: Elem => Elem): this.type
}

The other is the usual map in Iterable which can change the element type. We have precedent for this in the way String is defined in the strawman. One map on String takes a function of type Char => Char and yields a String, the other yields a sequence.

@julienrf
Copy link
Contributor Author

I’m not in favor of removing operations from mutable collections. I just wanted to make Iterators manipulation less error-prone (see this comment for more motivation).

I agree that operations on mutable collections should modify the collections in place. I think that this should even be the default behavior, but I understand that for backward compatibility we will have to put these operations under a different namespace (I like inPlace).

@julienrf
Copy link
Contributor Author

Also, I would be happy to deprecate the operations that return a new collection on mutable collections. I think the ones that mutate the collection in place should be preferred.

By the way, note that in the current design, mutable collections do not have the +: and :+ operators (but they do inherit from map and filter, though).

@odersky
Copy link
Contributor

odersky commented Dec 27, 2016

Also, I would be happy to deprecate the operations that return a new collection on mutable collections. I think the ones that mutate the collection in place should be preferred.

The problem with that is that there is a lot of code out there that uses things like:

val xs: Array[Int] = ...
xs.map(_ + 1)

I know of two Scala books and intro courses that promote this style. And why not? As long as the arrays are not humongous this is perfectly reasonable.

@odersky
Copy link
Contributor

odersky commented Dec 27, 2016

Since we have seen several discussions with interesting/difficult tradeoffs, I have tried to summarize my philosophy that guided the original Scala collections design. Here are some rules, which I think are still valid:

  • primary goal: have a set of operations that's as small as possible and that at the same time covers as many use-cases as possible. Here, size = number of operation names.

  • primary goal: as many operations as possible should be available on all forms of collections. If a common operation makes sense on a specific collection in some scenarios, include it.

  • side note: most collections are small, so big O performance is secondary.

  • secondary goal: avoid non-sensical or badly performing collection operations.

The secondary goal should be followed only as long as it does not conflict with the primary goals.

Also a goal now (somewhat orthogonal to the others) is to follow the principle of least power. Pick the simplest interface that can be made to fit the requirements. It's here that I believe we can improve on the current collections design.

@julienrf julienrf changed the base branch from mutable-immutable to master December 29, 2016 09:56
@szeiger
Copy link
Contributor

szeiger commented Jan 4, 2017

Regarding the changes in this PR: I'm in favor of keeping the Iterator methods in general. From what I've seen the linear semantics rarely cause problems in practice. Iterating OTOH looks too similar to View without being able to provide the same interface. Having both is probably overkill. It cannot fully replace the Iterator methods, either. In many cases (I/O in particular) you can only iterate once. Iterator currently provides useful methods for this case.

I agree with the general goals above. When it comes to in-place semantics, goal 2 pretty much mandates that shared operations are immutable and not in-place. And in my opinion that makes perfect sense. I'll have to disagree with @lihaoyi here. In my experience, in many cases you only "consume" a collection that you get from somewhere and you do not care how it was built (i.e. if it is really mutable or immutable). If you request a generic collection type (not mutable or immutable) there is an unwritten contract that the collection will not change behind the scenes while you use it (along with a guarantee that you only use it for an "obvious" duration, usually until the method returns). Any method that you call on such a generic collection should not break this contract in surprising ways. This requires that you treat all collections as de-facto immutable for these operations.

So where do we put the mutating methods? I think that def linear: this.type & Linear won't work. (Has the meaning of this.type changed in Dotty? You couldn't possibly return the singleton type with a new non-phantom type mixed in, right? I'll assume that you mean something along the lines of Repr with Linear for the remainder.) Since Repr with Linear <: Repr you couldn't tell anymore whether calling map or filter on Repr will be performed in place or by copying. If these methods come from a generic supertype, they will also break the contract that I mentioned in the previous paragraph. The case of the overloaded map is similarly confusing. Yes, you could overload it so that Linear only handles the version with the same element type, but then you end up with a Linear view of a collection where map has grossly different meanings depending on the static return type. One version updates in place, the other gives you a copy (or throws an exception).

If you want to move the mutating methods into a separate linear view trait it must not inherit from any collection type. I think I would prefer methods with different names on the mutable collection traits though (mapInPlace, filterInPlace, etc.). Otherwise you'll end up duplicating methods like ++= on both the collection and the linear view, or you have to remove them all from the collection trait, but then it's not really a mutable collection anymore but a generic one.

@lihaoyi
Copy link

lihaoyi commented Jan 4, 2017

The problem with that is that there is a lot of code out there that uses things like:

val xs: Array[Int] = ...
xs.map(_ + 1)
I know of two Scala books and intro courses that promote this style. And why not? As long as the arrays are not humongous this is perfectly reasonable.

I think this cuts at the heart of the problem: how much an API is used in practice is a key factor when considering whether it should be kept around.

I can accept that Array is special and often treated like an immutable collection. But how often do you see people calling mutable.Buffer#map? Or mutable.Set#map? Or mutable.Queue#map? I've been writing Scala for a few years and never bumped into any of these.

I do not have the answer to this question, but I think there's a path to one if we can invest the manpower: instrument the community build and dump out compile-time and run-time call-site statistics of all these collection methods, to see which ones people call in practice, and how often. This wouldn't be anywhere near a complete snapshot of the Scala world, but it would give us a good sense of the rough ratios between the importance of different methods. I expect (???) it would show most immutable operations on mutable collections, save for a select few like Array, get hardly any use at all.

I don't have the bandwidth the perform this research, but it seems to me that if we're going through the trouble of overhauling the collections and their internals, it would be worth someone first putting in the work to figure out how they're used right now 😛.

I think this point also supports my proposal:

secondary goal: avoid non-sensical or badly performing collection operations.

I think a method that's been defined for years but gets hardly any usage "in the wild" is almost the definition of nonsensical: even if the author of the method thinks it's awesome and elegant, clearly none of the users think it makes sense to use. Some empirical analysis of current usage patterns would tell us in-concrete-terms which of the methods users think are nonsensical, which I think is a better measure than what our internal-intuition tells us.

If you request a generic collection type (not mutable or immutable) there is an unwritten contract that the collection will not change behind the scenes while you use it (along with a guarantee that you only use it for an "obvious" duration, usually until the method returns). Any method that you call on such a generic collection should not break this contract in surprising ways.

I agree with this, but I don't think it's a big deal. We could easily (???) set it up such that mutable collections have immutable views, that are accessible through .view or .readonly or something, that provide the relevant operations only to the people who want them.

Just as an immutable collection would have only immutable operations by default and a .linear view to do mutable stuff, a mutable collection would have only mutable operations by default and a .readonly view to do immutable stuff. I think the symmetry is quite compelling, though empirical research into the usage patterns in the community build would tell us if this actually fits how people use collections in practice.

Again, I believe Array is probably a special case (partly due to it's privileged position as the one-true-unboxed-collection) but that doesn't mean all the other mutable collections should be special too

@Ichoran
Copy link
Contributor

Ichoran commented Jan 4, 2017

I guess I should probably do this research, @lihaoyi, given that I already did it when we were modifying the collections in 2.10/2.11. I'll dust off my bytecode analyzer sometime soon, but it's going to take a couple of weeks, probably, to actually have an answer.

@odersky
Copy link
Contributor

odersky commented Jan 5, 2017

So where do we put the mutating methods? I think that def linear: this.type & Linear won't work. (Has the meaning of this.type changed in Dotty? You couldn't possibly return the singleton type with a new non-phantom type mixed in, right?

Yes, this.type & InPlace would require serious compiler magic to make it work. I now think it should just be InPlace, and that InPlace should only contain the operations that make sense to be applied in place. Result type of each of these is this.type (with underlying type inPlace), so they can be chained. But in the end, typical usage would throw away the InPlace instance. E.g.

 xs.inPlace.map(f).filter(p) ++ elems
 // continue with xs

I still do think we should use the same names. If not what naming scheme would we use for mutating methods?

@julienrf
Copy link
Contributor Author

julienrf commented Jan 5, 2017

I can accept that Array is special and often treated like an immutable collection. But how often do you see people calling mutable.Buffer#map? Or mutable.Set#map? Or mutable.Queue#map? I've been writing Scala for a few years and never bumped into any of these.

A pattern that I occasionally see is to use e.g. an ArrayBuffer to build a collection and then directly pass it to some method that expects a Seq. This method typically uses map, filter, etc. but the underlying (runtime) type of the collection is ArrayBuffer.

Just as an immutable collection would have only immutable operations by default and a .linear view to do mutable stuff

.linear would first do a copy of the immutable collection, right?

@lihaoyi
Copy link

lihaoyi commented Jan 5, 2017 via email

@julienrf
Copy link
Contributor Author

julienrf commented Jan 5, 2017

No idea if .linear makes a copy of the immutable collection

Actually for most of the collections (e.g. List) that would be impossible to implement .linear without first copying the elements. But even if this was possible it would be wrong to do it because that would break the immutability contract.

I like the idea of using .readonly (or rather .view, as it is currently implemented) to get an immutable view of a mutable collection. Again, I would be happy to deprecate map, filter, etc. on mutable collections, in favor of using .view.map, etc.

A good benefit of that would be that we would not anymore have a reason to share the same hierarchy between mutable and immutable collections. Having distinct hierarchies would make the code a lot cleaner (e.g. some @uncheckedVariance and asInstanceOf would go away).

@dwijnand
Copy link
Member

dwijnand commented Jan 5, 2017

Complete yak-shave, but you might be interested to follow the JDK's vernacular by saying "unmodifiable view" rather than "immutable view".

See http://stackoverflow.com/q/8892350/463761

@szeiger
Copy link
Contributor

szeiger commented Jan 5, 2017

I agree with this, but I don't think it's a big deal. We could easily (???) set it up such that mutable collections have immutable views, that are accessible through .view or .readonly or something, that provide the relevant operations only to the people who want them.

Yes, that's an interesting alternative. I expect it will require extensive rewriting of existing code though. It's basically a removal of all generic collection types.

I still do think we should use the same names. If not what naming scheme would we use for mutating methods?

I don't have a preference for using different names but for putting the methods on the base traits together with the immutable versions, in which case they would have to use different names. This point is moot if we remove all shared immutable methods, of course. Let's assume we keep the generic collections: The mutable collections have lots of mutating methods like +=. If you remove them, there's nothing left that separates the mutable collection trait from the generic one. If you keep them, then why would you have a mutable map on a separate in-place view? For-comprehensions could be a good reason. Other than that it seems counter-intuitive to split up the mutating methods between the base trait and an in-place view. If you already have some mutating methods in the base trait, keep them all together.

Actually for most of the collections (e.g. List) that would be impossible to implement .linear without first copying the elements. But even if this was possible it would be wrong to do it because that would break the immutability contract.

Agreed. The symmetry of .linear and .readonly is not quite as useful as it appears at first glance. .linear requires knowledge about the future of a mutable object (which is easy if you simply don't use it anymore) while .readonly requires knowledge of the past of an immutable object. When you have a mutable collection you must already know where it is shared and who is allowed to change it at what time. Calling .readonly requires that you promise not to modify it in the future until you're done using the read-only view. On the other hand, if you have an immutable collection you generally do not know anything about structural sharing, and you have no way of knowing it because it is considered an implementation detail. But such knowledge of the genesis of an immutable collection would be required in order to call a version of .linear that does not copy the data.

From the discussion so far the major choice we have to make boils down to:

  • Either keep generic collection classes with common methods that assume a de-facto immutable collection;
  • Or remove the common abstraction and add explicit read-only views of mutable collections.

@julienrf
Copy link
Contributor Author

julienrf commented Jan 5, 2017

xs.inPlace.map(f).filter(p) ++ elems

I like this design, though I would not include ++ in the set of methods since we will already have ++=.

@szeiger
Copy link
Contributor

szeiger commented Jan 5, 2017

I like this design, though I would not include ++ in the set of methods since we will already have ++=.

Where would ++= be defined? In the main collection type, the in-place view, or both?

@julienrf
Copy link
Contributor Author

julienrf commented Jan 5, 2017

The main collection type.

@Ichoran
Copy link
Contributor

Ichoran commented Jan 5, 2017

There is an inherent tension here that hasn't been called out between correctness and readability.

It's really important to know whether your operations are mutating in place or not, if it's up to you to manage mutability. (A Rust-like borrow checker mostly removes this requirement.) And that suggests that code like the following:

val ys = xs.inPlace
val zs = ys.map(_+ 1).filter(_ > 15)

while delightfully readable and highly familiar, is potentially dangerously deceptive.

This suggests to me that we not split out methods into an inPlace view because it then will be too easy to lose the distinction between mutable and immutable operations. Also, the inPlace views will require extra machinery to make work, which occasionally can be really nice, but really impedes extensibility in the cases where I have employed it, especially if you optimize for efficiency (and you should, since efficiency is one of the main use-cases for mutable collections).

Thus, I suggest instead that we adopt a consistent naming scheme per method, so it's obvious every time that you use a method that it's the mutable version. InPlace is awfully long; maybe just Me is enough.

val xs = new ArrayBuffer[Int]
(xs += 7).mapMe(_ + 8).flatMapMe(1 to _).filterMe(_ % 2 == 0)
println(xs)

If Me seems inaccurate (since you're always mapping "me" in the sense that the method acts on the collection who invokes it), Here is slightly longer alternative that doesn't have this problem. @som-snytt can probably provide other ideas (like an en prefix, xs.enmap(_ + 1).enfilter(_ % 2 == 0), and Som will probably tell us that it needs to be em before b, m, and p, so it is xs.emmap(_ + 1).enfilter(_ % 2 == 0)).

Operators can be suffixed with =.

In any case, I think practically, without additional features like compiler-supported aliased views that are truly zero-cost (not "maybe zero cost after the JVM gets done with them" like value classes are), the direct methods should at least be provided as options even if you do have an inPlace method that aliases mapInPlace to simply map and so on.

@som-snytt
Copy link

som-snytt commented Jan 5, 2017

@Ichoran I'd be remiss if I didn't share this Shakespearean syllabic syllogism from which I'd propose xs.asMap(_ + 1) etc. I can never unsee "as" ("ass") as a prefix. Though I guess it is properly a suffix, because "ass" is always the "back end".

Also, +1 to I want to know if op is mutating. Also +1 to the set of Odersky intuitions, modulo I can't remember 50 operators, and for example iterator.headOption doesn't work for some reason (as I just rediscovered). I could remember N ops but not if they are sometimes this.type. I like the proposal to change the namespace for mutable ops, whatever the name. I think .ass would work well to warn users of mutable API, "You ass!"

@lihaoyi
Copy link

lihaoyi commented Jan 5, 2017

while delightfully readable and highly familiar, is potentially dangerously deceptive.

I don't think it's that deceptive; the types are different, trying to pass a Linear[immutable.Seq[T]] where an immutable.Seq[T] is required will fail (I think???), and if you are confused at any point your editor should be happy to tell you what something is.

In a way, it's no different from overloading ++= or += to immutable-append on immutable collections and in-place-append on mutable collections, which we (and every other language) already do. Yes I've gotten confused before once, as have others, but I think the symmetry you use every day would be worth the pretty-rare confusion.

From the discussion so far the major choice we have to make boils down to:

  • Either keep generic collection classes with common methods that assume a de-facto immutable collection;
  • Or remove the common abstraction and add explicit read-only views of mutable collections.

My vote would be to remove the common abstraction in favor of read-only views 😛 I think that

  • "trying to make too many things implement the same generic interface, past the point of usefulness"

Is one of the primary failings of Scala collections, worse than CanBuildFrom. While CanBuildFrom confuses you once in a while, having a flood of nonsensical methods floods my autocomplete and makes it hard to pick out what methods I actually want.

Things like immutable.Set#zip or immutable.List#foldRight are small examples, but I think the sharing of almost every method between mutable and immutable collections is definitely past-the-point-of-usefulness. And we're not really "removing" them either: just namespacing them within a .readonly or .view call so if you want to see all these methods, or if you want to do "dangerous" things like pass a mutable collections into someone expecting an immutable one, you have to state your intention clearly (with .view).

While people complain about Scala's immutable collections, IMO this flood of nonsensical methods greatly reduces the quality of Scala's mutable collections, far below that of most other languages' such as Java's or Python's. At least for those, the operations on a mutable collection generally tell me how I do the things I want. In Scala, trying to scroll through the autocomplete or the scaladoc for an unfamiliar collection such as mutable.Queue or mutable.Set is an exercise in frustration. As an exercise, try digging through the scaladoc and seeing if an in-place "XOR"/"difference" method exists on mutable.Set. Whether you end up finding the method or not is immaterial; just digging through trying to find it is frustrating.

But as I'm not the one doing the bulk of the work (...implementation, and the migration adding .views all over the community build...), I'm happy to defer to defer to whatever you guys end up deciding on.

@lrytz
Copy link
Member

lrytz commented Jan 6, 2017

I vote against using the same names for in-place operations, I agree with Rex's comment above. Having same-named methods doing different things (and having different signatures) can confuse learners. Some operations would be changed to in-place (map), but not others (foreach, forall). Using for-comprehensions could lead to very confusing results.

@julienrf
Copy link
Contributor Author

julienrf commented Jan 6, 2017

  • Either keep generic collection classes with common methods that assume a de-facto immutable collection;
  • Or remove the common abstraction and add explicit read-only views of mutable collections.

As stated above, I’m in favor in removing the common abstraction.

Also, I’m convinced by arguments of @lrytz, @Ichoran and @szeiger: I finally see more advantages in putting all the mutating methods in the collection type (rather than going through an inPlace: InPlace member). They should have different names than their immutable analogs, but so far I’m not super happy with any of the given propositions. What about xs.inPlaceMap(f).inPlaceFilter(p)? Or xs.mapThis(f).filterThis(p)?

@odersky
Copy link
Contributor

odersky commented Jan 7, 2017

I could live with either .inPlace or a systematic scheme that re-uses the immutable operation names and adds a prefix or suffix. The one thing I think is important is that we should not try to come up with creative new names for linear versions of immutable operations. My current favorite would be mapInPlace, filterInPlace, but if someone has a better idea, I am happy to change my preference.

@odersky
Copy link
Contributor

odersky commented Jan 7, 2017

As far as common superclasses of mutable/immutable go, I continue to lean strongly towards keeping them, for the following reasons:

  • Backwards compatibility. It would be a huge can of worms to remove this.
  • Functional operations are perfectly reasonable for small mutable collections.
  • Functional operations are mandatory for arrays. Keeping them for arrays and not having them for other mutable collections breaks orthogonality, which is one of the defining principles of Scala collections.

I agree that noise in ScalaDoc is a problem. Maybe this can be alleviated by massaging the doc-comments. I.e. clearly state that ++, map, flatMap, and so on create a new collection and point to in-place versions where available.

@odersky
Copy link
Contributor

odersky commented Jan 8, 2017

After thinking some more about it, I believe we could experiment with a scheme like this one:

  • Keep common superclasses in collection
  • But have immutable transformers (the methods in IterableMonoTranforms and IterablePolyTransforms) only in immutable collections, and in selected mutable collections, including arrays and iterators (actually neither is strictly speaking a mutable collection, but they are close).

With our decorator-based design, this is easy to achieve, I believe. We could then experiment and see how much of the community build breaks. Based on the results, we can either tweak the scheme, or go back to the current scheme where all collections support all transforms. WDYT?

@julienrf
Copy link
Contributor Author

julienrf commented Jan 9, 2017

Functional operations are mandatory for arrays.

Even if we have immutable arrays instead?

@julienrf
Copy link
Contributor Author

julienrf commented Jan 9, 2017

Also, about backward source compatibility: what do you think of introducing a deprecated implicit conversion from mutable.Iterable[A] to IterableMonoTransforms[A] & IterablePolyTransforms[A] pointing users to explicitly using .unmodifiableView instead?

@odersky
Copy link
Contributor

odersky commented Jan 9, 2017

@julienrf Yes, I think even if we have immutable arrays, we still need functional ops on normal arrays. There are simply too many books written that use this idiom.

@odersky
Copy link
Contributor

odersky commented Jan 9, 2017

Also, about backward source compatibility: what do you think of introducing a deprecated implicit conversion from mutable.Iterable[A] to IterableMonoTransforms[A] & IterablePolyTransforms[A] pointing users to explicitly using .unmodifiableView instead?

I think deprecation could work well here. Let's try it, and see how much breaks.

One other question: Do we want to keep mutable views? That's something I would love to get rid of, and I don't think they were used a lot (this needs to be verified). If we don't have mutable views, then we can just stick to .view for all collections, so we do not need to introduce a separate unmodifiableView.

@DarkDimius
Copy link

Do we want to keep mutable views?

While I feel like immutable views have a lot of uses, I don't see that for mutable ones, on the opposite: I think they are a possible source of surprising bugs, due to laziness and caching on views.

I also agree that we should remove operations from iterator, as this has caused multiple bugs and misconceptions, both for newcomers and even for experienced developers(happened twice in the dotty codebase)

I would also prefer to keep operations on mutable collections: people coming from Java find it very useful that arrays are a collection that works uniformly with other collections.

@szeiger
Copy link
Contributor

szeiger commented Jan 9, 2017

I agree on removing mutable views and only having views as reified operations on Iterators.

Would it help the situation with Iterator operations if we removed TraversableOnce (or at least its methods)? Does the current confusion arise from collections and Iterators sharing these operations through a common supertype? I think that transformations on Iterators are simply too useful to remove them. Adding an extra .iterator before a chain of map/filter/etc. calls is an easy way to improve performance in many cases.

@julienrf
Copy link
Contributor Author

julienrf commented Jan 9, 2017

Do we want to keep mutable views? That's something I would love to get rid of, and I don't think they were used a lot (this needs to be verified)

I’m currently running the community build with a scala-library were mutable views are removed (you can see the patch here, please tell me if I missed something!). So far it runs fine but please tell me if I forgot something in the patch!

@Ichoran
Copy link
Contributor

Ichoran commented Jan 9, 2017

Mutable views were a minefield of surprising behavior, bugs, and lack of functionality, so they weren't used much. (Last time I scanned the bytecode of a bunch of projects, I'm not sure they came up at all.)

However, that doesn't mean mutable collections shouldn't have views; it just means that if they do have views they need their own style of views with operations that are appropriate. Even if immutable and mutable collections share a common parent, their views need not necessarily.

@julienrf
Copy link
Contributor Author

julienrf commented Jan 9, 2017

it just means that if they do have views they need their own style of views with operations that are appropriate.

Which operations do you have in mind that would be appropriate for mutable collections only but would not constitute a minefield of surprising behavior?

@Ichoran
Copy link
Contributor

Ichoran commented Jan 9, 2017

Things like update and mapInPlace. Anything that alters the contents but not the number of elements in the collection. Also, mutable views do not sensibly support filter operations in the usual sense. (A reindexing view that was built from filter results is possible, though.)

@odersky
Copy link
Contributor

odersky commented Jan 10, 2017

Would it help the situation with Iterator operations if we removed TraversableOnce (or at least its methods)? Does the current confusion arise from collections and Iterators sharing these operations through a common supertype? I think that transformations on Iterators are simply too useful to remove them. Adding an extra .iterator before a chain of map/filter/etc. calls is an easy way to improve performance in many cases.

I don't think so. TraversableOnce was introduced after the other collections library (I think it was in 2.9) to solve a real pain point: Every operation taking an argument to be iterated over had to be duplicated, once for an Iterator argument, the other for an Iterable argument. Sometimes someone forgot, and had just one kind of argument, and then users got burned. So TraversableOnce (or IterableOnce which is what we have now) is really important.

@szeiger szeiger mentioned this pull request Jan 10, 2017
@szeiger
Copy link
Contributor

szeiger commented Jan 10, 2017

I've created a separate ticket for discussing views to avoid derailing the discussion here.

@julienrf
Copy link
Contributor Author

I’m closing this PR, let’s continue the discussion about views in the other issue.

@julienrf julienrf closed this Jan 16, 2017
@julienrf julienrf deleted the iterating branch June 28, 2017 09:14
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.

9 participants