Fix #196: Do not instrument default parameter for Scala.js #281
Conversation
|
Hello, could someone review this PR? |
| @@ -107,6 +107,15 @@ class ScoverageInstrumentationComponent(val global: Global, extraAfterPhase: Opt | |||
| private var options: ScoverageOptions = new ScoverageOptions() | |||
| private var coverageFilter: CoverageFilter = AllCoverageFilter | |||
|
|
|||
| private val isScalaJsEnabled: Boolean = { | |||
| try { | |||
| getClass.getClassLoader.loadClass("scala.scalajs.js.Any") | |||
There was a problem hiding this comment.
This is testing whether scala.scalajs.js.Any is on the classpath of the compiler. This is not what you want. You want to test whether it is on the classpath of the project being compiled. You can test that with
rootMirror.getClassIfDefined("foo") != NoSymbol| @@ -17,7 +19,7 @@ object ScoverageCompiler { | |||
| case _ => ScalaVersion | |||
| } | |||
|
|
|||
| def classPath = getScalaJars.map(_.getAbsolutePath) :+ sbtCompileDir.getAbsolutePath :+ runtimeClasses.getAbsolutePath | |||
| def classPath = (getScalaJars ++ getScalaJsJars ++ List(sbtCompileDir, runtimeClasses)).map(_.getAbsolutePath) | |||
There was a problem hiding this comment.
You should not put scalajs-library.jar on the classpath of the compiler, ever. It should be passed to the -classpath setting of the compiler so that it ends up on the classpath of the project being compiled instead.
There was a problem hiding this comment.
Addressed in 4dffb6c
I am uncertain about this, but addToClassPath seems to append the file at the tend of classpath.
| isScalaJsEnabled && symbol != null && symbol.isSynthetic && symbol.isMethod && | ||
| symbol.nameString.contains("$default$") && | ||
| symbol.tpe.resultType.annotations.exists(_.symbol.nameString == "uncheckedVariance") |
There was a problem hiding this comment.
This is too broad, and maybe also too restrictive (e.g., I don't know what the uncheckedVariance is doing here).
Here is how we identify an undefined parameter in the real Scala.js compiler:
https://github.com/scala-js/scala-js/blob/4619d906baef7feb5d0b6d555d5b33044669434e/compiler/src/main/scala/org/scalajs/nscplugin/GenJSCode.scala#L2696-L2721
You should basically copy-paste that logic, along with the helper methods that it calls.
There was a problem hiding this comment.
Alternatively, you could disable all default parameters under Scala.js. It is less precise, but it will be much easier. In that case you should write
isScalaJsEnabled && symbol.hasFlag(scala.reflect.internal.Flags.DEFAULTPARAM)There was a problem hiding this comment.
Copy-pasted from scala-js in f83f2ec
|
After 345721b, the below two tests, which import test("macro range positions should not break plugin") {
val compiler = ScoverageCompiler.default
macroSupportDeps.foreach(compiler.addToClassPath(_))
compiler.compileCodeSnippet( s"""import scoverage.macrosupport.Tester
|
|object MacroTest {
| Tester.test
|} """.stripMargin)
assert(!compiler.reporter.hasErrors)
assert(!compiler.reporter.hasWarnings)
}
test("plugin should not instrument expanded macro code http://github.com/skinny-framework/skinny-framework/issues/97") {
val compiler = ScoverageCompiler.default
macroSupportDeps.foreach(compiler.addToClassPath(_))
compiler.compileCodeSnippet( s"""import scoverage.macrosupport.Tester
|
|class MacroTest {
| Tester.test
|} """.stripMargin)
assert(!compiler.reporter.hasErrors)
assert(!compiler.reporter.hasWarnings)
compiler.assertNoCoverage()
}
|
|
Hello, what’s the status of this PR? |
|
Changed from last night,
I have no idea why CI do not run... |
|
@sksamuel @D-Roch @gslowikowski @sjrd @julienrf |
|
Seems reasonable from what little I know of scala.js |
Closes #196 and scala-js/scala-js#3803
Quick and dirty.
I am not familiar with Scala macro, so advices are appreciated.