Merge pull request #559 from scala/backport-lts-3.3-23697

Backport "Do not discard amended format when f-interpolator warns" to 3.3 LTS

Author: tgodzik

compiler/src/dotty/tools/dotc/transform/localopt/FormatChecker.scala

@@ -116,7 +116,7 @@ class TypedFormatChecker(partsElems: List[Tree], parts: List[String], args: List
       case Nil =>
 
     loop(parts, n = 0)
-    if reported then (Nil, Nil)
+    if reported then (Nil, Nil) // on error, Transform.checked will revert to unamended inputs
     else
       assert(argc == actuals.size, s"Expected ${argc} args but got ${actuals.size} for [${parts.mkString(", ")}]")
       (amended.toList, actuals.toList)
@@ -320,5 +320,4 @@ class TypedFormatChecker(partsElems: List[Tree], parts: List[String], args: List
         .tap(_ => reported = true)
     def partWarning(message: String, index: Int, offset: Int, end: Int): Unit =
       r.warning(BadFormatInterpolation(message), partPosAt(index, offset, end))
-        .tap(_ => reported = true)
 end TypedFormatChecker

compiler/src/dotty/tools/dotc/transform/localopt/FormatInterpolatorTransform.scala

@@ -10,6 +10,8 @@ import dotty.tools.dotc.core.Contexts.*
 import dotty.tools.dotc.core.StdNames.*
 import dotty.tools.dotc.core.Symbols.*
 import dotty.tools.dotc.core.Types.*
+import dotty.tools.dotc.printing.Formatting.*
+import dotty.tools.dotc.reporting.BadFormatInterpolation
 import dotty.tools.dotc.transform.MegaPhase.MiniPhase
 import dotty.tools.dotc.typer.ConstFold
 
@@ -22,16 +24,17 @@ import dotty.tools.dotc.typer.ConstFold
  */
 class StringInterpolatorOpt extends MiniPhase:
   import tpd.*
+  import StringInterpolatorOpt.*
 
-  override def phaseName: String = StringInterpolatorOpt.name
+  override def phaseName: String = name
 
   override def description: String = StringInterpolatorOpt.description
 
   override def checkPostCondition(tree: tpd.Tree)(using Context): Unit =
     tree match
       case tree: RefTree =>
         val sym = tree.symbol
-        assert(!StringInterpolatorOpt.isCompilerIntrinsic(sym),
+        assert(!isCompilerIntrinsic(sym),
           i"$tree in ${ctx.owner.showLocated} should have been rewritten by phase $phaseName")
       case _ =>
 
@@ -116,10 +119,10 @@ class StringInterpolatorOpt extends MiniPhase:
           !(tp =:= defn.StringType)
           && {
               tp =:= defn.UnitType
-              && { report.warning("interpolated Unit value", t.srcPos); true }
+              && { report.warning(bfi"interpolated Unit value", t.srcPos); true }
             ||
               !tp.isPrimitiveValueType
-              && { report.warning("interpolation uses toString", t.srcPos); true }
+              && { report.warning(bfi"interpolation uses toString", t.srcPos); true }
           }
       if ctx.settings.Whas.toStringInterpolated then
         checkIsStringify(t.tpe): Unit
@@ -133,10 +136,38 @@ class StringInterpolatorOpt extends MiniPhase:
         case _        => false
     // Perform format checking and normalization, then make it StringOps(fmt).format(args1) with tweaked args
     def transformF(fun: Tree, args: Tree): Tree =
-      val (fmt, args1) = FormatInterpolatorTransform.checked(fun, args)
+      // For f"${arg}%xpart", check format conversions and return (format, args) for String.format(format, args).
+      def checked(args0: Tree)(using Context): (Tree, Tree) =
+        val (partsExpr, parts) = fun match
+          case TypeApply(Select(Apply(_, (parts: SeqLiteral) :: Nil), _), _) =>
+            (parts.elems, parts.elems.map { case Literal(Constant(s: String)) => s })
+          case _ =>
+            report.error("Expected statically known StringContext", fun.srcPos)
+            (Nil, Nil)
+        val (args, elemtpt) = args0 match
+          case seqlit: SeqLiteral => (seqlit.elems, seqlit.elemtpt)
+          case _ =>
+            report.error("Expected statically known argument list", args0.srcPos)
+            (Nil, EmptyTree)
+
+        def literally(s: String) = Literal(Constant(s))
+        if parts.lengthIs != args.length + 1 then
+          val badParts =
+            if parts.isEmpty then "there are no parts"
+            else s"too ${if parts.lengthIs > args.length + 1 then "few" else "many"} arguments for interpolated string"
+          report.error(badParts, fun.srcPos)
+          (literally(""), args0)
+        else
+          val checker = TypedFormatChecker(partsExpr, parts, args)
+          val (format, formatArgs) = checker.checked
+          if format.isEmpty then (literally(parts.mkString), args0) // on error just use unchecked inputs
+          else (literally(format.mkString), SeqLiteral(formatArgs.toList, elemtpt))
+      end checked
+      val (fmt, args1) = checked(args)
       resolveConstructor(defn.StringOps.typeRef, List(fmt))
         .select(nme.format)
         .appliedTo(args1)
+    end transformF
     // Starting with Scala 2.13, s and raw are macros in the standard
     // library, so we need to expand them manually.
     // sc.s(args)    -->   standardInterpolator(processEscapes, args, sc.parts)
@@ -185,3 +216,7 @@ object StringInterpolatorOpt:
     sym == defn.StringContext_s ||
     sym == defn.StringContext_f ||
     sym == defn.StringContext_raw
+
+  extension (sc: StringContext)
+    def bfi(args: Shown*)(using Context): BadFormatInterpolation =
+      BadFormatInterpolation(i(sc)(args*))

compiler/src/dotty/tools/dotc/transform/localopt/StringInterpolatorOpt.scala

@@ -0,0 +1,22 @@
+//> using options -Wtostring-interpolated
+
+// verify ~warning messages and~ runtime result
+// never mind, the test rig doesn't log diagnostics! unlike beloved partest.
+
+// Sadly, junit is not available.
+//import org.junit.Assert.assertEquals as jassert
+
+def assertEquals(expected: String)(actual: String): Unit = assert(expected == actual)
+
+case class K(i: Int)
+
+@main def Test =
+  val k = K(42)
+  assertEquals("k == K(42)"):
+    s"k == $k"
+  assertEquals("\\k == \\K(42)"):
+    raw"\k == \$k"
+  assertEquals("k == K(42)"):
+    f"k == $k"
+  assertEquals("k == K(42)"):
+    f"k == $k%s"

tests/run/i23693.scala

@@ -0,0 +1,20 @@
+-- [E209] Interpolation Warning: tests/warn/i23693.scala:11:12 ---------------------------------------------------------
+11 |    s"k == $k" // warn
+   |            ^
+   |            interpolation uses toString
+-- [E209] Interpolation Warning: tests/warn/i23693.scala:13:16 ---------------------------------------------------------
+13 |    raw"\k == \$k" // warn
+   |                ^
+   |                interpolation uses toString
+-- [E209] Interpolation Warning: tests/warn/i23693.scala:15:12 ---------------------------------------------------------
+15 |    f"k == $k" // warn
+   |            ^
+   |            interpolation uses toString
+-- [E209] Interpolation Warning: tests/warn/i23693.scala:17:14 ---------------------------------------------------------
+17 |    f"k == $k%s" // warn
+   |              ^
+   |              interpolation uses toString
+-- [E209] Interpolation Warning: tests/warn/i23693.scala:19:18 ---------------------------------------------------------
+19 |    s"show == ${k.show}" // warn
+   |                ^^^^^^
+   |                interpolated Unit value

tests/warn/i23693.check

@@ -0,0 +1,19 @@
+//> using options -Wtostring-interpolated
+
+// verify warning messages; cf run test; we must verify runtime while warning.
+
+case class K(i: Int):
+  def show: Unit = ()
+
+@main def Test =
+  val k = K(42)
+  println:
+    s"k == $k" // warn
+  println:
+    raw"\k == \$k" // warn
+  println:
+    f"k == $k" // warn
+  println:
+    f"k == $k%s" // warn
+  println:
+    s"show == ${k.show}" // warn