resolve status of scala/scala#8732 for 2.12.11

[SethTisue]

it caused scalikejdbc to regress in the community build (https://github.com/scala/community-build/pull/1097)

the problem is reproducible outside dbuild (on scalikejdbc/scalikejdbc@12b8f45) with core/testOnly *ResultSetTraversableSpec

it may also have caused scalafix to regress. in the case of scalafix, reproducing it without dbuild proved difficult and I gave up. let's get scalikejdbc working again and then hope that fixes scalafix too.

@retronym @lrytz @mkeskells

[SethTisue]

how do we know that the isEmpty checks added in the PR don't cause a traversal?

[lrytz]

how do we know that the isEmpty checks added in the PR don't cause a traversal?

Summarizing our discussion: isEmpty should not consume any elements, as stated in the docs

Implementations in subclasses that are not repeatedly traversable must take care not to consume any elements when isEmpty is called

It's possible that scalikejdbc defines a subclass that violates this invariant. @retronym can you take a look? Maybe we have to err on the safe side for 2.12.11.

[mkeskells]

I did find a class in the scala lib that had isEmpty defined as size equal 0. I noticed this in the dev of this PR. That caused a reversal. These is a comment on the count method about it. Cant see the code ATM as on my phone

[SethTisue]

Maybe we have to err on the safe side for 2.12.11

Agree, I think that's best here, especially since:

• it's now so late in the 2.12.x series
• any performance gain from calling isEmpty was probably quite small

I'm preparing a PR that removes the calls.

[retronym]

ResultSetTraversable overrides foreach to some try/finally resource management of an underlying ResultSet.

It inherits the default implementation of TraversableLike.isEmpty:

  def isEmpty: Boolean = {
    var result = true
    breakable {
      for (x <- this) {
        result = false
        break
      }
    }
    result
  }

The failing test calls ResultSetTraversable.foldLeft. The new isEmpty check call ResultSetTraversable.foreach and close the ResultSet before that real iteration occurs.

I don't think we can assume that all implementations follow the rules for isEmpty when we've provided a default implementation of the method that is likely to violate the contract.

I'd suggest that we remove the isEmpty optimizations added in https://github.com/scala/scala/pull/8732. It is still a benefit to replace allocation of a lambda and one or more XxxRef objects with a single accumulator.

We could choose to add overrides of these methods in some widely used concrete collections. That would have the additional benefit of avoiding one virtual call within the loop -- as things stand, List.foldLeft results in List.foreach looping and calling the anon function in Traversable.foldLeft which in turn calls the user written function provided to foldLeft.

[SethTisue]

resolved by https://github.com/scala/scala/pull/8769