Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TraversableOnce requires more than foreach to be implemented #3958

Closed
scabug opened this issue Oct 25, 2010 · 8 comments
Closed

TraversableOnce requires more than foreach to be implemented #3958

scabug opened this issue Oct 25, 2010 · 8 comments
Assignees
Labels

Comments

@scabug
Copy link

scabug commented Oct 25, 2010

The !TraversableOnce documentation says:

This trait is composed of those methods which can be implemented solely in terms of foreach

However, you have to implement many more methods than just foreach to get a !TraversableOnce:

scala> val travOnce = new TraversableOnce[Int] { def foreach[U](f: Int => U) = f(1) }
<console>:5: error: object creation impossible, since:
method copyToArray in trait TraversableOnce of type [B >: Int](xs: Array[B],start: Int,len: Int)Unit is not defined
method find in trait TraversableOnce of type (p: (Int) => Boolean)Option[Int] is not defined
method exists in trait TraversableOnce of type (p: (Int) => Boolean)Boolean is not defined
method forall in trait TraversableOnce of type (p: (Int) => Boolean)Boolean is not defined
method toStream in trait TraversableOnce of type => Stream[Int] is not defined
method toTraversable in trait TraversableOnce of type => Traversable[Int] is not defined
method toIterator in trait TraversableOnce of type => Iterator[Int] is not defined
method isTraversableAgain in trait TraversableOnce of type => Boolean is not defined
method hasDefin...
       val travOnce = new TraversableOnce[Int] { def foreach[U](f: Int => U) = f(1) }

I see that there's a comment above the forall declaration in !TraversableOnce.scala that says:

Presently these are abstract because the Traversable versions use
breakable/break, and I wasn't sure enough of how that's supposed to
function to consolidate them with the Iterator versions.

So I gather it's a known issue, but I didn't see an issue tracking this problem, and this is the third time that it's bitten me, so I figured I should report this.

I guess the immediate solution for Scala 2.8 is to fix the documentation so that it says that many other things are required to define a !TraversableOnce, but it sure would be nice to only have to define foreach to get a !TraversableOnce like the documentation currently claims.

@scabug
Copy link
Author

scabug commented Oct 25, 2010

Imported From: https://issues.scala-lang.org/browse/SI-3958?orig=1
Reporter: Steven Bethard (bethard)

@scabug
Copy link
Author

scabug commented Oct 25, 2010

@paulp said:
It is presumably an errant search and replace making this claim in the documentation. It's not going to become true: the class which you can implement with foreach is Traversable.

@scabug
Copy link
Author

scabug commented Oct 25, 2010

Steven Bethard (bethard) said:
Ok, thanks for the clarification. I've switched this to a Documentation bug.

It's too bad TraversableOnce isn't intended to work this way - it's much easier to describe control flow with the foreach method than with next and hasNext from Iterator. Compare the hypothetical TraversableOnce version of a restarting iterator:

def restarting[T](getTrav: () => TraversableOnce[T]) = new TraversableOnce[T] {
  def foreach[U](f: T => U): Unit = {                                          
    try {                                                                      
      for (item <- getTrav())                                                  
        f(item)                                                                
    } catch {                                                                  
      case e: java.io.IOException => this.foreach(f)                           
    }                                                                          
  }                                                                            
}

with the Iterator implementation:

def restarting[T](getIter: () => Iterator[T]) = new Iterator[T] {
  var iter = getIter()
  def hasNext = {
    try {
      iter.hasNext
    } catch {
      case e: IOException => {
        this.iter = getIter()
        this.hasNext
      }
    }
  }
  def next = {
    try {
      iter.next
    } catch {
      case e: IOException => {
        this.iter = getIter()
        this.next
      }
    }
  }
}

@scabug
Copy link
Author

scabug commented Oct 25, 2010

@paulp said:
You can as easily write that as

  new Traversable[T] { def foreach ... } iterator

There are also methods like Iterator.iterate and Iterator.continually and more: you never have to define next and hasNext yourself unless you want to. TraversableOnce is a retrofit to eliminate code duplication between Iterator and Traversable, and except for internal use it's most likely less appropriate than one or the other of those.

@scabug
Copy link
Author

scabug commented Oct 26, 2010

Steven Bethard (bethard) said:
Replying to [comment:3 extempore]:

You can as easily write that as
{code}
new Traversable[T] { def foreach ... } iterator
}}

Except that the Traversable toIterator puts everything into a Buffer first, so you can't really use it if there are too many items in the iterator to keep them all around (e.g. an infinite stream read from a URL). If Traversable.iterator worked without building up the items, that would work perfectly though.

There are also methods like Iterator.iterate and Iterator.continually and more: you never have to define next and hasNext yourself unless you want to.

But those don't lend themselves to handling exceptions. And yes I know about util.control, but I haven't yet seen a pretty solution to the code above using e.g. Iterator.continually and util.control. If you have one, it would be great if you could add it here:

http://stackoverflow.com/questions/4010285/restart-iterator-on-exceptions-in-scala

TraversableOnce is a retrofit to eliminate code duplication between Iterator and Traversable, and except for internal use it's most likely less appropriate than one or the other of those.

Maybe when the TraversableOnce docs are updated to fix the bug above, they could also say something about TraversableOnce being intended for internal use and link to the more appropriate methods?

@scabug
Copy link
Author

scabug commented Oct 26, 2010

@dcsobral said:
I don't see the problem here.

Replying to [ticket:3958 bethard]:

The !TraversableOnce documentation says:
{code}
This trait is composed of those methods which can be implemented solely in terms of foreach
}}

However, you have to implement many more methods than just foreach to get a !TraversableOnce:

Ok. The question, therefore, is: can these many more methods be implemented in terms of foreach? If they can, the documentation is correct.

The documentation DOES NOT SAY you just have to implement foreach. It says only that there isn't any methods that cannot be implemented in terms of foreach. Let's consider, for example, "exists", which is abstract. Here's an implementation:

def exists(p: (A)  Boolean) : Boolean = {
  class ResultFound extends Exception
  var result = false
  try {
    foreach { el =>
      result &&= p(el)
      if (result)
        throw new ResultFound
    }
  } catch (ex) {
    case _: ResultFound =>
  }
  result
}

There, implemented in terms of foreach, as required. Alas, that sentence does not end there, and the text that follows it makes it more clear:

and which do not need access to a Builder

Methods which need access to a Builder are those methods that return a collection of best fitting type. So there is no map method there, for example.

@scabug
Copy link
Author

scabug commented Oct 26, 2010

Steven Bethard (bethard) said:
Maybe it's just a dialectal difference, but to me the sentence:

This trait is composed of those methods which can be implemented
solely in terms of foreach and which do not need access to a Builder.

strongly suggests that all the methods that can be implemented in terms of foreach have been (and the last clause implies that no methods that require access to a builder are included in the TraversableOnce interface). But maybe that's just my dialect.

How about the following suggested rewording that hopefully corresponds to the intention more clearly:

This trait exists primarily to eliminate code duplication between
Iterator and Traversable, and thus implements some of the common
methods that can be implemented solely in terms of foreach without
access to a Builder. It also includes a number of abstract methods
whose implementations are provided by Iterator, Traversable, etc.

Directly subclassing TraversableOnce is not recommended - instead,
consider creating an Iterator with a next and hasNext method, or
create an Iterator with one of the methods on the Iterator object.

@scabug
Copy link
Author

scabug commented Nov 17, 2010

@axel22 said:
(In r23538) Closes #3958. No review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants