Thanks a lot, @Blaisorblade, the pasto was the remaining issue! :-)
Do you want to review it?

I'm happy I was of help!
The commit looks good to me at first sight, but I don't feel qualified for a real review. I'll add some comments anyway.

That's all I had to say. Kudos, and good luck with the next reviewer!

StringBuilderClass is still used by the old GenASM backend. Given that the tests (afaik) only run against the new backend, I felt that the best way of action was to not touch the old backend to prevent introducing issues in less well-tested places of the code.

Given that the tests (afaik) only run against the new backend, I felt that the best way of action was to not touch the old backend to prevent introducing issues in less well-tested places of the code.

👍

review by @lrytz?

sure. sorry i was a bit off the pr queue lately..

While we're looking at this part of the compiler, I'd like to consider making def foo(c: CharSequence) = " " + c dispatch to the suitable overloaded append to avoid the toString call.

This would be a deviation from Java, but it would be handy for things like Name (assuming we made it implement CharSequence) to avoid temporary strings.

@retronym Not sure I understand this completely. Can you describe how the calls should look under the new scheme (vs. the old)?

Mhhh, do you mean https://github.com/soc/scala/blob/SI-9315/src/compiler/scala/tools/nsc/backend/jvm/BCodeIdiomatic.scala#L223, where it always picks Object for reference types?

Mhhh, I guess we should do the same for arguments of type StringBuilder...

Yes. The parameter type corresponds to signature of the fake method + we install on the type java.lang.String. We could add a second overload to +, although that technically is source incompatible. Or, we could perform the dispatch in the backend, based on the static type of the argument.

This change of course would be a behavioural difference for ill-behaved CharSequence implementations. Java string concatenation is stuck with the status quo for compatibiity reasons. I'm not sure the gain is so important that we'll take the risk of the change ourselves, but I just wanted to raise awareness of it and invite discussion.

This change of course would be a behavioural difference for ill-behaved CharSequence implementations.

I'd argue that, even for 2.12, the behavior should be equivalent for CharSequence implementations that respect the CharSequence specification. That includes even the odd CharBuffer, which is called out by the CharSequence docs, so we're fine? Or are there notable exceptions?

I think you could quite easily do it in the backend (not tested!):

diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeIdiomatic.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeIdiomatic.scala
index 535e1a8..99efb0d 100644
--- a/src/compiler/scala/tools/nsc/backend/jvm/BCodeIdiomatic.scala
+++ b/src/compiler/scala/tools/nsc/backend/jvm/BCodeIdiomatic.scala
@@ -219,9 +219,11 @@ abstract class BCodeIdiomatic extends SubComponent {
      */
     final def genStringConcat(el: BType, pos: Position): Unit = {

-      val jtype =
-        if (el.isArray || el.isClass) ObjectReference
-        else el
+      val jtype = el match {
+        case c: ClassBType if c.isSubtypeOf(CharSequenceReference) => CharSequenceReference
+        case t if t.isArray || t.isClass => ObjectReference
+        case t => t
+      }

       val bt = MethodBType(List(jtype), StringBuilderReference)

diff --git a/src/compiler/scala/tools/nsc/backend/jvm/CoreBTypes.scala b/src/compiler/scala/tools/nsc/backend/jvm/CoreBTypes.scala
index 00ca096..c0339de 100644
--- a/src/compiler/scala/tools/nsc/backend/jvm/CoreBTypes.scala
+++ b/src/compiler/scala/tools/nsc/backend/jvm/CoreBTypes.scala
@@ -108,6 +108,7 @@ class CoreBTypes[BTFS <: BTypesFromSymbols[_ <: Global]](val bTypes: BTFS) {

   lazy val StringReference             : ClassBType = classBTypeFromSymbol(StringClass)
   lazy val StringBuilderReference      : ClassBType = classBTypeFromSymbol(StringBuilderClass)
+  lazy val CharSequenceReference       : ClassBType = classBTypeFromSymbol(requiredClass[CharSequence])
   lazy val ThrowableReference          : ClassBType = classBTypeFromSymbol(ThrowableClass)
   lazy val jlCloneableReference        : ClassBType = classBTypeFromSymbol(JavaCloneableClass)        // java/lang/Cloneable
   lazy val jlNPEReference              : ClassBType = classBTypeFromSymbol(NullPointerExceptionClass) // java/lang/NullPointerException
@@ -254,6 +255,7 @@ final class CoreBTypesProxy[BTFS <: BTypesFromSymbols[_ <: Global]](val bTypes:

   def StringReference             : ClassBType = _coreBTypes.StringReference
   def StringBuilderReference      : ClassBType = _coreBTypes.StringBuilderReference
+  def CharSequenceReference       : ClassBType = _coreBTypes.CharSequenceReference
   def ThrowableReference          : ClassBType = _coreBTypes.ThrowableReference
   def jlCloneableReference        : ClassBType = _coreBTypes.jlCloneableReference
   def jlNPEReference              : ClassBType = _coreBTypes.jlNPEReference

I also added support for the StringBuffer overload, but stopped short of the char[] one, I hope this is the right balance...

Using the overload for char[] would be incorrect AFAICT: toString on a char is something like [C@74a14482, so that's the string that should be added when using a char array in a string concatenation. In other words, using the overload would change the current behavior:

scala> val cs = "hi".toArray
cs: Array[Char] = Array(h, i)

scala> "" + cs
res0: String = [C@6572421

It would be great to add a test for the overloading change. The easiest would be a unit test in test/junit/scala/tools/nsc/backend/jvm, you can use the tools in CodeGenTools.scala, see for example MethodLevelOpts.scala

@lrytz Agree, that was my reasoning for leaving out char[].
Thanks for the hints regarding testing, extremely useful!

Is there anything needed to make junit pick the new tests up?

I wrote a test class, added all the annotations, but the class is never tested.

it should happen by itself - maybe you missed some annotation? feel free to post the diff. btw, i very much recommend to write and run tests in intellij.

@lrytz Ooops, sorry, forgot to push! Here are the tests: https://github.com/soc/scala/commit/2e05ec4bd81b0dbbf8664cb079128e2bc618e94c#diff-7506aaabf8fa132381745c90a77c1606R23

JUnit doesn't seem to pick up the class:

    ...
    [junit] Running scala.tools.nsc.backend.jvm.BTypesTest
    [junit] Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 2.196 sec
    [junit] Running scala.tools.nsc.backend.jvm.DirectCompileTest
    [junit] Tests run: 6, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 4.341 sec
    [junit] Running scala.tools.nsc.backend.jvm.IndyLambdaTest
    [junit] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 3.972 sec
    [junit] Running scala.tools.nsc.backend.jvm.analysis.NullnessAnalyzerTest
    [junit] Tests run: 9, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 4.972 sec
    [junit] Running scala.tools.nsc.backend.jvm.analysis.ProdConsAnalyzerTest
    [junit] Tests run: 11, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 4.083 sec
    [junit] Running scala.tools.nsc.backend.jvm.opt.BTypesFromClassfileTest
    ...

no idea why it's not picked up.. i can run the test in intellij.

@soc the answer is here: https://github.com/scala/scala/blob/2.11.x/build.xml#L1562. You need to name the class StringConcatTest.

@lrytz Arrrgh, thanks a lot! :-)

Ok, this is weird: def result = "abc" + 1

  public java.lang.String result();
    Code:
      stack=2, locals=1, args_size=1
         0: new           #17                 // class java/lang/StringBuilder
         3: dup
         4: invokespecial #21                 // Method java/lang/StringBuilder."<init>":()V
         7: ldc           #23                 // String abc
         9: invokevirtual #27                 // Method java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
        12: iconst_1
        13: invokestatic  #33                 // Method scala/runtime/BoxesRunTime.boxToInteger:(I)Ljava/lang/Integer;
        16: invokevirtual #36                 // Method java/lang/StringBuilder.append:(Ljava/lang/Object;)Ljava/lang/StringBuilder;
        19: invokevirtual #39                 // Method java/lang/StringBuilder.toString:()Ljava/lang/String;
        22: areturn

I don't think it is intentional that we box the primitives and use append(Object), right?

That's why it's so great that finally somebody is writing tests for this thing :-)

Boxing is added in erasure.

$ scala -Xprint:erasure

scala> def result = "abc" + 1
[[syntax trees at end of                   erasure]] // <console>
package $line3 {
  object $read extends Object {
    def <init>(): $line3.$read.type = {
      $read.super.<init>();
      ()
    };
    object $iw extends Object {
      def <init>(): type = {
        $iw.super.<init>();
        ()
      };
      object $iw extends Object {
        def <init>(): type = {
          $iw.super.<init>();
          ()
        };
        def result(): String = "abc".+(scala.Int.box(1))
      }
    }
  }
}

It must be because there's only a single + method "added" to the string class: https://github.com/scala/scala/blob/2.11.x/src/reflect/scala/reflect/internal/Definitions.scala#L1061. We should add overloads for the primitives.

here's a starting point: lrytz@e90fb97

@soc this needs a rebase. Once the TODOs in #4737 (comment) are addressed I think we can get this in!

@soc do you have time to push this one forward? it would be a nice addition to 2.12.0-M4 (probably mid-february).

@lrytz Rebased, addressed comments, added test.

@lrytz Done!

LGTM, thanks for another iteration!