Merge pull request scala#10776 from som-snytt/topic/warn-toString-interpolated

Warn if interpolator uses toString

Author: som-snytt

src/compiler/scala/tools/nsc/Reporting.scala

@@ -572,6 +572,7 @@ object Reporting {
         WFlagNumericWiden,
         WFlagSelfImplicit,
         WFlagUnnamedBooleanLiteral,
+        WFlagTostringInterpolated,
         WFlagValueDiscard
       = wflag()
 

src/compiler/scala/tools/nsc/settings/Warnings.scala

@@ -123,6 +123,7 @@ trait Warnings {
   val warnOctalLiteral     = BooleanSetting("-Woctal-literal", "Warn on obsolete octal syntax.") withAbbreviation "-Ywarn-octal-literal"
   val warnUnnamedBoolean   = BooleanSetting("-Wunnamed-boolean-literal", "Warn about unnamed boolean literals if there is more than one or defaults are used, unless parameter has @deprecatedName.")
   val warnUnnamedStrict    = BooleanSetting("-Wunnamed-boolean-literal-strict", "Warn about all unnamed boolean literals, unless parameter has @deprecatedName or the method has a single leading boolean parameter.").enabling(warnUnnamedBoolean :: Nil)
+  val warnToString         = BooleanSetting("-Wtostring-interpolated", "Warn when a standard interpolator uses toString.")
 
   object PerformanceWarnings extends MultiChoiceEnumeration {
     val Captured       = Choice("captured",        "Modification of var in closure causes boxing.")

src/compiler/scala/tools/reflect/FastStringInterpolator.scala

@@ -13,7 +13,7 @@
 package scala.tools
 package reflect
 
-import nsc.Reporting.WarningCategory.Scala3Migration
+import nsc.Reporting.WarningCategory.{Scala3Migration, WFlagTostringInterpolated}
 
 trait FastStringInterpolator extends FormatInterpolator {
   import c.universe._
@@ -101,11 +101,15 @@ trait FastStringInterpolator extends FormatInterpolator {
       val treatedContents = lit.asInstanceOf[Literal].value.stringValue
       val emptyLit = treatedContents.isEmpty
       if (i < numLits - 1) {
-        concatArgs += argsIndexed(i)
-        if (!emptyLit) concatArgs += lit
-      } else if (!emptyLit) {
-        concatArgs += lit
+        val arg = argsIndexed(i)
+        if (linting && !(arg.tpe =:= definitions.StringTpe))
+          if (arg.tpe.typeSymbol eq definitions.UnitClass)
+            runReporting.warning(arg.pos, "interpolated Unit value", WFlagTostringInterpolated, c.internal.enclosingOwner)
+          else if (!definitions.isPrimitiveValueType(arg.tpe))
+            runReporting.warning(arg.pos, "interpolation uses toString", WFlagTostringInterpolated, c.internal.enclosingOwner)
+        concatArgs += arg
       }
+      if (!emptyLit) concatArgs += lit
     }
     def mkConcat(pos: Position, lhs: Tree, rhs: Tree): Tree =
       atPos(pos)(gen.mkMethodCall(gen.mkAttributedSelect(lhs, definitions.String_+), rhs :: Nil)).setType(definitions.StringTpe)

src/compiler/scala/tools/reflect/FormatInterpolator.scala

@@ -12,11 +12,12 @@
 
 package scala.tools.reflect
 
-import scala.reflect.macros.runtime.Context
-import scala.collection.mutable.ListBuffer
 import scala.PartialFunction.cond
-import scala.util.chaining._
+import scala.collection.mutable.ListBuffer
+import scala.reflect.macros.runtime.Context
+import scala.tools.nsc.Reporting.WarningCategory, WarningCategory.WFlagTostringInterpolated
 import scala.util.matching.Regex.Match
+import scala.util.chaining._
 
 import java.util.Formattable
 
@@ -31,6 +32,14 @@ abstract class FormatInterpolator {
   import definitions._
   import treeInfo.Applied
 
+  protected var linting = settings.warnToString.value
+
+  protected final def withoutLinting[A](body: => A): A = {
+    val linted = linting
+    linting = false
+    try body finally linting = linted
+  }
+
   private def bail(msg: String) = global.abort(msg)
 
   def concatenate(parts: List[Tree], args: List[Tree]): Tree
@@ -87,8 +96,9 @@ abstract class FormatInterpolator {
 
     def argType(argi: Int, types: Type*): Type = {
       val tpe = argTypes(argi)
-      types.find(t => argConformsTo(argi, tpe, t))
-        .orElse(types.find(t => argConvertsTo(argi, tpe, t)))
+      types.find(t => t != AnyTpe && argConformsTo(argi, tpe, t))
+        .orElse(types.find(t => t != AnyTpe && argConvertsTo(argi, tpe, t)))
+        .orElse(types.find(t => t == AnyTpe && argConformsTo(argi, tpe, t)))
         .getOrElse {
           val msg = "type mismatch" + {
             val req = raw"required: (.*)".r.unanchored
@@ -120,7 +130,7 @@ abstract class FormatInterpolator {
     // Check the % fields in this part.
     def loop(remaining: List[Tree], n: Int): Unit =
       remaining match {
-        case part0 :: more =>
+        case part0 :: remaining =>
           val part1 = part0 match {
             case Literal(Constant(x: String)) => x
             case _ => throw new IllegalArgumentException("internal error: argument parts must be a list of string literals")
@@ -130,14 +140,17 @@ abstract class FormatInterpolator {
 
           def insertStringConversion(): Unit = {
             amended += "%s" + part
-            convert += Conversion(formatPattern.findAllMatchIn("%s").next(), part0.pos, argc)  // improve
-            argType(n-1, AnyTpe)
+            val cv = Conversion(part0.pos, argc)
+            cv.accepts(argType(n-1, AnyTpe))
+            convert += cv
+            cv.lintToString(argTypes(n-1))
           }
           def errorLeading(op: Conversion) = op.errorAt(Spec)(s"conversions must follow a splice; ${Conversion.literalHelp}")
           def accept(op: Conversion): Unit = {
             if (!op.isLeading) errorLeading(op)
             op.accepts(argType(n-1, op.acceptableVariants: _*))
             amended += part
+            op.lintToString(argTypes(n-1))
           }
 
           if (n == 0) amended += part
@@ -164,7 +177,7 @@ abstract class FormatInterpolator {
             else if (!cv.isLiteral && !cv.isIndexed) errorLeading(cv)
             formatting = true
           }
-          loop(more, n = n + 1)
+          loop(remaining, n = n + 1)
         case Nil =>
       }
     loop(parts, n = 0)
@@ -178,7 +191,11 @@ abstract class FormatInterpolator {
     val format = amended.mkString
     if (actuals.isEmpty && !formatting) constantly(format)
     else if (!reported && actuals.forall(treeInfo.isLiteralString)) constantly(format.format(actuals.map(_.asInstanceOf[Literal].value.value).toIndexedSeq: _*))
-    else if (!formatting) concatenate(amended.map(p => constantly(p.stripPrefix("%s"))).toList, actuals.toList)
+    else if (!formatting) {
+      withoutLinting { // already warned
+        concatenate(amended.map(p => constantly(p.stripPrefix("%s"))).toList, actuals.toList)
+      }
+    }
     else {
       val scalaPackage = Select(Ident(nme.ROOTPKG), TermName("scala"))
       val newStringOps = Select(
@@ -218,6 +235,7 @@ abstract class FormatInterpolator {
         case ErrorXn                     => op(0)
         case DateTimeXn if op.length > 1 => op(1)
         case DateTimeXn                  => '?'
+        case StringXn if op.isEmpty      => 's' // accommodate the default %s
         case _ => op(0)
       }
 
@@ -295,13 +313,19 @@ abstract class FormatInterpolator {
           }
         case _ => true
       }
+    def lintToString(arg: Type): Unit =
+      if (linting && kind == StringXn && !(arg =:= StringTpe))
+        if (arg.typeSymbol eq UnitClass)
+          warningAt(CC)("interpolated Unit value", WFlagTostringInterpolated)
+        else if (!definitions.isPrimitiveValueType(arg))
+          warningAt(CC)("interpolation uses toString", WFlagTostringInterpolated)
 
     // what arg type if any does the conversion accept
     def acceptableVariants: List[Type] =
       kind match {
         case StringXn if hasFlag('#') => FormattableTpe :: Nil
         case StringXn                 => AnyTpe :: Nil
-        case BooleanXn                => BooleanTpe :: NullTpe :: Nil
+        case BooleanXn                => BooleanTpe :: NullTpe :: AnyTpe :: Nil // warn if not boolean
         case HashXn                   => AnyTpe :: Nil
         case CharacterXn              => CharTpe :: ByteTpe :: ShortTpe :: IntTpe :: Nil
         case IntegralXn               => IntTpe :: LongTpe :: ByteTpe :: ShortTpe :: BigIntTpe :: Nil
@@ -331,7 +355,7 @@ abstract class FormatInterpolator {
 
     def groupPosAt(g: SpecGroup, i: Int) = pos.withPoint(pos.point + descriptor.offset(g, i))
     def errorAt(g: SpecGroup, i: Int = 0)(msg: String)   = c.error(groupPosAt(g, i), msg).tap(_ => reported = true)
-    def warningAt(g: SpecGroup, i: Int = 0)(msg: String) = c.warning(groupPosAt(g, i), msg)
+    def warningAt(g: SpecGroup, i: Int = 0)(msg: String, cat: WarningCategory = WarningCategory.Other) = c.callsiteTyper.context.warning(groupPosAt(g, i), msg, cat, Nil)
   }
 
   object Conversion {
@@ -356,6 +380,9 @@ abstract class FormatInterpolator {
         case None     => new Conversion(m, p, ErrorXn, argc).tap(_.errorAt(Spec)(s"Missing conversion operator in '${m.matched}'; $literalHelp"))
       }
     }
+    // construct a default %s conversion
+    def apply(p: Position, argc: Int): Conversion =
+      new Conversion(formatPattern.findAllMatchIn("%").next(), p, StringXn, argc)
     val literalHelp = "use %% for literal %, %n for newline"
   }
 

test/files/neg/stringinterpolation_macro-neg.check

@@ -10,11 +10,6 @@ stringinterpolation_macro-neg.scala:15: error: too many arguments for interpolat
 stringinterpolation_macro-neg.scala:16: error: too few arguments for interpolated string
   new StringContext("", "").f()
                              ^
-stringinterpolation_macro-neg.scala:19: error: type mismatch;
- found   : String
- required: Boolean, Null
-  f"$s%b"
-     ^
 stringinterpolation_macro-neg.scala:20: error: type mismatch;
  found   : String
  required: Char, Byte, Short, Int
@@ -162,6 +157,9 @@ stringinterpolation_macro-neg.scala:77: error: Missing conversion operator in '%
 stringinterpolation_macro-neg.scala:80: error: conversions must follow a splice; use %% for literal %, %n for newline
   f"${d}random-leading-junk%d"
                            ^
+stringinterpolation_macro-neg.scala:19: warning: Boolean format is null test for non-Boolean
+  f"$s%b"
+       ^
 stringinterpolation_macro-neg.scala:63: warning: Index is not this arg
   f"${8}%d ${9}%1$$d"
                 ^
@@ -171,5 +169,5 @@ stringinterpolation_macro-neg.scala:64: warning: Argument index ignored if '<' f
 stringinterpolation_macro-neg.scala:65: warning: Index is not this arg
   f"$s%s $s%1$$s"
             ^
-3 warnings
-46 errors
+4 warnings
+45 errors

test/files/neg/tostring-interpolated.check

@@ -0,0 +1,41 @@
+tostring-interpolated.scala:7: error: interpolation uses toString
+Applicable -Wconf / @nowarn filters for this fatal warning: msg=<part of the message>, cat=w-flag-tostring-interpolated, site=T.f
+  def f = f"$c" // warn
+             ^
+tostring-interpolated.scala:8: error: interpolation uses toString
+Applicable -Wconf / @nowarn filters for this fatal warning: msg=<part of the message>, cat=w-flag-tostring-interpolated, site=T.s
+  def s = s"$c" // warn
+             ^
+tostring-interpolated.scala:9: error: interpolation uses toString
+Applicable -Wconf / @nowarn filters for this fatal warning: msg=<part of the message>, cat=w-flag-tostring-interpolated, site=T.r
+  def r = raw"$c" // warn
+               ^
+tostring-interpolated.scala:11: error: interpolation uses toString
+Applicable -Wconf / @nowarn filters for this fatal warning: msg=<part of the message>, cat=w-flag-tostring-interpolated, site=T.format
+  def format = f"${c.x}%d in $c or $c%s" // warn using c.toString // warn
+                              ^
+tostring-interpolated.scala:11: error: interpolation uses toString
+Applicable -Wconf / @nowarn filters for this fatal warning: msg=<part of the message>, cat=w-flag-tostring-interpolated, site=T.format
+  def format = f"${c.x}%d in $c or $c%s" // warn using c.toString // warn
+                                      ^
+tostring-interpolated.scala:13: warning: Boolean format is null test for non-Boolean
+  def bool = f"$c%b" // warn just a null check
+                  ^
+tostring-interpolated.scala:15: error: interpolation uses toString
+Applicable -Wconf / @nowarn filters for this fatal warning: msg=<part of the message>, cat=w-flag-tostring-interpolated, site=T.oops
+  def oops = s"${null} slipped thru my fingers" // warn
+                 ^
+tostring-interpolated.scala:20: error: interpolation uses toString
+Applicable -Wconf / @nowarn filters for this fatal warning: msg=<part of the message>, cat=w-flag-tostring-interpolated, site=T.greeting
+  def greeting = s"$sb, world" // warn
+                    ^
+tostring-interpolated.scala:31: error: interpolated Unit value
+Applicable -Wconf / @nowarn filters for this fatal warning: msg=<part of the message>, cat=w-flag-tostring-interpolated, site=Mitigations.unitized
+  def unitized = s"unfortunately $shown" // warn accidental unit value
+                                  ^
+tostring-interpolated.scala:32: error: interpolated Unit value
+Applicable -Wconf / @nowarn filters for this fatal warning: msg=<part of the message>, cat=w-flag-tostring-interpolated, site=Mitigations.funitized
+  def funitized = f"unfortunately $shown" // warn accidental unit value
+                                       ^
+1 warning
+9 errors

test/files/neg/tostring-interpolated.scala

@@ -0,0 +1,36 @@
+//> using options -Wconf:cat=w-flag-tostring-interpolated:e -Wtostring-interpolated
+
+case class C(x: Int)
+
+trait T {
+  def c = C(42)
+  def f = f"$c" // warn
+  def s = s"$c" // warn
+  def r = raw"$c" // warn
+
+  def format = f"${c.x}%d in $c or $c%s" // warn using c.toString // warn
+
+  def bool = f"$c%b" // warn just a null check
+
+  def oops = s"${null} slipped thru my fingers" // warn
+
+  def ok = s"${c.toString}"
+
+  def sb = new StringBuilder().append("hello")
+  def greeting = s"$sb, world" // warn
+}
+
+class Mitigations {
+
+  val s = "hello, world"
+  val i = 42
+  def shown() = println("shown")
+
+  def ok = s"$s is ok"
+  def jersey = s"number $i"
+  def unitized = s"unfortunately $shown" // warn accidental unit value
+  def funitized = f"unfortunately $shown" // warn accidental unit value
+
+  def nopct = f"$s is ok"
+  def nofmt = f"number $i"
+}

test/files/run/f-interpolator-unit.scala

@@ -33,7 +33,7 @@ object Test extends App {
   final val tester = "hello"
   final val number = "42"  // strings only, alas
 
-  def assertEquals(s0: String, s1: String) = assert(s0 == s1, s"$s0 == $s1")
+  def assertEquals(s0: String, s1: String, i: Int = -1) = assert(s0 == s1, s"$s0 == $s1${if (i >= 0) " at " + i.toString else ""}")
 
   def noEscape() = {
     val s = "string"
@@ -134,7 +134,7 @@ object Test extends App {
       f"${null}%b"  -> "false",
       f"${false}%b" -> "false",
       f"${true}%b"  -> "true",
-      f"${true && false}%b"                 -> "false",
+      f"${true && false}%b"                     -> "false",
       f"${java.lang.Boolean.valueOf(false)}%b"  -> "false",
       f"${java.lang.Boolean.valueOf(true)}%b"   -> "true",
 
@@ -255,7 +255,7 @@ object Test extends App {
       f"z" -> "z"
     )
 
-    for ((f, s) <- ss) assertEquals(s, f)
+    for (((f, s), i) <- ss.zipWithIndex) assertEquals(s, f, i)
   }
 
   noEscape()