Fix coverage instrumentation of Java-defined and parameterless methods

[TheElectronWill]

Note: dear reviewer, don't worry! Most of the changes are "expect" files for the tests. The actual changes in the compiler codebase are small :)

#13880 introduced code instrumentation for coverage analysis with the compiler option -coverage-out. #15530 fixed a few corner cases in the instrumentation, but 3 bugs remained:

1. Select and TypeApply trees weren't properly handled for Java-defined symbols, leading to a crash when selecting a method with parameter types. See java-methods/test.scala. I found this bug while trying to run coverage on ScalaPB's generated code.
2. Select and Ident trees weren't properly handled, and many occurences of parameterless methods were missed. For example:

def f = ()
f // was not instrumented, and thus the coverage analysis was wrong

def g() = ()
g() // was instrumented properly, and still is

The right way to detect parameterless methods seems to be

sym.is(Method) && sym.info.isParameterless

Most of the changes in the expect files are caused by this fix.

3. Some methods like asInstanceOf and isInstanceOf must not be instrumented, otherwise it crashes in Erasure. That is, we should never generate this:

{
  Invoker.invoked(id, dir)
  x.asInstanceOf
}[T]

I've also ignored the calls to methods on primitive types, like Int.+, because it doesn't exist at runtime and doesn't seem interesting to instrument. Note that in a + f(), the call to f will be instrumented properly. Also, if a is a parameterless method, the call to a will be instrumented.

For example:

1 + f()

previously resulted in:

{
  invoked(...) // covered method: "+" of Int
  {
    val f$1 = {
      invoked(...) // covered method: "f"
      f()
    }
    1 + f$1
  }
}

and now gives:

{
  val f$1 = {
    invoked(...) // covered method: "f"
    f()
  }
1 + f$1
}

[TheElectronWill]

Like #15530, I'll backport this to release-3.0 once it's good.

[ckipp01]

Hey @TheElectronWill, just to be transparent, I know you tagged me on this but I'm no longer really working on any coverage related stuff apart from just cutting releases of scoverage, so probably best to tag someone else for a more timely review.

[TheElectronWill]

Hey @TheElectronWill, just to be transparent, I know you tagged me on this but I'm no longer really working on any coverage related stuff apart from just cutting releases of scoverage, so probably best to tag someone else for a more timely review.

No problem! Thanks for your transparence :)

Tentatively assigning my homonym @smarter, who is suggested by GitHub (feel free to assign someone else if you don't have the time to review this, I know you've got things to write ^^).

[smarter]

Tentatively assigning my homonym @smarter, who is suggested by GitHub (feel free to assign someone else if you don't have the time to review this, I know you've got things to write ^^).

Yeah I wish I could but as I'm handing in my thesis in two weeks I definitely don't have the time to look at github, so passing the buck to @dwijnand 😅

[TheElectronWill]

Best of luck for your thesis!

[griggt]

While investigating a compiler crash using coverage (separate issue, ticket may be forthcoming), I observed what seems to be a regression with this PR.

The following snippet now emits an error:

import scala.compiletime.uninitialized
class Foo:
  var x: AnyRef = uninitialized

> scalac -coverage-out:. /tmp/coverage/uninitialized.scala

-- Error: /tmp/coverage/uninitialized.scala:3:18 -------------------------------
3 |  var x: AnyRef = uninitialized
  |                  ^^^^^^^^^^^^^
  |`uninitialized` can only be used as the right hand side of a mutable field definition

It compiles without error using -coverage-out on 3.2.0-RC2 and 3.2.1-RC1-bin-20220717-5c43324-NIGHTLY.

[TheElectronWill]

Ah yes, that's because it gets instrumented like this:

var x = {
  invoked(...)
  uninitialized
}

I'll add it to the blacklist.
Thank you for reporting this!

EDIT: done ✔️

[smarter]

@sjrd do you by any chance have some time to take a look at this PR?

[sjrd]

I'll find some time in the coming week.

[sjrd]

The changes to the compiler code LGTM. I have not checked the test changes, as I don't know what I'm looking at. ;)

[TheElectronWill]

Auto-generated boring coverage data, essentially 😄
When looking at such data, I check that the line numbers are globally correct, and that all the methods that are called in the source code appear in the generated file.

Thanks for the review!