Skip to content

Scala.js: Add neg tests for bad uses of reflect.Selectable. #9611

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 21, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions compiler/test/dotty/Properties.scala
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ object Properties {
/** dotty-library jar */
def dottyLibrary: String = sys.props("dotty.tests.classes.dottyLibrary")

/** dotty-library-js jar */
def dottyLibraryJS: String = sys.props("dotty.tests.classes.dottyLibraryJS")

/** dotty-compiler jar */
def dottyCompiler: String = sys.props("dotty.tests.classes.dottyCompiler")

Expand All @@ -74,4 +77,7 @@ object Properties {

/** jline-reader jar */
def jlineReader: String = sys.props("dotty.tests.classes.jlineReader")

/** scalajs-library jar */
def scalaJSLibrary: String = sys.props("dotty.tests.classes.scalaJSLibrary")
}
40 changes: 40 additions & 0 deletions compiler/test/dotty/tools/dotc/ScalaJSCompilationTests.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package dotty
package tools
package dotc

import org.junit.{ Test, BeforeClass, AfterClass }
import org.junit.experimental.categories.Category

import scala.concurrent.duration._
import vulpix._

@Category(Array(classOf[ScalaJSCompilationTests]))
class ScalaJSCompilationTests extends ParallelTesting {
import ParallelTesting._
import TestConfiguration._
import ScalaJSCompilationTests._
import CompilationTest.aggregateTests

// Test suite configuration --------------------------------------------------

def maxDuration = 60.seconds
def numberOfSlaves = 5
def safeMode = Properties.testsSafeMode
def isInteractive = SummaryReport.isInteractive
def testFilter = Properties.testsFilter
def updateCheckFiles: Boolean = Properties.testsUpdateCheckfile

// Negative tests ------------------------------------------------------------

@Test def negScalaJS: Unit = {
implicit val testGroup: TestGroup = TestGroup("negScalaJS")
aggregateTests(
compileFilesInDir("tests/neg-scalajs", scalaJSOptions),
).checkExpectedErrors()
}
}

object ScalaJSCompilationTests {
implicit val summaryReport: SummaryReporting = new SummaryReport
@AfterClass def cleanup(): Unit = summaryReport.echoSummary()
}
7 changes: 7 additions & 0 deletions compiler/test/dotty/tools/vulpix/TestConfiguration.scala
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,11 @@ object TestConfiguration {
lazy val withTastyInspectorClasspath =
withCompilerClasspath + File.pathSeparator + mkClasspath(List(Properties.dottyTastyInspector))

lazy val scalaJSClasspath = mkClasspath(List(
Properties.scalaJSLibrary,
Properties.dottyLibraryJS
))

def mkClasspath(classpaths: List[String]): String =
classpaths.map({ p =>
val file = new java.io.File(p)
Expand All @@ -61,6 +66,8 @@ object TestConfiguration {
defaultOptions.withClasspath(withStagingClasspath).withRunClasspath(withStagingClasspath)
lazy val withTastyInspectorOptions =
defaultOptions.withClasspath(withTastyInspectorClasspath).withRunClasspath(withTastyInspectorClasspath)
lazy val scalaJSOptions =
defaultOptions.and("-scalajs").withClasspath(scalaJSClasspath)
val allowDeepSubtypes = defaultOptions without "-Yno-deep-subtypes"
val allowDoubleBindings = defaultOptions without "-Yno-double-bindings"
val picklingOptions = defaultOptions and (
Expand Down
6 changes: 5 additions & 1 deletion project/Build.scala
Original file line number Diff line number Diff line change
Expand Up @@ -526,19 +526,22 @@ object Build {
(sourceManaged in Compile).value
}
val externalDeps = externalCompilerClasspathTask.value
val externalJSDeps = (externalDependencyClasspath in (LocalProject("dotty-library-bootstrappedJS"), Compile)).value
val jars = packageAll.value

Seq(
"-Ddotty.tests.dottyCompilerManagedSources=" + managedSrcDir,
"-Ddotty.tests.classes.dottyInterfaces=" + jars("dotty-interfaces"),
"-Ddotty.tests.classes.dottyLibrary=" + jars("dotty-library"),
"-Ddotty.tests.classes.dottyLibraryJS=" + jars("dotty-library-js"),
"-Ddotty.tests.classes.dottyCompiler=" + jars("dotty-compiler"),
"-Ddotty.tests.classes.tastyCore=" + jars("tasty-core"),
"-Ddotty.tests.classes.compilerInterface=" + findArtifactPath(externalDeps, "compiler-interface"),
"-Ddotty.tests.classes.scalaLibrary=" + findArtifactPath(externalDeps, "scala-library"),
"-Ddotty.tests.classes.scalaAsm=" + findArtifactPath(externalDeps, "scala-asm"),
"-Ddotty.tests.classes.jlineTerminal=" + findArtifactPath(externalDeps, "jline-terminal"),
"-Ddotty.tests.classes.jlineReader=" + findArtifactPath(externalDeps, "jline-reader"),
"-Ddotty.tests.classes.scalaJSLibrary=" + findArtifactPath(externalJSDeps, "scalajs-library_2.13"),
)
},

Expand Down Expand Up @@ -705,7 +708,8 @@ object Build {
// running the compiler, we should always have the bootstrapped
// library on the compiler classpath since the non-bootstrapped one
// may not be binary-compatible.
"dotty-library" -> packageBin.in(`dotty-library-bootstrapped`, Compile).value
"dotty-library" -> packageBin.in(`dotty-library-bootstrapped`, Compile).value,
"dotty-library-js" -> packageBin.in(`dotty-library-bootstrappedJS`, Compile).value,
Copy link
Member

@smarter smarter Aug 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I run dotty-compiler/run, the bootstrappedJS library is now compiled whereas it wasn't before, this would slow down working in dotty in most cases where we're not working on scala.js support. Similarly, I should be able to run dotty-compiler/testCompilation tests/pos/foo without having to compile anything scala.js-related. So I suggest moving this stuff to another project, either an existing scala.js project like sjsJUnitTests or a new one as you prefer.

Copy link
Member Author

@sjrd sjrd Aug 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It really doesn't slow anything down, because a) it gets compiled in parallel with dotty-library-bootstrapped and b) it takes roughly the same time. I see a difference of about 1 out of 9 seconds on my computers (my Windows at home and my Linux at work agree) between
a) dotty-library-bootstrapped/clean followed by dotty-compiler/packageAll, and
b) dotty-library-bootstrapped/clean and dotty-library-bootstrappedJS/clean followed by dotty-compiler/packageAll.

The alternative is to replicate a lot of infrastructure in a different project. There is currently no other project equipped to do that. This seems like a huge increase in complexity to avoid 1 second.

Copy link
Member

@smarter smarter Aug 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is currently no other project equipped to do that.

dotty-compiler-bootstrapped runs the regular tests + bootstrapped-only and staging-only tests, it works by overriding the packageAll task. You could either do the same in a different project or add the scalajs stuff to dotty-compiler-bootstrapped too, that way it doesn't impact non-bootstrapped tasks which would be acceptable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright. Adding to the bootstrapped tests only was not an option for me, because that makes my dev cycle much much longer, since I would have to rebuild the bootstrapped compiler for every single change.

I added a separate project instead.

).mapValues(_.getAbsolutePath)
}
}.value,
Expand Down
45 changes: 45 additions & 0 deletions tests/neg-scalajs/reflective-calls.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import scala.reflect.{ClassTag, Selectable => ReflectSel}
import ReflectSel.reflectiveSelectable

object Test {
/* Make sure that an explicit desugaring of the legit cases actually compiles,
* ensuring that the error cases we test are actually testing the right things.
*/
def sanityCheck(): Unit = {
val receiver: Any = ???
reflectiveSelectable(receiver).selectDynamic("foo") // OK
reflectiveSelectable(receiver).applyDynamic("foo")() // OK
reflectiveSelectable(receiver).applyDynamic("foo", ClassTag(classOf[String]), ClassTag(classOf[List[_]]))("bar", Nil) // OK
}

def badReceider(): Unit = {
val receiver: ReflectSel = ???
receiver.selectDynamic("foo") // error
receiver.applyDynamic("foo")() // error
}

def nonLiteralMethodName(): Unit = {
val receiver: Any = ???
val methodName: String = "foo"
reflectiveSelectable(receiver).selectDynamic(methodName) // error
reflectiveSelectable(receiver).applyDynamic(methodName)() // error
}

def nonLiteralClassTag(): Unit = {
val receiver: Any = ???
val myClassTag: ClassTag[String] = ClassTag(classOf[String])
reflectiveSelectable(receiver).applyDynamic("foo", myClassTag, ClassTag(classOf[List[_]]))("bar", Nil) // error
}

def classTagVarArgs(): Unit = {
val receiver: Any = ???
val classTags: List[ClassTag[_]] = List(ClassTag(classOf[String]), ClassTag(classOf[List[_]]))
reflectiveSelectable(receiver).applyDynamic("foo", classTags: _*)("bar", Nil) // error
}

def argsVarArgs(): Unit = {
val receiver: Any = ???
val args: List[Any] = List("bar", Nil)
reflectiveSelectable(receiver).applyDynamic("foo", ClassTag(classOf[String]), ClassTag(classOf[List[_]]))(args: _*) // error
}
}