Topic/tidy collections #3103

Merged
merged 1 commit into from Nov 8, 2013

Projects

None yet

7 participants

@Ichoran
Collaborator
Ichoran commented Nov 6, 2013

This set of commits significantly cleans up the interface of the collections library. In particular, it deprecates collections that are either broken or will continually break due to needing too much manual attention to keep them consistent: SynchronizedColl, CollProxy, CollForwarder, and ImmutableCollAdaptor. It also attempts to deprecate inheritance and/or overriding where these things do not make sense so that there is more flexibility to alter internal representations and/or less danger of inadvertently introducing inconsistent behavior. In particular, many immutable classes that implement nontrivial collections have deprecated inheritance (in many cases they should become sealed not final); certain methods have deprecated overriding (including CollLike toColl almost always returning self, += forwarding to put or add in maps). Finally, a few scarcely-used classes or traits and/or those with idiosyncratic APIs have been deprecated, including all of collection.script and all the low-level mutable buffers (LinkedListLike and friends).

@adriaanm
Member
adriaanm commented Nov 6, 2013

.check files for test with -deprecation in their .flags will need to be updated
partest-ack and partest --update-check are your friends

@paulp paulp and 1 other commented on an outdated diff Nov 6, 2013
src/repl/scala/tools/nsc/interpreter/JavapClass.scala
}
override def clear() = diagnostics.clear()
/** All diagnostic messages.
* @param locale Locale for diagnostic messages, null by default.
*/
- def messages(implicit locale: Locale = null) = (diagnostics map (_ getMessage locale)).toList
+ def messages(implicit locale: Locale = null) = {
+ val it = diagnostics.iterator
+ val lb = List.newBuilder[String]
+ while (it.hasNext) lb += it.next.getMessage(locale)
+ lb.result
+ }
@paulp
paulp Nov 6, 2013

Why hand roll this? diagnostics.iterator.asScala.toList map (_ getMessage locale)

@Ichoran
Ichoran Nov 6, 2013 collaborator

I was optimizing unnecessarily, I guess. I doubt the method is used heavily enough to warrant it (and if it is, manual list creation isn't the best solution).

@Ichoran Ichoran Collections library tidying and deprecation. Separate parts are liste…
…d below.

Collections library tidying, part one: scripting.

Everything in scala.collection.scripting is deprecated now, along with the
<< method that is implemented in a few other classes.  Scripting does not
seem used at all, and anyone who did can easily write a wrapper that does
the same thing.

Deprecated *Proxy collections.

The only place proxies were used in the library was in swing.ListView, and
that was easy to change to a lazy val.

Proxy itself is used in ScalaNumberProxy and such, so it was left
undeprecated.

Deprecated Synchronized* traits from collections.

Synchronizability does not compose well, and it requires careful examination
of every method (which has not actually been done).

Places where the Scala codebase needs to be fixed (eventually) include:
  scala.reflect.internal.util.Statistics$QuantMap
  scala.tools.nsc.interactive.Global (several places)

Deprecated LinkedList (including Double- and -Like variants).

Interface is idiosyncratic and dangerously low-level.  Although some
low-level functionality of this sort would be useful, this doesn't seem
to be the ideal implementation.

Also deprecated the extractFirst method in Queue as it exposes LinkedList.
Cannot shift internal representations away from LinkedList at this time
because of that method.

Deprecated non-finality of several toX collection methods.

Improved documentation of most toX collection methods to describe what the
expectation is for their behavior.  Additionally deprecated overriding of
  - toIterator in IterableLike (should always forward to iterator)
  - toTraversable in TraversableLike (should always return self)
  - toIndexedSeq in immutable.IndexedSeq (should always return self)
  - toMap in immutable.Map (should always return self)
  - toSet in immutable.Set (should always return self)

Did not do anything with IterableLike.toIterable or Seq/SeqLike.toSeq since
for some odd reason immutable.Range overrides those.

Deprecated Forwarders from collections.

Forwarding, without an automatic mechanism to keep up to date with changes
in the forwarded class, is inherently unreliable.  Absent a mechanism to
keep current, they're deprecated.  ListBuffer is the only class in the
collections library that uses forwarders, and that functionality can be
rolled into ListBuffer itself.

Deprecating immutable set/map adaptors.

They're a bad idea (barring compiler support) for the same reason that all
the other adaptors are a bad idea: they get out of date and probably have a
variety of performance bugs.

Deprecated inheritance from leaf classes in immutable collections.

Inheriting from leaf-classes in immutable collections is rarely a good idea
since whenever you use any interesting collections method you'll revert to
the original class.  Also, the methods are often designed to work with only
particular behavior, and an override would be difficult (at best) to make
work.  Fortunately, people seem to have realized this and there are few to
no cases of people extending PagedSeq and TreeSet and the like.

Note that in many cases the classes will become sealed not final.

Deprecated overriding of methods and inheritance from various mutable
collections.

Some mutable collections seem unsuited for overriding since to override
anything interesting you would need vast knowledge of internal data
structures and/or access to private methods.  These include
  - ArrayBuilder.ofX classes.
  - ArrayOps
  - Some methods of BitSet (moved others from private to protected final)
  - Some methods of HashTable and FlatHashTable
  - Some methods of HashMap and HashSet (esp += and -= which just forward)
  - Some methods of other maps and sets (LinkedHashX, ListMap, TreeSet)
  - PriorityQueue
  - UnrolledBuffer

This is a somewhat aggressive deprecation, the theory being better to try it
out now and back off if it's too much than not attempt the change and be
stuck with collections that can neither be safely inherited nor have
implementation details changed.

Note that I have made no changes--in this commit--which would cause
deprecation warnings in any of the Scala projects available on Maven (at
least as gathered by Adriaan).  There are deprecation warnings induced
within the library (esp. for classes/traits that should become static) and
the compiler.  I have not attempted to fix all the deprecations in the
compiler as some of them touch the IDE API (but these mostly involved
Synchronized which is inherently unsafe, so this should be fixed
eventually in coordination with the IDE code base(s)).

Updated test checks to include new deprecations.

Used a higher level implementation for messages in JavapClass.
3cc99d7
@retronym
Member
retronym commented Nov 8, 2013

LGTM. Really important work. In the future, I would recommend to submit these as a sequence of commits, rather than one big squashed commit, as it makes it easier to selectively revert. Given that we're just dealing with annotations here, I'll admit this PR regardless.

@JamesIry
JamesIry commented Nov 8, 2013

LGTM as well

@JamesIry JamesIry merged commit 4cdce85 into scala:master Nov 8, 2013

1 check passed

Details default pr-scala Took 89 min.
@retronym
Member

This deprecation addresses 2/3 of https://issues.scala-lang.org/browse/SI-4290

@Ichoran ListBuffer still extends the now-deprecated SeqForwarder. What's the plan for that in 2.12?

@Ichoran
Collaborator
Ichoran commented Nov 26, 2013

Maybe @adriaanm can comment. I was tentatively just planning on moving the functionality needed for ListBuffer into ListBuffer. But I didn't have a definitive plan.

@stuhood stuhood commented on the diff Feb 15, 2014
src/library/scala/collection/mutable/BitSet.scala
def += (elem: Int): this.type = { add(elem); this }
+
+ @deprecatedOverriding("Override add to prevent += and add from exhibiting different behavior.", "2.11.0")
@stuhood
stuhood Feb 15, 2014

This should probably refer to remove.

(PS: I love this pull so, so much. Will it make 2.11?)

@adriaanm
adriaanm Feb 15, 2014 The Scala Programming Language member

Thanks! Yes, it will be part of 2.11.0. The first milestone release to include it was 2.11.0-M7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment