-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Make coverage instrumentation more robust #16235
Conversation
b4c5ef5
to
e73a7bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise LGTM!
compiler/src/dotty/tools/dotc/transform/InstrumentCoverage.scala
Outdated
Show resolved
Hide resolved
compiler/src/dotty/tools/dotc/transform/InstrumentCoverage.scala
Outdated
Show resolved
Hide resolved
compiler/src/dotty/tools/dotc/transform/InstrumentCoverage.scala
Outdated
Show resolved
Hide resolved
compiler/src/dotty/tools/dotc/transform/InstrumentCoverage.scala
Outdated
Show resolved
Hide resolved
compiler/src/dotty/tools/dotc/transform/InstrumentCoverage.scala
Outdated
Show resolved
Hide resolved
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
e73a7bc
to
02fd9de
Compare
Ah, we got hit by Windows path separator again :( |
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:
InstrumentedParts
. The initial problem was thatTypeApply
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 instrumentexpr
and then, if it was successfully instrumented, we move the call toinvoked(...)
to the right place. This gives us the following code intransform
:Erased
trees and calls to the parents' constructor inTemplate#parents
.Serializer
.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 calledInlines.dropInlined
👀. Any suggestion is welcome!