Fix coverage instrumentation of java and parameterless methods

1. Select and TypeApply trees weren't properly handled for JavaDefined symbols, leading to a crash when selecting a static method with parameter types.
2. Select and Ident trees weren't properly handled, and many occurences of parameterless methods were missed.
3. Some methods like asInstanceOf and isInstanceOf must not be instrumented, otherwise it crashes in Erasure.

Author: TheElectronWill

compiler/src/dotty/tools/dotc/transform/InstrumentCoverage.scala

@@ -10,15 +10,14 @@ import core.Flags.*
 import core.Contexts.{Context, ctx, inContext}
 import core.DenotTransformers.IdentityDenotTransformer
 import core.Symbols.{defn, Symbol}
-import core.Decorators.{toTermName, i}
 import core.Constants.Constant
 import core.NameOps.isContextFunction
 import core.Types.*
 import typer.LiftCoverage
 import util.{SourcePosition, Property}
 import util.Spans.Span
 import coverage.*
-import localopt.StringInterpolatorOpt.isCompilerIntrinsic
+import localopt.StringInterpolatorOpt
 
 /** Implements code coverage by inserting calls to scala.runtime.coverage.Invoker
   * ("instruments" the source code).
@@ -66,7 +65,16 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
         tree match
           // simple cases
           case tree: (Import | Export | Literal | This | Super | New) => tree
-          case tree if tree.isEmpty || tree.isType => tree // empty Thicket, Ident, TypTree, ...
+          case tree if tree.isEmpty || tree.isType => tree // empty Thicket, Ident (referring to a type), TypeTree, ...
+
+          // identifier
+          case tree: Ident =>
+            val sym = tree.symbol
+            if canInstrumentParameterless(sym) then
+              // call to a local parameterless method f
+              instrument(tree)
+            else
+              tree
 
           // branches
           case tree: If =>
@@ -82,20 +90,6 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
               finalizer = instrument(transform(tree.finalizer), branch = true)
             )
 
-          // a.f(args)
-          case tree @ Apply(fun: Select, args) =>
-            // don't transform the first Select, but do transform `a.b` in `a.b.f(args)`
-            val transformedFun = cpy.Select(fun)(transform(fun.qualifier), fun.name)
-            if canInstrumentApply(tree) then
-              if needsLift(tree) then
-                val transformed = cpy.Apply(tree)(transformedFun, args) // args will be transformed in instrumentLifted
-                instrumentLifted(transformed)
-              else
-                val transformed = transformApply(tree, transformedFun)
-                instrument(transformed)
-            else
-              transformApply(tree, transformedFun)
-
           // f(args)
           case tree: Apply =>
             if canInstrumentApply(tree) then
@@ -106,24 +100,19 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
             else
               transformApply(tree)
 
-          // (f(x))[args]
-          case TypeApply(fun: Apply, args) =>
+          // (fun)[args]
+          case TypeApply(fun, args) =>
             cpy.TypeApply(tree)(transform(fun), args)
 
           // a.b
           case Select(qual, name) =>
-            if qual.symbol.exists && qual.symbol.is(JavaDefined) then
-              //Java class can't be used as a value, we can't instrument the
-              //qualifier ({<Probe>;System}.xyz() is not possible !) instrument it
-              //as it is
-              instrument(tree)
+            val transformed = cpy.Select(tree)(transform(qual), name)
+            val sym = tree.symbol
+            if canInstrumentParameterless(sym) then
+              // call to a parameterless method
+              instrument(transformed)
             else
-              val transformed = cpy.Select(tree)(transform(qual), name)
-              if transformed.qualifier.isDef then
-                // instrument calls to methods without parameter list
-                instrument(transformed)
-              else
-                transformed
+              transformed
 
           case tree: CaseDef => instrumentCaseDef(tree)
           case tree: ValDef =>
@@ -142,7 +131,9 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
               val rhs = transform(tree.rhs)
               val finalRhs =
                 if canInstrumentDefDef(tree) then
-                  // Ensure that the rhs is always instrumented, if possible
+                  // Ensure that the rhs is always instrumented, if possible.
+                  // This is useful because methods can be stored and called later, or called by reflection,
+                  // and if the rhs is too simple to be instrumented (like `def f = this`), the method won't show up as covered.
                   instrumentBody(tree, rhs)
                 else
                   rhs
@@ -162,7 +153,7 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
         }
 
     /** Lifts and instruments an application.
-      * Note that if only one arg needs to be lifted, we just lift everything.
+      * Note that if only one arg needs to be lifted, we just lift everything (see LiftCoverage).
       */
     private def instrumentLifted(tree: Apply)(using Context) =
       // lifting
@@ -178,10 +169,7 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
       )
 
     private inline def transformApply(tree: Apply)(using Context): Apply =
-      transformApply(tree, transform(tree.fun))
-
-    private inline def transformApply(tree: Apply, transformedFun: Tree)(using Context): Apply =
-      cpy.Apply(tree)(transformedFun, transform(tree.args))
+      cpy.Apply(tree)(transform(tree.fun), transform(tree.args))
 
     private inline def instrumentCases(cases: List[CaseDef])(using Context): List[CaseDef] =
       cases.map(instrumentCaseDef)
@@ -292,7 +280,7 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
          * they shouldn't be lifted.
          */
         val sym = fun.symbol
-        sym.exists && (isShortCircuitedOp(sym) || isCompilerIntrinsic(sym))
+        sym.exists && (isShortCircuitedOp(sym) || StringInterpolatorOpt.isCompilerIntrinsic(sym))
       end
 
       val fun = tree.fun
@@ -312,7 +300,9 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
 
     /** Check if an Apply can be instrumented. Prevents this phase from generating incorrect code. */
     private def canInstrumentApply(tree: Apply)(using Context): Boolean =
-      !tree.symbol.isOneOf(Synthetic | Artifact) && // no need to instrument synthetic apply
+      val sym = tree.symbol
+      !sym.isOneOf(Synthetic | Artifact) && // no need to instrument synthetic apply
+      !isCompilerIntrinsicMethod(sym) &&
       (tree.typeOpt match
         case AppliedType(tycon: NamedType, _) =>
           /* If the last expression in a block is a context function, we'll try to
@@ -339,6 +329,24 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
           true
       )
 
+    /** Is this the symbol of a parameterless method that we can instrument?
+      * Note: it is crucial that `asInstanceOf` and `isInstanceOf`, among others,
+      * do NOT get instrumented, because that would generate invalid code and crash
+      * in post-erasure checking.
+      */
+    private def canInstrumentParameterless(sym: Symbol)(using Context): Boolean =
+      sym.is(Method, butNot = Synthetic | Artifact) &&
+      sym.info.isParameterless &&
+      !isCompilerIntrinsicMethod(sym)
+
+    /** Does sym refer to a "compiler intrinsic" method, which only exist during compilation,
+      * like Any.isInstanceOf?
+      * If this returns true, the call souldn't be instrumented.
+      */
+    private def isCompilerIntrinsicMethod(sym: Symbol)(using Context): Boolean =
+      val owner = sym.maybeOwner
+      owner.eq(defn.AnyClass) || owner.isPrimitiveValueClass
+
 object InstrumentCoverage:
   val name: String = "instrumentCoverage"
   val description: String = "instrument code for coverage checking"

tests/coverage/pos/Constructor.scoverage.check

@@ -59,6 +59,23 @@ C
 Class
 covtest.C
 <init>
+62
+63
+5
+x
+Select
+false
+0
+false
+x
+
+3
+Constructor.scala
+covtest
+C
+Class
+covtest.C
+<init>
 60
 64
 5
@@ -69,7 +86,7 @@ false
 false
 f(x)
 
-3
+4
 Constructor.scala
 covtest
 O$
@@ -86,7 +103,7 @@ false
 false
 def g
 
-4
+5
 Constructor.scala
 covtest
 O$
@@ -103,7 +120,24 @@ false
 false
 def y
 
-5
+6
+Constructor.scala
+covtest
+O$
+Object
+covtest.O$
+<init>
+112
+113
+10
+y
+Ident
+false
+0
+false
+y
+
+7
 Constructor.scala
 covtest
 O$

tests/coverage/pos/ContextFunctions.scoverage.check

@@ -58,108 +58,6 @@ covtest
 Imperative
 Class
 covtest.Imperative
-$anonfun
-267
-294
-13
-apply
-Apply
-false
-0
-false
-readName2(using e)(using s)
-
-3
-ContextFunctions.scala
-covtest
-Imperative
-Class
-covtest.Imperative
-readPerson
-252
-295
-13
-<init>
-Apply
-false
-0
-false
-OnError((e) => readName2(using e)(using s))
-
-4
-ContextFunctions.scala
-covtest
-Imperative
-Class
-covtest.Imperative
-readPerson
-252
-295
-13
-invoked
-Apply
-false
-0
-false
-OnError((e) => readName2(using e)(using s))
-
-5
-ContextFunctions.scala
-covtest
-Imperative
-Class
-covtest.Imperative
-$anonfun
-267
-294
-13
-invoked
-Apply
-false
-0
-false
-readName2(using e)(using s)
-
-6
-ContextFunctions.scala
-covtest
-Imperative
-Class
-covtest.Imperative
-$anonfun
-267
-294
-13
-apply
-Apply
-false
-0
-false
-readName2(using e)(using s)
-
-7
-ContextFunctions.scala
-covtest
-Imperative
-Class
-covtest.Imperative
-readPerson
-252
-295
-13
-<init>
-Apply
-false
-0
-false
-OnError((e) => readName2(using e)(using s))
-
-8
-ContextFunctions.scala
-covtest
-Imperative
-Class
-covtest.Imperative
 readPerson
 252
 309
@@ -171,7 +69,7 @@ false
 false
 OnError((e) => readName2(using e)(using s)).onError(None)
 
-9
+3
 ContextFunctions.scala
 covtest
 Imperative