Review by @som-snytt

I squashed all your commits into one.

Compared to the latest commit in #5064 the following has changed:

• Fixed the few test failures
• Eliminated a few remaining references to JavaConversions, wrapAs.., decorateAs..
• Completely deprecated WrapAsJava / WrapAsScala (instead of just deprecatedInheritance). Also deprecated the WrapAsJava / WrapAsScala objects
• Added ImplicitConversionsToScala and ImplicitConversionsToJava next to your ImplicitConversions. Added a doc-comment explaining that explicit conversions are to be preferred.

There's one problem created by this PR. Now JavaConverters provides explicit conversion methods such as

scala> val jit: java.util.Iterator[String] = java.util.Collections.emptyList[String]().iterator()
jit: java.util.Iterator[String] = java.util.Collections$EmptyIterator@68b58644

scala> collection.JavaConverters.asScalaIterator(jit)
res2: Iterator[String] = empty iterator

The JavaConverters.asScalaIterator method did not exist previously.

Unfortunately, the names of these explicit converter methods are exactly the same as the names of the implicit converter methods in the deprecated JavaConversions. A problem manifests if a user imports both JavaConverters._ and JavaConversions._:

scala> :paste
// Entering paste mode (ctrl-D to finish)

object T {
  import collection.JavaConverters._
  import collection.JavaConversions._
  val jit: java.util.Iterator[String] = java.util.Collections.emptyList[String]().iterator()
  val sit: Iterator[String] = jit
}

// Exiting paste mode, now interpreting.

<console>:16: error: type mismatch;
 found   : java.util.Iterator[String]
 required: scala.collection.Iterator[String]
         val sit: Iterator[String] = jit
                                     ^

What happens here (-Xlog-implicits) is that implicit search finds JavaConversions.asScalaIterator, but when it tries to type-check asScalaIterator(jit) it encounters an error (ambiguous reference, the method is imported twice) and discards the implicit conversion.

So basically when somebody has both imports, the implicit conversions will stop working after this PR. There will not be a deprecation warning for JavaConversions because these are issued later, in refchecks, while the type error occurs during typer.

The alternative would be to rename all implicit conversion methods in JavaConversions, which would break code that refers to them explicitly (JavaConversions.asScalaIterator(jit)). I think this is actually more common than having both imports. On the other hand, the error message would be more clear in this case (value asScalaIterator is not a member of JavaConversions), so maybe it would be better to go with this option.

Thanks for picking this up. Otherwise, it's kind of thankless. I had another pass to make on the old PR.

I didn't notice that consequence of the naming conflict you point out. I don't have a clever workaround. I would definitely go with not supporting wildcard imports from both. They are presented as alternatives, and no examples show imports from both.

If the issue were serious enough, the "new" explicit methods could be renamed toSomething, although that ruins the naming convention. I don't think it's serious enough; but it muddies the release notes.

Actually, the workaround is to add implicits back to JavaConversions by adding them to WrapAsScala, etc, under new innocuous names. So this makes the example work again. Adding a method seems like going in the wrong direction, but Wrap is deprecated so it's OK. In fact, someone commented somewhere that no one ever introduces API and deprecates it at the same time. Ha!

  implicit def `deprecated asScalaIterator`[A](it: ju.Iterator[A]): Iterator[A] =
    asScalaIterator(it)

Actually it has to be a lower-priority implicit, to allow implicit selection.

Yeah, tried it out:

// Identical fallback implicits, in case old names are hidden by new members of JavaConverters
@deprecated("Use JavaConverters or consider ToScalaImplicits", since="2.12")
trait WrapAsScala extends LowPriorityWrapAsScala {
  implicit def `deprecated asScalaIterator`[A](it: ju.Iterator[A]): Iterator[A] = asScalaIterator(it)
}

trait LowPriorityWrapAsScala {

Making the new method more specific makes it easier to type in.

Sample WrapAsScala

https://github.com/scala/scala/compare/2.12.x...som-snytt:review/pr5064?expand=1

@som-snytt i pushed two more commits to address your feedback.

generated docs at https://dl.dropboxusercontent.com/u/4881107/library/scala/collection/JavaConverters$.html

Your solution of renaming the implicits is different from what I proposed. Does it work? It's too late in afternoon here for me to know without trying. I thought importing f twice makes it unavailable because of the ambiguity, even if only one is implicit. My proposal was let f stop working but add a g to the deprecated interface. The g has to be more specific than f. I only like the idea because it sounds like a crazy solution.

It seems to work, here's the test: 75b8875#diff-9acf6de82fd57ddd8dc4c907a07bfe72

I liked your idea, but then realized that renaming the implicits and providing non-implicit methods with the old name also works, and it seems simpler.

Renaming implicits is itself not source compatible: someone might have imported one specifically, rather than via a wildcard.

ah, true.. i'll give @som-snytt's idea try then.

Now I see what you had done. I was on minimal sleep and had to get the daughter to Met Opera at the cinema, Sondra Radvanovsky as Q.E. 1. What a performance. If the daughter grows up to be like either Radvanovsky or Elizabeth, I'd be OK with it. Modulo beheading your ex. I didn't actually sleep in Act 1, but I may have rested my eyes.

I'd still like a motivating use case for @deprecatedImplicitly to disable implicit candidacy under -deprecation.

I'd still like a motivating use case for @deprecatedImplicitly to disable implicit candidacy under -deprecation.

Would this mean that -deprecation changes semantics? I think that's not a good idea.

I pushed another commit that adds the higher-priority implicits with non-clashing names to JavaConversions.

Backquotes are totally the boss.

You're right about -deprecation. Maybe it's a job for the deprecator plugin that elides deprecated API, in which case a special annotation isn't needed.

The use case for @deprecatedImplicitly would be to warn only on implicit usage. But maybe it could function as the explicit priority that people have asked for. It would lower the specificity score. For example, in the current commit, instead of a low priority trait, just annotate the old name and add the new name.

rebased due to merge conflict. @som-snytt lgty?

LGTM

I see you were cleverly efficient with some more doc updates.

/rebuild 3bf207b

Have we considered the case where Java code uses the methods in JavaConversions explicitly because it's more convenient than using JavaConverters explicitly? I've seen a few cases like that in existing open-source projects.

@ijuma The doc says "Alternatively, the conversion methods have descriptive names and can be invoked explicitly."

So now they are available on JavaConverters. That wasn't the case previously, only in order to make the implicits work.

lrytz deals with the naming starting at #5109 (comment)