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

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

Merged
merged 27 commits into from Apr 22, 2017

Conversation

@Godin
Member

Godin commented Mar 16, 2017

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Mar 17, 2017

Member

@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.

Member

marchof commented Mar 17, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Mar 17, 2017

Member

@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.

Member

Godin commented Mar 17, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@bjkail

bjkail Mar 17, 2017

Contributor

@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.).

Contributor

bjkail commented Mar 17, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Mar 17, 2017

Member

@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 👍

Member

Godin commented Mar 17, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@bjkail

bjkail Mar 17, 2017

Contributor

@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.

Contributor

bjkail commented Mar 17, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@bjkail

bjkail Mar 17, 2017

Contributor

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 :-).

Contributor

bjkail commented Mar 17, 2017

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 Godin added this to the 0.7.10 milestone Mar 25, 2017

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Mar 27, 2017

Member

@marchof I made functional fixes here compared to initial version, as well as introduced packages/interfaces as was discussed in #496 (comment) similarly to what was done in #501. As well as made changes to improve performance of this filter - analysis of 17391 classes from rt.jar (≈ 65 Mb) from JDK 1.8.0_121 using following build.xml

<project xmlns:jacoco="antlib:org.jacoco.ant" default="report">
  <taskdef uri="antlib:org.jacoco.ant" resource="org/jacoco/ant/antlib.xml">
    <classpath path="jacocoant.jar"/>
  </taskdef>
  <target name="report">
    <jacoco:report>
      <executiondata>
        <file file="jacoco.exec"/>
      </executiondata>
      <structure name="rt.jar">
        <classfiles>
          <fileset file="rt.jar"/>
        </classfiles>
      </structure>
    </jacoco:report>
  </target>
</project>

takes 5 seconds on my laptop, both without this change as well as with it, i.e. overhead is not clearly observable.

So all in all - from a functional point of view IMO all done here and will be really great to receive your comments to merge this into master.

Member

Godin commented Mar 27, 2017

@marchof I made functional fixes here compared to initial version, as well as introduced packages/interfaces as was discussed in #496 (comment) similarly to what was done in #501. As well as made changes to improve performance of this filter - analysis of 17391 classes from rt.jar (≈ 65 Mb) from JDK 1.8.0_121 using following build.xml

<project xmlns:jacoco="antlib:org.jacoco.ant" default="report">
  <taskdef uri="antlib:org.jacoco.ant" resource="org/jacoco/ant/antlib.xml">
    <classpath path="jacocoant.jar"/>
  </taskdef>
  <target name="report">
    <jacoco:report>
      <executiondata>
        <file file="jacoco.exec"/>
      </executiondata>
      <structure name="rt.jar">
        <classfiles>
          <fileset file="rt.jar"/>
        </classfiles>
      </structure>
    </jacoco:report>
  </target>
</project>

takes 5 seconds on my laptop, both without this change as well as with it, i.e. overhead is not clearly observable.

So all in all - from a functional point of view IMO all done here and will be really great to receive your comments to merge this into master.

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Mar 27, 2017

Member

@Godin How do you think we should proceed with this series of PRs? My issue is that they all rely on the same new APIs and strategies where I have several comments on like in #501. I propose to clean up #501 first and afterwards base the other PRs on this work.

Alternatively we create a "filtering" branch and integrate all PRs one after the other onto it.

Member

marchof commented Mar 27, 2017

@Godin How do you think we should proceed with this series of PRs? My issue is that they all rely on the same new APIs and strategies where I have several comments on like in #501. I propose to clean up #501 first and afterwards base the other PRs on this work.

Alternatively we create a "filtering" branch and integrate all PRs one after the other onto it.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Mar 27, 2017

Member

@marchof deal - let's clean up #501 first

Member

Godin commented Mar 27, 2017

@marchof deal - let's clean up #501 first

@marchof

Some first formal remarks.

Try to wrap my mind about the actual filter implementation now. At a first glance it looks like we need some better byte code pattern matching abstraction to create maintainable code.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Mar 28, 2017

Member

At a first glance it looks like we need some better byte code pattern matching abstraction to create maintainable code.

@marchof believe me - current version is a way way better than initial monsters ;)

Try to wrap my mind about the actual filter implementation now.

Overall principle is that matching performed sequentially (&&, if (!condition) exit) with a bit of backtracking. And one of key points - is assignment of names, which helped to simplify initial version a lot. Also could be noted and seen on example of two commits here that code/overall algorithm based on those principles is quite recombinable/changeable. Most of the time and headache were spent on generation of test cases and understanding of bytecode and differences in it for different cases.

Member

Godin commented Mar 28, 2017

At a first glance it looks like we need some better byte code pattern matching abstraction to create maintainable code.

@marchof believe me - current version is a way way better than initial monsters ;)

Try to wrap my mind about the actual filter implementation now.

Overall principle is that matching performed sequentially (&&, if (!condition) exit) with a bit of backtracking. And one of key points - is assignment of names, which helped to simplify initial version a lot. Also could be noted and seen on example of two commits here that code/overall algorithm based on those principles is quite recombinable/changeable. Most of the time and headache were spent on generation of test cases and understanding of bytecode and differences in it for different cases.

Godin added some commits Mar 28, 2017

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Mar 29, 2017

Member

@Godin Where can I find the patterns that you where matching for? in the test case? Maybe you can dump a informal description in our wiki: https://github.com/jacoco/jacoco/wiki/filtering-JAVAC.TRYWITH

I think I get the idea how the control flow of matching is implemented. My point is that we mix two complex concepts into one piece of code:

  1. Description of different byte code sequences for different compilers
  2. Pattern matching implementation

To create maintainable code I would prefer to separate some concepts here:

  1. Separate filters for different compilers
  2. Pure pattern declaration in the filter
  3. Generic matcher facility

This should greatly reduce the complexity of the filters. I already started working on this on my early prototype. If you give me a couple of days I can come up with a proposal. Would be helpful though to understand what the specific patterns are you're matching for here (see above).

Member

marchof commented Mar 29, 2017

@Godin Where can I find the patterns that you where matching for? in the test case? Maybe you can dump a informal description in our wiki: https://github.com/jacoco/jacoco/wiki/filtering-JAVAC.TRYWITH

I think I get the idea how the control flow of matching is implemented. My point is that we mix two complex concepts into one piece of code:

  1. Description of different byte code sequences for different compilers
  2. Pattern matching implementation

To create maintainable code I would prefer to separate some concepts here:

  1. Separate filters for different compilers
  2. Pure pattern declaration in the filter
  3. Generic matcher facility

This should greatly reduce the complexity of the filters. I already started working on this on my early prototype. If you give me a couple of days I can come up with a proposal. Would be helpful though to understand what the specific patterns are you're matching for here (see above).

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Mar 29, 2017

Member

@marchof

Where can I find the patterns that you where matching for? in the test case?

Yes - I believe that unit test is exhaustive in terms of patterns.

Maybe you can dump a informal description in our wiki: https://github.com/jacoco/jacoco/wiki/filtering-JAVAC.TRYWITH

I consider wiki as a temporary storage (recall the #496 (comment)) and expect documentation to become part of code (ref to today's tweet from our friend Mickhael Istria 😉) and so indeed - more explanations should be added into this change.

I'll draft some in wiki.

I think I get the idea how the control flow of matching is implemented.

👍

My point is that we mix two complex concepts into one piece of code:

  1. Description of different byte code sequences for different compilers
  2. Pattern matching implementation

That's the purpose of comments in code, isn't it (https://blog.codinghorror.com/code-tells-you-how-comments-tell-you-why/) ? 😉

More seriously - implementation workflow looked like:

  1. generate bytecode for a test (given that in different cases bytecode can be different and that cases are unknown in advance, was not that easy, but mostly matter of time)
  2. understand bytecode by adding comments into test
  3. looking at comments understand patterns
  4. after that implementation is almost one-to-one maps to comments in tests

But indeed some description/overview of patterns is missing here (see above).

To create maintainable code I would prefer to separate some concepts here:

  1. Separate filters for different compilers

They are already separated into different methods. Can extract into different classes with some shared logic.

  1. Pure pattern declaration in the filter
  2. Generic matcher facility

This should greatly reduce the complexity of the filters. I already started working on this on my early prototype.

In fact in the middle of implementation I tried to have pure declarative approach, but code became far more bigger and far less readable.

One of the complications with declarative approach here - is that some part of code uses imperative style inherently (see loop that counts number of resources while matching ECJ bytecode and names of variables that are not constants because represent different resources). Possibility to come up with some nice DSL not excluded, but doubt about ROI.

If you give me a couple of days I can come up with a proposal.

Maybe we can do it vice versa - add descriptions, confirm that this implementation works, merge it and then peacefully work on "generic matching facility"? But no pressure 😉

Member

Godin commented Mar 29, 2017

@marchof

Where can I find the patterns that you where matching for? in the test case?

Yes - I believe that unit test is exhaustive in terms of patterns.

Maybe you can dump a informal description in our wiki: https://github.com/jacoco/jacoco/wiki/filtering-JAVAC.TRYWITH

I consider wiki as a temporary storage (recall the #496 (comment)) and expect documentation to become part of code (ref to today's tweet from our friend Mickhael Istria 😉) and so indeed - more explanations should be added into this change.

I'll draft some in wiki.

I think I get the idea how the control flow of matching is implemented.

👍

My point is that we mix two complex concepts into one piece of code:

  1. Description of different byte code sequences for different compilers
  2. Pattern matching implementation

That's the purpose of comments in code, isn't it (https://blog.codinghorror.com/code-tells-you-how-comments-tell-you-why/) ? 😉

More seriously - implementation workflow looked like:

  1. generate bytecode for a test (given that in different cases bytecode can be different and that cases are unknown in advance, was not that easy, but mostly matter of time)
  2. understand bytecode by adding comments into test
  3. looking at comments understand patterns
  4. after that implementation is almost one-to-one maps to comments in tests

But indeed some description/overview of patterns is missing here (see above).

To create maintainable code I would prefer to separate some concepts here:

  1. Separate filters for different compilers

They are already separated into different methods. Can extract into different classes with some shared logic.

  1. Pure pattern declaration in the filter
  2. Generic matcher facility

This should greatly reduce the complexity of the filters. I already started working on this on my early prototype.

In fact in the middle of implementation I tried to have pure declarative approach, but code became far more bigger and far less readable.

One of the complications with declarative approach here - is that some part of code uses imperative style inherently (see loop that counts number of resources while matching ECJ bytecode and names of variables that are not constants because represent different resources). Possibility to come up with some nice DSL not excluded, but doubt about ROI.

If you give me a couple of days I can come up with a proposal.

Maybe we can do it vice versa - add descriptions, confirm that this implementation works, merge it and then peacefully work on "generic matching facility"? But no pressure 😉

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Mar 29, 2017

Member

@Godin I don't fully agree on all points, so I assume your remark about comments indicates that we have some serious code smells here.

Definitely I think the current filter implementation is hard to maintain and has some "write once" characteristics...

But I can agree on the proposed aproach: We make this work first and look for a declarative pattern matching algorithm later.

Member

marchof commented Mar 29, 2017

@Godin I don't fully agree on all points, so I assume your remark about comments indicates that we have some serious code smells here.

Definitely I think the current filter implementation is hard to maintain and has some "write once" characteristics...

But I can agree on the proposed aproach: We make this work first and look for a declarative pattern matching algorithm later.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Mar 29, 2017

Member

@marchof just for the record: note about comments was not about "code smells", but about complexity of understanding bytecode (i.e. not how code does something, but why), so drafted overview/description on a wiki page - https://github.com/jacoco/jacoco/wiki/filtering-JAVAC.TRYWITH

Member

Godin commented Mar 29, 2017

@marchof just for the record: note about comments was not about "code smells", but about complexity of understanding bytecode (i.e. not how code does something, but why), so drafted overview/description on a wiki page - https://github.com/jacoco/jacoco/wiki/filtering-JAVAC.TRYWITH

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Mar 29, 2017

Member

@marchof as we discussed by phone: test of number of instructions using

import java.io.File;

import org.jacoco.core.analysis.Analyzer;
import org.jacoco.core.analysis.CoverageBuilder;
import org.jacoco.core.analysis.IBundleCoverage;
import org.jacoco.core.data.ExecutionDataStore;

public class CountInstructions {
	public static void main(String[] args) throws Exception {
		final CoverageBuilder coverageBuilder = new CoverageBuilder();
		new Analyzer(new ExecutionDataStore(), coverageBuilder)
				.analyzeAll(new File(args[0]));
		final IBundleCoverage bundle = coverageBuilder.getBundle("");
		System.out.println("Instructions: "
				+ bundle.getInstructionCounter().getTotalCount());

	}
}

As expected - no changes in 5 and 6, decrease in 7 and 8:

rt.jar w/o filter w/ filter
JDK 5u22 3 320 205 3 320 205
JDK 6u45 4 390 154 4 390 154
JDK 7u80 4 760 771 4 758 117
JDK 8u121 4 700 913 4 695 163

Haven't thought about this before: can also use 3.8 Gb of 6865 JARs in my local Maven repository or/and 88135 artifacts from our Maven Central corporate proxy after sorting JARs (classes?) on pre/post 7 😉

Member

Godin commented Mar 29, 2017

@marchof as we discussed by phone: test of number of instructions using

import java.io.File;

import org.jacoco.core.analysis.Analyzer;
import org.jacoco.core.analysis.CoverageBuilder;
import org.jacoco.core.analysis.IBundleCoverage;
import org.jacoco.core.data.ExecutionDataStore;

public class CountInstructions {
	public static void main(String[] args) throws Exception {
		final CoverageBuilder coverageBuilder = new CoverageBuilder();
		new Analyzer(new ExecutionDataStore(), coverageBuilder)
				.analyzeAll(new File(args[0]));
		final IBundleCoverage bundle = coverageBuilder.getBundle("");
		System.out.println("Instructions: "
				+ bundle.getInstructionCounter().getTotalCount());

	}
}

As expected - no changes in 5 and 6, decrease in 7 and 8:

rt.jar w/o filter w/ filter
JDK 5u22 3 320 205 3 320 205
JDK 6u45 4 390 154 4 390 154
JDK 7u80 4 760 771 4 758 117
JDK 8u121 4 700 913 4 695 163

Haven't thought about this before: can also use 3.8 Gb of 6865 JARs in my local Maven repository or/and 88135 artifacts from our Maven Central corporate proxy after sorting JARs (classes?) on pre/post 7 😉

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Apr 2, 2017

Member

@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.

Member

Godin commented Apr 2, 2017

@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 added some commits Apr 21, 2017

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Apr 21, 2017

Member

@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.

Member

Godin commented Apr 21, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Apr 22, 2017

Member

@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!

Member

marchof commented Apr 22, 2017

@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!

@Godin Godin merged commit e93053e into master Apr 22, 2017

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@Godin Godin deleted the issue-500 branch Apr 22, 2017

@Godin Godin removed this from IN PROGRESS in Current work items Apr 22, 2017

@Godin Godin moved this from IN PROGRESS to DONE in Filtering Apr 22, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 29, 2017

@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.

romani commented Jun 29, 2017

@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

This comment has been minimized.

Show comment
Hide comment
Member

Godin commented Jun 29, 2017

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.