shadaj/slinky

@react macro fails when building docs if Props has scaladocs

harpocrates opened this issue · 6 comments

harpocrates commented

When trying to build docs with sbt doc, the @react annotation doesn't work if the Props defined inside the class has a scaladoc on it. Outside of trying to build docs, everything works fine.

$ sbt doc
Main Scala API documentation to /Users/alec/Scratch/slinky-bug/target/scala-2.13/api...
[error] /Users/alec/Scratch/slinky-bug/src/main/scala/bug/Main.scala:11:2: Components must define a Props type or case class, but none was found.
[error] @react class MyApp extends StatelessComponent {
[error]  ^
[error] /Users/alec/Scratch/slinky-bug/src/main/scala/bug/Main.scala:23:21: not found: value MyApp
[error]     ReactDOM.render(MyApp("hello world"), container)
[error]                     ^
[error] two errors found
[error] (Compile / doc) Scaladoc generation failed
[error] Total time: 4 s, completed 6-Jun-2020 6:07:15 PM

Complete example:

package bug

import slinky.core._
import slinky.core.annotations.react
import slinky.web.ReactDOM
import slinky.web.html._

import scala.scalajs.js.annotation.{JSExportTopLevel}
import org.scalajs.dom

@react class MyApp extends StatelessComponent {

  /** @param message the message */
  case class Props(message: String)

  def render = span(props.message)
}

object Exports {

  @JSExportTopLevel("mountMyApp")
  def mountMyApp(container: dom.raw.Element): Unit = {
    ReactDOM.render(MyApp("hello world"), container)
  }
}

Version details:

  • Scala 2.13.1 or 2.12.10 (both trigger the bug)
  • Slinky 0.6.5
  • React 16.12.0

Other than that, just wanted to say think you for the amazing project. ❤️

shadaj commented

Thanks for the bug report! Probably a bug with how we look for the Props definition in the macro.

harpocrates commented

I spent a little time yesterday looking into this. I worry it won't be an easy fix - see the Scala bug where I referenced this issue. The DocDef issue affects almost all quasiquote pattern matching of ASTs. Some other places this issue crops up:

  1. the Props/State/Snapshot members of a class annotated with @react don't work if they have scaladocs (the original bug report)
  2. the Props members of an object annotated with @react doesn't work if it has a scaladoc
  3. object and class annotated with @react don't match if they have scaladocs

It is possible work around these one by one, but it gets tedious and error-prone.

Here's a sample diff which solves 1:

diff --git a/core/build.sbt b/core/build.sbt
index 809fd1a..2150c21 100644
--- a/core/build.sbt
+++ b/core/build.sbt
@@ -3,6 +3,7 @@ enablePlugins(ScalaJSPlugin)
 name := "slinky-core"
 
 libraryDependencies += "org.scala-lang" % "scala-reflect" % scalaVersion.value
+libraryDependencies += "org.scala-lang" % "scala-compiler" % scalaVersion.value
 
 scalacOptions ++= {
   if (scalaJSVersion.startsWith("0.6.")) Seq("-P:scalajs:sjsDefinedByDefault")
diff --git a/core/src/main/scala/slinky/core/annotations/react.scala b/core/src/main/scala/slinky/core/annotations/react.scala
index b6ce202..d8b034d 100644
--- a/core/src/main/scala/slinky/core/annotations/react.scala
+++ b/core/src/main/scala/slinky/core/annotations/react.scala
@@ -32,7 +32,19 @@ object ReactMacrosImpl {
   def createComponentBody(c: whitebox.Context)(cls: c.Tree): (c.Tree, List[c.Tree]) = {
     import c.universe._
     val q"..$_ class ${className: Name} extends ..$parents { $self => ..$stats}" = cls // scalafix:ok
-    val (propsDefinition, applyMethods) = stats.flatMap {
+
+    // Workaround for <https://github.com/scala/bug/issues/7993>
+    val unwrapDocDef = (t: Tree) => {
+      import scala.tools.nsc.ast.Trees
+
+      if (t.isInstanceOf[Trees#DocDef]) {
+        t.asInstanceOf[Trees#DocDef].definition.asInstanceOf[Tree]
+      } else {
+        t
+      }
+    }
+
+    val (propsDefinition, applyMethods) = stats.map(unwrapDocDef).flatMap {
       case defn @ q"..$_ type Props = ${_}" =>
         Some((defn, Seq()))
 
@@ -75,7 +87,7 @@ object ReactMacrosImpl {
     }.headOption
       .getOrElse(c.abort(c.enclosingPosition, "Components must define a Props type or case class, but none was found."))
 
-    val stateDefinition = stats.flatMap {
+    val stateDefinition = stats.map(unwrapDocDef).flatMap {
       case defn @ q"..$_ type State = ${_}" =>
         Some(defn)
       case defn @ q"case class State[..$_](...$_) extends ..$_ { $_ => ..$_ }" =>
@@ -83,7 +95,7 @@ object ReactMacrosImpl {
       case _ => None
     }.headOption
 
-    val snapshotDefinition = stats.flatMap {
+    val snapshotDefinition = stats.map(unwrapDocDef).flatMap {
       case defn @ q"type Snapshot = ${_}" =>
         Some(defn)
       case defn @ q"case class Snapshot[..$_](...$_) extends ..$_ { $_ => ..$_ }" =>
@@ -105,8 +117,10 @@ object ReactMacrosImpl {
                 null.asInstanceOf[Snapshot]
                 ()
               })
-              ..${stats.filterNot(s =>
-        s == propsDefinition || s == stateDefinition.orNull || s == snapshotDefinition.orNull
+              ..${stats.filterNot(s => {
+                val s2 = unwrapDocDef(s)
+                s2 == propsDefinition || s2 == stateDefinition.orNull || s2 == snapshotDefinition.orNull
+              }
       )}
             }"""

Note that this solution just throws away the doc (which is probably not the right thing to do - the doc should just be moved to the right place).

shadaj commented

@harpocrates thanks so much for looking into this! Would you be able to make a PR with these changes?

For now, I think it's fine to throw away the docs given that there's no other nice solution for quasiquotes, and we can create a separate issue for passing through the docs. Even though it's not the ideal result, it's probably better than just crashing the compiler.

harpocrates commented

Are you OK with the part that adds libraryDependencies += "org.scala-lang" % "scala-compiler" % scalaVersion.value though? I dislike being tied to concrete compiler types, but agree it would be better than a crash.

shadaj commented

@harpocrates apologies for the late response! I think it would be fine to include that dependency, as you said it's better than having compiler crashes.

visox commented

Hi, i had the same problem. I tried many things, what worked (not sure if only that) was when i improved my scalacOptions.

I left only

    "-deprecation", // Emit warning and location for usages of deprecated APIs.
    "-encoding",
    "utf-8", // Specify character encoding used by source files.
    "-explaintypes", // Explain type errors in more detail.
    "-feature", // Emit warning and location for usages of features that should be imported explicitly.
    "-language:existentials", // Existential types (besides wildcard types) can be written and inferred
    "-language:experimental.macros", // Allow macro definition (besides implementation and application)
    "-language:higherKinds", // Allow higher-kinded types
    "-language:implicitConversions", // Allow definition of implicit functions called views
    "-unchecked",  // Enable additional warnings where generated code depends on assumptions.
    "-Ywarn-extra-implicit", // Warn when more than one implicit parameter section is defined.
    "-Ywarn-numeric-widen",
    "-Ymacro-annotations",

i use scala 2.13.3 and slinky 0.6.6

edit: maybe its not the same problem i had only one error:

/Main.scala:51:7: not found: value App
[error]       App(()),
[error]       ^
[error] one error found