Squash: Add neg tests, and make them pass.

Author: sjrd

compiler/src/dotty/tools/backend/sjs/JSExportsGen.scala

@@ -22,7 +22,7 @@ import Types._
 import TypeErasure.ErasedValueType
 
 import dotty.tools.dotc.transform.Erasure
-import dotty.tools.dotc.util.SourcePosition
+import dotty.tools.dotc.util.{SourcePosition, SrcPos}
 import dotty.tools.dotc.util.Spans.Span
 import dotty.tools.dotc.report
 
@@ -280,8 +280,12 @@ final class JSExportsGen(jsCodeGen: JSCodeGen)(using Context) {
 
     if (conflicting.exists) {
       val kind = if (isProp) "property" else "method"
-      val alts = conflicting.alternatives
-      report.error(em"Exported $kind $jsName conflicts with ${alts.head}", alts.head.symbol.srcPos)
+      val conflictingMember = conflicting.alternatives.head.symbol.fullName
+      val errorPos: SrcPos = alts.map(_.symbol).filter(_.owner == classSym) match {
+        case Nil         => classSym
+        case altsInClass => altsInClass.minBy(_.span.point)
+      }
+      report.error(em"Exported $kind $jsName conflicts with $conflictingMember", errorPos)
     }
 
     genMemberExportOrDispatcher(JSName.Literal(jsName), isProp, alts.map(_.symbol), static = false)

compiler/src/dotty/tools/dotc/transform/sjs/PrepJSExports.scala

@@ -54,12 +54,12 @@ object PrepJSExports {
 
   private final case class ExportInfo(jsName: String, destination: ExportDestination)(val pos: SrcPos)
 
-  /** Checks a class or module for export.
+  /** Checks a class or module class for export.
    *
-   *  Note that Scala classes are never actually exported; their constructors are.
+   *  Note that non-module Scala classes are never actually exported; their constructors are.
    *  However, the checks are performed on the class when the class is annotated.
    */
-  def checkClassExports(sym: Symbol)(using Context): Unit = {
+  def checkClassOrModuleExports(sym: Symbol)(using Context): Unit = {
     val exports = exportsOf(sym)
     if (exports.nonEmpty)
       checkClassOrModuleExports(sym, exports.head.pos)
@@ -129,6 +129,8 @@ object PrepJSExports {
       err("You may not export a native JS " + (if (isMod) "object" else "class"))
     } else if (!hasLegalExportVisibility(sym)) {
       err("You may only export public and protected " + (if (isMod) "objects" else "classes"))
+    } else if (isJSAny(sym.owner)) {
+      err("You may not export a " + (if (isMod) "object" else "class") + " in a subclass of js.Any")
     } else if (sym.isLocalToBlock) {
       err("You may not export a local " + (if (isMod) "object" else "class"))
     } else if (!sym.isStatic) {
@@ -187,8 +189,10 @@ object PrepJSExports {
       val isStaticExport = annot.symbol == JSExportStaticAnnot
       val hasExplicitName = annot.arguments.nonEmpty
 
+      val exportPos: SrcPos = if (isExportAll) sym else annot.tree
+
       assert(!isTopLevelExport || hasExplicitName,
-          em"Found a top-level export without an explicit name at ${annot.tree.sourcePos}")
+          em"Found a top-level export without an explicit name at ${exportPos.sourcePos}")
 
       val name = {
         if (hasExplicitName) {
@@ -224,14 +228,11 @@ object PrepJSExports {
 
       // Enforce proper setter signature
       if (sym.isJSSetter)
-        checkSetterSignature(sym, annot.tree, exported = true)
+        checkSetterSignature(sym, exportPos, exported = true)
 
       // Enforce no __ in name
-      if (!isTopLevelExport && name.contains("__")) {
-        // Get position for error message
-        val pos: SrcPos = if (hasExplicitName) annot.arguments.head else trgSym
-        report.error("An exported name may not contain a double underscore (`__`)", pos)
-      }
+      if (!isTopLevelExport && name.contains("__"))
+        report.error("An exported name may not contain a double underscore (`__`)", exportPos)
 
       /* Illegal function application exports, i.e., method named 'apply'
        * without an explicit export name.
@@ -249,24 +250,21 @@ object PrepJSExports {
 
             // Don't allow apply without explicit name
             if (!shouldBeTolerated) {
-              // Get position for error message
-              val pos: SrcPos = if (isExportAll) trgSym else annot.tree
-
               report.error(
                   "A member cannot be exported to function application. " +
                   "Add @JSExport(\"apply\") to export under the name apply.",
-                  pos)
+                  exportPos)
             }
 
           case _: ExportDestination.TopLevel =>
             throw new AssertionError(
-                em"Found a top-level export without an explicit name at ${annot.tree.sourcePos}")
+                em"Found a top-level export without an explicit name at ${exportPos.sourcePos}")
 
           case ExportDestination.Static =>
             report.error(
                 "A member cannot be exported to function application as static. " +
                 "Use @JSExportStatic(\"apply\") to export it under the name 'apply'.",
-                annot.tree)
+                exportPos)
         }
       }
 
@@ -285,42 +283,33 @@ object PrepJSExports {
           if (isIllegalToString) {
             report.error(
                 "You may not export a zero-argument method named other than 'toString' under the name 'toString'",
-                annot.tree)
+                exportPos)
           }
 
-          // Disallow @JSExport on non-members.
-          if (!isMember && !sym.is(Trait)) {
+          // Disallow @JSExport at the top-level, as well as on objects and classes
+          if (symOwner.is(Package) || symOwner.isPackageObject) {
+            report.error("@JSExport is forbidden on top-level definitions. Use @JSExportTopLevel instead.", exportPos)
+          } else if (!isMember && !sym.is(Trait)) {
             report.error(
-                "@JSExport is forbidden on objects and classes. Use @JSExportTopLevel instead.",
-                annot.tree)
+                "@JSExport is forbidden on objects and classes. Use @JSExport'ed factory methods instead.",
+                exportPos)
           }
 
         case _: ExportDestination.TopLevel =>
-          if (sym.is(Lazy)) {
-            report.error(
-                "You may not export a lazy val to the top level",
-                annot.tree)
-          } else if (!sym.isOneOf(Accessor | Module) && sym.isJSProperty) {
-            report.error(
-                "You may not export a getter or a setter to the top level",
-                annot.tree)
-          }
+          if (sym.is(Lazy))
+            report.error("You may not export a lazy val to the top level", exportPos)
+          else if (!sym.is(Accessor) && sym.isTerm && sym.isJSProperty)
+            report.error("You may not export a getter or a setter to the top level", exportPos)
 
           /* Disallow non-static methods.
            * Note: Non-static classes have more specific error messages in checkClassOrModuleExports.
            */
-          if (sym.is(Method) && (!symOwner.isStatic || !symOwner.is(ModuleClass))) {
-            report.error(
-                "Only static objects may export their members to the top level",
-                annot.tree)
-          }
+          if (sym.isTerm && (!symOwner.isStatic || !symOwner.is(ModuleClass)))
+            report.error("Only static objects may export their members to the top level", exportPos)
 
           // The top-level name must be a valid JS identifier
-          if (!isValidTopLevelExportName(name)) {
-            report.error(
-                "The top-level export name must be a valid JavaScript identifier name",
-                annot.tree)
-          }
+          if (!isValidTopLevelExportName(name))
+            report.error("The top-level export name must be a valid JavaScript identifier name", exportPos)
 
         case ExportDestination.Static =>
           def companionIsNonNativeJSClass: Boolean = {
@@ -334,21 +323,21 @@ object PrepJSExports {
           if (!symOwner.isStatic || !symOwner.is(ModuleClass) || !companionIsNonNativeJSClass) {
             report.error(
                 "Only a static object whose companion class is a non-native JS class may export its members as static.",
-                annot.tree)
+                exportPos)
           }
 
           if (isMember) {
             if (sym.is(Lazy))
-              report.error("You may not export a lazy val as static", annot.tree)
+              report.error("You may not export a lazy val as static", exportPos)
           } else {
             if (sym.is(Trait))
-              report.error("You may not export a trait as static.", annot.tree)
+              report.error("You may not export a trait as static.", exportPos)
             else
-              report.error("Implementation restriction: cannot export a class or object as static", annot.tree)
+              report.error("Implementation restriction: cannot export a class or object as static", exportPos)
           }
       }
 
-      ExportInfo(name, destination)(annot.tree)
+      ExportInfo(name, destination)(exportPos)
     }
 
     allExportInfos.filter(_.destination == ExportDestination.Normal)
@@ -481,9 +470,7 @@ object PrepJSExports {
 
   /** Checks whether there are default parameters not at the end of the flattened parameter list. */
   private def hasIllegalDefaultParam(sym: Symbol)(using Context): Boolean = {
-    // TODO
-    //val isDefParam = (_: Symbol).hasFlag(Flags.DEFAULTPARAM)
-    //sym.paramss.flatten.reverse.dropWhile(isDefParam).exists(isDefParam)
-    false
+    sym.hasDefaultParams
+      && sym.paramSymss.flatten.reverse.dropWhile(_.is(HasDefault)).exists(_.is(HasDefault))
   }
 }

compiler/src/dotty/tools/dotc/transform/sjs/PrepJSInterop.scala

@@ -145,8 +145,7 @@ class PrepJSInterop extends MacroTransform with IdentityDenotTransformer { thisP
 
       tree match {
         case tree: TypeDef if tree.isClassDef =>
-          if (!sym.is(ModuleClass))
-            checkClassExports(sym)
+          checkClassOrModuleExports(sym)
 
           if (isJSAny(sym))
             transformJSClassDef(tree)
@@ -228,6 +227,8 @@ class PrepJSInterop extends MacroTransform with IdentityDenotTransformer { thisP
       exporters.get(clsSym).fold {
         transformedTree
       } { exports =>
+        checkNoDoubleDeclaration(clsSym)
+
         cpy.Template(transformedTree)(
           transformedTree.constr,
           transformedTree.parents,

tests/neg-scalajs/jsexport-bad-tostring.scala

@@ -0,0 +1,12 @@
+import scala.scalajs.js
+import scala.scalajs.js.annotation._
+
+class A {
+  @JSExport("toString") // error
+  def a(): Int = 5
+}
+
+class B {
+  @JSExport("toString") // ok
+  def a(x: Int): Int = x + 1
+}

tests/neg-scalajs/jsexport-conflict-method-prop.scala

@@ -0,0 +1,23 @@
+import scala.scalajs.js
+import scala.scalajs.js.annotation._
+
+class A1 {
+  @JSExport("a") // error
+  def bar(): Int = 2
+
+  @JSExport("a") // error
+  val foo = 1
+}
+
+class B1 {
+  @JSExport("a")
+  def bar(): Int = 2
+}
+
+class B2 extends B1 {
+  @JSExport("a") // error
+  def foo_=(x: Int): Unit = ()
+
+  @JSExport("a")
+  val foo = 1
+}

tests/neg-scalajs/jsexport-conflicts.scala

@@ -0,0 +1,40 @@
+import scala.scalajs.js
+import scala.scalajs.js.annotation._
+
+class A {
+  @JSExport
+  def rtType(x: js.Any): js.Any = x
+
+  @JSExport // error
+  def rtType(x: js.Dynamic): js.Dynamic = x
+}
+
+class B {
+  @JSExport
+  def foo(x: Int)(ys: Int*): Int = x
+
+  @JSExport // error
+  def foo(x: Int*): Seq[Int] = x
+}
+
+class C {
+  @JSExport
+  def foo(x: Int = 1): Int = x
+  @JSExport // error
+  def foo(x: String*): Seq[String] = x
+}
+
+class D {
+  @JSExport
+  def foo(x: Double, y: String)(z: Int = 1): Double = x
+  @JSExport // error
+  def foo(x: Double, y: String)(z: String*): Double = x
+}
+
+class E {
+  @JSExport
+  def a(x: scala.scalajs.js.Any): Int = 1
+
+  @JSExport // error
+  def a(x: Any): Int = 2
+}

tests/neg-scalajs/jsexport-double-definition.scala

@@ -0,0 +1,19 @@
+import scala.scalajs.js
+import scala.scalajs.js.annotation._
+
+class A {
+  @JSExport("value") // error
+  def hello: String = "foo"
+
+  @JSExport("value")
+  def world: String = "bar"
+}
+
+class B {
+  class Box[T](val x: T)
+
+  @JSExport // error
+  def ub(x: Box[String]): String = x.x
+  @JSExport
+  def ub(x: Box[Int]): Int = x.x
+}

tests/neg-scalajs/jsexport-double-underscore.scala

@@ -0,0 +1,18 @@
+import scala.scalajs.js
+import scala.scalajs.js.annotation._
+
+class A {
+  @JSExport(name = "__") // error
+  def foo: Int = 1
+
+  @JSExport // error
+  def bar__(x: Int): Int = x
+}
+
+object B {
+  @JSExportTopLevel(name = "__") // ok
+  val foo: Int = 1
+
+  @JSExportTopLevel("bar__") // ok
+  def bar(x: Int): Int = x
+}

tests/neg-scalajs/jsexport-implicit-apply.scala

@@ -0,0 +1,24 @@
+import scala.scalajs.js
+import scala.scalajs.js.annotation._
+
+class A {
+  @JSExport // error
+  def apply(): Int = 1
+}
+
+@JSExportAll
+class B {
+  def apply(): Int = 1 // error
+}
+
+@JSExportAll
+class C {
+  @JSExport("foo")
+  def apply(): Int = 1 // error
+}
+
+@JSExportAll
+class D {
+  @JSExport("apply")
+  def apply(): Int = 1 // ok
+}

tests/neg-scalajs/jsexport-js-member.scala

@@ -0,0 +1,14 @@
+import scala.scalajs.js
+import scala.scalajs.js.annotation._
+
+@js.native
+@JSGlobal
+class A extends js.Object {
+  @JSExport // error
+  def foo: Int = js.native
+}
+
+class B extends js.Object {
+  @JSExport // error
+  def foo: Int = js.native
+}