Class compiled with JDK:

private void implicitExceptionAfterForLoop();
 0  iconst_0
 1  istore_1 [i]
 2  iload_1 [i]
 3  iconst_3
 4  if_icmpge 16
 7  invokestatic org.jacoco.core.test.validation.targets.Stubs.nop() : void [13]
10  iinc 1 1 [i]
13  goto 2
16  invokestatic org.jacoco.core.test.validation.targets.Stubs.ex() : void [14]
19  return
  Line numbers:
    [pc: 0, line: 136]
    [pc: 7, line: 137]
    [pc: 10, line: 136]
    [pc: 16, line: 139]
    [pc: 19, line: 140]
  Local variable table:
    [pc: 2, pc: 16] local: i index: 1 type: int
    [pc: 0, pc: 20] local: this index: 0 type: org.jacoco.core.test.validation.targets.Target03

Class compiled with Eclipse:

private void implicitExceptionAfterForLoop();
 0  iconst_0
 1  istore_1 [i]
 2  goto 11
 5  invokestatic org.jacoco.core.test.validation.targets.Stubs.nop() : void [49]
 8  iinc 1 1 [i]
11  iload_1 [i]
12  iconst_3
13  if_icmplt 5
16  invokestatic org.jacoco.core.test.validation.targets.Stubs.ex() : void [54]
19  return
  Line numbers:
    [pc: 0, line: 136]
    [pc: 5, line: 137]
    [pc: 8, line: 136]
    [pc: 16, line: 139]
    [pc: 19, line: 140]
  Local variable table:
    [pc: 0, pc: 20] local: this index: 0 type: org.jacoco.core.test.validation.targets.Target03
    [pc: 2, pc: 16] local: i index: 1 type: int

Reproducer is here: https://gist.github.com/marchof/c6f1b18c371f6e69622c

@marchof should we include this in 0.7.6 since seems you already started work on it?

@Godin Let's try -- though not sure whether I'll find a proper solution.

Another reproducer shows the problem with ECJ as well as with javac. No branches are shown as covered for the if statement:

private void implicitExceptionAfterBranch() {
    if (f()) { // $line-implicitExceptionAfterBranch.if$
        return; // $line-implicitExceptionAfterBranch.ifbody$
    }
    ex(); // $line-implicitExceptionAfterBranch.exception$
}

I suppose that this is caused by missing probe before ex() in both cases.

@Godin It is. The situation is tricky here as beside the label for the new line we have another label for the control flow.

@marchof do you mean that fix, which I attached to this thread, is incomplete? if so, wondering which test should be added to reveal this?

So @marchof , what do you think about the fix proposed here?

@Godin Can you please do the following sanity check: What is the total number of probes e.g. in org.jacoco.core without and with this patch? Should be a small increase only.

If we fix this in any case we need to increment exec file version :-/

@marchof yes - I know that version should be bumped and this is unavoidable evil, with dramatic consequences for some of consumers of exec files, such as Jenkins Plugin, last time when we did this there were serious impacts. And that's why I'd like to be sure that we have a fix prior to exploring what needs to be done to actually promote this fix.

@Godin Maybe we prepare the fix and let it sit here until we have another reason for a breaking change.

Sidenote: For the filtering story I would consider reworking the whole probe story from the event API to the tree API. In the meantime we materialize a tree anyways and this might reduce complexity dramatically. On the other hand this might cause slight changes to probe positions/enumeration.

@marchof I was thinking more about strategy: prepare the fix and let it sit here until we help downstream projects to deal with multiple JaCoCo versions - on example of Jenkins plugin this needs to be done in case of any change of exec-file version independently of underlying reasons for it, becase can't be expected that all projects hosted on a single Jenkins instance will upgrade version of JaCoCo synchronously, meaning that either users wouldn't use new JaCoCo version, either won't have their reports in Jenkins.

For org.jacoco.core-0.7.7.201606060606.jar: without this change 1756 probes, 1820 with.

So do you think that we have a fix here? If so, I'll add item about help for downstream projects into my todo-list.

@Godin Yep, I think this is a valid fix. A increase of 3,5% in probes probably reflects exactly the cases we missed before. So we only need to fix your TODO comments.

Regarding exec file compatibility: I always tried to make clear that exec files are a temporary storage facility that make no guarantees about compatibility. It's an internal, undocumented binary format. If you want to integrate against a stable and documented file format, the XML report is currently the only format we can offer. We might add new formats in future, or even include analysis in the agent when data is dumped. But maintaining code that supports multiple exec file versions does not look very scalable to me. Beside exec file versions another common problem of our users is that we also need exact same class files available during analysis.

@marchof yup, I have no doubts that content of exec-file should be considered as internal implementation details without guarantees about compatibility, even if I added some oil into the fire of its usage by relying on it in first version of SonarQube plugin several years ago when JaCoCo was just born. But past is past, author of Jenkins Plugin would also love to revert time back and base plugin on XML - jenkinsci/jacoco-plugin#55 (comment) , but unfortunately in a real world that's not that easy, someone should anyway pay back and deal with current state of the things.

Having said all this, I'm not saying that we should provide this layer of compatibility. In fact it is not that hard to do on their side - few packaging tricks allow SonarQube plugin to deal with two versions of exec-file at the same time.

@marchof Forgot to say in previous comment: even if at the root Jenkins / SonarQube plugins abuse reading exec-file, people would blindly blame JaCoCo. That's why IMO it is in our interests to help downstream projects to find one or another way thru this.

We implemented loading multiple versions of JaCoCo in our internal tooling so we can handle multiple versions of .exec files because it's difficult to coordinate upgrades for many teams at the same time. Loading multiple versions of JaCoCo generally works ok for us, but it doesn't work as well when features are needed from later JaCoCo versions (e.g., bug fixes or Java 9 support) while maintaining .exec file compatibility. It would be ideal for us if JaCoCo were somehow factored so that we could vary the module that deals with .exec file format but stay at the latest for all other code. I guess that would require JaCoCo to have a stable internal interface to the code that handles the .exec file (or at least fallback gracefully when interacting with an older version of it), so I'm not sure that's practical or maintainable, but I thought I'd share our experience.

Looks like that there is one more case that doesn't work properly even with those changes! Reproducer shows the problem with ECJ as well as with javac - no branches are shown as covered for the switch statement:

private static void implicitExceptionInSwitchCase() {
	switch (i2()) { // $line-implicitExceptionInSwitchCase.switch$
		case 2:
			ex(); // $line-implicitExceptionInSwitchCase.exception$
			break;
		default:
			nop();
			break;
	}
}

For the clarity: we had the same behavior for all these cases in version 0.7.4 before implementation of #310 in version 0.7.5, so this is not about regression - simply additional cases that were missed in #310.

For my own education, can you please confirm if incomplete coverage of Groovy assert statements is also a consequence of this issue?