Warn on S-interpolator usage for assert, assume and printf (backport #…

…2751) (#2757)

* Add internal methods to maintain binary compatibility

Co-authored-by: Megan Wachs <megan@sifive.com>
Co-authored-by: Jack Koenig <koenig@sifive.com>

build.sbt

@@ -297,7 +297,6 @@ lazy val docs = project       // new documentation project
   .settings(commonSettings)
   .settings(
     scalacOptions ++= Seq(
-      "-Xfatal-warnings",
       "-language:reflectiveCalls",
       "-language:implicitConversions"
     ),

core/src/main/scala/chisel3/Printf.scala

@@ -2,10 +2,11 @@
 
 package chisel3
 
-import scala.language.experimental.macros
 import chisel3.internal._
 import chisel3.internal.Builder.pushCommand
 import chisel3.internal.sourceinfo.SourceInfo
+import scala.language.experimental.macros
+import scala.reflect.macros.blackbox
 
 /** Prints a message in simulation
   *
@@ -76,7 +77,44 @@ object printf {
     * @param data format string varargs containing data to print
     */
   def apply(fmt: String, data: Bits*)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): Printf =
-    apply(Printable.pack(fmt, data: _*))
+    macro _applyMacroWithInterpolatorCheck
+
+  def _applyMacroWithInterpolatorCheck(
+    c:              blackbox.Context
+  )(fmt:            c.Tree,
+    data:           c.Tree*
+  )(sourceInfo:     c.Tree,
+    compileOptions: c.Tree
+  ): c.Tree = {
+    import c.universe._
+    fmt match {
+      case q"scala.StringContext.apply(..$_).s(..$_)" =>
+        c.warning(
+          c.enclosingPosition,
+          "The s-interpolator prints the Scala .toString of Data objects rather than the value " +
+            "of the hardware wire during simulation. Use the cf-interpolator instead. If you want " +
+            "an elaboration time print, use println."
+        )
+      case _ =>
+    }
+    val apply_impl_do = symbolOf[this.type].asClass.module.info.member(TermName("printfWithReset"))
+    q"$apply_impl_do(_root_.chisel3.Printable.pack($fmt, ..$data))($sourceInfo, $compileOptions)"
+  }
+
+  // Private internal methods that serve to maintain binary
+  // compatibility after interpolator check updates
+  @deprecated("This Printf.apply method has been deprecated and will be removed in Chisel 3.6")
+  def apply(fmt: String, sourceInfo: SourceInfo, compileOptions: CompileOptions): Printf =
+    apply(fmt, Nil, sourceInfo, compileOptions)
+
+  @deprecated("This Printf.apply method has been deprecated and will be removed in Chisel 3.6")
+  def apply(
+    fmt:            String,
+    data:           Seq[Bits],
+    sourceInfo:     SourceInfo,
+    compileOptions: CompileOptions
+  ): Printf =
+    apply(Printable.pack(fmt, data: _*))(sourceInfo, compileOptions)
 
   /** Prints a message in simulation
     *
@@ -92,7 +130,15 @@ object printf {
     * @see [[Printable]] documentation
     * @param pable [[Printable]] to print
     */
-  def apply(pable: Printable)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): Printf = {
+  def apply(pable: Printable)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): Printf =
+    printfWithReset(pable)(sourceInfo, compileOptions)
+
+  private[chisel3] def printfWithReset(
+    pable: Printable
+  )(
+    implicit sourceInfo: SourceInfo,
+    compileOptions:      CompileOptions
+  ): Printf = {
     var printfId: Printf = null
     when(!Module.reset.asBool) {
       printfId = printfWithoutReset(pable)

core/src/main/scala/chisel3/VerificationStatement.scala

@@ -48,7 +48,7 @@ object assert extends VerifPrintMacrosDoc {
   )(
     implicit sourceInfo: SourceInfo,
     compileOptions:      CompileOptions
-  ): Assert = macro _applyMacroWithStringMessage
+  ): Assert = macro _applyMacroWithInterpolatorCheck
 
   /** Checks for a condition to be valid in the circuit at all times. If the
     * condition evaluates to false, the circuit simulation stops with an error.
@@ -79,6 +79,32 @@ object assert extends VerifPrintMacrosDoc {
   def apply(cond: Bool)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): Assert =
     macro _applyMacroWithNoMessage
 
+  import VerificationStatement._
+
+  /** @group VerifPrintMacros */
+  def _applyMacroWithInterpolatorCheck(
+    c:              blackbox.Context
+  )(cond:           c.Tree,
+    message:        c.Tree,
+    data:           c.Tree*
+  )(sourceInfo:     c.Tree,
+    compileOptions: c.Tree
+  ): c.Tree = {
+    import c.universe._
+    message match {
+      case q"scala.StringContext.apply(..$_).s(..$_)" =>
+        c.warning(
+          c.enclosingPosition,
+          "The s-interpolator prints the Scala .toString of Data objects rather than the value " +
+            "of the hardware wire during simulation. Use the cf-interpolator instead. If you want " +
+            "an elaboration time check, call assert with a Boolean condition."
+        )
+      case _ =>
+    }
+    val apply_impl_do = symbolOf[this.type].asClass.module.info.member(TermName("_applyWithSourceLinePrintable"))
+    q"$apply_impl_do($cond, ${getLine(c)}, _root_.scala.Some(_root_.chisel3.Printable.pack($message, ..$data)))($sourceInfo, $compileOptions)"
+  }
+
   /** An elaboration-time assertion. Calls the built-in Scala assert function. */
   def apply(cond: Boolean, message: => String): Unit = Predef.assert(cond, message)
 
@@ -88,8 +114,6 @@ object assert extends VerifPrintMacrosDoc {
   /** Named class for assertions. */
   final class Assert private[chisel3] () extends VerificationStatement
 
-  import VerificationStatement._
-
   /** @group VerifPrintMacros */
   @deprecated(
     "This method has been deprecated in favor of _applyMacroWithStringMessage. Please use the same.",
@@ -215,7 +239,7 @@ object assume extends VerifPrintMacrosDoc {
   )(
     implicit sourceInfo: SourceInfo,
     compileOptions:      CompileOptions
-  ): Assume = macro _applyMacroWithStringMessage
+  ): Assume = macro _applyMacroWithInterpolatorCheck
 
   /** Assumes a condition to be valid in the circuit at all times.
     * Acts like an assertion in simulation and imposes a declarative
@@ -256,6 +280,30 @@ object assume extends VerifPrintMacrosDoc {
 
   import VerificationStatement._
 
+  /** @group VerifPrintMacros */
+  def _applyMacroWithInterpolatorCheck(
+    c:              blackbox.Context
+  )(cond:           c.Tree,
+    message:        c.Tree,
+    data:           c.Tree*
+  )(sourceInfo:     c.Tree,
+    compileOptions: c.Tree
+  ): c.Tree = {
+    import c.universe._
+    message match {
+      case q"scala.StringContext.apply(..$_).s(..$_)" =>
+        c.warning(
+          c.enclosingPosition,
+          "The s-interpolator prints the Scala .toString of Data objects rather than the value " +
+            "of the hardware wire during simulation. Use the cf-interpolator instead. If you want " +
+            "an elaboration time check, call assert with a Boolean condition."
+        )
+      case _ =>
+    }
+    val apply_impl_do = symbolOf[this.type].asClass.module.info.member(TermName("_applyWithSourceLinePrintable"))
+    q"$apply_impl_do($cond, ${getLine(c)}, _root_.scala.Some(_root_.chisel3.Printable.pack($message, ..$data)))($sourceInfo, $compileOptions)"
+  }
+
   /** @group VerifPrintMacros */
   @deprecated(
     "This method has been deprecated in favor of _applyMacroWithStringMessage. Please use the same.",

docs/src/explanations/printing.md

@@ -13,11 +13,23 @@ Chisel provides the `printf` function for debugging purposes. It comes in two fl
 
 ### Scala-style
 
-Chisel also supports printf in a style similar to [Scala's String Interpolation](http://docs.scala-lang.org/overviews/core/string-interpolation.html). Chisel provides a custom string interpolator `cf` which follows C-style format specifiers (see section [C-style](#c-style) below). Here's a few examples of using the `cf` interpolator:
+Chisel also supports printf in a style similar to [Scala's String Interpolation](http://docs.scala-lang.org/overviews/core/string-interpolation.html). Chisel provides a custom string interpolator `cf` which follows C-style format specifiers (see section [C-style](#c-style) below).
+
+Note that the Scala s-interpolator is not supported in Chisel constructs and will issue a compile-time warning:
 
 ```scala mdoc:invisible
 import chisel3._
 ```
+
+```scala mdoc:warn
+class MyModule extends Module {
+  val in = IO(Input(UInt(8.W)))
+  printf(s"in = $in\n")
+}
+```
+
+Instead, use Chisel's `cf` interpolator as in the following examples:
+
 ```scala mdoc:compile-only
 val myUInt = 33.U
 printf(cf"myUInt = $myUInt") // myUInt = 33

src/test/scala/chiselTests/IntervalSpec.scala

@@ -211,7 +211,7 @@ class ClipSqueezeWrapDemo(
   val wrapped = counter.wrap(0.U.asInterval(targetRange))
 
   when(counter === startValue) {
-    printf(s"Target range is $range\n")
+    printf(cf"Target range is $range\n")
     printf("value     clip      squeeze      wrap\n")
   }
 
@@ -245,10 +245,7 @@ class SqueezeFunctionalityTester(range: IntervalRange, startNum: BigDecimal, end
   squeezeTemplate := toSqueeze.squeeze(squeezeInterval)
 
   printf(
-    s"SqueezeTest %d    %d.squeeze($range) => %d\n",
-    counter,
-    toSqueeze.asSInt(),
-    squeezeTemplate.asSInt()
+    cf"SqueezeTest $counter%d ${toSqueeze.asSInt()}%d.squeeze($range) => ${squeezeTemplate.asSInt()}%d\n"
   )
 }
 

src/test/scala/chiselTests/Vec.scala

@@ -111,7 +111,7 @@ class FillTester(n: Int, value: Int) extends BasicTester {
   val x = VecInit(Array.fill(n)(value.U))
   val u = VecInit.fill(n)(value.U)
 
-  assert(x.asUInt() === u.asUInt(), s"Expected Vec to be filled like $x, instead VecInit.fill created $u")
+  assert(x.asUInt() === u.asUInt(), cf"Expected Vec to be filled like $x, instead VecInit.fill created $u")
   stop()
 }
 
@@ -235,7 +235,7 @@ class IterateTester(start: Int, len: Int)(f: UInt => UInt) extends BasicTester {
   val testVec = VecInit.iterate(start.U, len)(f)
   assert(
     controlVec.asUInt() === testVec.asUInt(),
-    s"Expected Vec to be filled like $controlVec, instead creaeted $testVec\n"
+    cf"Expected Vec to be filled like $controlVec, instead created $testVec\n"
   )
   stop()
 }

src/test/scala/chiselTests/VecLiteralSpec.scala

@@ -205,7 +205,7 @@ class VecLiteralSpec extends ChiselFreeSpec with Utils {
 
     assertTesterPasses {
       new BasicTester {
-        chisel3.assert(outsideVecLit(0) === 0xdd.U, s"v(0)")
+        chisel3.assert(outsideVecLit(0) === 0xdd.U, "v(0)")
         stop()
       }
     }
@@ -216,7 +216,7 @@ class VecLiteralSpec extends ChiselFreeSpec with Utils {
 
     assertTesterPasses {
       new BasicTester {
-        chisel3.assert(outsideVecLit(0) === 0xdd.U, s"v(0)")
+        chisel3.assert(outsideVecLit(0) === 0xdd.U, "v(0)")
         chisel3.assert(outsideVecLit(1) === 0xcc.U)
         chisel3.assert(outsideVecLit(2) === 0xbb.U)
         chisel3.assert(outsideVecLit(3) === 0xaa.U)