Make coverage more similar to the one in Scala 2 #23722
Conversation
jchyb
force-pushed
the
fix-coverage-s2-s3
branch
from
fb9a3c7 to
da43ab4
Compare
| @@ -145,6 +145,29 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer: | |||
| val span = pos.span.toSynthetic | |||
| invokeCall(statementId, span) | |||
|
|
|||
| private def transformApplyArgs(trees: List[Tree])(using Context): List[Tree] = | |||
| if (allConstArgs(trees)) trees else transform(trees) | |||
There was a problem hiding this comment.
nit: chould you change to the new if-then-else syntax?
| private def transformInnerApply(tree: Tree)(using Context): Tree = tree match | ||
| case a: Apply if a.fun.symbol == defn.StringContextModule_apply => | ||
| a | ||
| case a: Apply => | ||
| cpy.Apply(a)( | ||
| transformInnerApply(a.fun), | ||
| transformApplyArgs(a.args) | ||
| ) | ||
| case a: TypeApply => | ||
| cpy.TypeApply(a)( | ||
| transformInnerApply(a.fun), | ||
| transformApplyArgs(a.args) | ||
| ) | ||
| case s: Select => | ||
| cpy.Select(s)(transformInnerApply(s.qualifier), s.name) | ||
| case i: (Ident | This) => i | ||
| case other => transform(other) |
There was a problem hiding this comment.
Question: I don't fully understand the motivation for this. Could you give a motivating code example that shows the intended coverage change?
Is it simply something of the shape: f(args1)(args2)(args3)(args4)? In that case, can something like a Typed node cause the logic to run away from this function into the main transform function?
There was a problem hiding this comment.
Yes, its so that f(args1)(args2)(args3)(args4), f(args1)(args2)(args3), f(args1)(args2) f(args1), f don't all get redundantly tagged (the args will still be, if they are not literals). The main annoyance was with math/logic operators, like for a + b + c, where a+b would also get redundantly tagged. This will not defend against something like a + (b + c), where the second apply node is in the args field - but that is also precisely how it works in scala 2.
I tested the Typed example now (something like (f(0): (Int => Unit))(1)), and it surprisingly manages to ignore the prefixing Apply in Scala 2, but not in this PR, so I'll will fix this now - thank you for the catch!
* remove coverage of inlined nodes (as mentioned in the accompanying comment, those are impossible to represent in most cases) * add coverage for Literals (ones directly in Apply are omitted) * remove coverage of `throw` contents * if apply node is tagged, do not tag it's prefix, outside of other prefixing Apply's arguments (eg. when we tag `a+b+c` we do not redundantly tag `a+b`) * allow instrumenting synthetic method calls (like apply of a case class) Also fixes issue with certain generated Block nodes not having assigned the correct type
jchyb
force-pushed
the
fix-coverage-s2-s3
branch
from
da43ab4 to
2c479e1
Compare
Closes #21877
throwcontentsa+b+cwe do not redundantly taga+b)After all of these changes the statements tagged are much more similar to Scala 2, let's look at the #21877 minimisation:
There are some differences still remaining, most notably the tagging the DefDefs and its default parameters, but I left them for now, as those seem more useful than harmful.
BEcouse of those changed most of the .covergae files had to be regenerated, however I want through each and every diff to make sure that all of those changes there are expected.
Additionally, this PR also fixes #21695 (issue with certain generated Block nodes not having assigned the correct type, causing later undefined errors).