Bug 576502 - Add try-with-resources quick fix for unclosed resource
- add new static getTryWithResourceProposals() method
- add quick fix support for unclosed and potentially unclosed
resource errors/warnings to QuickFixProcessor
- refactor some static methods in QuickAssistProcessor to
QuickAssistProcessorUtil
- add new tests to QuickFixTest1d8
Change-Id: If345868a875aecfe2f5e907221e3d07528e877c0
Reviewed-on: https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/186461
Tested-by: JDT Bot <jdt-bot@eclipse.org>
Tested-by: Jeff Johnston <jjohnstn@redhat.com>
Reviewed-by: Jeff Johnston <jjohnstn@redhat.com>
diff --git a/org.eclipse.jdt.core.manipulation/common/org/eclipse/jdt/internal/ui/text/correction/QuickAssistProcessorUtil.java b/org.eclipse.jdt.core.manipulation/common/org/eclipse/jdt/internal/ui/text/correction/QuickAssistProcessorUtil.java
index c54919f..96c6c13 100644
--- a/org.eclipse.jdt.core.manipulation/common/org/eclipse/jdt/internal/ui/text/correction/QuickAssistProcessorUtil.java
+++ b/org.eclipse.jdt.core.manipulation/common/org/eclipse/jdt/internal/ui/text/correction/QuickAssistProcessorUtil.java
@@ -16,8 +16,11 @@
import java.util.ArrayList;
import java.util.Collections;
+import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
import java.util.Set;
import org.eclipse.jface.text.link.LinkedPositionGroup;
@@ -25,6 +28,8 @@
import org.eclipse.jdt.core.IMethod;
import org.eclipse.jdt.core.JavaModelException;
import org.eclipse.jdt.core.dom.AST;
+import org.eclipse.jdt.core.dom.ASTNode;
+import org.eclipse.jdt.core.dom.ASTVisitor;
import org.eclipse.jdt.core.dom.ArrayCreation;
import org.eclipse.jdt.core.dom.ArrayType;
import org.eclipse.jdt.core.dom.Block;
@@ -37,6 +42,7 @@
import org.eclipse.jdt.core.dom.IBinding;
import org.eclipse.jdt.core.dom.IMethodBinding;
import org.eclipse.jdt.core.dom.ITypeBinding;
+import org.eclipse.jdt.core.dom.IVariableBinding;
import org.eclipse.jdt.core.dom.LambdaExpression;
import org.eclipse.jdt.core.dom.MethodInvocation;
import org.eclipse.jdt.core.dom.MethodReference;
@@ -53,12 +59,14 @@
import org.eclipse.jdt.core.dom.TypeMethodReference;
import org.eclipse.jdt.core.dom.VariableDeclaration;
import org.eclipse.jdt.core.dom.VariableDeclarationFragment;
+import org.eclipse.jdt.core.dom.VariableDeclarationStatement;
import org.eclipse.jdt.core.dom.rewrite.ASTRewrite;
import org.eclipse.jdt.core.dom.rewrite.ImportRewrite;
import org.eclipse.jdt.internal.core.manipulation.StubUtility;
import org.eclipse.jdt.internal.corext.dom.ASTNodes;
import org.eclipse.jdt.internal.corext.dom.Bindings;
+import org.eclipse.jdt.internal.corext.dom.LinkedNodeFinder;
import org.eclipse.jdt.internal.corext.dom.ScopeAnalyzer;
import org.eclipse.jdt.internal.corext.fix.LinkedProposalModelCore;
@@ -385,4 +393,87 @@
rewrite.set(lambda, LambdaExpression.BODY_PROPERTY, blockBody, null);
}
+ /**
+ * Return the first auto closable nodes. When a node that isn't Autoclosable is found the method
+ * returns.
+ *
+ * @param astNodes The nodes covered.
+ * @return List of the first AutoClosable nodes found
+ */
+ public static List<ASTNode> getCoveredAutoClosableNodes(List<ASTNode> astNodes) {
+ List<ASTNode> autoClosableNodes= new ArrayList<>();
+ for (ASTNode astNode : astNodes) {
+ if (isAutoClosable(astNode)) {
+ autoClosableNodes.add(astNode);
+ } else {
+ return autoClosableNodes;
+ }
+ }
+ return autoClosableNodes;
+ }
+
+ public static int findEndPostion(ASTNode node) {
+ int end= node.getStartPosition() + node.getLength();
+ Map<SimpleName, IVariableBinding> nodeSimpleNameBindings= getVariableStatementBinding(node);
+ List<SimpleName> nodeNames= new ArrayList<>(nodeSimpleNameBindings.keySet());
+ if (nodeNames.isEmpty()) {
+ return -1;
+ }
+ SimpleName nodeSimpleName= nodeNames.get(0);
+ SimpleName[] coveredNodeBindings= LinkedNodeFinder.findByNode(node.getRoot(), nodeSimpleName);
+ if (coveredNodeBindings.length == 0) {
+ return -1;
+ }
+ for (ASTNode astNode : coveredNodeBindings) {
+ end= Math.max(end, (astNode.getStartPosition() + astNode.getLength()));
+ }
+ return end;
+ }
+
+ public static Map<SimpleName, IVariableBinding> getVariableStatementBinding(ASTNode astNode) {
+ Map<SimpleName, IVariableBinding> variableBindings= new HashMap<>();
+ astNode.accept(new ASTVisitor() {
+ @Override
+ public boolean visit(VariableDeclarationStatement node) {
+ for (Object o : node.fragments()) {
+ if (o instanceof VariableDeclarationFragment) {
+ VariableDeclarationFragment vdf= (VariableDeclarationFragment) o;
+ SimpleName name= vdf.getName();
+ IBinding binding= name.resolveBinding();
+ if (binding instanceof IVariableBinding) {
+ variableBindings.put(name, (IVariableBinding) binding);
+ break;
+ }
+ }
+ }
+ return false;
+ }
+ });
+ return variableBindings;
+ }
+
+ public static boolean isAutoClosable(ITypeBinding typeBinding) {
+ return Bindings.findTypeInHierarchy(typeBinding, "java.lang.AutoCloseable") != null; //$NON-NLS-1$
+ }
+
+ public static boolean isAutoClosable(ASTNode astNode) {
+ Map<SimpleName, IVariableBinding> simpleNames= getVariableStatementBinding(astNode);
+ for (Entry<SimpleName, IVariableBinding> entry : simpleNames.entrySet()) {
+ ITypeBinding typeBinding= null;
+ switch (entry.getKey().getParent().getNodeType()) {
+ case ASTNode.VARIABLE_DECLARATION_FRAGMENT:
+ case ASTNode.VARIABLE_DECLARATION_STATEMENT:
+ case ASTNode.ASSIGNMENT:
+ typeBinding= entry.getValue().getType();
+ break;
+ default:
+ continue;
+ }
+ if (typeBinding != null && isAutoClosable(typeBinding)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
}
diff --git a/org.eclipse.jdt.ui.tests/ui/org/eclipse/jdt/ui/tests/quickfix/QuickFixTest1d8.java b/org.eclipse.jdt.ui.tests/ui/org/eclipse/jdt/ui/tests/quickfix/QuickFixTest1d8.java
index 17e8a99..1ba051a 100644
--- a/org.eclipse.jdt.ui.tests/ui/org/eclipse/jdt/ui/tests/quickfix/QuickFixTest1d8.java
+++ b/org.eclipse.jdt.ui.tests/ui/org/eclipse/jdt/ui/tests/quickfix/QuickFixTest1d8.java
@@ -1,5 +1,5 @@
/*******************************************************************************
- * Copyright (c) 2013, 2020 IBM Corporation and others.
+ * Copyright (c) 2013, 2021 IBM Corporation and others.
*
* This program and the accompanying materials
* are made available under the terms of the Eclipse Public License 2.0
@@ -40,6 +40,7 @@
import org.eclipse.jdt.core.dom.CompilationUnit;
import org.eclipse.jdt.core.formatter.DefaultCodeFormatterConstants;
+import org.eclipse.jdt.internal.compiler.impl.CompilerOptions;
import org.eclipse.jdt.internal.core.manipulation.CodeTemplateContextType;
import org.eclipse.jdt.internal.core.manipulation.StubUtility;
@@ -71,7 +72,6 @@
options.put(DefaultCodeFormatterConstants.FORMATTER_INSERT_NEW_LINE_AFTER_ANNOTATION_ON_LOCAL_VARIABLE, JavaCore.DO_NOT_INSERT);
options.put(DefaultCodeFormatterConstants.FORMATTER_ALIGNMENT_FOR_ANNOTATIONS_ON_LOCAL_VARIABLE, DefaultCodeFormatterConstants.createAlignmentValue(false, DefaultCodeFormatterConstants.WRAP_NO_SPLIT));
-
JavaCore.setOptions(options);
IPreferenceStore store= JavaPlugin.getDefault().getPreferenceStore();
@@ -2374,4 +2374,140 @@
assertExpectedExistInProposals(proposals, new String[] {expected});
}
+ // bug 576502 : try-with-resources quick fix should be offered for resource leaks
+ @Test
+ public void testBug576502_potentiallyUnclosedCloseable() throws Exception {
+ Hashtable<String, String> options = JavaCore.getOptions();
+ options.put(CompilerOptions.OPTION_ReportPotentiallyUnclosedCloseable, CompilerOptions.ERROR);
+ JavaCore.setOptions(options);
+ IPackageFragment pack2= fSourceFolder.createPackageFragment("test1", false, null);
+
+
+ StringBuilder buf= new StringBuilder();
+ buf.append("package test1;\n");
+ buf.append("import java.io.File;\n");
+ buf.append("import java.io.FileReader;\n");
+ buf.append("import java.io.IOException;\n");
+ buf.append("class E {\n");
+ buf.append(" public void foo() throws IOException {\n");
+ buf.append(" final File file = new File(\"somefile\");\n");
+ buf.append(" FileReader fileReader = new FileReader(file);\n");
+ buf.append(" char[] in = new char[50];\n");
+ buf.append(" fileReader.read(in);\n");
+ buf.append(" new Runnable() {\n");
+ buf.append(" public void run() { \n");
+ buf.append(" try {\n");
+ buf.append(" fileReader.close();\n");
+ buf.append(" } catch (IOException ex) { /* nop */ }\n");
+ buf.append(" }}.run();\n");
+ buf.append(" }\n");
+ buf.append("}\n");
+ buf.append("");
+
+
+ ICompilationUnit cu= pack2.createCompilationUnit("E.java", buf.toString(), false, null);
+
+ CompilationUnit astRoot= getASTRoot(cu);
+ ArrayList<IJavaCompletionProposal> proposals= collectCorrections(cu, astRoot);
+ assertCorrectLabels(proposals);
+
+ buf= new StringBuilder();
+ buf.append("package test1;\n");
+ buf.append("import java.io.File;\n");
+ buf.append("import java.io.FileReader;\n");
+ buf.append("import java.io.IOException;\n");
+ buf.append("class E {\n");
+ buf.append(" public void foo() throws IOException {\n");
+ buf.append(" final File file = new File(\"somefile\");\n");
+ buf.append(" try (FileReader fileReader = new FileReader(file)) {\n");
+ buf.append(" char[] in = new char[50];\n");
+ buf.append(" fileReader.read(in);\n");
+ buf.append(" new Runnable() {\n");
+ buf.append(" public void run() { \n");
+ buf.append(" try {\n");
+ buf.append(" fileReader.close();\n");
+ buf.append(" } catch (IOException ex) { /* nop */ }\n");
+ buf.append(" }}.run();\n");
+ buf.append(" }\n");
+ buf.append(" }\n");
+ buf.append("}\n");
+ buf.append("");
+ String expected= buf.toString();
+
+ assertExpectedExistInProposals(proposals, new String[] {expected});
+ }
+
+ // bug 576502 : try-with-resources quick fix should be offered for resource leaks
+ @Test
+ public void testBug576502_unclosedCloseable() throws Exception {
+ Hashtable<String, String> options = JavaCore.getOptions();
+ options.put(CompilerOptions.OPTION_ReportUnclosedCloseable, CompilerOptions.ERROR);
+ JavaCore.setOptions(options);
+ IPackageFragment pack2= fSourceFolder.createPackageFragment("test1", false, null);
+
+
+ StringBuilder buf= new StringBuilder();
+ buf.append("package test1;\n");
+ buf.append("import java.io.File;\n");
+ buf.append("import java.io.FileReader;\n");
+ buf.append("import java.io.IOException;\n");
+ buf.append("class E {\n");
+ buf.append(" public void foo(boolean f1, boolean f2) throws IOException {\n");
+ buf.append(" File file = new File(\"somefile\");\n");
+ buf.append(" if (f1) {\n");
+ buf.append(" FileReader fileReader = new FileReader(file); // err: not closed\n");
+ buf.append(" char[] in = new char[50];\n");
+ buf.append(" fileReader.read(in);\n");
+ buf.append(" while (true) {\n");
+ buf.append(" FileReader loopReader = new FileReader(file); // don't warn, properly closed\n");
+ buf.append(" loopReader.close();\n");
+ buf.append(" break;\n");
+ buf.append(" }\n");
+ buf.append(" } else {\n");
+ buf.append(" FileReader fileReader = new FileReader(file); // warn: not closed on all paths\n");
+ buf.append(" if (f2)\n");
+ buf.append(" fileReader.close();\n");
+ buf.append(" }\n");
+ buf.append(" }\n");
+ buf.append("}\n");
+ buf.append("");
+
+
+ ICompilationUnit cu= pack2.createCompilationUnit("E.java", buf.toString(), false, null);
+
+ CompilationUnit astRoot= getASTRoot(cu);
+ ArrayList<IJavaCompletionProposal> proposals= collectCorrections(cu, astRoot);
+ assertCorrectLabels(proposals);
+
+ buf= new StringBuilder();
+ buf.append("package test1;\n");
+ buf.append("import java.io.File;\n");
+ buf.append("import java.io.FileReader;\n");
+ buf.append("import java.io.IOException;\n");
+ buf.append("class E {\n");
+ buf.append(" public void foo(boolean f1, boolean f2) throws IOException {\n");
+ buf.append(" File file = new File(\"somefile\");\n");
+ buf.append(" if (f1) {\n");
+ buf.append(" try (FileReader fileReader = new FileReader(file)) {\n");
+ buf.append(" char[] in = new char[50];\n");
+ buf.append(" fileReader.read(in);\n");
+ buf.append(" }\n");
+ buf.append(" while (true) {\n");
+ buf.append(" FileReader loopReader = new FileReader(file); // don't warn, properly closed\n");
+ buf.append(" loopReader.close();\n");
+ buf.append(" break;\n");
+ buf.append(" }\n");
+ buf.append(" } else {\n");
+ buf.append(" FileReader fileReader = new FileReader(file); // warn: not closed on all paths\n");
+ buf.append(" if (f2)\n");
+ buf.append(" fileReader.close();\n");
+ buf.append(" }\n");
+ buf.append(" }\n");
+ buf.append("}\n");
+ buf.append("");
+ String expected= buf.toString();
+
+ assertExpectedExistInProposals(proposals, new String[] {expected});
+ }
+
}
diff --git a/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/correction/LocalCorrectionsSubProcessor.java b/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/correction/LocalCorrectionsSubProcessor.java
index 7821c95..2f96238 100644
--- a/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/correction/LocalCorrectionsSubProcessor.java
+++ b/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/correction/LocalCorrectionsSubProcessor.java
@@ -2443,6 +2443,18 @@
}
}
+ public static void getTryWithResourceProposals(IInvocationContext context, IProblemLocation problem, Collection<ICommandAccess> proposals) {
+ ASTNode coveringNode= problem.getCoveringNode(context.getASTRoot());
+ if (coveringNode != null) {
+ try {
+ ArrayList<ASTNode> coveredNodes= AdvancedQuickAssistProcessor.getFullyCoveredNodes(context, coveringNode);
+ QuickAssistProcessor.getTryWithResourceProposals(context, coveringNode, coveredNodes, proposals);
+ } catch (IllegalArgumentException | CoreException e) {
+ // do nothing
+ }
+ }
+ }
+
public static void getConvertLambdaToAnonymousClassCreationsProposals(IInvocationContext context, IProblemLocation problem, Collection<ICommandAccess> proposals) {
ASTNode coveringNode= problem.getCoveringNode(context.getASTRoot());
if (coveringNode != null) {
diff --git a/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/correction/QuickAssistProcessor.java b/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/correction/QuickAssistProcessor.java
index 4d500e3..9c8c068 100644
--- a/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/correction/QuickAssistProcessor.java
+++ b/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/correction/QuickAssistProcessor.java
@@ -30,7 +30,6 @@
import java.util.Hashtable;
import java.util.List;
import java.util.Map;
-import java.util.Map.Entry;
import org.eclipse.swt.graphics.Image;
@@ -2227,7 +2226,7 @@
localProposal.setCommandId(ASSIGN_TO_LOCAL_ID);
resultingCollections.add(localProposal);
- if (isAutoClosable(typeBinding)) {
+ if (QuickAssistProcessorUtil.isAutoClosable(typeBinding)) {
AssignToVariableAssistProposal tryWithResourcesProposal= new AssignToVariableAssistProposal(cu, AssignToVariableAssistProposal.TRY_WITH_RESOURCES, expressionStatement, typeBinding,
IProposalRelevance.ASSIGN_IN_TRY_WITH_RESOURCES);
tryWithResourcesProposal.setCommandId(ASSIGN_IN_TRY_WITH_RESOURCES_ID);
@@ -3080,7 +3079,7 @@
}
}
}
- List<ASTNode> coveredAutoClosableNodes= getCoveredAutoClosableNodes(coveredStatements);
+ List<ASTNode> coveredAutoClosableNodes= QuickAssistProcessorUtil.getCoveredAutoClosableNodes(coveredStatements);
if (coveredAutoClosableNodes.isEmpty()) {
return false;
}
@@ -3093,7 +3092,7 @@
int end= start;
for (ASTNode astNode : coveredAutoClosableNodes) {
- int endPosition= findEndPostion(astNode);
+ int endPosition= QuickAssistProcessorUtil.findEndPostion(astNode);
end= Math.max(end, endPosition);
}
@@ -3103,7 +3102,7 @@
while (true) {
int newEnd= oldEnd;
for (ASTNode astNode : nodesInRange) {
- int endPosition= findEndPostion(astNode);
+ int endPosition= QuickAssistProcessorUtil.findEndPostion(astNode);
newEnd= Math.max(newEnd, endPosition);
}
if (newEnd > oldEnd) {
@@ -3302,89 +3301,6 @@
return true;
}
- private static int findEndPostion(ASTNode node) {
- int end= node.getStartPosition() + node.getLength();
- Map<SimpleName, IVariableBinding> nodeSimpleNameBindings= getVariableStatementBinding(node);
- List<SimpleName> nodeNames= new ArrayList<>(nodeSimpleNameBindings.keySet());
- if (nodeNames.isEmpty()) {
- return -1;
- }
- SimpleName nodeSimpleName= nodeNames.get(0);
- SimpleName[] coveredNodeBindings= LinkedNodeFinder.findByNode(node.getRoot(), nodeSimpleName);
- if (coveredNodeBindings.length == 0) {
- return -1;
- }
- for (ASTNode astNode : coveredNodeBindings) {
- end= Math.max(end, (astNode.getStartPosition() + astNode.getLength()));
- }
- return end;
- }
-
- /**
- * Return the first auto closable nodes. When a node that isn't Autoclosable is found the method
- * returns.
- *
- * @param astNodes The nodes covered.
- * @return List of the first AutoClosable nodes found
- */
- private static List<ASTNode> getCoveredAutoClosableNodes(List<ASTNode> astNodes) {
- List<ASTNode> autoClosableNodes= new ArrayList<>();
- for (ASTNode astNode : astNodes) {
- if (isAutoClosable(astNode)) {
- autoClosableNodes.add(astNode);
- } else {
- return autoClosableNodes;
- }
- }
- return autoClosableNodes;
- }
-
- private static boolean isAutoClosable(ASTNode astNode) {
- Map<SimpleName, IVariableBinding> simpleNames= getVariableStatementBinding(astNode);
- for (Entry<SimpleName, IVariableBinding> entry : simpleNames.entrySet()) {
- ITypeBinding typeBinding= null;
- switch (entry.getKey().getParent().getNodeType()) {
- case ASTNode.VARIABLE_DECLARATION_FRAGMENT:
- case ASTNode.VARIABLE_DECLARATION_STATEMENT:
- case ASTNode.ASSIGNMENT:
- typeBinding= entry.getValue().getType();
- break;
- default:
- continue;
- }
- if (typeBinding != null && isAutoClosable(typeBinding)) {
- return true;
- }
- }
- return false;
- }
-
- private static boolean isAutoClosable(ITypeBinding typeBinding) {
- return Bindings.findTypeInHierarchy(typeBinding, "java.lang.AutoCloseable") != null; //$NON-NLS-1$
- }
-
- private static Map<SimpleName, IVariableBinding> getVariableStatementBinding(ASTNode astNode) {
- Map<SimpleName, IVariableBinding> variableBindings= new HashMap<>();
- astNode.accept(new ASTVisitor() {
- @Override
- public boolean visit(VariableDeclarationStatement node) {
- for (Object o : node.fragments()) {
- if (o instanceof VariableDeclarationFragment) {
- VariableDeclarationFragment vdf= (VariableDeclarationFragment) o;
- SimpleName name= vdf.getName();
- IBinding binding= name.resolveBinding();
- if (binding instanceof IVariableBinding) {
- variableBindings.put(name, (IVariableBinding) binding);
- break;
- }
- }
- }
- return false;
- }
- });
- return variableBindings;
- }
-
private static boolean getAddBlockProposals(IInvocationContext context, ASTNode node, Collection<ICommandAccess> resultingCollections) {
if (!(node instanceof Statement)) {
return false;
diff --git a/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/correction/QuickFixProcessor.java b/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/correction/QuickFixProcessor.java
index dbea1b0..6dc44e3 100644
--- a/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/correction/QuickFixProcessor.java
+++ b/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/correction/QuickFixProcessor.java
@@ -320,6 +320,8 @@
case IProblem.FeatureNotSupported:
case IProblem.SwitchExpressionsReturnWithinSwitchExpression:
case IProblem.DanglingReference:
+ case IProblem.UnclosedCloseable:
+ case IProblem.PotentiallyUnclosedCloseable:
return true;
default:
return SuppressWarningsSubProcessor.hasSuppressWarningsProposal(cu.getJavaProject(), problemId)
@@ -903,6 +905,10 @@
case IProblem.SwitchExpressionsReturnWithinSwitchExpression:
ReturnTypeSubProcessor.replaceReturnWithYieldStatementProposals(context, problem, proposals);
break;
+ case IProblem.UnclosedCloseable:
+ case IProblem.PotentiallyUnclosedCloseable:
+ LocalCorrectionsSubProcessor.getTryWithResourceProposals(context, problem, proposals);
+ break;
default:
}
if (JavaModelUtil.is50OrHigher(context.getCompilationUnit().getJavaProject())) {
