Skip to content
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

Make coverage instrumentation more robust #16235

Merged

Conversation

TheElectronWill
Copy link
Contributor

@TheElectronWill TheElectronWill commented Oct 23, 2022

As with the last coverage PR, don't worry: most of the changes are "expect" files for tests. On top of that, many files have only changed by the order in which the statements are recorded, but this order doesn't matter.

Small changes which, together, make the instrumentation more robust and fix many bugs:

  1. Address comments in Keep 'fun' and 'args' together when instrumenting TypeApply for coverage #15739 by introducing a type InstrumentedParts. The initial problem was that TypeApply cannot be instrumented in a straightforward way: expr[T] cannot be turned into {invoked(...); expr}[T] but must be {invoked(...); expr[T]}. To do this, we first try to instrument expr and then, if it was successfully instrumented, we move the call to invoked(...) to the right place. This gives us the following code in transform:
case TypeApply(fun, args) =>
  val InstrumentedParts(pre, coverageCall, expr) = tryInstrument(fun) // may generate a call to invoked(...), but it's not part of the resulting tree yet
  if coverageCall.isEmpty then
    tree
  else
    Block(
      pre :+ coverageCall, // put the call at the right place (pre contains lifted definitions, if any)
      cpy.TypeApply(tree)(expr, args)
  )
  1. Exclude more trees from instrumentation, like Erased trees and calls to the parents' constructor in Template#parents.
  2. Escape special characters in Serializer.
  3. Reposition Inlined trees to the current source in order to avoid referencing an unreachable compilation unit. This might be the most controversial change because I've called Inlines.dropInlined 👀. Any suggestion is welcome!

Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM!

case tree: Inlined =>
// Ideally, tree.call would provide precise information about the inlined call,
// and we would use this information for the coverage report.
// But PostTyper simplifies tree.call, so we can't report the actual method that was inlined.
Copy link
Member

Choose a reason for hiding this comment

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

Isn't tree.call kept as-is by PostTyper? What information are we losing that affects the coverage report exactly?

Copy link
Contributor Author

@TheElectronWill TheElectronWill Oct 24, 2022

Choose a reason for hiding this comment

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

PostTyper replaces the call tree by a "call trace", see PostTyper:362. The doc of Inlined explains:

@param  call      Info about the original call that was inlined.
Until PostTyper, this is the full call, afterwards only
a reference to the toplevel class from which the call was inlined.

I don't know why it's like this, but the consequence is that for instrumentCoverage, tree.call only contains the toplevel class, not the original inlined call. Therefore, we lose the name of the method (or val) that was inlined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally, for:

assert(x) // inline def in Predef

we would report that assert was called, but that's not possible for the moment.

Copy link
Member

Choose a reason for hiding this comment

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

ah yeah that's right, I think it's just an optimization, in particular it reduces the amount of stuff we store in the tasty files, but if it negatively affects the coverage checking report maybe this could be revisited.

@smarter smarter assigned TheElectronWill and unassigned smarter Oct 24, 2022
@smarter smarter merged commit f3fec58 into scala:main Oct 24, 2022
@TheElectronWill
Copy link
Contributor Author

Ah, we got hit by Windows path separator again :(
#16245

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment