Resolve conflict with JDK15's new isEmpty method on CharSequence

[catap]

java.lang.CharSequence has isEmpty method since JDK 15 that makes impossibly to compile scala by JDK15.

Fix it on the way that allows to compiled with JDK15 and before JDK15.

Fixes scala/bug#12172

[catap]

This way allows only to compile but not use it.

Close pull request because it is a wrong and very bad way.

[som-snytt]

OK I'm cancelling the 1-800-flowers. It's a very annoying issue, so the swing is appreciated.

[catap]

@som-snytt I was very optimised about impact.

Right now I doubt that it can be solved.

[SethTisue]

@catap Note that it would have been sufficient to PR the change on 2.11.x, unless nontrivially different code was needed on the different branches. We merge everything forward from 2.11.x -> 2.12.x -> 2.13.x.

[catap]

@SethTisue I can't just cherry-pick it because it created some conflicts => I solved it.

Anyway, after a day of thinking about this bug I see that it should be solved on two levels:

• inside scala-library by my PRs
• inside scalac by some hacks

So, I would like to reopen my PRs. Anyway tests are broken and if someone from the team thinks that it shouldn't be used — I'm OK with this.

[catap]

@SethTisue I made the first attempt to force selection of scala isEmpty.

This allows to produce on JDK15 a bytecode that runs on JVM before 15 that is good.

Anyway, I'm a bit concern about my naive approach. I've used termSymbol.fullName == "scala.Predef" as the last filter with.

But I afraid that it costs too much in terms of performance.

If you have something better in mind I'd love to ask some help.

[dwijnand]

So this is the special-case-it-in-the-compiler way. When I heard about this breakage I was thinking we could make the CharSequence extending implicit non-implicit (so binary compatible) and have another implicit class that brings CharSequence's methods without actually extending CharSequence ... except that's not forwards-compatible... 🤔

[catap]

@dwijnand but by this "hack" scala produces the same bytecode that it produces on JDK before 15.

Other approach is broking a lot of compatible in strange places, and here I just help to make right decision :)

[lrytz]

I don't see any other way out than compiler support for Array[Char].isEmpty.

[sjrd]

I just thought: what if we add a (intrinsified) macro def isEmpty: Boolean directly in class Array, that would expand x.isEmpty into x.length == 0? As a macro, it wouldn't violate the binary compatibility requirements. And it might be cleaner than special-casing implicit conversion insertion.

[smarter]

I mean sure, but I have a thousand other things to worry about and all I'm asking is for a decision to be reached soon so I can work with that.

[lrytz]

Will take a look if I can get it to work, or have to add the stub method to Array.scala.

I don't think there's a clean way to get the synthetic method to work. In order for it to be visible it has to be forced. This works fine except when compiling the standard library using -sourcepath. In this case the Array symbol is has a SourcefileLoader, i.e., a lazy type which completes by sending the source file through the compiler pipeline.

Other synthetic symbols are forced when the Global.Run instance is created, in definitions.init(). Completing a SourcefileLoader this early doesn't work (run.firstPhase is still null). We'd have to find some other hook, ideally somehow add the method right after completing the Array symbol, but there's no good way to do that.

So to me it seems that the best solution is to add the stub to Array.scala.

[lrytz]

Hmm, that doesn't work so easily either... After adding isEmpty to Array.scala the 2.13.3 compiler can no longer compile the standard library, because calls within the standard library (like clazz.getDeclaredFields.isEmpty) now resolve to the new method, but it's not rewritten by the 2.13.3 compiler. So the backend fails with unexpected array call.

[lrytz]

My only idea here is to do it in 2 steps:

1. Add compiler support for a stub isEmpty method in Array.scala
2. After the next release when we update starr, actually add that method to Array.scala

This means it won't be fixed for 2.13.4, only for 2.13.5. We could use a mergely as starr to avoid that.

I think this is still better than special-casing implicit search.

[catap]

@lrytz am I right that this way is broke compatibility with class file that was compiled with a new standard library and that is using on old one?

Let image that some library is updated a scala version that is used to build it to 2.13.5 and user of this library is using 2.13.3.

If I understand it right the end user will have very wired runtime exception.

[lrytz]

The Array.class is never used at run-time, Scala arrays are encoded as native arrays in the JVM. A call someArray.isEmpty is translated to someArray.length == 0, which works also when an older Scala library is on the runtime classpath.

[smarter]

I think this is still better than special-casing implicit search.

This seems a lot more complicated then special-casing implicit search, all for a temporary solution anyway (since the correct solution is to change Predef.scala whenever we can break BC), we've also discussed adding a special case in implicit search like this in dotty at a recent dotty meeting and there wasn't any opposition to it, why do you think it's a worse solution?

[lrytz]

It's not that temporary, we'd have to keep it forever in 2.13. Adding the method to Array preserves Scala semantics, so for example IntelliJ's typer would understand it. It's also nothing new conceptually, we do the same for the other Array methods.

[smarter]

so for example IntelliJ's typer would understand it.

That would need to be tested, I don't know how their typer deals with macro methods but I wouldn't be surprised if something breaks.

After the next release when we update starr, actually add that method to Array.scala

Can already prepare a PR with that change so I can test dotty against the updated Array.scala?

[smarter]

Also what prevents you from using an intermediate starr based on a nightly instead of waiting for ~3-4 months?

[SethTisue]

re-STARRing on a mergely seems quite acceptable to me, we've done it a number of times before

[lrytz]

Also what prevents you from using an intermediate starr based on a nightly instead of waiting for ~3-4 months?

I mentioned that as an option above. We only do that when really necessary, I'd be fine doing it here.

I'm not sure if a nightly on our artifactory is OK or if we should publish something to maven central, for reproducability.

[SethTisue]

Historically we have done it to Maven Central. I see no downside.

[lrytz]

Testing the follow up after restarr here: #9288

[smarter]

Another approach which could be considered would be to simply make ArrayCharSequence not implicit at the cost of breaking source-compat in hopefully rare cases (but not binary compat):

diff --git src/library/scala/Predef.scala src/library/scala/Predef.scala
index c9911f9299..de2b8984a8 100644
--- src/library/scala/Predef.scala
+++ src/library/scala/Predef.scala
@@ -388,12 +388,13 @@ object Predef extends LowPriorityImplicits {
   }

   /** @group implicit-classes-char */
-  implicit final class ArrayCharSequence(arrayOfChars: Array[Char]) extends CharSequence {
+  final class ArrayCharSequence(arrayOfChars: Array[Char]) extends CharSequence {
     def length: Int                                     = arrayOfChars.length
     def charAt(index: Int): Char                        = arrayOfChars(index)
     def subSequence(start: Int, end: Int): CharSequence = new runtime.ArrayCharSequence(arrayOfChars, start, end)
     override def toString                               = arrayOfChars.mkString
   }
+  def ArrayCharSequence(arrayOfChars: Array[Char]): ArrayCharSequence = new ArrayCharSequence(arrayOfChars)

   /** @group conversions-string */
   @inline implicit def augmentString(x: String): StringOps = new StringOps(x)

That's one less implicit conversion in the world which would be a good thing, and for people who really want a CharSequence backed by their Array[Char], the Internet tells me that can be easily accomplished using java.nio.CharBuffer.wrap. The transition can be made smoother by first deprecating ArrayCharSequence in 2.13.4 with a message mentioning the alternative, and then removing the implicit keyword in 2.13.5 (this is the approach I took in scalacheck for a similar issue: typelevel/scalacheck#498)

[dwijnand]

Yes! So by giving it the same name it's not forward binary incompatible. Of course... -.-

I'm happy with dropping source compatibility for implicitly enriching Array[Char].

[sjrd]

That does seem like a reasonable option, indeed.

[lrytz]

Intriguing idea... Should we then also deprecate SeqCharSequence?

I looked a bit through the history

• 34fe33a (2008, Scala 2.7) added seqToCharSequence, it seems there was no similar implicit conversion before
• d5b02c8 (2009, Scala 2.8) added arrayToCharSequence, probably for consistency
• b5e9e4b optimized subSequence to avoid copying the array. The correspoinding ticket Predef.arrayToCharSequence.subSequence copies arrays around leading to bad performance in code relying on subSequence being fast bug#5641 suggests that the conversion is used in parser combinators.

I guess the main question is how much code we'd break. The community build can tell something about that.

[dwijnand]

Alt: #9292

[dwijnand]

Went with #9292 instead.

Thank you, @catap, for jumping on this so quickly. Although we didn't end up merging your fix, this was really helpful to think through different approaches. ❤️