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

Fix lifting of arguments with -coverage-out #15530

Merged
merged 5 commits into from
Jul 2, 2022

Conversation

TheElectronWill
Copy link
Contributor

@TheElectronWill TheElectronWill commented Jun 27, 2022

Fixes #15078, fixes #15487, fixes #15505

Summary:

  • LiftCoverage extended LiftComplex, it now extends LiftImpure and forces the lifting of non-erased applications
  • StringContext.s, f, raw are now excluded from lifting
  • ElimRepeated didn't handle lifted arguments properly, because it assumed that every argument was a Typed tree, which is not the case for arguments lifted by the coverage transformation. This is now fixed.

Most of the added lines are tests :)

How I fixed it

Let's see if I can imitate the awesome explanations of Martin 😄

InstrumentCoverage modifies the AST just after the pickling, and insert calls to scala.runtime.coverage.Invoker.invoked that record, at runtime, which methods are called.
See an example here. To lift functions' arguments, it uses LiftCoverage, which lives with its LiftXX siblings in EtaExpansion.scala.

LiftCoverage previously extended LiftComplex. A quick look at the definition of LiftComplex tells me that it lifts everything, except trees that are constants or have the maximal purity PurePath. And TreeInfo.exprPurity says that erased things are Pure, not PurePath! It seems that LiftImpure would be more appropriate than LiftComplex. Therefore, I happily type LiftCoverage extends LiftImpure. But stopping there would introduce another bug!

For example, given this code:

def f() = throw RuntimeException() // throws an exception
def g() = 0 // pure function
someFunction(f(), g())

We want to generate something like this, even if g has no side effects:

Invoker.invoked("f", ...)
val _f = f()
Invoker.invoked("g", ...)
val _g = g()
Invoker.invoked("someFunction", ...)
someFunction(_f, _g)

This way, f will throw its exception and prevent g from being marked as covered, which would be wrong.

Therefore, all non-erased applications must be lifted. This means that, when lifting arguments, LiftCoverage.noLift must return false for all non-erased Apply:

override def noLift(expr: tpd.Tree)(using Context) =
  if liftingArgs then noLiftArg(expr) else super.noLift(expr)

private def noLiftArg(arg: tpd.Tree)(using Context): Boolean =
  arg match
    case a: tpd.Apply => a.symbol.is(Erased) // don't lift erased applications, but lift all others
    // the other cases recurse on more complex trees that may contain applications

Similarly, string interpolators s, f and raw shouldn't be lifted. I've excluded those three functions from lifting in InstrumentCoverage.needsLift.

Finally, I took a look at the mysterious issue #15078.
For a call to this java method:

Path createTempDirectory(String prefix, FileAttribute<?>... attrs)

-Ycheck-all reported the following problem:

java.lang.AssertionError: assertion failed: Found:    ($2$ : java.nio.file.attribute.FileAttribute[?]*)
Required: Array[? <: java.nio.file.attribute.FileAttribute[?]]

And the code crashed at runtime...

Yet again, it only happened when the arguments were lifted by -coverage-out. A quick minimization revealed that the problem was the handling of the java varargs, in the presence of lifted arguments.

I know that ElimRepeated is the phase that takes care of handling varargs. Some debugging showed that ElimRepeated.transformApply was triggered, with:

  • isWildcardStarArg(arg): true
  • arg of type Ident

isWildcardStarArg properly handled Ident, but not ElimRepeated, which assumed that everything was a Typed tree. Fixing that made the bug vanish.

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!

@smarter
Copy link
Member

smarter commented Jul 2, 2022

Looks like the ci is failing with a coverage test check file mismatch

@smarter smarter assigned TheElectronWill and unassigned smarter Jul 2, 2022
@TheElectronWill
Copy link
Contributor Author

Ah yes, it must be because of #15504. I'll update the checkfiles

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.

LGTM! If you have the time, I think this is something that would be worth backporting to the release-3.2.0 branch too (and #15504 too presumably?)

@smarter smarter merged commit 3a80e3a into scala:main Jul 2, 2022
@TheElectronWill
Copy link
Contributor Author

LGTM! If you have the time, I think this is something that would be worth backporting to the release-3.2.0 branch too (and #15504 too presumably?)

Sure! Here you are: #15573 :)

@Kordyjan Kordyjan added this to the 3.2.0 backports milestone Jul 4, 2022
@TheElectronWill TheElectronWill added the backport:done This PR was successfully backported. label Jul 25, 2022
@Kordyjan Kordyjan modified the milestones: 3.2.0 backports, 3.2.1 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:done This PR was successfully backported. how-i-fixed-it
Projects
None yet
3 participants