Bug 405569 - Resource leak check false positive when using

DbUtils.closeQuietly

3 files changed, 98 insertions, 19 deletions

diff --git a/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/ResourceLeakTests.java b/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/ResourceLeakTests.java
index de2c6103d6..564af5e8a6 100644
--- a/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/ResourceLeakTests.java
+++ b/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/ResourceLeakTests.java
@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2011, 2012 GK Software AG and others.
+ * Copyright (c) 2011, 2013 GK Software AG and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License v1.0
  * which accompanies this distribution, and is available at
@@ -25,13 +25,25 @@ import org.eclipse.jdt.internal.compiler.impl.CompilerOptions;
 
 public class ResourceLeakTests extends AbstractRegressionTest {
 
-// well-known helper class:
+// well-known helper classes:
 private static final String GUAVA_CLOSEABLES_JAVA = "com/google/common/io/Closeables.java";
 private static final String GUAVA_CLOSEABLES_CONTENT = "package com.google.common.io;\n" +
 	"public class Closeables {\n" +
 	"    public static void closeQuietly(java.io.Closeable closeable) {}\n" +
 	"    public static void close(java.io.Closeable closeable, boolean flag) {}\n" +
 	"}\n";
+private static final String APACHE_DBUTILS_JAVA = "org/apache/commons/dbutils/DbUtils.java";
+private static final String APACHE_DBUTILS_CONTENT = "package org.apache.commons.dbutils;\n" +
+	"import java.sql.*;\n" +
+	"public class DbUtils {\n" +
+	"    public static void close(Connection connection) {}\n" +
+	"    public static void close(ResultSet resultSet) {}\n" +
+	"    public static void close(Statement statement) {}\n" +
+	"    public static void closeQuietly(Connection connection) {}\n" +
+	"    public static void closeQuietly(ResultSet resultSet) {}\n" +
+	"    public static void closeQuietly(Statement statement) {}\n" +
+	"    public static void closeQuietly(Connection conn, Statement stmt, ResultSet rs) {}\n" +
+	"}\n";
 
 static {
 //	TESTS_NAMES = new String[] { "testBug376053" };
@@ -4077,6 +4089,61 @@ public void testBug381445_1() {
 		null);	
 }
 
+// Bug 405569 - Resource leak check false positive when using DbUtils.closeQuietly
+// A resource is closed using more known close helpers
+public void testBug381445_1b() {
+	Map options = getCompilerOptions();
+	options.put(CompilerOptions.OPTION_ReportPotentiallyUnclosedCloseable, CompilerOptions.ERROR);
+	options.put(CompilerOptions.OPTION_ReportUnclosedCloseable, CompilerOptions.ERROR);
+	runNegativeTest(
+		new String[] {
+			APACHE_DBUTILS_JAVA,
+			APACHE_DBUTILS_CONTENT,
+			"Bug381445.java",
+			"import java.sql.*;\n" +
+			"\n" +
+			"public class Bug381445 {\n" +
+			"	public void performQuery1(String url, String q1, String q2) throws Exception {\n" +
+			"		Connection conn = DriverManager.getConnection(url);\n" +
+			"		Statement stat = conn.createStatement();\n" +
+			"		ResultSet rset = stat.executeQuery(q1);\n" +
+			"		ResultSet rset2 = stat.executeQuery(q2);\n" +
+			"		try {\n" +
+			"			// empty\n" +
+			"		} finally {\n" +
+			"			org.apache.commons.dbutils.DbUtils.closeQuietly(conn);\n" +
+			"			org.apache.commons.dbutils.DbUtils.close(stat);\n" +
+			"			org.apache.commons.dbutils.DbUtils.closeQuietly(rset);\n" +
+			"			Closeables.closeQuietly(rset2);\n" +
+			"		}\n" +
+			"	}\n" +
+			"	public void performQuery2(String url, String q1, String q2) throws Exception {\n" +
+			"		Connection conn = DriverManager.getConnection(url);\n" +
+			"		Statement stat = conn.createStatement();\n" +
+			"		ResultSet rset = stat.executeQuery(q1);\n" +
+			"		try {\n" +
+			"			// empty\n" +
+			"		} finally {\n" +
+			"			org.apache.commons.dbutils.DbUtils.closeQuietly(conn, stat, rset);\n" +
+			"		}\n" +
+			"	}\n" +
+			"}\n" +
+			"class Closeables {\n" + // fake, should not be recognized
+			"	public static void closeQuietly(java.lang.AutoCloseable closeable) {}\n" +
+			"}\n"
+		},
+		"----------\n" + 
+		"1. ERROR in Bug381445.java (at line 8)\n" + 
+		"	ResultSet rset2 = stat.executeQuery(q2);\n" +
+		"	          ^^^^^\n" + 
+		"Potential resource leak: \'rset2\' may not be closed\n" + 
+		"----------\n",
+		null,
+		true,
+		options,
+		null);	
+}
+
 // Bug 381445 - [compiler][resource] Can the resource leak check be made aware of Closeables.closeQuietly?
 // A resource is closed in different places of the flow
 public void testBug381445_2() {
diff --git a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/MessageSend.java b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/MessageSend.java
index 82e2853c45..7665234d29 100644
--- a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/MessageSend.java
+++ b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/MessageSend.java
@@ -26,6 +26,7 @@
  *								bug 382069 - [null] Make the null analysis consider JUnit's assertNotNull similarly to assertions
  *								bug 403086 - [compiler][null] include the effect of 'assert' in syntactic null analysis for fields
  *								bug 403147 - [compiler][null] FUP of bug 400761: consolidate interaction between unboxing, NPE, and deferred checking
+ *								Bug 405569 - Resource leak check false positive when using DbUtils.closeQuietly
  *     Jesper S Moller - Contributions for
  *								Bug 378674 - "The method can be declared as static" is wrong
  *******************************************************************************/
@@ -89,11 +90,10 @@ public FlowInfo analyseCode(BlockScope currentScope, FlowContext flowContext, Fl
 	// recording the closing of AutoCloseable resources:
 	boolean analyseResources = currentScope.compilerOptions().analyseResourceLeaks;
 	if (analyseResources) {
-		Expression closeTarget = null;
 		if (nonStatic) {
 			// closeable.close()
 			if (CharOperation.equals(TypeConstants.CLOSE, this.selector)) {
-				closeTarget = this.receiver;
+				recordCallingClose(currentScope, flowContext, flowInfo, this.receiver);
 			}
 		} else if (this.arguments != null && this.arguments.length > 0 && FakedTrackingVariable.isAnyCloseable(this.arguments[0].resolvedType)) {
 			// Helper.closeMethod(closeable, ..)
@@ -102,21 +102,13 @@ public FlowInfo analyseCode(BlockScope currentScope, FlowContext flowContext, Fl
 				if (CharOperation.equals(record.selector, this.selector)
 						&& CharOperation.equals(record.typeName, this.binding.declaringClass.compoundName)) 
 				{
-					closeTarget = this.arguments[0];
+					int len = Math.min(record.numCloseableArgs, this.arguments.length);
+					for (int j=0; j<len; j++)
+						recordCallingClose(currentScope, flowContext, flowInfo, this.arguments[j]);
 					break;
 				}
 			}
 		}
-		if (closeTarget != null) {
-			FakedTrackingVariable trackingVariable = FakedTrackingVariable.getCloseTrackingVariable(closeTarget, flowInfo, flowContext);
-			if (trackingVariable != null) { // null happens if target is not a local variable or not an AutoCloseable
-				if (trackingVariable.methodScope == currentScope.methodScope()) {
-					trackingVariable.markClose(flowInfo, flowContext);
-				} else {
-					trackingVariable.markClosedInNestedMethod();
-				}
-			}
-		}
 	}
 
 	if (nonStatic) {
@@ -169,6 +161,16 @@ public FlowInfo analyseCode(BlockScope currentScope, FlowContext flowContext, Fl
 	flowContext.expireNullCheckedFieldInfo(); // no longer trust this info after any message send
 	return flowInfo;
 }
+private void recordCallingClose(BlockScope currentScope, FlowContext flowContext, FlowInfo flowInfo, Expression closeTarget) {
+	FakedTrackingVariable trackingVariable = FakedTrackingVariable.getCloseTrackingVariable(closeTarget, flowInfo, flowContext);
+	if (trackingVariable != null) { // null happens if target is not a local variable or not an AutoCloseable
+		if (trackingVariable.methodScope == currentScope.methodScope()) {
+			trackingVariable.markClose(flowInfo, flowContext);
+		} else {
+			trackingVariable.markClosedInNestedMethod();
+		}
+	}
+}
 
 // classification of well-known assertion utilities:
 private static final int TRUE_ASSERTION = 1;
diff --git a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/TypeConstants.java b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/TypeConstants.java
index d6a517466f..6c801d9a3a 100644
--- a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/TypeConstants.java
+++ b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/TypeConstants.java
@@ -13,6 +13,7 @@
  *								bug 381445 - [compiler][resource] Can the resource leak check be made aware of Closeables.closeQuietly?
  *								bug 400421 - [compiler] Null analysis for fields does not take @com.google.inject.Inject into account
  *								bug 382069 - [null] Make the null analysis consider JUnit's assertNotNull similarly to assertions
+ *								Bug 405569 - Resource leak check false positive when using DbUtils.closeQuietly
  *******************************************************************************/
 package org.eclipse.jdt.internal.compiler.lookup;
 
@@ -176,18 +177,27 @@ public interface TypeConstants {
 	public static class CloseMethodRecord {
 		public char[][] typeName;
 		public char[] selector;
-		public CloseMethodRecord(char[][] typeName, char[] selector) {
+		public int numCloseableArgs;
+		public CloseMethodRecord(char[][] typeName, char[] selector, int num) {
 			this.typeName = typeName;
 			this.selector = selector;
+			this.numCloseableArgs = num;
 		}
 	}
 	char[][] GUAVA_CLOSEABLES = { COM, GOOGLE, "common".toCharArray(), IO, "Closeables".toCharArray() }; //$NON-NLS-1$ //$NON-NLS-2$
 	char[][] APACHE_IOUTILS = { ORG, APACHE, COMMONS, IO, "IOUtils".toCharArray() }; //$NON-NLS-1$
+	char[][] APACHE_DBUTILS = { ORG, APACHE, COMMONS, "dbutils".toCharArray(), "DbUtils".toCharArray() }; //$NON-NLS-1$ //$NON-NLS-2$
 	char[] CLOSE_QUIETLY = "closeQuietly".toCharArray(); //$NON-NLS-1$
 	CloseMethodRecord[] closeMethods = new CloseMethodRecord[] {
-		new CloseMethodRecord(GUAVA_CLOSEABLES, CLOSE_QUIETLY),
-		new CloseMethodRecord(GUAVA_CLOSEABLES, CLOSE),
-		new CloseMethodRecord(APACHE_IOUTILS, CLOSE_QUIETLY)
+		new CloseMethodRecord(GUAVA_CLOSEABLES, CLOSE_QUIETLY, 1),
+		new CloseMethodRecord(GUAVA_CLOSEABLES, CLOSE, 1),
+		new CloseMethodRecord(APACHE_IOUTILS, CLOSE_QUIETLY, 1),
+		new CloseMethodRecord(APACHE_DBUTILS, CLOSE, 1),
+		new CloseMethodRecord(APACHE_DBUTILS, CLOSE_QUIETLY, 3), // closeQuietly(Connection,Statement,ResultSet) 
+		new CloseMethodRecord(APACHE_DBUTILS, "commitAndClose".toCharArray(), 1), //$NON-NLS-1$
+		new CloseMethodRecord(APACHE_DBUTILS, "commitAndCloseQuietly".toCharArray(), 1), //$NON-NLS-1$
+		new CloseMethodRecord(APACHE_DBUTILS, "rollbackAndClose".toCharArray(), 1), //$NON-NLS-1$
+		new CloseMethodRecord(APACHE_DBUTILS, "rollbackAndCloseQuietly".toCharArray(), 1), //$NON-NLS-1$
 	};
 	// white lists of closeables:
 	char[][] JAVA_IO_WRAPPER_CLOSEABLES = new char[][] {