Merge pull request #10080 from som-snytt/tweak/cleanup-nil-small-seq-…

…list

Restore dealias Nil in Cleanup

src/compiler/scala/tools/nsc/transform/CleanUp.scala

@@ -489,25 +489,8 @@ abstract class CleanUp extends Statics with Transform with ast.TreeDSL {
     }
 
     def transformApply(tree: Apply, fun: Tree, args: List[Tree]): Tree = tree match {
-     /*
-      * This transformation should identify Scala symbol invocations in the tree and replace them
-      * with references to a statically cached instance.
-      *
-      * The reasoning behind this transformation is the following. Symbols get interned - they are stored
-      * in a global map which is protected with a lock. The reason for this is making equality checks
-      * quicker. But calling Symbol.apply, although it does return a unique symbol, accesses a locked object,
-      * making symbol access slow. To solve this, the unique symbol from the global symbol map in Symbol
-      * is accessed only once during class loading, and after that, the unique symbol is in the statically
-      * initialized call site returned by invokedynamic. Hence, it is cheap to both reach the unique symbol
-      * and do equality checks on it.
-      *
-      * And, finally, be advised - Scala's Symbol literal (scala.Symbol) and the Symbol class of the compiler
-      * have little in common.
-      */
-      case Apply(Select(qual, _), (arg @ Literal(Constant(symname: String))) :: Nil)
-      if fun.symbol == Symbol_apply && !currentClass.isTrait && treeInfo.isQualifierSafeToElide(qual) =>
-
-        treeCopy.ApplyDynamic(tree, atPos(fun.pos)(Ident(SymbolLiteral_dummy).setType(SymbolLiteral_dummy.info)), LIT(SymbolLiteral_bootstrap) :: arg :: Nil).transform(this)
+      case Apply(Select(qual, nm), Nil) if nm == nme.Nil && qual.symbol == ScalaPackageObject =>
+        typedWithPos(tree.pos)(gen.mkNil)
 
       // Drop the TypeApply, which was used in Erasure to make `synchronized { ... } ` erase like `...`
       // (and to avoid boxing the argument to the polymorphic `synchronized` method).
@@ -541,29 +524,32 @@ abstract class CleanUp extends Statics with Transform with ast.TreeDSL {
               }
             }
         }
+
       case Apply(appMeth @ Select(appMethQual, _), elem0 :: Apply(wrapArrayMeth, (rest @ ArrayValue(elemtpt, _)) :: Nil) :: Nil)
       if wrapArrayMeth.symbol == wrapVarargsArrayMethod(elemtpt.tpe) && appMeth.symbol == ArrayModule_apply(elemtpt.tpe) && treeInfo.isQualifierSafeToElide(appMethQual) =>
         treeCopy.ArrayValue(rest, rest.elemtpt, elem0 :: rest.elems).transform(this)
+
       // See scala/bug#12201, should be rewrite as Primitive Array.
       // Match Array
       case Apply(appMeth @ Select(appMethQual, _), Apply(wrapRefArrayMeth, StripCast(ArrayValue(elemtpt, elems)) :: Nil) :: _ :: Nil) 
       if appMeth.symbol == ArrayModule_genericApply && treeInfo.isQualifierSafeToElide(appMethQual) && currentRun.runDefinitions.primitiveWrapArrayMethod.contains(wrapRefArrayMeth.symbol) =>
         typedWithPos(elemtpt.pos)(
           ArrayValue(TypeTree(elemtpt.tpe), elems)
         ).transform(this)
+
       case Apply(appMeth @ Select(appMethQual, _), elem :: (nil: RefTree) :: Nil)
       if nil.symbol == NilModule && appMeth.symbol == ArrayModule_apply(elem.tpe.widen) && treeInfo.isExprSafeToInline(nil) && treeInfo.isQualifierSafeToElide(appMethQual) =>
         typedWithPos(elem.pos)(
           ArrayValue(TypeTree(elem.tpe), elem :: Nil)
         ).transform(this)
-      // List(a, b, c) ~> new ::(a, new ::(b, new ::(c, Nil)))
-      // Seq(a, b, c) ~> new ::(a, new ::(b, new ::(c, Nil)))
-      case Apply(appMeth @ Select(appQual, _), List(Apply(wrapArrayMeth, List(StripCast(rest @ ArrayValue(elemtpt, _))))))
+
+      // <List or Seq>(a, b, c) ~> new ::(a, new ::(b, new ::(c, Nil))) but only for reference types
+      case StripCast(Apply(appMeth @ Select(appQual, _), List(Apply(wrapArrayMeth, List(StripCast(rest @ ArrayValue(elemtpt, _)))))))
       if wrapArrayMeth.symbol == currentRun.runDefinitions.wrapVarargsRefArrayMethod
-        && currentRun.runDefinitions.isSeqApply(appMeth) && rest.elems.lengthIs < transformListApplyLimit =>
-        val consed = rest.elems.reverse.foldLeft(gen.mkAttributedRef(NilModule): Tree)(
-          (acc, elem) => New(ConsClass, elem, acc)
-        )
+        && currentRun.runDefinitions.isSeqApply(appMeth)  // includes List
+        && rest.elems.lengthIs < transformListApplyLimit
+      =>
+        val consed = rest.elems.foldRight(gen.mkAttributedRef(NilModule): Tree)(New(ConsClass, _, _))
         // Limiting extra stack frames consumed by generated code
         reducingTransformListApply(rest.elems.length) {
           super.transform(typedWithPos(tree.pos)(consed))
@@ -634,9 +620,28 @@ abstract class CleanUp extends Statics with Transform with ast.TreeDSL {
 
       // Seq() ~> Nil (note: List() ~> Nil is rewritten in the Typer)
       case Apply(Select(_, _), List(nil))
-      if nil.symbol == NilModule && currentRun.runDefinitions.isSeqApply(fun) =>
+      if currentRun.runDefinitions.isNil(nil.symbol) && currentRun.runDefinitions.isSeqApply(fun) =>
         gen.mkAttributedRef(NilModule)
 
+      /* This transformation should identify Scala symbol invocations in the tree and replace them
+       * with references to a statically cached instance.
+       *
+       * The reasoning behind this transformation is the following. Symbols get interned - they are stored
+       * in a global map which is protected with a lock. The reason for this is making equality checks
+       * quicker. But calling Symbol.apply, although it does return a unique symbol, accesses a locked object,
+       * making symbol access slow. To solve this, the unique symbol from the global symbol map in Symbol
+       * is accessed only once during class loading, and after that, the unique symbol is in the statically
+       * initialized call site returned by invokedynamic. Hence, it is cheap to both reach the unique symbol
+       * and do equality checks on it.
+       *
+       * And, finally, be advised - Scala's Symbol literal (scala.Symbol) and the Symbol class of the compiler
+       * have little in common.
+       */
+      case Apply(Select(qual, _), (arg @ Literal(Constant(symname: String))) :: Nil)
+      if fun.symbol == Symbol_apply && !currentClass.isTrait && treeInfo.isQualifierSafeToElide(qual) =>
+
+        treeCopy.ApplyDynamic(tree, atPos(fun.pos)(Ident(SymbolLiteral_dummy).setType(SymbolLiteral_dummy.info)), LIT(SymbolLiteral_bootstrap) :: arg :: Nil).transform(this)
+
       case _ =>
         super.transform(tree)
     }
@@ -718,7 +723,7 @@ abstract class CleanUp extends Statics with Transform with ast.TreeDSL {
         super.transform(tree)
     }
 
-  } // CleanUpTransformer
+  } // end CleanUpTransformer
 
 
   private val objectMethods = Map[Name, TermName](

src/partest/scala/tools/partest/CompilerTest.scala

@@ -15,7 +15,11 @@ package scala.tools.partest
 import scala.reflect.runtime.{universe => ru}
 import scala.tools.nsc._
 
-/** For testing compiler internals directly.
+/** A DirectTest for testing compiler internals.
+ *  The test must implement the `check` function to check
+ *  the result of compiling the `code`; the test may override
+ *  `sources` instead to check multiple sources.
+ *
  *  Each source code string in "sources" will be compiled, and
  *  the check function will be called with the source code and the
  *  resulting CompilationUnit.  The check implementation should
@@ -28,9 +32,7 @@ abstract class CompilerTest extends DirectTest {
   lazy val global: Global = newCompiler()
   lazy val units: List[global.CompilationUnit] = compilationUnits(global)(sources: _ *)
   import global._
-  import definitions.{ compilerTypeFromTag }
-
-  override def extraSettings = "-usejavacp -d " + testOutput.path
+  import definitions.compilerTypeFromTag
 
   def show() = sources.lazyZip(units).foreach(check)
 

src/reflect/scala/reflect/internal/Definitions.scala

@@ -196,6 +196,7 @@ trait Definitions extends api.StandardDefinitions {
     lazy val JavaLangPackageClass = JavaLangPackage.moduleClass.asClass
     lazy val ScalaPackage         = getPackage("scala")
     lazy val ScalaPackageClass    = ScalaPackage.moduleClass.asClass
+    lazy val ScalaPackageObject   = getPackageObjectIfDefined("scala")
     lazy val RuntimePackage       = getPackage("scala.runtime")
     lazy val RuntimePackageClass  = RuntimePackage.moduleClass.asClass
 
@@ -493,6 +494,7 @@ trait Definitions extends api.StandardDefinitions {
          def List_apply       = getMemberMethod(ListModule, nme.apply)
     lazy val ListModuleAlias  = getMemberValue(ScalaPackageClass, nme.List)
     lazy val NilModule        = requiredModule[scala.collection.immutable.Nil.type]
+    lazy val NilModuleAlias   = getMemberValue(ScalaPackageClass, nme.Nil)
     @migration("SeqModule now refers to scala.collection.immutable.Seq", "2.13.0")
     lazy val SeqModule        = requiredModule[scala.collection.immutable.Seq.type]
     lazy val SeqModuleAlias   = getMemberValue(ScalaPackageClass, nme.Seq)
@@ -1747,6 +1749,8 @@ trait Definitions extends api.StandardDefinitions {
         })
       }
 
+      final def isNil(sym: Symbol) = sym == NilModule || sym == NilModuleAlias
+
       def isPredefClassOf(sym: Symbol) = if (PredefModule.hasCompleteInfo) sym == Predef_classOf else isPredefMemberNamed(sym, nme.classOf)
 
       lazy val TagMaterializers = Map[Symbol, Symbol](

src/reflect/scala/reflect/runtime/JavaUniverseForce.scala

@@ -236,6 +236,7 @@ trait JavaUniverseForce { self: runtime.JavaUniverse  =>
     definitions.JavaLangPackageClass
     definitions.ScalaPackage
     definitions.ScalaPackageClass
+    definitions.ScalaPackageObject
     definitions.RuntimePackage
     definitions.RuntimePackageClass
     definitions.AnyClass
@@ -316,6 +317,7 @@ trait JavaUniverseForce { self: runtime.JavaUniverse  =>
     definitions.ListModule
     definitions.ListModuleAlias
     definitions.NilModule
+    definitions.NilModuleAlias
     definitions.SeqModule
     definitions.SeqModuleAlias
     definitions.Collection_SeqModule

test/files/run/smallseq.check

@@ -0,0 +1,15 @@
+[[syntax trees at end of                   cleanup]] // newSource1.scala
+package <empty> {
+  class C extends Object {
+    def f0(): Seq = scala.collection.immutable.Nil.$asInstanceOf[Seq]();
+    def f1(): Seq = scala.collection.immutable.Nil.$asInstanceOf[Seq]();
+    def g(): Seq = scala.collection.immutable.Nil.$asInstanceOf[Seq]();
+    def h(): Seq = scala.collection.immutable.Nil.$asInstanceOf[Seq]();
+    def nil(): scala.collection.immutable.Nil.type = scala.collection.immutable.Nil;
+    def <init>(): C = {
+      C.super.<init>();
+      ()
+    }
+  }
+}
+

test/files/run/smallseq.scala

@@ -0,0 +1,24 @@
+
+import scala.tools.partest._
+
+object Test extends DirectTest {
+  override def extraSettings: String = "-usejavacp -Vprint:cleanup"
+  override def code = """
+    class C {
+      def f0 = Seq(Nil: _*)
+      def f1 = Seq.apply[String](Nil: _*)
+
+      def g = Seq.apply[String](scala.collection.immutable.Nil: _*)
+
+      def h = scala.collection.immutable.Seq.apply[String](scala.collection.immutable.Nil: _*)
+
+      def nil = Nil
+    }
+  """
+  override def show() = assert(compile())
+}
+
+/*
+was:
+def f0(): Seq = scala.`package`.Seq().apply(scala.`package`.Nil()).$asInstanceOf[Seq]();
+ */

test/files/run/t2318.scala

@@ -1,15 +1,17 @@
+// scalac: -Xlint:deprecation
 // java: -Ddummy=fresh_jvm_needed_to_test_security_manager
 // filter: WARNING.*
 // for now, ignore warnings due to reflective invocation
 import java.security._
 
-import scala.language.reflectiveCalls
 import scala.annotation.nowarn
+import scala.language.reflectiveCalls
+import scala.util.Properties.isJavaAtLeast
 
 // SecurityManager is deprecated on JDK 17, so we sprinkle `@nowarn` around
 
 object Test {
-  trait Bar { def bar: Unit }
+  trait Bar { def bar(): Unit }
 
   @nowarn("cat=deprecation")
   object Mgr extends SecurityManager {
@@ -28,21 +30,25 @@ object Test {
   }
 
   def t1() = {
+    @nowarn("cat=deprecation")
     val p = Runtime.getRuntime().exec("ls");
     type Destroyable = { def destroy() : Unit }
     def doDestroy( obj : Destroyable ) : Unit = obj.destroy();
     doDestroy( p );
   }
 
   @nowarn("cat=deprecation")
-  def t2() = {
+  def t2() = if (!isJavaAtLeast(18)) {
     System.setSecurityManager(Mgr)
+    var count = 0
+
+    val b = new Bar { def bar() = count += 1 }
+    b.bar()
 
-    val b = new Bar { def bar = println("bar") }
-    b.bar
+    val structural = b.asInstanceOf[{ def bar(): Unit }]
+    structural.bar()
 
-    val structural = b.asInstanceOf[{ def bar: Unit }]
-    structural.bar
+    assert(count == 2, "Expected 2 invocations")
   }
 
   def main(args: Array[String]): Unit = {

test/files/run/t5064.check

@@ -34,6 +34,6 @@ newSource1.scala:6: warning: a pure expression does nothing in statement positio
 [42:45]         scala.`package`.Seq()
 [42:45]         scala.`package`.Seq
 <42:45>         scala.`package`
-[48:51]         scala.`package`.Nil()
-[48:51]         scala.`package`.Nil
-<48:51>         scala.`package`
+[48:51]         scala.collection.immutable.Nil
+[48:51]         scala.collection.immutable
+[48:51]         scala.collection

test/files/run/t5064.scala

@@ -15,7 +15,7 @@ object Test extends CompilerTest {
   def check(source: String, unit: CompilationUnit): Unit = {
     for (ClassDef(_, _, _, Template(_, _, stats)) <- unit.body ; stat <- stats ; t <- stat) {
       t match {
-        case _: Select | _: Apply | _: This => println("%-15s %s".format(t.pos.show, t))
+        case _: Select | _: Apply | _: This => println(f"${t.pos.show}%-15s $t")
         case _                              =>
       }
     }

test/files/run/t6989.check

@@ -107,12 +107,14 @@ isProtected = false
 isPublic = true
 privateWithin = <none>
 ============
+#partest !java18
 sym = value this$0, signature = foo.JavaClass_1, owner = class $PrivateJavaClass
 isPrivate = false
 isProtected = false
 isPublic = false
 privateWithin = package foo
 ============
+#partest
 sym = class $PrivateJavaClass, signature = JavaClass_1.this.$PrivateJavaClass.type, owner = class JavaClass_1
 isPrivate = true
 isProtected = false
@@ -131,12 +133,14 @@ isProtected = false
 isPublic = true
 privateWithin = <none>
 ============
+#partest !java18
 sym = value this$0, signature = foo.JavaClass_1, owner = class $ProtectedJavaClass
 isPrivate = false
 isProtected = false
 isPublic = false
 privateWithin = package foo
 ============
+#partest
 sym = class $ProtectedJavaClass, signature = JavaClass_1.this.$ProtectedJavaClass.type, owner = class JavaClass_1
 isPrivate = false
 isProtected = false
@@ -155,12 +159,14 @@ isProtected = false
 isPublic = true
 privateWithin = <none>
 ============
+#partest !java18
 sym = value this$0, signature = foo.JavaClass_1, owner = class $PublicJavaClass
 isPrivate = false
 isProtected = false
 isPublic = false
 privateWithin = package foo
 ============
+#partest
 sym = class $PublicJavaClass, signature = JavaClass_1.this.$PublicJavaClass.type, owner = class JavaClass_1
 isPrivate = false
 isProtected = false

test/files/run/t7852.scala

@@ -19,7 +19,8 @@ object Test extends BytecodeTest {
     test("string", expected = 0)
     test("module", expected = 0)
     test("moduleIndirect", expected = 2)
-    test("moduleViaPredefAlias", expected = 2)
+    test("nilModuleViaScalaPackageAlias", expected = 0)
+    test("moduleViaScalaPackageAlias", expected = 2)
   }
 
   def countNullChecks(insnList: asm.tree.InsnList): Int =
@@ -40,7 +41,11 @@ class Lean {
     n == (toString: Any) // still need null checks here.
   }
 
-  def moduleViaPredefAlias: Unit = {
+  def nilModuleViaScalaPackageAlias: Unit = {
     Nil == (toString: Any)
   }
+
+  def moduleViaScalaPackageAlias: Unit = {
+    Right == (toString: Any)
+  }
 }

test/junit/scala/collection/immutable/ListTest.scala

@@ -6,11 +6,11 @@ import org.junit.runner.RunWith
 import org.junit.runners.JUnit4
 
 import scala.ref.WeakReference
-import scala.tools.testkit.{AllocationTest, CompileTime}
+import scala.tools.testkit.AllocationTest
 import scala.collection.Sizes
 
 @RunWith(classOf[JUnit4])
-class ListTest extends AllocationTest{
+class ListTest extends AllocationTest {
   /**
    * Test that empty iterator does not hold reference
    * to complete List
@@ -106,9 +106,7 @@ class ListTest extends AllocationTest{
     nonAllocating(List())
   }
 
-
   @Test def smallListAllocation(): Unit = {
-    if (CompileTime.versionNumberString == "2.13.2") return
     exactAllocates(Sizes.list * 1, "list  size 1")(List("0"))
     exactAllocates(Sizes.list * 2, "list  size 2")(List("0", "1"))
     exactAllocates(Sizes.list * 3, "list  size 3")(List("0", "1", ""))