scala.collection.mutable.Node breaks code

[Naftoli Gugenheim]

In scala 2.10 there's a change that needlessly breaks a lot of code. Any file that imports scala.collection.mutable._ and scala.xml._, and uses Node, will no longer compile without being disambiguated, since there's now a Node class in both packages.

Is this something that it would make sense to file a ticket about? Or am I wrong for complaining?

[Josh Suereth]

I'm not sure this is something we can/should guarantee between releases.   Especially since you can just do:

import scala.XML.{Node=>XmlNode,_}

To resolve the ambiguity.

Given that *most* collection classed are duplicated between immutable and mutable packages, I don't see much difference here.

Thanks for the report though.   I'll make sure it gets in the release notes somehow.

[Daniel Sobral]

What *is* this node anyway? I can see only these:

private case class Node[A](val data: A, val left: AVLTree[A], val
right: AVLTree[A]) extends AVLTree[A]
protected class Node(override protected val elem: A) extends
ListSet[A] with Serializable (on ListMap)

The latter is inside ListMap's class or object, so it shouldn't be
imported. The former is private, so it shouldn't cause conflict.
Scaladoc doesn't show it.

--
Daniel C. Sobral

I travel to the future all the time.

[Jason Zaugg]

On Wed, Aug 15, 2012 at 2:48 PM, Daniel Sobral <dcso...@gmail.com> wrote:
> What *is* this node anyway? I can see only these:
>
> private case class Node[A](val data: A, val left: AVLTree[A], val
> right: AVLTree[A]) extends AVLTree[A]
> protected class Node(override protected val elem: A) extends
> ListSet[A] with Serializable (on ListMap)
>
> The latter is inside ListMap's class or object, so it shouldn't be
> imported. The former is private, so it shouldn't cause conflict.
> Scaladoc doesn't show it.

Private members are importable. There can only be one binding for a
name in a scope, irregardless of access.

scala> object A { private val a = 0 }; object B { val a = 1 }
defined module A
defined module B

scala> { import A._, B._; a }
<console>:10: error: reference to a is ambiguous;
it is imported twice in the same scope by
import B._
and import A._
{ import A._, B._; a }
^

private[this] members are not importable.

scala> object A { private[this] val a = 0 }; object B { val a = 1 }
defined module A
defined module B

scala> { import A._, B._; a }
res2: Int = 1

https://issues.scala-lang.org/browse/SI-2133

Why are private members importable at all? Well, a class might import
the privates of it's companion, or vice-versa. Or a nested template
might import the privates of it's enclosing class.

To avoid namespace pollution, you can do something like:

object A { private object myPrivates { val a = 0 }; import myPrivates._; ... }

Personally, I would prefer to exclude privates from (at least)
wildcard imports. Admittedly, this isn't particularly regular.

-jason

[Paul Phillips]

On Wed, Aug 15, 2012 at 6:05 AM, Jason Zaugg <jza...@gmail.com> wrote:

Personally, I would prefer to exclude privates from (at least)
wildcard imports. Admittedly, this isn't particularly regular.

We can have our regularity cake and also consume it.  All we have to do is defer issuing an ambiguity error until both imported members are known to be accessible at the import site.  This is one of my least favorite bugs - see my last comments (which I realize you jason know well already) at

  https://issues.scala-lang.org/browse/SI-3160

Persistence eventually paid off with SI-3836, maybe it will here too.

[Paul Phillips]

Not to lose sight of something important:

On Tue, Aug 14, 2012 at 11:15 PM, Naftoli Gugenheim <nafto...@gmail.com> wrote:

In scala 2.10 there's a change that needlessly breaks a lot of code. Any file that imports scala.collection.mutable._ and scala.xml._

You should *never* import  scala.collection.mutable._  (or immutable._, or collection._.) Really, trust me on this.  If I had a style checker I would make this a compile error in code which I have anything to do with. It means that names like "Map" and "Set" invisibly change meaning through the rest of the file.  This is about the worst thing you can do to the poor next guy (which can easily be yourself) who has to read the code.  And it's so easy to create latent bugs this way - you likely won't even know anything changed until later when suddenly you can't call a method because it accepts a mutable.Map.

The thing to do is import scala.collection.{ immutable, mutable } and refer to them as mutable.Map and immutable.Map.  The cost in verbosity is small compared to the alternative.

None of this takes away from the severity of SI-3160, which I would dearly love to see fixed.  But as a rule you cannot expect to be wildcard importing from packages and suffer no consequences, especially across major version bumps.  We're not about to give everything a unique name as if it were one flat namespace, which is the only way one could avoid such conflicts.

[Daniel Sobral]

In that case, I'd favor renaming Node to AVLNode or something,
regardless of how bad an idea is to wildcard import
scala.collection.mutable.

[Paul Phillips]

On Wed, Aug 15, 2012 at 7:55 AM, Daniel Sobral <dcso...@gmail.com> wrote:

In that case, I'd favor renaming Node to AVLNode or something,
regardless of how bad an idea is to wildcard import
scala.collection.mutable.

Down that road lies madness.  Why encourage the wrong thing in an effort to achieve the impossible? If there is anything to do, it is to better advertise the tradeoffs associated with promiscuous import.  (Come to think of it, calling it "promiscuous import" rather than "wildcard import" would be a good start.)

[Rex Kerr]

Having your most common interfaces not have name conflicts is not impossible; it's good API design.

The choice of "Node" for scala.xml was maybe not fantastic, but now that the stealth Node from collection.mutable is conflicting, the latter seems ripe for change _especially_ since it's interfering without being exposed and part of a pleasant interface for the user.

One can't control everything, and eventually with large projects wildcard imports start to bite too viciously to be tolerated, but that doesn't mean we shouldn't safeguard the common use cases in small and intermediate-scale projects where a little care with the API can make all the difference.

  --Rex

[Paul Phillips]

On Wed, Aug 15, 2012 at 11:05 AM, Rex Kerr <ich...@gmail.com> wrote:

Having your most common interfaces not have name conflicts is not impossible; it's good API design.

Well, be that as it may, that ship has saaaaaaaaaaailed.

Unless the existence of collection.Map, mutable.Map, and immutable.Map somehow does not represent a naming conflict.

[Rex Kerr]

They are in many ways drop-in replacements for each other; that was the original justification I heard.  Perhaps it's not very good justification (even though they do inherit from a common parent).  But the Node in AVLTree and the Node in xml aren't candidates for drop-in replacement, so it's a rather different ship.  If it's even a ship.  Looks awfully leaky to me, more like a sieve.  Can we at least stick to marginally seaworthy ships (even if we aren't totally happy with the destination)?

  --Rex

[Paul Phillips]

On Wed, Aug 15, 2012 at 11:26 AM, Rex Kerr <ich...@gmail.com> wrote:

They are in many ways drop-in replacements for each other; that was the original justification I heard.  Perhaps it's not very good justification (even though they do inherit from a common parent).  But the Node in AVLTree and the Node in xml aren't candidates for drop-in replacement, so it's a rather different ship.  If it's even a ship.  Looks awfully leaky to me, more like a sieve.  Can we at least stick to marginally seaworthy ships (even if we aren't totally happy with the destination)?

What I'm primarily against is renaming a *private* class to help people perform promiscuous import, because I don't want to make it any easier to continue not fixing SI-3160.

If we can't have private classes with names like "Node", what have we come to?

[Rex Kerr]

I'd rather than SI-3160 was fixed for 2.10.0.final than that the private Node was renamed.

But I'd rather have the private Node renamed than have it clobber existing uses of xml.Node and other Nodes that I may or may not have created.  Since it's private, it can go back to being just Node again once SI-3160 is fixed.

  --Rex

[Daniel Sobral]

That's not my point (hence the "regardless"). Node is a common term
for trees and graphs, so it makes sense to make it clear that *this*
node is part of the AVL data structure right in the name, instead of
forcing you to do code spelunking.

[Paul Phillips]

On Wed, Aug 15, 2012 at 12:00 PM, Daniel Sobral <dcso...@gmail.com> wrote:

That's not my point (hence the "regardless"). Node is a common term
for trees and graphs, so it makes sense to make it clear that *this*
node is part of the AVL data structure right in the name, instead of
forcing you to do code spelunking.

Yes, Node is a very common term, that's why private namespaces are important.  That anyone ever became aware there was a PRIVATE class in scala.collection.mutable called "Node", or that they had to perform any code spelunking involving this PRIVATE class, is as far as I'm concerned clearly a bug.  That's the bug I'm available to fix.

[Paul Phillips]

On Wed, Aug 15, 2012 at 12:52 PM, martin odersky <martin....@epfl.ch> wrote:

There's no chance private imports will be fixed for 2.10. I have to spend all available cycles on desperately needed performance improvements. So rather than continuing arguing, would someone PLEASE rename the avl Node?

Thanks

 - Martin

[The above was sent only to me, but I think it's clear from the use of "someone" that it was intended for the list.  I'm only noticing it wasn't to the list now, when replying with the following.]

Okay, but it won't be for lack of an implementation.

  https://github.com/paulp/scala/tree/topic/whats-in-a-name

Maybe it is more complicated than I am making it out to be, but if it is, we don't have any tests which reveal what that complication is.  The only test which failed was a neg test which works now, intentionally (the ticket is SI-4537.)

object Bippy {

  private class Node

}

class Bippy {

  import Bippy._

  import scala.xml._

  def f(x: Node): String = ???  // ambiguous, because Bippy.Node is accessible

}

class Other {

  import Bippy._

  import scala.xml._

  def f(x: Node): String = ???  // unambiguous, because Bippy.Node is inaccessible

}

// mine

% pscalac -d /tmp test/files/neg/t3160ambiguous.scala

test/files/neg/t3160ambiguous.scala:8: error: reference to Node is ambiguous;

it is imported twice in the same scope by

import scala.xml._

and import Bippy._

  def f(x: Node): String = ???  // ambiguous, because Bippy.Node is accessible

           ^

one error found

// trunk

% scalac3 -d /tmp test/files/neg/t3160ambiguous.scala

test/files/neg/t3160ambiguous.scala:8: error: reference to Node is ambiguous;

it is imported twice in the same scope by

import scala.xml._

and import Bippy._

  def f(x: Node): String = ???  // ambiguous, because Bippy.Node is accessible

           ^

test/files/neg/t3160ambiguous.scala:14: error: reference to Node is ambiguous;

it is imported twice in the same scope by

import scala.xml._

and import Bippy._

  def f(x: Node): String = ???  // unambiguous, because Bippy.Node is inaccessible

           ^

two errors found