-
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
Add filter for duplicates of finally blocks #604
Conversation
fb9d8d7
to
d426e84
Compare
@@ -44,7 +44,7 @@ public void test() { | |||
// without filter next line has branches: | |||
assertLine("test.close", ICounter.EMPTY); | |||
assertLine("test.catch", ICounter.NOT_COVERED); | |||
assertLine("test.finally", ICounter.PARTLY_COVERED); | |||
assertLine("test.finally", ICounter.FULLY_COVERED); |
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.
@Godin Just out of curiosity: The new FinallyFilter now also applies to tryWithResouces?
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.
@marchof javac
translates try-with-resources
into try-catch-finally
, so for it for sure applies. However most likely not applies for bytecode that ecj
generates for closing of resources in try-with-resources
. But applies for try-with-resources
with finally
for both ecj
and javac
as this test shows. Can add more precise tests about this, but definitely would prefer to do this in a separate pull request.
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.
@Godin Ok, makes sense. I was wondering whether our validadtion tests for filters should only use the filter under test.
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.
@marchof we already discussed this: for start it is very good to see combined behavior - this way with low investment we observe what end-user will observe and we observe that filters correctly cooperate and do not interfere with each other in a bad way. However I agree that in long run ability to execute filters separately will be also valuable.
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.
@Godin Sure. For I don't want to change test strategy.
import static org.jacoco.core.test.validation.targets.Stubs.f; | ||
import static org.jacoco.core.test.validation.targets.Stubs.nop; | ||
|
||
public class Finally { |
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.
@Godin Can we add a nested finally here to see how it works?
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.
@marchof done
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.
@Godin Thanks! ✅
|
||
if (t != finallyBlock && t.start == finallyBlock.start | ||
&& t.end == finallyBlock.end) { | ||
final AbstractInsnNode i = next(next(t.handler)); |
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.
@Godin Potential NPE? next(t.handler)
could possibly return null
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.
@marchof t.handler
is a label that must have instruction after it, so next(t.handler)
can't be null
.
next(next(t.handler))
potentially can be null
only if handler has exactly one instruction that is the last one in method, but in this case safe to call merge(size, e, null)
that safely calls isSame(size, e, null)
and returns.
} while (!(Opcodes.ALOAD == i.getOpcode() | ||
&& var == ((VarInsnNode) i).var)); | ||
i = next(i); | ||
if (Opcodes.ATHROW != i.getOpcode()) { |
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.
@Godin Potential NPE? i
could be null
here -- but only for invalid class files. So probably not an issue.
try { | ||
nop(); | ||
} finally { // $line-alwaysCompletesAbruptly.0$ | ||
return; // $line-alwaysCompletesAbruptly.1$ |
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.
@Godin Here I get a warning in Eclipse, maybe annotate method with @SuppressWarnings("finally")
?
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.
@marchof added this suppression, however warnings and suppressions are incredibly inconsistent in IDEs, e.g. even after addition I see warnings in this target file and in some other target files in IntelliJ. Ideally, I think, that our main code should not contain warnings and suppressions, but should be possible to write whatever in targets without bothering with warnings/suppressions.
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.
@Godin We could simply add @SuppressWarnings("all")
to our validation targets. Should work for all IDEs.
@Godin Looks like the latest update broke some tests. |
} | ||
} | ||
|
||
private static class Matcher { |
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.
@Godin The only state in Matcher
class is member inside
, which is used only locally within method filter()
. Therefore should be better al local variable within this method. Then the Matcher
class can be removed at all. Tried this here: https://gist.github.com/marchof/5adb1930188136c3ea340ff430a2cac5
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.
@marchof done
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.
@Godin Thanks! ✅
* </pre> | ||
*/ | ||
@Test | ||
public void should_analyze_control_flow() { |
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.
@Godin While I'm trying to wrap my mind around the filter implementation I was wondering why we need to loop over all tryCatchBlocks with the same handler. This seems to be the only test case which triggers this. Therefore I wonder whether it might make sense to add this scenario also to the validation test.
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.
@marchof done
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.
@Godin When I remove the two loops like this:
// for (final TryCatchBlockNode t : tryCatchBlocks) {
{
final TryCatchBlockNode t = finallyBlock;
the validation tests are still fully green (ECJ) and a different test fails for javac:
emptyCatch(org.jacoco.core.test.filter.FinallyTest): Status in line 79: nop("finally"); // $line-emptyCatch.1$ expected:<[FUL]LY_COVERED> but was:<[PART]LY_COVERED>
But new test twoRegions()
stays green. What am I missing here?
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.
@marchof after this modification of both loops
emptyCatch
fails because condition ofif
statement in second loop alwaysfalse
- unit test for the case of
twoRegions
shows that merged too much, but validation test has assertion only forfinal
, for same reasonecj
stays green
Missing assertions added.
Also this test demonstrates https://bugs.openjdk.java.net/browse/JDK-7008643
try { | ||
nop("try"); | ||
} catch (Exception e) { // $line-catchNotExecuted$ | ||
nop("catch"); |
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.
@Godin Add assertion that this line is NOT_COVERED
? To ensure that no finally code sneaks in here.
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.
@marchof done
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.
@Godin Thanks! ✅
Ouch, forgot to update test that studies placement of |
e79a4f7
to
2449e7d
Compare
private static final boolean isJDK8u152; | ||
|
||
static { | ||
final Matcher m = Pattern.compile("1\\.8\\.0_(\\d++)(-ea)?") |
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.
@Godin Deletion leads to unused imports warning for java.util.regex.*
.
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.
@marchof fixed
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.
@Godin Thx! ✅
private Set<Integer> toIndexes(Set<AbstractInsnNode> set) { | ||
final Set<Integer> result = new HashSet<Integer>(); | ||
for (final AbstractInsnNode i : set) { | ||
result.add(m.instructions.indexOf(i)); |
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.
@Godin Eclipse shows a boxing warning here.
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.
@marchof if replaced by Integer.valueOf(m.instructions.indexOf(i)))
, then no warning in Eclipse, but warning in IntelliJ with suggestion to remove unnecessary boxing, so added @SuppressWarnings("boxing")
to this method
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.
@Godin Ok. But should give you many warnings at other places then in IntelliJ? ✅
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.
@marchof yes, I have quite some warnings in other places in IntelliJ, but we can put this aside
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.
@Godin Loods good to me know from a formal point of view! However I had a very hard time to understand the implementation.
Maybe the class comment can contain the description of the basic structures created by finally and our merge/ignore merge strategy.
Also maybe some inline comments, like:
- why looping over all handlers with same target?
- what exactly does the large while/switch block?
@marchof description and comments added, could you please re-review? If all fine, I'll squash and merge. |
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.
Thanks @Godin for the great explanation!
Finally filter for
finally
, hopefully finally final version 😆