issue #5752: Activate checks related to java.io.Closeable #5753

pbludov · 2018-04-22T02:44:28Z

Force use of try-with-resources

pbludov · 2018-04-22T02:50:03Z

.ci/eclipse-compiler-javac.sh

@@ -17,8 +17,7 @@ if [ ! -f $ECJ_PATH ]; then
    wget http://ftp-stud.fht-esslingen.de/pub/Mirrors/eclipse/eclipse/downloads/drops4/$ECJ_MAVEN_VERSION/$ECJ_JAR -O $ECJ_PATH
 fi

-mkdir -p target/classes
-mkdir -p target/eclipse
+mkdir -p target/classes target/test-classes target/eclipse


To solve warning incorrect classpath: src/checkstyle/target/test-classes on clean environment

codecov-io · 2018-04-22T03:14:08Z

Codecov Report

Merging #5753 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff            @@
##             master   #5753   +/-   ##
========================================
  Coverage       100%    100%           
- Complexity     7381    7386    +5     
========================================
  Files           296     296           
  Lines         16123   16138   +15     
  Branches       3254    3255    +1     
========================================
+ Hits          16123   16138   +15

Impacted Files	Coverage Δ	Complexity Δ
...ain/java/com/puppycrawl/tools/checkstyle/Main.java	`100% <100%> (ø)`	`78 <4> (+2)`	⬆️
...com/puppycrawl/tools/checkstyle/DefaultLogger.java	`100% <100%> (ø)`	`23 <0> (+2)`	⬆️
...ava/com/puppycrawl/tools/checkstyle/XMLLogger.java	`100% <100%> (ø)`	`44 <0> (+1)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4209baa...3cc1f00. Read the comment docs.

romani

Items to improve:

romani · 2018-04-23T18:02:05Z

src/main/java/com/puppycrawl/tools/checkstyle/Main.java

@@ -609,6 +609,7 @@ private static Properties loadProperties(File file)
     * @return a fresh new {@code AuditListener}
     * @exception IOException when provided output location is not found
     */
+    @SuppressWarnings("resource")


Please explain suppress reason in javadoc.
Why IDEA does not have there suppression ? Is it a hug in Eclipse analyser?

I do not think this is a bug. The algorithms that use IDEA and Eclipse are different. And they can behave differently.

ok, call it new feature request, enhancement, any type of ticket against Eclipse to be better.
If IDEA found way to make less false positives, why Eclipse should be behind ?

If Eclipse is less configurable/smart ... why we should activate it ? Does it make sense to activate only IDEA ?
Do we get some benefit from Eclipse only(not detected by IDEA) ?

romani · 2018-04-23T18:02:35Z

src/test/java/com/puppycrawl/tools/checkstyle/AbstractModuleTestSupport.java

@@ -69,6 +69,10 @@

    private static final String ROOT_MODULE_NAME = "root";

+    /**
+     * Output stream to hold the test results.
+     * @noinspection resource


Please explain suppress reason in javadoc.

romani · 2018-04-23T18:02:58Z

src/test/java/com/puppycrawl/tools/checkstyle/XMLLoggerTest.java

@@ -41,6 +41,10 @@
 // -@cs[AbbreviationAsWordInName] Test should be named as its main class.
 public class XMLLoggerTest extends AbstractXmlTestSupport {

+    /**
+     * Output stream to hold the test results.
+     * @noinspection resource


Please explain suppress reason in javadoc.

romani · 2018-04-23T18:09:08Z

config/intellij-idea-inspections.xml

@@ -2151,6 +2153,9 @@ isolated classes and we cannot put them to separate package as it will affect us
                <!-- XDocs UT is very long and complex -->
                <option value="OverlyComplexBooleanExpression" />
                <option value="ReuseOfLocalVariable" />
+                <!-- The output stream created by Main.createListener will be closed later. This is
+                       by design. A warning issued by Eclipse JDT need to be suppressed. -->
+                <option value="resource" />


Please make comment more general. As we sometime postpone close for resource by design, not only for this particular case - Main.createListener

sorry to enforce bureaucracy, but it is proven by practice, principle to do "done" for each point.
It is just final recheck that all items from numerous are done.

romani

I need to review this further:

romani · 2018-04-25T23:14:44Z

config/intellij-idea-inspections.xml

-    <!-- we are ok to use auto-unboxing as we use modern java -->
+    <inspection_tool class="AutoCloseableResource" enabled="true" level="ERROR" enabled_by_default="true" >
+        <option name="ignoreFromMethodCall" value="true" />
+    </inspection_tool>


comment for myself.
from IDEA:

Reports AutoCloseable instances which are not used in a try-with-resources statement, also known as Automatic
Resource Management. This means that the open resource before/in try, close in finally style which was used
before try-with-resources was available is also reported. This inspection is meant to replace all opened but not
safely closed inspections when developing in Java 7 and higher.
Use the first table below to specify which AutoCloseable subclasses should be ignored by this inspection. Specify
AutoCloseable subclasses here which do not need to be closed.
Use the second table below to specify which methods returning AutoCloseable will be ignored when called.
Use the first checkbox below to ignore an AutoCloseable if it the result of a method call. When enabled, the results
of factory methods will also be ignored.
Use the second checkbox below to specify that the inspection should not warn if an AutoCloseable instance is
passed as a method call argument. If enabled the inspection assumes the resource is closed in the called method. Method calls inside a finally block with close in the name and an AutoCloseable argument will not be ignored.

Config for proposed xml looks like this in IDEA:

Yes, it is related to the ignoreFromMethodCall option.
By turning it off the IDEA reports the line

out = Files.newOutputStream(Paths.get(outputLocation));

as violation in the same way as Eclipse does. One more: the suppression

@SuppressWarnings("resource")

Works for IDEA in the same way as for Eclipse. So it is possible to set ignoreFromMethodCall to false.

pbludov · 2018-04-26T13:28:23Z

After setting ignoreFromMethodCall to false, IDEA found 11 more violations. All of them are in the folder src/test/java. The Eclipse compiler does not issue any warnings for test classes.

This is unexpected and should be investigated.

romani · 2018-05-02T13:06:30Z

@pbludov ,

ignoreFromMethodCall to false .. should be investigated.

ignoreFromMethodCall=true allow too much.
please use following config for IDEA:

  <inspection_tool class="AutoCloseableResource" enabled="true" level="ERROR" enabled_by_default="true">
    <option name="ignoredTypes" value="java.util.stream.Stream,java.util.stream.IntStream,java.util.stream.LongStream,java.util.stream.DoubleStream,com.puppycrawl.tools.checkstyle.internal.utils.CloseAndFlushTestByteArrayOutputStream" />
    <option name="METHOD_MATCHER_CONFIG" value="java.util.Formatter,format,java.io.Writer,append,com.google.common.base.Preconditions,checkNotNull,org.hibernate.Session,close,java.io.PrintWriter,printf,org.mockito.Mockito,mock|verify,org.mockito.stubbing.Stubber,when" />
  </inspection_tool>

Please merge my draft branch https://github.com/checkstyle/checkstyle/tree/i5752-close-resources to your code, together update will be good to merge I think.

pbludov · 2018-05-06T21:08:52Z

I'm afk till 14 may, sorry.

pbludov · 2018-05-12T05:52:48Z

please use following config for IDEA

done.

romani

items to improve and confirm:

romani · 2018-05-12T13:19:17Z

src/main/java/com/puppycrawl/tools/checkstyle/Main.java

@@ -602,13 +602,15 @@ private static Properties loadProperties(File file)
    }

    /**
-     * Creates the audit listener.
+     * Creates the audit listener. The JDT compiler issues the warning explicitlyClosedAutoCloseable
+     * for {@code Files.newOutputStream}, so it need to be suppressed.


this javadoc is public, let be more accurate here.

please remove "The JDT compiler issues the warning explicitlyClosedAutoCloseable ...." and make it like This method create in AuditListener open stream for validation data, it must be closed by {@link RootModule} (default implementation is {@link Checker}) by launching {@link AuditListener#auditFinished(AuditEvent)}

romani · 2018-05-12T13:30:06Z

config/intellij-idea-inspections.xml

+        <option name="ignoredTypes" value="java.io.ByteArrayOutputStream,java.util.stream.Stream,java.util.stream.IntStream,java.util.stream.LongStream,java.util.stream.DoubleStream,com.puppycrawl.tools.checkstyle.internal.utils.CloseAndFlushTestByteArrayOutputStream" />
+        <!-- False positives for the Closable returned from a builder method (which return "this").
+             Mock stubs shouldn't be closed. -->
+        <option name="METHOD_MATCHER_CONFIG" value="java.util.Formatter,format,java.io.Writer,append,com.google.common.base.Preconditions,checkNotNull,org.hibernate.Session,close,java.io.PrintWriter,printf,org.mockito.ArgumentCaptor,getValue,org.mockito.Mockito,mock|verify,org.mockito.stubbing.Stubber,when" />


getValue

it is not ok. We can resolve this bad code.

please review commits, sorry if they are messy, I can not squash in main repo:
2b0ac1b
9c6aa47
I refactored createListener to create streams when required getOutputStream(outputLocation) and getOutputStreamOptions(String outputLocation).

If it still messy, I will recreate branch.
If you have some concerns with such implementation, we can merge this PR, but discuss update in separate issue.

done. Your code has been squashed to the PR.

romani · 2018-05-12T14:51:37Z

eclipse is not happy.

pbludov · 2018-05-13T04:31:57Z

eclipse is not happy.

done

romani

Last minor:

romani · 2018-05-14T13:04:53Z

src/main/java/com/puppycrawl/tools/checkstyle/Main.java

-     *
+     * This method creates in AuditListener an open stream for validation data, it must be closed by
+     * {@link RootModule} (default implementation is {@link Checker}) by calling
+     * {@link AuditListener#auditFinished(AuditEvent)}.


Please move explanation on why stream is not closed to getOutputStream(String outputLocation) .
Unfortunately there is no way to keep reason of suppression in same annotation, so javadoc is only place to keep reason of suppression

romani

@rnveach , please do final review

rnveach · 2018-05-13T16:36:48Z

config/intellij-idea-inspections.xml

+             Mock stubs shouldn't be closed. -->
+        <option name="METHOD_MATCHER_CONFIG" value="java.util.Formatter,format,java.io.Writer,append,com.google.common.base.Preconditions,checkNotNull,org.hibernate.Session,close,java.io.PrintWriter,printf,org.mockito.Mockito,mock|verify,org.mockito.stubbing.Stubber,when" />
+    </inspection_tool>
+     <!-- we are ok to use auto-unboxing as we use modern java -->


incorrect indentation, -1 space

rnveach · 2018-05-15T11:18:15Z

config/intellij-idea-inspections.xml

-    <!-- we are ok to use auto-unboxing as we use modern java -->
+    <inspection_tool class="AutoCloseableResource" enabled="true" level="ERROR" enabled_by_default="true">
+        <!-- These classes do not contain any resources that need to be closed. -->
+        <option name="ignoredTypes" value="java.io.ByteArrayOutputStream,java.util.stream.Stream,java.util.stream.IntStream,java.util.stream.LongStream,java.util.stream.DoubleStream,com.puppycrawl.tools.checkstyle.internal.utils.CloseAndFlushTestByteArrayOutputStream" />


can we split all new lines so they are less than 100 characters where possible?

if this is not obvious how to make, we can postpone it for later.

done. If the CI fails, i'll revert this.

It fails: https://teamcity.jetbrains.com/viewLog.html?buildId=1439514&tab=Inspection&buildTypeId=Checkstyle_IdeaInspectionsPullRequest

The line wrapping reverted.

rnveach · 2018-05-15T11:18:24Z

config/intellij-idea-inspections.xml

+        <option name="ignoredTypes" value="java.io.ByteArrayOutputStream,java.util.stream.Stream,java.util.stream.IntStream,java.util.stream.LongStream,java.util.stream.DoubleStream,com.puppycrawl.tools.checkstyle.internal.utils.CloseAndFlushTestByteArrayOutputStream" />
+        <!-- False positives for the Closable returned from a builder method (which return "this").
+             Mock stubs shouldn't be closed. -->
+        <option name="METHOD_MATCHER_CONFIG" value="java.util.Formatter,format,java.io.Writer,append,com.google.common.base.Preconditions,checkNotNull,org.hibernate.Session,close,java.io.PrintWriter,printf,org.mockito.Mockito,mock|verify,org.mockito.stubbing.Stubber,when" />


if this is not obvious how to make, we can postpone it for later.

rnveach · 2018-05-15T11:26:51Z

src/test/java/com/puppycrawl/tools/checkstyle/DefaultLoggerTest.java

+    public void testNullInfoStreamOptions() {
+        try {
+            final DefaultLogger logger = new DefaultLogger(new ByteArrayOutputStream(), null);
+            assertNotNull("Null instance", logger);


isn't this assertion unnecessary since eventually we will hit fail below? Also when can a new instance return null?

Without this assertion, the warning "The allocated object is never used" appears. See
https://travis-ci.org/checkstyle/checkstyle/jobs/378234050#L761
We need to use the logger somehow to make the eclipse compiler happy.

This won't work:

@Test public void testNullOutputStreamOptions() { try { new XMLLogger(outStream, null); fail("Exception was expected"); } catch (IllegalArgumentException exception) { assertEquals("Invalid error message", "Parameter outputStreamOptions can not be null", exception.getMessage()); } }

If you have any idea how to do it right, let us know.

happiness for all - is tough.
This nuance nobody will remember, it far from obvious.
What if we will put in assert message "Null instance; assert required to calm down eclipse's 'The allocated object is never used' violation" or just make single line comment above assert ?

What if we will put in assert message "Null instance; assert required to calm down eclipse's 'The allocated object is never used' violation" or just make single line comment above assert ?

done

This sounds familiar which we had to do with other tests.

rnveach · 2018-05-15T11:27:00Z

src/test/java/com/puppycrawl/tools/checkstyle/DefaultLoggerTest.java

+        try {
+            final DefaultLogger logger = new DefaultLogger(new ByteArrayOutputStream(),
+                AutomaticBean.OutputStreamOptions.CLOSE, new ByteArrayOutputStream(), null);
+            assertNotNull("Null instance", logger);


rnveach · 2018-05-15T11:27:16Z

src/test/java/com/puppycrawl/tools/checkstyle/XMLLoggerTest.java

+    public void testNullOutputStreamOptions() {
+        try {
+            final XMLLogger logger = new XMLLogger(outStream, null);
+            assertNotNull("Null instance", logger);


rnveach · 2018-05-15T11:29:23Z

src/test/java/com/puppycrawl/tools/checkstyle/checks/javadoc/WriteTagCheckTest.java

+     * @param messageFileName message file name.
+     * @param expected an array of expected messages.
+     * @throws Exception might happen
+     */


Why is this documentation needed when no code was changed? This is a test area.

yes, looks strange.

I believe that there was a suppression. Later, the code was refactored. The comment is removed now.

rnveach · 2018-05-15T11:30:26Z

src/main/java/com/puppycrawl/tools/checkstyle/DefaultLogger.java

@@ -176,7 +176,13 @@ public DefaultLogger(OutputStream infoStream,
                         OutputStream errorStream,
                         OutputStreamOptions errorStreamOptions,
                         AuditEventFormatter messageFormatter) {
+        if (infoStreamOptions == null) {
+            throw new IllegalArgumentException("Parameter infoStreamOptions can not be null");


Just curious, why are we adding these IAEs and do we need them in other areas?

Pitest. It says "the result may be replaced with null here

private static AutomaticBean.OutputStreamOptions getOutputStreamOptions(String outputLocation) { final AutomaticBean.OutputStreamOptions result; if (outputLocation == null) { result = AutomaticBean.OutputStreamOptions.NONE; // THIS line was red } else { result = AutomaticBean.OutputStreamOptions.CLOSE; } return result; }

The expression

closeStream = outputStreamOptions == OutputStreamOptions.CLOSE;

work just fine when the parameter outputStreamOptions is null.
It turns out that we can use an enumeration with just one value! The second value will be null.
This is logical, but wrong. I explicitly forbid null for these parameters.

romani · 2018-05-17T12:31:25Z

@pbludov , please take a look at items to improve and confim. If smth is not clear let us know.

rnveach

Can be merged when CI passes.

romani · 2018-05-18T20:10:02Z

Merged, thanks a lot to all

pbludov changed the title ~~Issue #5291: fixed xdoc lines of 100 characters or more~~ Issue #5752: Activate checks related to java.io.Closeable Apr 22, 2018

pbludov commented Apr 22, 2018

View reviewed changes

romani requested changes Apr 23, 2018

View reviewed changes

romani reviewed Apr 25, 2018

View reviewed changes

pbludov changed the title ~~Issue #5752: Activate checks related to java.io.Closeable~~ [WIP] Issue #5752: Activate checks related to java.io.Closeable Apr 26, 2018

romani mentioned this pull request May 8, 2018

Cut down on Checkstyle's dependencies on Guava, phase II #3484

Closed

pbludov changed the title ~~[WIP] Issue #5752: Activate checks related to java.io.Closeable~~ issue #5752: Activate checks related to java.io.Closeable May 12, 2018

romani requested changes May 12, 2018

View reviewed changes

romani mentioned this pull request May 12, 2018

format all files to be 100 symbols in length #5791

Closed

romani requested changes May 14, 2018

View reviewed changes

romani approved these changes May 15, 2018

View reviewed changes

romani assigned rnveach May 15, 2018

rnveach suggested changes May 15, 2018

View reviewed changes

Issue #5752: Activate checks related to java.io.Closeable

3cc1f00

rnveach approved these changes May 18, 2018

View reviewed changes

romani merged commit effba27 into checkstyle:master May 18, 2018

pbludov deleted the issue-5752 branch July 12, 2018 07:12

This was referenced Dec 8, 2023

dependency: bump com.github.spotbugs:spotbugs-maven-plugin from 4.7.3.5 to 4.8.2.0 #14117

Merged

Recfactor classes to avoid throwing exceptions from constructors #14122

Open

Uh oh!

issue #5752: Activate checks related to java.io.Closeable #5753

issue #5752: Activate checks related to java.io.Closeable #5753

Uh oh!

Conversation

pbludov commented Apr 22, 2018 • 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!

codecov-io commented Apr 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

romani left a comment

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!

romani Apr 25, 2018 • 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!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

romani Apr 25, 2018 • 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!

pbludov commented Apr 26, 2018

Uh oh!

romani commented May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pbludov commented May 6, 2018

Uh oh!

pbludov commented May 12, 2018

Uh oh!

romani left a comment

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!

romani commented May 12, 2018

Uh oh!

pbludov commented May 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romani left a comment

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!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

pbludov commented Apr 22, 2018 •

edited

Loading

codecov-io commented Apr 22, 2018 •

edited

Loading

romani Apr 25, 2018 •

edited

Loading

romani Apr 25, 2018 •

edited

Loading

romani commented May 2, 2018 •

edited

Loading

pbludov commented May 13, 2018 •

edited

Loading