Explicitly discard non-unit values in @react macro (#630)

* Add sbt-tpolecat to tests * Fix warnings in tests * Fix warnings in tests * Fix scalacOptions * Increase sbt max memory * Remove kind projector * Update to Scala 2.13.10 * Disable non-unit statement warning in tests * Fix non-unit statements in @react macro * Add Scala 3 scalafix config * Run scalafix * Fix style * Fix discarded non-unit statements in @react macro * Move annotated components back to the tests * Disable fatal warnings for Scala 3 build
shadaj · Jul 17, 2023 · 19408aa · 19408aa
1 parent 7a624b6
commit 19408aa
Show file tree

Hide file tree

Showing 20 changed files with 158 additions and 76 deletions.
diff --git a/.sbtopts b/.sbtopts
@@ -0,0 +1,2 @@
+-J-Xmx2048M
+-J-XX:+UseG1GC
diff --git a/.scalafix-scala3.conf b/.scalafix-scala3.conf
@@ -0,0 +1,27 @@
+# This has to be maintained separately as Scala 3 does not currently support the rules:
+# - RemoveUnused (https://github.com/scalacenter/scalafix/issues/1682).
+# - ProcedureSyntax.
+# See https://github.com/scalacenter/scalafix/issues/1747.
+rules = [
+  DisableSyntax
+  LeakingImplicitClassVal
+  NoValInForComprehension
+]
+
+DisableSyntax.noVars = false
+DisableSyntax.noThrows = false
+DisableSyntax.noNulls = false
+DisableSyntax.noReturns = true
+DisableSyntax.noAsInstanceOf = false
+DisableSyntax.noIsInstanceOf = true
+DisableSyntax.noXml = true
+DisableSyntax.noFinalVal = true
+DisableSyntax.noFinalize = true
+DisableSyntax.noValPatterns = true
+DisableSyntax.regex = [
+  {
+    id = noJodaTime
+    pattern = "org\\.joda\\.time"
+    message = "Use java.time instead"
+  }
+]
diff --git a/build.sbt b/build.sbt
@@ -1,3 +1,5 @@
+import _root_.io.github.davidgregory084._
+
 ThisBuild / organization := "me.shadaj"
 
 addCommandAlias("style", "compile:scalafix; test:scalafix; compile:scalafmt; test:scalafmt; scalafmtSbt")
@@ -14,6 +16,8 @@ ThisBuild / scalaVersion := scala213
 ThisBuild / semanticdbEnabled := true
 ThisBuild / semanticdbVersion := "4.7.6"
 
+ThisBuild / tpolecatDefaultOptionsMode := DevMode
+
 lazy val slinky = project
   .in(file("."))
   .aggregate(
@@ -57,6 +61,12 @@ lazy val crossScalaSettings = Seq(
       case Some((2, n)) if n >= 13 => Seq(sourceDir / "scala-2.13+")
       case _                       => Seq(sourceDir / "scala-2.13-")
     }
+  },
+  scalafixConfig := {
+    CrossVersion.partialVersion(scalaVersion.value) match {
+      case Some((3, _)) => Some(file(".scalafix-scala3.conf"))
+      case _            => None
+    }
   }
 )
 
@@ -77,23 +87,13 @@ lazy val librarySettings = Seq(
     val opt = if (scalaVersion.value == scala3) "-scalajs-mapSourceURI" else "-P:scalajs:mapSourceURI"
     s"$opt:$a->$g/$githubVersion/${baseDirectory.value.getName}/"
   },
-  scalacOptions ++= Seq(
-    "-encoding",
-    "UTF-8",
-    "-feature",
-    "-language:implicitConversions"
-  ) ++ (CrossVersion.partialVersion(scalaVersion.value) match {
+  scalacOptions ++= (CrossVersion.partialVersion(scalaVersion.value) match {
     case Some((3, _)) =>
       Seq(
-        "-unchecked",
         "-source:3.0-migration"
       )
     case _ =>
-      Seq(
-        "-deprecation",
-        "-language:higherKinds",
-        "-Ywarn-unused:imports,privates,locals"
-      )
+      Seq.empty
   })
 )
 

diff --git a/core/src/main/scala-2/slinky/core/ExternalPropsWriterProvider.scala b/core/src/main/scala-2/slinky/core/ExternalPropsWriterProvider.scala
@@ -2,7 +2,6 @@ package slinky.core
 
 import scala.scalajs.js
 
-import scala.language.experimental.macros
 import scala.reflect.macros.whitebox
 
 // same as PropsWriterProvider except it always returns the typeclass instead of nulling it out in fullOpt mode

diff --git a/core/src/main/scala-2/slinky/core/FunctionalComponentName.scala b/core/src/main/scala-2/slinky/core/FunctionalComponentName.scala
@@ -1,6 +1,5 @@
 package slinky.core
 
-import scala.language.experimental.macros
 import scala.reflect.macros.whitebox
 
 final class FunctionalComponentName(val name: String) extends AnyVal

diff --git a/core/src/main/scala-2/slinky/core/StateReaderProvider.scala b/core/src/main/scala-2/slinky/core/StateReaderProvider.scala
@@ -2,7 +2,6 @@ package slinky.core
 
 import scala.scalajs.js
 
-import scala.language.experimental.macros
 import scala.reflect.macros.whitebox
 
 trait StateReaderProvider extends js.Object

diff --git a/core/src/main/scala-2/slinky/core/StateWriterProvider.scala b/core/src/main/scala-2/slinky/core/StateWriterProvider.scala
@@ -2,7 +2,6 @@ package slinky.core
 
 import scala.scalajs.js
 
-import scala.language.experimental.macros
 import scala.reflect.macros.whitebox
 
 trait StateWriterProvider extends js.Object

diff --git a/core/src/main/scala-2/slinky/core/annotations/react.scala b/core/src/main/scala-2/slinky/core/annotations/react.scala
@@ -3,7 +3,6 @@ package slinky.core.annotations
 import slinky.core._
 
 import scala.annotation.compileTimeOnly
-import scala.language.experimental.macros
 import scala.reflect.macros.whitebox
 import scala.scalajs.js
 
@@ -12,7 +11,9 @@ class react extends scala.annotation.StaticAnnotation {
   def macroTransform(annottees: Any*): Any = macro ReactMacrosImpl.reactImpl
 }
 
+@deprecated(since = "0.7.4")
 object react {
+  @deprecated(since = "0.7.4")
   @inline def bump(thunk: => Unit): Unit = {}
 }
 
@@ -35,7 +36,7 @@ object ReactMacrosImpl {
     val q"..$_ class ${className: Name} extends ..$parents { $self => ..$stats}" = cls // scalafix:ok
     val (propsDefinition, applyMethods) = stats.flatMap {
       case defn @ q"..$_ type Props = ${_}" =>
-        Some((defn, Seq()))
+        Some((defn, Nil))
 
       case defn @ q"case class Props[..$tparams](...${caseClassparamssRaw}) extends ..$_ { $_ => ..$_ }" =>
         val caseClassparamss = caseClassparamssRaw.asInstanceOf[Seq[Seq[ValDef]]]
@@ -70,7 +71,7 @@ object ReactMacrosImpl {
                 this.apply(Props.apply[..$tparams](...$applyValues))"""
         }
 
-        Some((defn, Seq(caseClassApply)))
+        Some((defn, List(caseClassApply)))
 
       case _ => None
     }.headOption
@@ -97,27 +98,46 @@ object ReactMacrosImpl {
 
     val definitionClass = q"type Def = $clazz"
 
+    // Props, State and Snapshot get moved to the companion object but they are likely to be referenced from within the
+    // class. We have to import them from the companion object since their usage will be unqualified. We also need to
+    // ensure that they are used so that there are no unused import warnings and do so in a way that does not produce
+    // discarded non-unit value warnings.
+
+    // Unfortunately adding a Unit type ascription here does not work as it inserts another unit value and so the null
+    // value ends up being discarded.
+    def use(name: TypeName) =
+      q"""
+        locally {
+          val _ = null.asInstanceOf[$name]
+        }
+       """
+
+    // Add if (false) to ensure it is dead code and gets eliminated.
+    val imports =
+      q"""
+         import $companion.{Props, State, Snapshot}
+         if (false) {
+           ${use(TypeName("Props"))}
+           ${use(TypeName("State"))}
+           ${use(TypeName("Snapshot"))}
+         }
+       """
+
     val newClazz =
       q"""class $clazz(jsProps: _root_.scala.scalajs.js.Object) extends ${clazz.toTermName}.Definition(jsProps) {
-              import $companion.{Props, State, Snapshot}
-              _root_.slinky.core.annotations.react.bump({
-                null.asInstanceOf[Props]
-                null.asInstanceOf[State]
-                null.asInstanceOf[Snapshot]
-                ()
-              })
+              ..$imports
               ..${stats.filterNot(s =>
         s == propsDefinition || s == stateDefinition.orNull || s == snapshotDefinition.orNull
       )}
             }"""
 
     (
       newClazz,
-      ((q"null.asInstanceOf[${parents.head}]" +:
-        propsDefinition +:
-        stateDefinition.toList) ++
-        snapshotDefinition.toList ++
-        (definitionClass +: applyMethods)).asInstanceOf[List[c.Tree]]
+      propsDefinition +:
+        stateDefinition.toList ++:
+        snapshotDefinition.toList ++:
+        definitionClass +:
+        applyMethods
     )
   }
 
@@ -210,7 +230,7 @@ object ReactMacrosImpl {
 
       case Seq(
           cls @ q"..$_ class $className extends ..$parents { $_ => ..$_}",
-          obj @ q"..$_ object $_ extends ..$_ { $_ => ..$objStats }"
+          q"..$_ object $_ extends ..$_ { $_ => ..$objStats }"
           )
           if parentsContainsType(c)(parents, typeOf[Component]) ||
             parentsContainsType(c)(parents, typeOf[StatelessComponent]) =>
@@ -231,12 +251,12 @@ object ReactMacrosImpl {
           q"object $objName extends _root_.slinky.core.ExternalComponentWithAttributesWithRefType[$elementType, $refType] { ..${objStats ++ companionStats} }"
         )
 
-      case Seq(obj @ q"$pre object $objName extends ..$parents { $self => ..$objStats }") if (objStats.exists {
+      case Seq(q"$pre object $objName extends ..$parents { $self => ..$objStats }") if (objStats.exists {
             case q"$_ val component: $_ = $_" => true
             case _                            => false
           }) =>
         val applyMethods = objStats.flatMap {
-          case defn @ q"$pre class Props[..$tparams](...${caseClassparamssRaw}) extends ..$_ { $_ => ..$_ }"
+          case q"$pre class Props[..$tparams](...${caseClassparamssRaw}) extends ..$_ { $_ => ..$_ }"
               if pre.hasFlag(Flag.CASE) =>
             val caseClassparamss = caseClassparamssRaw.asInstanceOf[Seq[Seq[ValDef]]]
             val childrenParam    = caseClassparamss.flatten.find(_.name.toString == "children")
@@ -286,7 +306,7 @@ object ReactMacrosImpl {
 
         List(q"$pre object $objName extends ..$parents { $self => ..${objStats ++ applyMethods} }")
 
-      case defn =>
+      case _ =>
         c.abort(
           c.enclosingPosition,
           """@react must annotate:

diff --git a/native/src/test/scala/slinky/native/NativeComponentRenderTest.scala b/native/src/test/scala/slinky/native/NativeComponentRenderTest.scala
@@ -199,7 +199,7 @@ class NativeComponentRenderTest extends AnyFunSuite {
           FlatList[Int](
             data = Seq(1, 2),
             renderItem = {
-              case RenderItemInfo(d, index, _) =>
+              case RenderItemInfo(d, _, _) =>
                 Text(d.toString)
             }
           )

diff --git a/project/build.properties b/project/build.properties
@@ -1 +1,2 @@
+# Remove semanticdbVersion from build.sbt when this is updated.
 sbt.version=1.7.3
diff --git a/project/plugins.sbt b/project/plugins.sbt
@@ -18,9 +18,10 @@ libraryDependencies ++= {
   else Seq("org.scala-js" %% "scalajs-linker" % "1.0.1")
 }
 
-addSbtPlugin("com.jsuereth"   % "sbt-pgp"         % "2.1.1")
-addSbtPlugin("org.xerial.sbt" % "sbt-sonatype"    % "3.9.18")
-addSbtPlugin("com.dwijnand"   % "sbt-dynver"      % "4.1.1")
-addSbtPlugin("org.jetbrains"  % "sbt-idea-plugin" % "3.18.3")
-addSbtPlugin("org.scalameta"  % "sbt-scalafmt"    % "2.4.5")
-addSbtPlugin("ch.epfl.scala"  % "sbt-scalafix"    % "0.10.4")
+addSbtPlugin("ch.epfl.scala"             % "sbt-scalafix"    % "0.10.4")
+addSbtPlugin("com.dwijnand"              % "sbt-dynver"      % "4.1.1")
+addSbtPlugin("com.jsuereth"              % "sbt-pgp"         % "2.1.1")
+addSbtPlugin("io.github.davidgregory084" % "sbt-tpolecat"    % "0.4.2")
+addSbtPlugin("org.jetbrains"             % "sbt-idea-plugin" % "3.18.3")
+addSbtPlugin("org.scalameta"             % "sbt-scalafmt"    % "2.4.5")
+addSbtPlugin("org.xerial.sbt"            % "sbt-sonatype"    % "3.9.18")
diff --git a/readWrite/src/main/scala-2/slinky/readwrite/CoreReadersMacro.scala b/readWrite/src/main/scala-2/slinky/readwrite/CoreReadersMacro.scala
@@ -1,6 +1,5 @@
 package slinky.readwrite
 
-import scala.language.experimental.macros
 import scala.reflect.macros.whitebox
 
 trait MacroReaders {

diff --git a/readWrite/src/main/scala-2/slinky/readwrite/CoreWritersMacro.scala b/readWrite/src/main/scala-2/slinky/readwrite/CoreWritersMacro.scala
@@ -1,6 +1,5 @@
 package slinky.readwrite
 
-import scala.language.experimental.macros
 import scala.reflect.macros.whitebox
 
 trait MacroWriters {

diff --git a/tests/build.sbt b/tests/build.sbt
@@ -1,24 +1,54 @@
+import _root_.io.github.davidgregory084._
+
 enablePlugins(ScalaJSPlugin)
 enablePlugins(JSDependenciesPlugin)
 
 libraryDependencies += "org.scalatest" %%% "scalatest" % "3.2.9" % Test
-libraryDependencies += ("org.scala-js" %%% "scalajs-fake-insecure-java-securerandom" % "1.0.0").cross(CrossVersion.for3Use2_13)
+libraryDependencies += ("org.scala-js" %%% "scalajs-fake-insecure-java-securerandom" % "1.0.0")
+  .cross(CrossVersion.for3Use2_13)
 
 Test / jsEnv := new org.scalajs.jsenv.jsdomnodejs.JSDOMNodeJSEnv()
 
-Test / scalaJSLinkerConfig ~= { _.withESFeatures(_.withUseECMAScript2015(
-  Option(System.getenv("ES2015_ENABLED")).map(_ == "true").getOrElse(false)
-)) }
+Test / scalaJSLinkerConfig ~= {
+  _.withESFeatures(
+    _.withUseECMAScript2015(
+      Option(System.getenv("ES2015_ENABLED")).map(_ == "true").getOrElse(false)
+    )
+  )
+}
 
 Test / unmanagedResourceDirectories += baseDirectory.value / "node_modules"
 
 jsDependencies ++= Seq(
-  (ProvidedJS / "react/umd/react.development.js"
-    minified "react/umd/react.production.min.js" commonJSName "React") % Test,
-  (ProvidedJS / "react-dom/umd/react-dom.development.js"
-    minified "react-dom/umd/react-dom.production.min.js" dependsOn "react/umd/react.development.js" commonJSName "ReactDOM") % Test,
-  (ProvidedJS / "react-dom/umd/react-dom-test-utils.development.js"
-    minified "react-dom/umd/react-dom-test-utils.production.min.js" dependsOn "react-dom/umd/react-dom.development.js" commonJSName "ReactTestUtils") % Test,
-  (ProvidedJS / "react-dom/umd/react-dom-server.browser.development.js"
-    minified "react-dom/umd/react-dom-server.browser.production.min.js" dependsOn "react-dom/umd/react-dom.development.js" commonJSName "ReactDOMServer") % Test
+  ((ProvidedJS / "react/umd/react.development.js")
+    .minified("react/umd/react.production.min.js")
+    .commonJSName("React")) % Test,
+  ((ProvidedJS / "react-dom/umd/react-dom.development.js")
+    .minified("react-dom/umd/react-dom.production.min.js")
+    .dependsOn("react/umd/react.development.js")
+    .commonJSName("ReactDOM")) % Test,
+  ((ProvidedJS / "react-dom/umd/react-dom-test-utils.development.js")
+    .minified("react-dom/umd/react-dom-test-utils.production.min.js")
+    .dependsOn("react-dom/umd/react-dom.development.js")
+    .commonJSName("ReactTestUtils")) % Test,
+  ((ProvidedJS / "react-dom/umd/react-dom-server.browser.development.js")
+    .minified("react-dom/umd/react-dom-server.browser.production.min.js")
+    .dependsOn("react-dom/umd/react-dom.development.js")
+    .commonJSName("ReactDOMServer")) % Test
 )
+
+// The Scala 3 tests still have a bunch of warnings that need fixing such as https://github.com/shadaj/slinky/issues/643
+// before CiMode can be used.
+tpolecatOptionsMode := (CrossVersion.partialVersion(scalaVersion.value) match {
+  case Some((3, _)) => DevMode
+  case _            => CiMode
+})
+
+scalacOptions ++= (CrossVersion.partialVersion(scalaVersion.value) match {
+  case Some((2, _)) => Seq("-P:scalajs:nowarnGlobalExecutionContext")
+  case _            => Seq.empty
+})
+
+// Unit statements are prevalent in the tests. There is no way to suppress them:
+// See https://github.com/typelevel/sbt-tpolecat/issues/134.
+Test / tpolecatExcludeOptions += ScalacOptions.warnNonUnitStatement
diff --git a/tests/src/test/scala-2/slinky/core/annotations/ReactAnnotatedComponentTest.scala b/tests/src/test/scala-2/slinky/core/annotations/ReactAnnotatedComponentTest.scala
@@ -188,7 +188,7 @@ object TakeValuesFromCompanionObject {
 }
 
 object DerivedStateComponent {
-  override val getDerivedStateFromProps = (nextProps: Props, prevState: State) => {
+  override val getDerivedStateFromProps = (nextProps: Props, _: State) => {
     nextProps.num
   }
 }
@@ -312,7 +312,7 @@ class ReactAnnotatedComponentTest extends AsyncFunSuite {
   }
 
   test("Can construct a component taking Unit props with refs and key") {
-    val element: ReactElement = NoPropsComponent.withKey("hi").withRef((r: js.Object) => {})
+    val element: ReactElement = NoPropsComponent.withKey("hi").withRef((_: js.Object) => {})
     assert(element.asInstanceOf[js.Dynamic].key.toString == "hi")
     assert(!js.isUndefined(element.asInstanceOf[js.Dynamic].ref))
   }
@@ -334,7 +334,7 @@ class ReactAnnotatedComponentTest extends AsyncFunSuite {
     val promise: Promise[Assertion] = Promise()
 
     ReactDOM.render(
-      ErrorBoundaryComponent(true, (error, info) => {
+      ErrorBoundaryComponent(true, (_, _) => {
         promise.success(assert(true))
       }),
       dom.document.createElement("div")
@@ -347,7 +347,7 @@ class ReactAnnotatedComponentTest extends AsyncFunSuite {
     var sawError = false
 
     ReactDOM.render(
-      ErrorBoundaryComponent(false, (error, info) => {
+      ErrorBoundaryComponent(false, (_, _) => {
         sawError = true
       }),
       dom.document.createElement("div")

diff --git a/tests/src/test/scala/slinky/core/ComponentTest.scala b/tests/src/test/scala/slinky/core/ComponentTest.scala
@@ -341,7 +341,7 @@ class ComponentTest extends AsyncFunSuite {
   }
 
   test("Can construct a component taking Unit props with refs and key") {
-    val element: ReactElement = NoPropsComponent.withKey("hi").withRef((r: js.Object) => {})
+    val element: ReactElement = NoPropsComponent.withKey("hi").withRef((_: js.Object) => {})
     assert(element.asInstanceOf[js.Dynamic].key.toString == "hi")
     assert(!js.isUndefined(element.asInstanceOf[js.Dynamic].ref))
   }
Original file line number	Diff line number	Diff line change
		@@ -1 +1,2 @@
		# Remove semanticdbVersion from build.sbt when this is updated.
		sbt.version=1.7.3