search

scoverage / scalac-scoverage-plugin

Scoverage Scala Code Coverage Core Libs
https://github.com/scoverage
Apache License 2.0
425 stars 126 forks source link

Re-enable `PluginCoverageScalaJsTest`, fix `dangling UndefinedParam` bug #464

Closed armanbilge closed 2 years ago

armanbilge commented 2 years ago

This PR re-enables the PluginCoverageScalaJsTest that is currently being ignored. Fortunately, it still passes :)

Fixes https://github.com/scoverage/scalac-scoverage-plugin/issues/447.

armanbilge commented 2 years ago

Strange, I could have sworn the test was passing before! But failure is good too, it might mean https://github.com/scoverage/scalac-scoverage-plugin/issues/447 is a regression and not a new issue.

armanbilge commented 2 years ago

Ok, this is ready for review :) I'll work on https://github.com/scoverage/scalac-scoverage-plugin/issues/447 in a follow-up PR.

armanbilge commented 2 years ago

Actually, I am skeptical if this is invoking the compiler the right way yet. Partly because I can't reproduce https://github.com/scoverage/scalac-scoverage-plugin/issues/447, and also because I realized I didn't make any configuration changes besides adding the sjs compiler to the classpath, which surely won't activate it magically 😅

armanbilge commented 2 years ago

Ok, I know I'm going in circles here, but now that I think I'm invoking the compiler correctly, indeed the existing test is broken with the same issue as https://github.com/scoverage/scalac-scoverage-plugin/issues/447.

/tmp/scoverage_snippet16088674966675160284.scala:3: error: Found a dangling UndefinedParam at Position(file:/tmp/scoverage_snippet16088674966675160284.scala,3,46). This is likely due to a bad interaction between a macro or a compiler plugin and the Scala.js compiler plugin. If you hit this, please let us know.
object JSONHelper {
armanbilge commented 2 years ago

Politely pinging @exoego who PRed the original fix in https://github.com/scoverage/scalac-scoverage-plugin/pull/281. Now that I have the test working, if you are able to help at all would be much appreciated! Thanks :)

armanbilge commented 2 years ago

So far it seems like the problem is detecting Scala.js itself. https://github.com/scoverage/scalac-scoverage-plugin/blob/a557126898f9ada995b04e3c879324a41add1d4e/plugin/src/main/scala/scoverage/ScoveragePlugin.scala#L101-L107

If I change that to return true in the catch, then the test passes (with a minor adjustment to number of measured statements).

I'm hopeless to get it to print out the error 😕

armanbilge commented 2 years ago

Ok, got a stack trace that's thrown when calling rootMirror. Any ideas?

java.lang.NullPointerException
        at scala.reflect.internal.SymbolTable.phase_$eq(SymbolTable.scala:243)
        at scala.reflect.internal.SymbolTable.pushPhase(SymbolTable.scala:247)
        at scala.reflect.internal.SymbolTable.exitingPhase(SymbolTable.scala:290)
        at scala.reflect.internal.Symbols$TypeHistory.phaseString(Symbols.scala:3850)
        at scala.reflect.internal.Symbols$TypeHistory.$anonfun$toString$1(Symbols.scala:3852)
        at scala.collection.Iterator$$anon$9.next(Iterator.scala:577)
        at scala.collection.IterableOnceOps.addString(IterableOnce.scala:1184)
        at scala.collection.IterableOnceOps.addString$(IterableOnce.scala:1179)
        at scala.collection.AbstractIterator.addString(Iterator.scala:1293)
        at scala.collection.IterableOnceOps.mkString(IterableOnce.scala:1129)
        at scala.collection.IterableOnceOps.mkString$(IterableOnce.scala:1127)
        at scala.collection.AbstractIterator.mkString(Iterator.scala:1293)
        at scala.reflect.internal.Symbols$TypeHistory.toString(Symbols.scala:3852)
        at java.base/java.lang.String.valueOf(String.java:2951)
        at scala.reflect.internal.SymbolTable.throwAssertionError(SymbolTable.scala:171)
        at scala.reflect.internal.Symbols$TypeHistory.<init>(Symbols.scala:3831)
        at scala.reflect.internal.Symbols$TypeHistory$.apply(Symbols.scala:3829)
        at scala.reflect.internal.Symbols$Symbol.info_$eq(Symbols.scala:1586)
        at scala.reflect.internal.Symbols$TypeSymbol.info_$eq(Symbols.scala:3256)
        at scala.reflect.internal.Symbols$Symbol.setInfo(Symbols.scala:1592)
        at scala.reflect.internal.Mirrors$Roots$RootClass.<init>(Mirrors.scala:313)
        at scala.reflect.internal.Mirrors$Roots.RootClass$lzycompute(Mirrors.scala:327)
        at scala.reflect.internal.Mirrors$Roots.RootClass(Mirrors.scala:327)
        at scala.reflect.internal.Mirrors$Roots$EmptyPackageClass.<init>(Mirrors.scala:336)
        at scala.reflect.internal.Mirrors$Roots.EmptyPackageClass$lzycompute(Mirrors.scala:342)
        at scala.reflect.internal.Mirrors$Roots.EmptyPackageClass(Mirrors.scala:342)
        at scala.reflect.internal.Mirrors$Roots.EmptyPackageClass(Mirrors.scala:282)
        at scala.reflect.internal.Mirrors$RootsBase.init(Mirrors.scala:256)
        at scala.tools.nsc.Global.rootMirror$lzycompute(Global.scala:75)
        at scala.tools.nsc.Global.rootMirror(Global.scala:73)
        at scoverage.ScoverageInstrumentationComponent.liftedTree1$1(ScoveragePlugin.scala:104)
armanbilge commented 2 years ago

Aha, I think making it lazy does the trick.

armanbilge commented 2 years ago

I can confirm the fix (as released in 2.0.0-M7+1-9cfca69b-SNAPSHOT) works in downstreams 👍