Exclude from a report a part of bytecode that compiler generates for a try-with-resources #500

Godin · 2017-03-16T23:31:33Z

https://github.com/jacoco/jacoco/wiki/filtering-JAVAC.TRYWITH

marchof · 2017-03-17T15:06:14Z

@Godin Count me deeply impressed!!! And I'm very sorry that I couldn't immediately jump into both PRs. So+Mo are travel dates, so I hope to study it in detail then.

Godin · 2017-03-17T15:18:01Z

@marchof no worry, will be cool to get all this on board, but no need to rush - let's do a proper careful review and testing. Slowly, but at least we are progressing on this big subject. And I personally start to see the light at the end of the tunnel, which wasn't the case few months ago.

FYI next week I'm also kind of traveling - will attend DevoxxUS, and hope to not be offline all the time.

bjkail · 2017-03-17T16:31:01Z

@Godin It would be useful if the members of Pattern had high-level descriptions of the code being matched and which conditions cause the compiler to generate that pattern. It would be good to have a test of t-w-r with custom finally block (try (...) { ... } finally { nop() }) to be sure it doesn't cause problems for the analyses. It would also be good to have tests with nested t-w-r nested in the try, catch, and finally blocks (e.g., try (...) { ... } finally { try (...) { ... } catch (...) { ... } finally { ... } }, etc.).

Godin · 2017-03-17T16:57:26Z

@bjkail Hey, glad to see you here! Actually was about to ask if you're interested in this subject and if so maybe you could leave some valuable comments as before 😉 And here they are 😋

You are right on all points - let's cleanup and harden this PoC. Also wondering WDTY about approach in general? About it we'd better add explanatory comments also. As well as estimate impact on performance of analysis - can imagine some optimizations... but traded them on the correctness and attempt to keep relative simplicity of code for now.

Anyway thank you! we really appreciate your help 👍

bjkail · 2017-03-17T20:52:34Z

@Godin The overall approach seems reasonable to me of skipping ignored instructions in MethodAnalyzer.visitEnd and matching bytecode sequences in TryWithResourcesMatcher.

Last time I thought about t-w-r, I was thinking to require the t-w-r close sequence be replicated at the end of the try block and each catch block (including Throwable) to ensure it was either written by a compiler or fully correctly hand-written. My motivation was to avoid filtering out a badly hand-written close sequence (e.g., at the end of try block only but not all catch blocks or missing Throwable block, etc.), but the algorithm to do the rigid matching seemed quite complex, especially for custom finally blocks and nested t-w-r (thus my suggested testcases). However, I am now thinking your approach is fine. There is some risk of false positive (matching a hand-written close sequence), but the risk is low since the close sequence is relatively complex, and we can leave detecting badly hand-written close sequences (like missing close sequence on some possible code path) to static analyzers like FindBugs.

bjkail · 2017-03-17T21:21:40Z

As an anecdote, last time I investigated t-w-r was because we kept regressing coverage in classes that use t-w-r heavily because drops go unnoticed due to all the uncovered generated code. I tried forcing all the generate code paths to be executed (our code/tests made it easy to force all the conditions: null resource, exception with null resource, exception with non-null resource, close exception, exception and close exception), but I ultimately gave up when I discovered javac emits completely dead code:

if (#primaryExc) != null) is always false in the try block and is always true in the catch blocks.
if (Identifier != null) can be always false in the try block. JDK-7020499 improves that for Java 9, but I wish it also had "effectively non-null" analysis like the "effectively final" analysis that would elide the check.

So, I think this will be a great feature to have in JaCoCo :-).

Godin · 2017-04-02T22:26:52Z

@marchof

Regarding your idea with tests on the Maven repo: For many artifacts there are source attachements. How hard would it be to identify try-with-resource statements on source level (e.g. with Sonar language tools)?

Indeed - I forgot that 3.8 Gb of 6865 JARs in my local Maven repository also includes some JARs with sources. However sources are not downloaded by default, so not that much: 309 artifacts with sources, their corresponding JARs with classes account for 125 Mb, and diversity also maybe not that big - see log.txt

So taking this a step further one could think of a test where for every method found in the Maven repo a specific language construct is detected on source level. The corresponding bytecode filter should come to the same result. That would be a crazy integration test for the filters...

ECJ filter is not triggered by any of the above scanned JARs. In case of javac first instruction of a first ignored block is a line number corresponding to try keyword and matches to 174 try keywords of try-with-resources parsed from sources, 14 582 of normal try statements are skipped by filters.

Doing the same test using rt.jar and src.zip of JDK 7u80 and JDK 8u121: matched correctly 52 try keywords of try-with-resources and correctly skipped 13102 of normal try statements.

For bigger set of artifacts with sources will need to think about how to obtain them effectively: don't know how many sources is stored in cache in our corporate proxy as they are not downloaded by default; and anyway will need to think about how to get list no matter from where to download - central or proxy.

Godin · 2017-04-04T08:39:05Z

org.jacoco.core.test/src-java7/org/jacoco/core/test/filter/TryWithResourcesTest.java

+		assertLine("returnInBody.open", ICounter.FULLY_COVERED);
+		// without filter next line has branches:
+		// TODO with javac 1.8.0_31 part of return instructions belong to close
+		// assertLine("returnInBody.close", ICounter.EMPTY);


In case of

private static Object returnInBody() throws IOException { try ( // $line-returnInBody.try$ Closeable r = new Resource() // $line-returnInBody.open$ ) { return read(r); // $line-returnInBody.return$ } // $line-returnInBody.close$ }

we correctly find instructions, but javac 7 as well as javac 8 up to 8u77 (tested public updates) generates:

LINENUMBER $line-returnInBody.return$ ... ASTORE LINENUMBER $line-returnInBody.close$ ... // close resource ALOAD ARETURN LINENUMBER $line-returnInBody.try$ ... // save exception LINENUMBER $line-returnInBody.close$ ... // close resource

while javac starting from 8u92 (notice additional LINENUMBER before ALOAD) as well as latest 9 EA:

LINENUMBER $line-returnInBody.return$ ... ASTORE LINENUMBER $line-returnInBody.close$ ... // close resource LINENUMBER $line-returnInBody.return$ ALOAD ARETURN LINENUMBER $line-returnInBody.try$ ... // save exception LINENUMBER $line-returnInBody.close$ ... // close resource

Looks like this relates to JDK-8134759 (log of bisect - bisect.txt)

There is no such problem when finally is present, but this case is covered by another existing test.

So I'm wondering what can we do with this test?

Travis provides JDK 8u31 by default and we also test JDK 8 EA. Can imagine that for the bytecode version 7 generated by javac we will be testing old behavior, while will be testing and using updated stable JDK 8 in Travis as well as locally.

But maybe @marchof you have some other thoughts/ideas?

@Godin Parse java.version, and then skip or adjust the asserts? We have precedent in BadCycleInterfaceTest.

@bjkail java.version is about version of JRE and BadCycleInterfaceTest is exactly about runtime behavior. This will work here if we assume that execution and compilation of tests use the same JDK version.

@Godin You're right. Is that too fragile? If so, the only other option that comes to mind is to use ASM to detect the missing line number. That would weaken the test a bit, but perhaps it's better than nothing?

@Godin @bjkail Maybe our integration tests for filtering are too specific. What if we just assert that our filtering examples do not produce any uncovered lines and branches? We already have plenty of validation test for expected covered/missed lines.

@marchof Excuse me for the delay with reply - was at DevoxFR, where BTW session "Java Code Coverage Mechics" was a quite success.

On a subject:

@bjkail java.version maybe not so fragile.

ASM to detect the missing line number

is fragile in terms that it allows compiler to regress on this.

@marchof

Maybe our integration tests for filtering are too specific.

I personally like such specificity for the following reasons

not all examples here reach full coverage (no uncovered lines and branches) and also IMO will not reach in case of future filters

consistency across tests

allows to see exactly what user will see

allows to catch regressions in compilers

We already have plenty of validation test for expected covered/missed lines

Apparently not for such tricky control flows as shown here 😉

Do you see any troubles to simply bump version of javac 8 up to latest update as in my initial proposal - "past is past"?

@Godin We have a large user base out there where older JDK versions still might be in use. The primary purpose of the integration tests for filters should be to ensure that the filters fulfill their purpose. I would imagine filter integration tests consisting of source code only (maybe with annotated lines for the intentionally un-covered ones).

My primary concern is maintainability. If you think we can handle the specific checks in the long run, I can live with the current approach.

@marchof

We have a large user base out there where older JDK versions still might be in use.

I haven't said that we should stop testing all old JDK versions - only range of JDK 8 till 8u92.
JDK 7 will be testing the same bytecode that generates JDK 8 from this range. And this range was shown to be working with other existing test cases.

I wish that we run tests against range of all public updates of all supported JDK versions, but this is quite overkill and we are not doing this anyway 😉

Some other options:

version of compiler can be passed by Maven into tests, but then such conditional assertions should be disabled in IDEs

tests can perform execution of compiler so that they will precisely know version independently from build system and IDEs

But all this quite out of scope of this PR. So for now I excluded assertion on this line only for bytecode version 8 - this is at least better that current TODO-comment and we can come back to this discussion later.

excluded assertion on this line only for bytecode version 8

arr, in fact even this is fragile as one can ask generation of bytecode version 7 or 8 using JDK 9

Experimented with this over past weeks and few times experienced fragility on
mvn clean install -Dbytecode.version=1.7 using JDK 8, so finally in favor of @bjkail suggestion to assume that execution and compilation of tests is done with same JDK.

✅

Godin · 2017-04-20T17:06:59Z

org.jacoco.core/src/org/jacoco/core/internal/analysis/filter/TryWithResourcesJavacFilter.java

+	 * initialized using new
+	 * synthetic method $closeResource containing

            
+	 * null check of primaryExc and calls to methods
+	 * addSuppressed and close is used when numbers of


typo - numbers instead of number

Godin · 2017-04-21T19:25:39Z

@marchof I scanned bigger subset of Maven Central - latest versions of some artifacts within group org. This was not easy, because not counting time that were spent to download - some jars with sources should be excluded as they are incomplete, contain more than jars with classes, contain templates for generation of sources, etc...

Nevertheless 9753 try-with-resources statements were filtered correctly from ≈20 Gb of JAR files with classes. And this time ECJ filter was triggered quite a good amount of times.

Case that was already presented in our tests and not filtered - when body of try-with-resources is empty

try (Resource r = new Resource()) {
}

was encountered in

org/apache/cassandra/cassandra-all/3.10
org/apache/cayenne/modeler/cayenne-modeler/4.0.M5
org/apache/reef/reef-runtime-hdinsight/0.15.0
org/apache/tomcat/embed/tomcat-embed-jasper/9.0.0.M19
org/apache/tomcat/tomcat-jasper/9.0.0.M19
org/apache/solr/solr-core/6.5.0
org/eclipse/scout/sdk/deps/org.eclipse.jdt.launching/3.8.100.v20160505-0636
org/integratedmodelling/klab-common/0.9.10
org/jboss/arquillian/container/arquillian-gae-common/1.0.0.Beta10
org/jboss/cdi/tck/cdi-tck-impl/2.0.0.CR1
org/neo4j/app/neo4j-server/3.2.0-alpha07
org/neo4j/neo4j-ha/3.2.0-alpha07
org/neo4j/neo4j-ogm/1.1.6
org/sonatype/nexus/bundles/org.sonatype.nexus.bundles.elasticsearch/3.2.1-01
org/sonarsource/sonarqube/sonar-process/6.3
org/simplericity/jettyconsole/jetty-console-core/1.60
org/vesalainen/comm/1.0.3
org/qi4j/core/org.qi4j.core.testsupport/2.1

Another funny case that was already presented in our tests and not filtered - when try-with-resources contains only throw statement

try (Resource r = new Resource()) {
  throw new Exception();
}

was encountered in

org/apache/nifi/nifi-framework-core/1.1.2
org/apache/reef/reef-common/0.15.0
org/codelibs/fess/fess-crawler/1.1.0
org/tentackle/tentackle-persistence/2.0.7
org/sonatype/nexus/bundles/org.sonatype.nexus.bundles.elasticsearch/3.2.1-01

Not explicitly presented in out tests, but a kind of variation of previous and not filtered - when there is no non-exceptional path from a body as for example

try (Resource r = new Resource()) {
  while (true) {
    read(r);
  }
} catch (EOFException e) {
}

was encountered in

org/apache/cassandra/cassandra-all/3.10
org/apache/fluo/fluo-accumulo/1.0.0-incubating
org/github/evenjn/knit/0.6.0
org/infinispan/infinispan-embedded/9.0.0.Final
org/jgroups/jgroups/4.0.1.Final
org/kohsuke/elb-dns/1.1

An interesting case where source code contains try-with-resources, but retrolambda removes call to Throwable.addSuppressed and so not filtered, was encountered in

org/ansj/ansj_seg/5.1.1

An interesting case where AspectJ adds try-with-resources into bytecode was encountered in

org/no-hope/aspectj/metrics-aspectj-el/1.7

All in all: I satisfied by results and would like to merge this PR. We can deal with cases of an empty body and absence of non-exceptional path in a separate PR. If you agree I'll squash and merge into master.

marchof · 2017-04-22T05:16:18Z

@Godin Awesome work, Evgeny! I'm absolutly curious about the exact test setup. And yes, we can add corner cases later.

I was already about to ask about the performance of the new, way more readable matching code. As there is no negative impact -> yes, please merge this!

romani · 2017-06-29T17:34:48Z

@Godin , when do you plan to release this ?
Checkstyle project plan to give jacoco another try this summer, this feature could be a good stimulus to switch to jacoco for coverage.
jacoco is already 5month without release with bunch of good fixes.

Godin · 2017-06-29T17:45:13Z

@romani please see already existing answers on such question:
https://groups.google.com/forum/#!msg/jacoco/gd8xD30TDNo/SHj-hEq1AAAJ
https://groups.google.com/forum/#!msg/jacoco/-MmsVzC6szM/Q9sPTbAeBAAJ

Godin added component: core.filters type: enhancement labels Mar 16, 2017

Godin force-pushed the issue-500 branch from 0d7a711 to cea277a Compare March 16, 2017 23:39

Godin self-assigned this Mar 16, 2017

Godin requested a review from marchof March 16, 2017 23:59

Godin mentioned this pull request Mar 17, 2017

Exclude from a report a part of bytecode that ECJ generates for a String in switch statement and that is hard to cover by tests #496

Closed

Godin force-pushed the issue-500 branch 16 times, most recently from 50d98bc to 1adc2d1 Compare March 25, 2017 15:55

Godin added this to the 0.7.10 milestone Mar 25, 2017

Godin force-pushed the issue-500 branch from 1adc2d1 to dc69244 Compare March 25, 2017 16:14

Godin commented Apr 4, 2017

View reviewed changes

Godin force-pushed the issue-500 branch from f2dec3c to 96bc5d4 Compare April 4, 2017 08:58

Merge branch 'master'

96bc5d4

Godin mentioned this pull request Apr 10, 2017

Exclude methods annotated with @lombok.Generated #513

Merged

marchof approved these changes Apr 11, 2017

View reviewed changes

Godin force-pushed the issue-500 branch from edff741 to 79cacf3 Compare April 11, 2017 22:25

(wip) get rid of TODO in test

df07579

Godin force-pushed the issue-500 branch from 79cacf3 to df07579 Compare April 11, 2017 22:50

Godin added 2 commits April 12, 2017 00:51

(wip) get rid of TODO

2240661

(wip) add comments to TryWithResourcesEcjFilterTest

2929ed7

Godin force-pushed the issue-500 branch from fd9bb95 to 2929ed7 Compare April 12, 2017 19:15

Godin added 3 commits April 12, 2017 22:38

(wip) add comments to TryWithResourcesJavacFilterTest

c9b3c28

(wip) add comments to TryWithResourcesJavacFilter

020a850

(wip) make more declarative

61cf25f

Godin force-pushed the issue-500 branch from a3a9f9a to 61cf25f Compare April 13, 2017 13:13

(wip) add missing unit test

5e622fb

Godin commented Apr 20, 2017

View reviewed changes

Godin added 3 commits April 21, 2017 19:30

(wip) fix typo

da3b6e3

(wip) make test less fragile

297c45d

(wip) improve test

e29ec72

Godin merged commit e93053e into master Apr 22, 2017

Godin deleted the issue-500 branch April 22, 2017 10:39

fdaugan mentioned this pull request Nov 5, 2017

Update "jacoco 0.7.10" before "commons-io 2.6" ligoj/bootstrap#8

Closed

jacoco locked as resolved and limited conversation to collaborators Jan 11, 2018

Godin added the language: Java label Oct 12, 2023

Exclude from a report a part of bytecode that compiler generates for a try-with-resources #500

Exclude from a report a part of bytecode that compiler generates for a try-with-resources #500

Uh oh!

Conversation

Godin commented Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marchof commented Mar 17, 2017

Uh oh!

Godin commented Mar 17, 2017

Uh oh!

bjkail commented Mar 17, 2017

Uh oh!

Godin commented Mar 17, 2017

Uh oh!

bjkail commented Mar 17, 2017

Uh oh!

bjkail commented Mar 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Godin commented Apr 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjkail Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Godin Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Godin Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Godin commented Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marchof commented Apr 22, 2017

Uh oh!

romani commented Jun 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Godin commented Jun 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Godin commented Mar 16, 2017 •

edited

Loading

bjkail commented Mar 17, 2017 •

edited

Loading

Godin commented Apr 2, 2017 •

edited

Loading

bjkail Apr 4, 2017 •

edited

Loading

Godin Apr 11, 2017 •

edited

Loading

Godin Apr 21, 2017 •

edited

Loading

Godin commented Apr 21, 2017 •

edited

Loading

romani commented Jun 29, 2017 •

edited

Loading

Godin commented Jun 29, 2017 •

edited

Loading