scala / scala-dev

Scala 2 team issues. Not for user-facing bugs or directly actionable user-facing improvements. For build/test/infra and for longer-term planning and idea tracking. Our bug tracker is at https://github.com/scala/bug/issues
Apache License 2.0
130 stars 14 forks source link

Optimize Constant.escape #878

Closed retronym closed 1 week ago

retronym commented 1 month ago

Something like this:

Index: src/reflect/scala/reflect/internal/Constants.scala
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/reflect/scala/reflect/internal/Constants.scala b/src/reflect/scala/reflect/internal/Constants.scala
--- a/src/reflect/scala/reflect/internal/Constants.scala    (revision b9f3f5609379aae3585b60a8e402a9a17283de56)
+++ b/src/reflect/scala/reflect/internal/Constants.scala    (date 1729069211440)
@@ -233,7 +233,9 @@
       else if (tag == ClazzTag) signature(typeValue)
       else value.toString()

-    def escapedChar(ch: Char): String = (ch: @switch) match {
+    def escapedChar(ch: Char): String = Option(escapedCharOrNull(ch)).getOrElse(String.valueOf(ch))
+
+    private def escapedCharOrNull(ch: Char): String = (ch: @switch) match {
       case '\b' => "\\b"
       case '\t' => "\\t"
       case '\n' => "\\n"
@@ -242,11 +244,26 @@
       case '"'  => "\\\""
       case '\'' => "\\\'"
       case '\\' => "\\\\"
-      case _    => if (ch.isControl) "\\u%04X".format(ch.toInt) else String.valueOf(ch)
+      case _    => if (ch.isControl) "\\u%04X".format(ch.toInt) else null
     }

     def escapedStringValue: String = {
-      def escape(text: String): String = text flatMap escapedChar
+      def escape(text: String): String = {
+        val builder = new java.lang.StringBuilder(text.length)
+        var i = 0
+        var replaced = false
+        while (i < builder.length) {
+          val ch = builder.charAt(i)
+          val replacement = escapedCharOrNull(ch)
+          if (replacement != null) {
+            replaced = true
+            builder.append(replacement)
+          } else builder.append(ch)
+          i += 1
+        }
+        if (replaced) builder.toString
+        else text
+      }
       tag match {
         case NullTag   => "null"
         case StringTag => "\"" + escape(stringValue) + "\""

The motivation is that this gets called indirectly by SBT ExtractApi.mkAnnotations in a codepath when assocs is empty but args is not. It prints the tree for the annotation argument in that case.

https://github.com/scala/scala/blob/5f4087b3aa029a30cb5a21fa003e180f000ce552/src/sbt-bridge/scala/tools/xsbt/ExtractAPI.scala#L259-L262

I can't remember what flavour of annotation ends up with this representation. Would be good to relate this to a particular source construct to complete our mental picture.

som-snytt commented 1 month ago

@SethTisue if you haven't started editing yet, I'll type it in. I think I saw Dotty has the same code, so I'll do that at the same time. I fell asleep last night when I was going to look at the async in repl ticket.

SethTisue commented 1 month ago

If you want to take it, feel free! If you lose interest or get stuck, I can take it over.

som-snytt commented 1 week ago

Somehow, I never lost interest.