diff --git a/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java b/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java index 87a1efabe18..5f17b97b885 100644 --- a/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java +++ b/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java @@ -515,9 +515,6 @@ public Void visitIdentifier(IdentifierTree tree, AnnotatedTypeMirror type) { type.replaceAnnotation(BOTTOM); } } - if (ElementUtils.isResourceVariable(elt)) { - type.replaceAnnotation(withoutClose(type.getPrimaryAnnotationInHierarchy(TOP))); - } return super.visitIdentifier(tree, type); } } diff --git a/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallTransfer.java b/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallTransfer.java index 6b22b3ed4bf..936d6cf4933 100644 --- a/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallTransfer.java +++ b/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallTransfer.java @@ -12,13 +12,11 @@ import javax.lang.model.element.AnnotationMirror; import javax.lang.model.element.Element; import javax.lang.model.type.TypeMirror; -import org.checkerframework.checker.mustcall.qual.MustCall; import org.checkerframework.checker.nullness.qual.MonotonicNonNull; import org.checkerframework.checker.nullness.qual.Nullable; import org.checkerframework.checker.resourceleak.ResourceLeakChecker; import org.checkerframework.dataflow.analysis.TransferInput; import org.checkerframework.dataflow.analysis.TransferResult; -import org.checkerframework.dataflow.cfg.node.AssignmentNode; import org.checkerframework.dataflow.cfg.node.LocalVariableNode; import org.checkerframework.dataflow.cfg.node.MethodInvocationNode; import org.checkerframework.dataflow.cfg.node.Node; @@ -31,7 +29,6 @@ import org.checkerframework.framework.flow.CFStore; import org.checkerframework.framework.flow.CFTransfer; import org.checkerframework.framework.flow.CFValue; -import org.checkerframework.javacutil.ElementUtils; import org.checkerframework.javacutil.TreePathUtil; import org.checkerframework.javacutil.TreeUtils; import org.checkerframework.javacutil.TypesUtils; @@ -114,28 +111,6 @@ private AnnotationMirror getDefaultStringType(StringConversionNode n) { return this.defaultStringType; } - @Override - public TransferResult visitAssignment( - AssignmentNode n, TransferInput in) { - TransferResult result = super.visitAssignment(n, in); - // Remove "close" from the type in the store for resource variables. - // The Resource Leak Checker relies on this code to avoid checking that - // resource variables are closed. - if (ElementUtils.isResourceVariable(TreeUtils.elementFromTree(n.getTarget().getTree()))) { - CFStore store = result.getRegularStore(); - JavaExpression expr = JavaExpression.fromNode(n.getTarget()); - CFValue value = store.getValue(expr); - AnnotationMirror withClose = - atypeFactory.getAnnotationByClass(value.getAnnotations(), MustCall.class); - if (withClose == null) { - return result; - } - AnnotationMirror withoutClose = atypeFactory.withoutClose(withClose); - insertIntoStores(result, expr, withoutClose); - } - return result; - } - @Override public TransferResult visitMethodInvocation( MethodInvocationNode n, TransferInput in) { diff --git a/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallVisitor.java b/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallVisitor.java index 368a8508996..397ee34bd94 100644 --- a/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallVisitor.java +++ b/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallVisitor.java @@ -17,7 +17,6 @@ import javax.lang.model.element.ExecutableElement; import javax.lang.model.element.TypeElement; import javax.lang.model.type.TypeMirror; -import org.checkerframework.checker.compilermsgs.qual.CompilerMessageKey; import org.checkerframework.checker.mustcall.qual.InheritableMustCall; import org.checkerframework.checker.mustcall.qual.MustCall; import org.checkerframework.checker.mustcall.qual.MustCallAlias; @@ -267,75 +266,6 @@ protected boolean skipReceiverSubtypeCheck( return true; } - /** - * This boolean is used to communicate between different levels of the common assignment check - * whether a given check is being carried out on a (pseudo-)assignment to a resource variable. In - * those cases, close doesn't need to be considered when doing the check, since close will always - * be called by Java. - * - *

The check for whether the LHS is a resource variable can only be carried out on the element, - * but the effect needs to happen at the stage where the type is available (i.e. close needs to be - * removed from the type). Thus, this variable is used to communicate that a resource variable was - * detected on the LHS. - */ - private boolean commonAssignmentCheckOnResourceVariable = false; - - /** - * {@inheritDoc} - * - *

Mark (using the {@code #commonAssignmentCheckOnResourceVariable} field of this class) any - * assignments where the LHS is a resource variable, so that close doesn't need to be considered. - * See {@link #commonAssignmentCheck(AnnotatedTypeMirror, AnnotatedTypeMirror, Tree, String, - * Object...)} for the code that uses and removes the mark. - */ - @Override - protected boolean commonAssignmentCheck( - Tree varTree, - ExpressionTree valueExp, - @CompilerMessageKey String errorKey, - Object... extraArgs) { - Element elt = TreeUtils.elementFromTree(varTree); - if (elt != null && elt.getKind() == ElementKind.RESOURCE_VARIABLE) { - commonAssignmentCheckOnResourceVariable = true; - } - return super.commonAssignmentCheck(varTree, valueExp, errorKey, extraArgs); - } - - /** - * {@inheritDoc} - * - *

Iff the LHS is a resource variable, then {@code #commonAssignmentCheckOnResourceVariable} - * will be true. This method guarantees that {@code #commonAssignmentCheckOnResourceVariable} will - * be false when it returns. - */ - @Override - protected boolean commonAssignmentCheck( - AnnotatedTypeMirror varType, - AnnotatedTypeMirror valueType, - Tree valueTree, - @CompilerMessageKey String errorKey, - Object... extraArgs) { - - if (commonAssignmentCheckOnResourceVariable) { - commonAssignmentCheckOnResourceVariable = false; - // The LHS has been marked as a resource variable. Skip the standard common assignment - // check; instead do a check that does not include "close". - AnnotationMirror varAnno = varType.getPrimaryAnnotationInHierarchy(atypeFactory.TOP); - AnnotationMirror valueAnno = valueType.getPrimaryAnnotationInHierarchy(atypeFactory.TOP); - if (qualHierarchy.isSubtypeShallow( - atypeFactory.withoutClose(valueAnno), - valueType.getUnderlyingType(), - atypeFactory.withoutClose(varAnno), - varType.getUnderlyingType())) { - return true; - } - // Note that in this case, the rest of the common assignment check should fail (barring - // an exception). Control falls through here to avoid duplicating error-issuing code. - } - // commonAssignmentCheckOnResourceVariable is already false, so no need to set it. - return super.commonAssignmentCheck(varType, valueType, valueTree, errorKey, extraArgs); - } - /** * This method typically issues a warning if the result type of the constructor is not top, * because in top-default type systems that indicates a potential problem. The Must Call Checker diff --git a/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java b/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java index f5fd3c0250a..ff6f7d7bf5f 100644 --- a/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java +++ b/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java @@ -74,7 +74,6 @@ import org.checkerframework.dataflow.util.NodeUtils; import org.checkerframework.framework.flow.CFStore; import org.checkerframework.framework.flow.CFValue; -import org.checkerframework.framework.type.AnnotatedTypeMirror; import org.checkerframework.framework.util.JavaExpressionParseUtil.JavaExpressionParseException; import org.checkerframework.framework.util.StringToJavaExpression; import org.checkerframework.javacutil.AnnotationUtils; @@ -711,8 +710,7 @@ private void checkCreatesMustCallForInvocation( *

  • 2) the expression already has a tracked Obligation (i.e. there is already a resource * alias in some Obligation's resource alias set that refers to the expression), or *
  • 3) the method in which the invocation occurs also has an @CreatesMustCallFor annotation, - * with the same expression, or - *
  • 4) the expression is a resource variable. + * with the same expression. * * * @param obligations the currently-tracked Obligations; this value is side-effected if there is @@ -739,10 +737,6 @@ private boolean isValidCreatesMustCallForExpression( // formal parameters are also represented by LocalVariable in the bodies of methods. // This satisfies case 1. return true; - } else if (ElementUtils.isResourceVariable(elt)) { - // The expression is a resource variable, and therefore will be closed, so we can - // treat it as owning (case 4). - return true; } else { Obligation toRemove = null; Obligation toAdd = null; @@ -1224,16 +1218,6 @@ private void updateObligationsForAssignment( new ResourceAlias(JavaExpression.fromNode(lhs), lhsElement, lhs.getTree())); } } - } else if (lhsElement.getKind() == ElementKind.RESOURCE_VARIABLE && isMustCallClose(rhs)) { - if (rhs instanceof FieldAccessNode) { - // Handling of @Owning fields is a completely separate check. - } else if (rhs instanceof LocalVariableNode) { - removeObligationsContainingVar( - obligations, - (LocalVariableNode) rhs, - MustCallAliasHandling.RETAIN_OBLIGATIONS_DERIVED_FROM_A_MUST_CALL_ALIAS_PARAMETER, - MethodExitKind.ALL); - } } else if (lhs instanceof LocalVariableNode) { LocalVariableNode lhsVar = (LocalVariableNode) lhs; updateObligationsForPseudoAssignment(obligations, assignmentNode, lhsVar, rhs); @@ -1264,22 +1248,6 @@ private boolean hasAtMostOneOwningField(TypeElement element) { return true; } - /** - * Returns true if must-call type of node only contains close. This is a helper method for - * handling try-with-resources statements. - * - * @param node the node - * @return true if must-call type of node only contains close - */ - /*package-private*/ boolean isMustCallClose(Node node) { - MustCallAnnotatedTypeFactory mcAtf = - typeFactory.getTypeFactoryOfSubchecker(MustCallChecker.class); - AnnotatedTypeMirror mustCallAnnotatedType = mcAtf.getAnnotatedType(node.getTree()); - AnnotationMirror mustCallAnnotation = - mustCallAnnotatedType.getPrimaryAnnotation(MustCall.class); - return typeFactory.getMustCallValues(mcAtf.withoutClose(mustCallAnnotation)).isEmpty(); - } - /** * Add a new alias to all Obligations that have {@code var} in their resource-alias set. This * method should be used when {@code var} and {@code newAlias} definitively point to the same diff --git a/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallInference.java b/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallInference.java index 2e04ce118b8..506ed69f0c8 100644 --- a/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallInference.java +++ b/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallInference.java @@ -369,8 +369,6 @@ private void analyzeOwnershipTransferAtAssignment( disposedFields.remove((VariableElement) lhsElement); } addOwningToParamsIfDisposedAtAssignment(obligations, rhsObligation, rhs); - } else if (lhsElement.getKind() == ElementKind.RESOURCE_VARIABLE && mcca.isMustCallClose(rhs)) { - addOwningToParamsIfDisposedAtAssignment(obligations, rhsObligation, rhs); } else if (lhs instanceof LocalVariableNode) { LocalVariableNode lhsVar = (LocalVariableNode) lhs; mcca.updateObligationsForPseudoAssignment(obligations, assignmentNode, lhsVar, rhs); diff --git a/checker/tests/mustcall/TryWithResourcesSimple.java b/checker/tests/mustcall/TryWithResourcesSimple.java index 79ff6497680..8c018db4446 100644 --- a/checker/tests/mustcall/TryWithResourcesSimple.java +++ b/checker/tests/mustcall/TryWithResourcesSimple.java @@ -1,4 +1,4 @@ -// A test that try-with-resources variables are always @MustCall({}). +// A test that try-with-resources variables are always @MustCall({"close"}). import java.io.*; import java.net.*; @@ -7,7 +7,7 @@ public class TryWithResourcesSimple { static void test(String address, int port) { try (Socket socket = new Socket(address, port)) { - @MustCall({}) Object s = socket; + @MustCall({"close"}) Object s = socket; } catch (Exception e) { } @@ -23,7 +23,7 @@ void test_fancy_sock(String address, int port) { // "close", which is the only MC method for Socket itself. try (Socket socket = getFancySocket()) { // :: error: (assignment) - @MustCall({}) Object s = socket; + @MustCall({"close"}) Object s = socket; } catch (Exception e) { } @@ -31,8 +31,8 @@ void test_fancy_sock(String address, int port) { static void test_poly(String address, int port) { try (Socket socket = new Socket(address, port)) { - // getChannel is @MustCallAlias (= poly) with the socket, so it should also be @MC({}) - @MustCall({}) Object s = socket.getChannel(); + // getChannel is @MustCallAlias (= poly) with the socket, so it should also be @MC({"close"}) + @MustCall({"close"}) Object s = socket.getChannel(); } catch (Exception e) { } @@ -41,7 +41,7 @@ static void test_poly(String address, int port) { static void test_two_mca_variables(String address, int port) { try (Socket socket = new Socket(address, port); InputStream in = socket.getInputStream()) { - @MustCall({}) Object s = in; + @MustCall({"close"}) Object s = in; } catch (Exception e) { } diff --git a/checker/tests/resourceleak/TryWithResourcesSimple.java b/checker/tests/resourceleak/TryWithResourcesDeclaration.java similarity index 64% rename from checker/tests/resourceleak/TryWithResourcesSimple.java rename to checker/tests/resourceleak/TryWithResourcesDeclaration.java index c53ccdaab94..b25e22eaaf9 100644 --- a/checker/tests/resourceleak/TryWithResourcesSimple.java +++ b/checker/tests/resourceleak/TryWithResourcesDeclaration.java @@ -1,11 +1,11 @@ -// A simple test that a try-with-resources socket doesn't issue a false positive. +// Tests for try-with-resources that declare local variables in the resource declaration. import java.io.*; import java.net.Socket; import java.nio.channels.*; import java.util.*; -class TryWithResourcesSimple { +class TryWithResourcesDeclaration { static void test(String address, int port) { try (Socket socket = new Socket(address, port)) { @@ -31,4 +31,13 @@ public boolean isPreUpgradableLayout(File oldF) throws IOException { return false; } } + + public void testNestedTryWithResourcesDecls(Properties prop, ClassLoader cl, String propfile) + throws Exception { + try (InputStream in = cl.getResourceAsStream(propfile)) { + try (InputStream fis = new FileInputStream(propfile)) { + prop.load(fis); + } + } + } } diff --git a/checker/tests/resourceleak/TryWithResourcesVariable.java b/checker/tests/resourceleak/TryWithResourcesVariable.java new file mode 100644 index 00000000000..26be08d9a56 --- /dev/null +++ b/checker/tests/resourceleak/TryWithResourcesVariable.java @@ -0,0 +1,124 @@ +// Test for try-with-resources where the resource declaration uses an existing variable + +import java.io.*; +import java.net.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +class TryWithResourcesVariable { + static void test1() throws Exception { + Socket socket = new Socket("127.0.0.1", 5050); + try (socket) { + + } catch (Exception e) { + + } + } + + static void test2(@Owning Socket socket) { + try (socket) { + + } catch (Exception e) { + + } + } + + static void test3(InetSocketAddress isa) { + Socket socket = new Socket(); + try (socket) { + socket.connect(isa); + } catch (Exception e) { + + } + } + + // :: error: (required.method.not.called) + static void test4(@Owning InputStream i1, @Owning InputStream i2) { + try { + try (i2) {} + // This will not run if i2.close() throws an IOException + i1.close(); + } catch (Exception e) { + + } + } + + static void test4Fixed(@Owning InputStream i1, @Owning InputStream i2) throws IOException { + try { + try (i2) {} + } catch (Exception e) { + } + i1.close(); + } + + @InheritableMustCall("disposer") + static class FinalResourceField { + final @Owning Socket socketField; + + FinalResourceField() { + try { + socketField = new Socket("127.0.0.1", 5050); + } catch (Exception e) { + throw new RuntimeException(e); + } + } + + @EnsuresCalledMethods(value = "this.socketField", methods = "close") + void disposer() { + try (socketField) { + + } catch (Exception e) { + + } + } + } + + static void closeFinalFieldUnsupported() throws Exception { + // This is a false positive (i.e., there is no resource leak), but our checker reports a warning + // since it does not support this coding pattern + // :: error: (required.method.not.called) + FinalResourceField finalResourceField = new FinalResourceField(); + try (finalResourceField.socketField) {} + } + + @InheritableMustCall("disposer") + static class FinalResourceFieldWrapper { + + final @Owning FinalResourceField frField = new FinalResourceField(); + + @EnsuresCalledMethods(value = "this.frField", methods = "disposer") + void disposer() { + this.frField.disposer(); + } + } + + static void closeWrapperUnsupported() throws Exception { + // This is a false positive (i.e., there is no resource leak), but our checker reports a warning + // since it does not support this coding pattern + // :: error: (required.method.not.called) + FinalResourceFieldWrapper finalResourceFieldWrapper = new FinalResourceFieldWrapper(); + try (finalResourceFieldWrapper.frField.socketField) {} + } + + @InheritableMustCall("disposer") + static class TwoFinalResourceFields { + final @Owning Socket socketField1; + final @Owning Socket socketField2; + + TwoFinalResourceFields(@Owning Socket socket1, @Owning Socket socket2) { + socketField1 = socket1; + socketField2 = socket2; + } + + @EnsuresCalledMethods(value = "this.socketField1", methods = "close") + @EnsuresCalledMethods(value = "this.socketField2", methods = "close") + void disposer() { + try (socketField1; + socketField2) { + + } catch (Exception e) { + + } + } + } +} diff --git a/dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java b/dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java index 6fee5c1095a..b2f13fe473f 100644 --- a/dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java +++ b/dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java @@ -1503,15 +1503,7 @@ public MethodInvocationNode visitMethodInvocation(MethodInvocationTree tree, Voi MethodInvocationNode node = new MethodInvocationNode(tree, target, arguments, getCurrentPath()); - List thrownTypes = method.getThrownTypes(); - Set thrownSet = - new LinkedHashSet<>(thrownTypes.size() + uncheckedExceptionTypes.size()); - // Add exceptions explicitly mentioned in the throws clause. - thrownSet.addAll(thrownTypes); - // Add types to account for unchecked exceptions - thrownSet.addAll(uncheckedExceptionTypes); - - ExtendedNode extendedNode = extendWithNodeWithExceptions(node, thrownSet); + ExtendedNode extendedNode = extendWithMethodInvocationNode(method, node); /* Check for the TerminatesExecution annotation. */ boolean terminatesExecution = @@ -1523,6 +1515,27 @@ public MethodInvocationNode visitMethodInvocation(MethodInvocationTree tree, Voi return node; } + /** + * Extends the CFG with a MethodInvocationNode, accounting for potential exceptions thrown by the + * invocation. + * + * @param method the invoked method + * @param node the invocation + * @return an ExtendedNode representing the invocation and its possible thrown exceptions + */ + private ExtendedNode extendWithMethodInvocationNode( + ExecutableElement method, MethodInvocationNode node) { + List thrownTypes = method.getThrownTypes(); + Set thrownSet = + new LinkedHashSet<>(thrownTypes.size() + uncheckedExceptionTypes.size()); + // Add exceptions explicitly mentioned in the throws clause. + thrownSet.addAll(thrownTypes); + // Add types to account for unchecked exceptions + thrownSet.addAll(uncheckedExceptionTypes); + + return extendWithNodeWithExceptions(node, thrownSet); + } + @Override public Node visitAssert(AssertTree tree, Void p) { @@ -3534,9 +3547,7 @@ public Node visitTry(TryTree tree, Void p) { List> catchLabels = CollectionsPlume.mapList( - (CatchTree c) -> { - return IPair.of(TreeUtils.typeOf(c.getParameter().getType()), new Label()); - }, + (CatchTree c) -> IPair.of(TreeUtils.typeOf(c.getParameter().getType()), new Label()), catches); // Store return/break/continue labels, just in case we need them for a finally block. @@ -3568,25 +3579,19 @@ public Node visitTry(TryTree tree, Void p) { tryStack.pushFrame(new TryCatchFrame(types, catchLabels)); - // Must scan the resources *after* we push frame to tryStack. Otherwise we can lose catch - // blocks. - // TODO: Should we handle try-with-resources blocks by also generating code for - // automatically closing the resources? - List resources = tree.getResources(); - for (Tree resource : resources) { - scan(resource, p); - } - extendWithNode( new MarkerNode( tree, "start of try block #" + TreeUtils.treeUids.get(tree), env.getTypeUtils())); - scan(tree.getBlock(), p); + + handleTryResourcesAndBlock(tree, p, tree.getResources()); + extendWithNode( new MarkerNode( tree, "end of try block #" + TreeUtils.treeUids.get(tree), env.getTypeUtils())); extendWithExtendedNode(new UnconditionalJump(firstNonNull(finallyLabel, doneLabel))); + // This pops the try-catch frame tryStack.popFrame(); int catchIndex = 0; @@ -3602,172 +3607,354 @@ public Node visitTry(TryTree tree, Void p) { } if (finallyLabel != null) { - // Reset values before analyzing the finally block! + handleFinally( + tree, + doneLabel, + finallyLabel, + exceptionalFinallyLabel, + () -> scan(finallyBlock, p), + oldReturnTargetLC, + oldBreakTargetLC, + oldBreakLabels, + oldContinueTargetLC, + oldContinueLabels); + } - tryStack.popFrame(); + addLabelForNextNode(doneLabel); - { // Scan 'finallyBlock' for only 'finallyLabel' (a successful path) - addLabelForNextNode(finallyLabel); - extendWithNode( - new MarkerNode( - tree, - "start of finally block #" + TreeUtils.treeUids.get(tree), - env.getTypeUtils())); - scan(finallyBlock, p); - extendWithNode( - new MarkerNode( - tree, "end of finally block #" + TreeUtils.treeUids.get(tree), env.getTypeUtils())); - extendWithExtendedNode(new UnconditionalJump(doneLabel)); - } + return null; + } - if (hasExceptionalPath(exceptionalFinallyLabel)) { - // If an exceptional path exists, scan 'finallyBlock' for 'exceptionalFinallyLabel', - // and scan copied 'finallyBlock' for 'finallyLabel' (a successful path). If there - // is no successful path, it will be removed in later phase. - // TODO: Don't we need a separate finally block for each kind of exception? - addLabelForNextNode(exceptionalFinallyLabel); - extendWithNode( - new MarkerNode( - tree, - "start of finally block for Throwable #" + TreeUtils.treeUids.get(tree), - env.getTypeUtils())); + /** + * A recursive helper method to handle the resource declarations (if any) in a {@link TryTree} and + * its main block. If the {@code resources} list is empty, the method scans the main block of the + * try statement and returns. Otherwise, the first resource declaration in {@code resources} is + * desugared, following the logic in JLS 14.20.3.1. A resource declaration r is desugared + * by adding the nodes for r itself to the CFG, followed by a synthetic nested {@code try} + * block and {@code finally} block. The synthetic {@code try} block contains any remaining + * resource declarations and the original try block (handled via recursion). The synthetic {@code + * finally} block contains a call to {@code close} for r, guaranteeing that on every path + * through the CFG, r is closed. + * + * @param tryTree the original try tree (with 0 or more resources) from the AST + * @param p the value to pass to calls to {@code scan} + * @param resources the remaining resource declarations to handle + */ + private void handleTryResourcesAndBlock(TryTree tryTree, Void p, List resources) { + if (resources.isEmpty()) { + // Either `tryTree` was not a try-with-resources, or this method was called recursively and + // all the resources have been handled. Just scan the main try block. + scan(tryTree.getBlock(), p); + return; + } - scan(finallyBlock, p); + // Handle the first resource declaration in the list. The rest will be handled by a recursive + // call. + Tree resourceDeclarationTree = resources.get(0); - NodeWithExceptionsHolder throwing = - extendWithNodeWithException( - new MarkerNode( - tree, - "end of finally block for Throwable #" + TreeUtils.treeUids.get(tree), - env.getTypeUtils()), - throwableType); + extendWithNode( + new MarkerNode( + resourceDeclarationTree, + "start of try for resource #" + TreeUtils.treeUids.get(resourceDeclarationTree), + env.getTypeUtils())); - throwing.setTerminatesExecution(true); - } + // Store return/break/continue labels. Generating a synthetic finally block for closing the + // resource requires creating fresh return/break/continue labels and then restoring the old + // labels afterward. + LabelCell oldReturnTargetLC = returnTargetLC; + LabelCell oldBreakTargetLC = breakTargetLC; + Map oldBreakLabels = breakLabels; + LabelCell oldContinueTargetLC = continueTargetLC; + Map oldContinueLabels = continueLabels; - if (returnTargetLC.wasAccessed()) { - addLabelForNextNode(returnTargetLC.peekLabel()); - returnTargetLC = oldReturnTargetLC; + // Add nodes for the resource declaration to the CFG. NOTE: it is critical to add these nodes + // *before* pushing a TryFinallyFrame for the finally block that will close the resource. If + // any exception occurs due to code within the resource declaration, the corresponding variable + // or field is *not* automatically closed (as it was never assigned a value). + Node resourceCloseNode = scan(resourceDeclarationTree, p); - extendWithNode( - new MarkerNode( - tree, - "start of finally block for return #" + TreeUtils.treeUids.get(tree), - env.getTypeUtils())); - scan(finallyBlock, p); - extendWithNode( - new MarkerNode( - tree, - "end of finally block for return #" + TreeUtils.treeUids.get(tree), - env.getTypeUtils())); - extendWithExtendedNode(new UnconditionalJump(returnTargetLC.accessLabel())); - } else { - returnTargetLC = oldReturnTargetLC; - } + // Now, set things up for our synthetic finally block that closes the resource. + Label doneLabel = new Label(); + Label finallyLabel = new Label(); + + Label exceptionalFinallyLabel = new Label(); + tryStack.pushFrame(new TryFinallyFrame(exceptionalFinallyLabel)); + + returnTargetLC = new LabelCell(); + + breakTargetLC = new LabelCell(); + breakLabels = new TryFinallyScopeMap(); + + continueTargetLC = new LabelCell(); + continueLabels = new TryFinallyScopeMap(); - if (breakTargetLC.wasAccessed()) { - addLabelForNextNode(breakTargetLC.peekLabel()); - breakTargetLC = oldBreakTargetLC; + extendWithNode( + new MarkerNode( + resourceDeclarationTree, + "start of try block for resource #" + TreeUtils.treeUids.get(resourceDeclarationTree), + env.getTypeUtils())); + // Recursively handle any remaining resource declarations and the main block of the try + handleTryResourcesAndBlock(tryTree, p, resources.subList(1, resources.size())); + extendWithNode( + new MarkerNode( + resourceDeclarationTree, + "end of try block for resource #" + TreeUtils.treeUids.get(resourceDeclarationTree), + env.getTypeUtils())); + + extendWithExtendedNode(new UnconditionalJump(finallyLabel)); + + // Generate the finally block that closes the resource + handleFinally( + resourceDeclarationTree, + doneLabel, + finallyLabel, + exceptionalFinallyLabel, + () -> addCloseCallForResource(resourceDeclarationTree, resourceCloseNode), + oldReturnTargetLC, + oldBreakTargetLC, + oldBreakLabels, + oldContinueTargetLC, + oldContinueLabels); + + addLabelForNextNode(doneLabel); + } + + /** + * Adds a synthetic {@code close} call to the CFG to close some resource variable declared or used + * in a try-with-resources. + * + * @param resourceDeclarationTree the resource declaration + * @param resourceToCloseNode node represented the variable or field on which {@code close} should + * be invoked + */ + private void addCloseCallForResource(Tree resourceDeclarationTree, Node resourceToCloseNode) { + Tree receiverTree = resourceDeclarationTree; + if (receiverTree instanceof VariableTree) { + receiverTree = treeBuilder.buildVariableUse((VariableTree) receiverTree); + handleArtificialTree(receiverTree); + } + + MemberSelectTree closeSelect = + treeBuilder.buildCloseMethodAccess((ExpressionTree) receiverTree); + handleArtificialTree(closeSelect); + + MethodInvocationTree closeCall = treeBuilder.buildMethodInvocation(closeSelect); + handleArtificialTree(closeCall); + + Node receiverNode = resourceToCloseNode; + if (receiverNode instanceof AssignmentNode) { + // variable declaration; use the LHS + receiverNode = ((AssignmentNode) resourceToCloseNode).getTarget(); + } + // TODO do we need to insert some kind of node representing a use of receiverNode + // (which can be either a LocalVariableNode or a FieldAccessNode)? + MethodAccessNode closeAccessNode = new MethodAccessNode(closeSelect, receiverNode); + closeAccessNode.setInSource(false); + extendWithNode(closeAccessNode); + MethodInvocationNode closeCallNode = + new MethodInvocationNode( + closeCall, closeAccessNode, Collections.emptyList(), getCurrentPath()); + closeCallNode.setInSource(false); + extendWithMethodInvocationNode(TreeUtils.elementFromUse(closeCall), closeCallNode); + } + + /** + * Shared logic for CFG generation for a finally block. The block may correspond to a {@link + * TryTree} originally in the source code, or it may be a synthetic finally block used to model + * closing of a resource due to try-with-resources. + * + * @param markerTree tree to reference when creating {@link MarkerNode}s for the finally block + * @param doneLabel label for the normal successor of the try block (no exceptions, returns, + * breaks, or continues) + * @param finallyLabel label for the entry of the finally block for the normal case + * @param exceptionalFinallyLabel label for entry of the finally block for when the try block + * throws an exception + * @param finallyBlockCFGGenerator generates CFG nodes and edges for the finally block + * @param oldReturnTargetLC old return target label cell, which gets restored to {@link + * #returnTargetLC} while handling the finally block + * @param oldBreakTargetLC old break target label cell, which gets restored to {@link + * #breakTargetLC} while handling the finally block + * @param oldBreakLabels old break labels, which get restored to {@link #breakLabels} while + * handling the finally block + * @param oldContinueTargetLC old continue target label cell, which gets restored to {@link + * #continueTargetLC} while handling the finally block + * @param oldContinueLabels old continue labels, which get restored to {@link #continueLabels} + * while handling the finally block + */ + private void handleFinally( + Tree markerTree, + Label doneLabel, + Label finallyLabel, + Label exceptionalFinallyLabel, + Runnable finallyBlockCFGGenerator, + LabelCell oldReturnTargetLC, + LabelCell oldBreakTargetLC, + Map oldBreakLabels, + LabelCell oldContinueTargetLC, + Map oldContinueLabels) { + // Reset values before analyzing the finally block! + + tryStack.popFrame(); + + { // Scan 'finallyBlock' for only 'finallyLabel' (a successful path) + addLabelForNextNode(finallyLabel); + extendWithNode( + new MarkerNode( + markerTree, + "start of finally block #" + TreeUtils.treeUids.get(markerTree), + env.getTypeUtils())); + finallyBlockCFGGenerator.run(); + extendWithNode( + new MarkerNode( + markerTree, + "end of finally block #" + TreeUtils.treeUids.get(markerTree), + env.getTypeUtils())); + extendWithExtendedNode(new UnconditionalJump(doneLabel)); + } + + if (hasExceptionalPath(exceptionalFinallyLabel)) { + // If an exceptional path exists, scan 'finallyBlock' for 'exceptionalFinallyLabel', + // and scan copied 'finallyBlock' for 'finallyLabel' (a successful path). If there + // is no successful path, it will be removed in later phase. + // TODO: Don't we need a separate finally block for each kind of exception? + addLabelForNextNode(exceptionalFinallyLabel); + extendWithNode( + new MarkerNode( + markerTree, + "start of finally block for Throwable #" + TreeUtils.treeUids.get(markerTree), + env.getTypeUtils())); + + finallyBlockCFGGenerator.run(); + + NodeWithExceptionsHolder throwing = + extendWithNodeWithException( + new MarkerNode( + markerTree, + "end of finally block for Throwable #" + TreeUtils.treeUids.get(markerTree), + env.getTypeUtils()), + throwableType); + + throwing.setTerminatesExecution(true); + } + + if (returnTargetLC.wasAccessed()) { + addLabelForNextNode(returnTargetLC.peekLabel()); + returnTargetLC = oldReturnTargetLC; + + extendWithNode( + new MarkerNode( + markerTree, + "start of finally block for return #" + TreeUtils.treeUids.get(markerTree), + env.getTypeUtils())); + finallyBlockCFGGenerator.run(); + extendWithNode( + new MarkerNode( + markerTree, + "end of finally block for return #" + TreeUtils.treeUids.get(markerTree), + env.getTypeUtils())); + extendWithExtendedNode(new UnconditionalJump(returnTargetLC.accessLabel())); + } else { + returnTargetLC = oldReturnTargetLC; + } + + if (breakTargetLC.wasAccessed()) { + addLabelForNextNode(breakTargetLC.peekLabel()); + breakTargetLC = oldBreakTargetLC; + extendWithNode( + new MarkerNode( + markerTree, + "start of finally block for break #" + TreeUtils.treeUids.get(markerTree), + env.getTypeUtils())); + finallyBlockCFGGenerator.run(); + extendWithNode( + new MarkerNode( + markerTree, + "end of finally block for break #" + TreeUtils.treeUids.get(markerTree), + env.getTypeUtils())); + extendWithExtendedNode(new UnconditionalJump(breakTargetLC.accessLabel())); + } else { + breakTargetLC = oldBreakTargetLC; + } + + Map accessedBreakLabels = ((TryFinallyScopeMap) breakLabels).getAccessedNames(); + if (!accessedBreakLabels.isEmpty()) { + breakLabels = oldBreakLabels; + + for (Map.Entry access : accessedBreakLabels.entrySet()) { + addLabelForNextNode(access.getValue()); extendWithNode( new MarkerNode( - tree, - "start of finally block for break #" + TreeUtils.treeUids.get(tree), + markerTree, + "start of finally block for break label " + + access.getKey() + + " #" + + TreeUtils.treeUids.get(markerTree), env.getTypeUtils())); - scan(finallyBlock, p); + finallyBlockCFGGenerator.run(); extendWithNode( new MarkerNode( - tree, - "end of finally block for break #" + TreeUtils.treeUids.get(tree), + markerTree, + "end of finally block for break label " + + access.getKey() + + " #" + + TreeUtils.treeUids.get(markerTree), env.getTypeUtils())); - extendWithExtendedNode(new UnconditionalJump(breakTargetLC.accessLabel())); - } else { - breakTargetLC = oldBreakTargetLC; + extendWithExtendedNode(new UnconditionalJump(breakLabels.get(access.getKey()))); } + } else { + breakLabels = oldBreakLabels; + } - Map accessedBreakLabels = ((TryFinallyScopeMap) breakLabels).getAccessedNames(); - if (!accessedBreakLabels.isEmpty()) { - breakLabels = oldBreakLabels; - - for (Map.Entry access : accessedBreakLabels.entrySet()) { - addLabelForNextNode(access.getValue()); - extendWithNode( - new MarkerNode( - tree, - "start of finally block for break label " - + access.getKey() - + " #" - + TreeUtils.treeUids.get(tree), - env.getTypeUtils())); - scan(finallyBlock, p); - extendWithNode( - new MarkerNode( - tree, - "end of finally block for break label " - + access.getKey() - + " #" - + TreeUtils.treeUids.get(tree), - env.getTypeUtils())); - extendWithExtendedNode(new UnconditionalJump(breakLabels.get(access.getKey()))); - } - } else { - breakLabels = oldBreakLabels; - } + if (continueTargetLC.wasAccessed()) { + addLabelForNextNode(continueTargetLC.peekLabel()); + continueTargetLC = oldContinueTargetLC; + + extendWithNode( + new MarkerNode( + markerTree, + "start of finally block for continue #" + TreeUtils.treeUids.get(markerTree), + env.getTypeUtils())); + finallyBlockCFGGenerator.run(); + extendWithNode( + new MarkerNode( + markerTree, + "end of finally block for continue #" + TreeUtils.treeUids.get(markerTree), + env.getTypeUtils())); + extendWithExtendedNode(new UnconditionalJump(continueTargetLC.accessLabel())); + } else { + continueTargetLC = oldContinueTargetLC; + } - if (continueTargetLC.wasAccessed()) { - addLabelForNextNode(continueTargetLC.peekLabel()); - continueTargetLC = oldContinueTargetLC; + Map accessedContinueLabels = + ((TryFinallyScopeMap) continueLabels).getAccessedNames(); + if (!accessedContinueLabels.isEmpty()) { + continueLabels = oldContinueLabels; + for (Map.Entry access : accessedContinueLabels.entrySet()) { + addLabelForNextNode(access.getValue()); extendWithNode( new MarkerNode( - tree, - "start of finally block for continue #" + TreeUtils.treeUids.get(tree), + markerTree, + "start of finally block for continue label " + + access.getKey() + + " #" + + TreeUtils.treeUids.get(markerTree), env.getTypeUtils())); - scan(finallyBlock, p); + finallyBlockCFGGenerator.run(); extendWithNode( new MarkerNode( - tree, - "end of finally block for continue #" + TreeUtils.treeUids.get(tree), + markerTree, + "end of finally block for continue label " + + access.getKey() + + " #" + + TreeUtils.treeUids.get(markerTree), env.getTypeUtils())); - extendWithExtendedNode(new UnconditionalJump(continueTargetLC.accessLabel())); - } else { - continueTargetLC = oldContinueTargetLC; - } - - Map accessedContinueLabels = - ((TryFinallyScopeMap) continueLabels).getAccessedNames(); - if (!accessedContinueLabels.isEmpty()) { - continueLabels = oldContinueLabels; - - for (Map.Entry access : accessedContinueLabels.entrySet()) { - addLabelForNextNode(access.getValue()); - extendWithNode( - new MarkerNode( - tree, - "start of finally block for continue label " - + access.getKey() - + " #" - + TreeUtils.treeUids.get(tree), - env.getTypeUtils())); - scan(finallyBlock, p); - extendWithNode( - new MarkerNode( - tree, - "end of finally block for continue label " - + access.getKey() - + " #" - + TreeUtils.treeUids.get(tree), - env.getTypeUtils())); - extendWithExtendedNode(new UnconditionalJump(continueLabels.get(access.getKey()))); - } - } else { - continueLabels = oldContinueLabels; + extendWithExtendedNode(new UnconditionalJump(continueLabels.get(access.getKey()))); } + } else { + continueLabels = oldContinueLabels; } - - addLabelForNextNode(doneLabel); - - return null; } /** diff --git a/javacutil/src/main/java/org/checkerframework/javacutil/trees/TreeBuilder.java b/javacutil/src/main/java/org/checkerframework/javacutil/trees/TreeBuilder.java index 7cac9176956..87c3d7047be 100644 --- a/javacutil/src/main/java/org/checkerframework/javacutil/trees/TreeBuilder.java +++ b/javacutil/src/main/java/org/checkerframework/javacutil/trees/TreeBuilder.java @@ -126,6 +126,40 @@ public MemberSelectTree buildIteratorMethodAccess(ExpressionTree iterableExpr) { return iteratorAccess; } + /** + * Build a {@link MemberSelectTree} for accessing the {@code close} method of an expression that + * implements {@link AutoCloseable}. This method is used when desugaring try-with-resources + * statements during CFG construction. + * + * @param autoCloseableExpr the expression + * @return the member select tree + */ + public MemberSelectTree buildCloseMethodAccess(ExpressionTree autoCloseableExpr) { + DeclaredType exprType = + (DeclaredType) TypesUtils.upperBound(TreeUtils.typeOf(autoCloseableExpr)); + assert exprType != null + : "expression must be of declared type AutoCloseable: " + autoCloseableExpr; + + TypeElement exprElement = (TypeElement) exprType.asElement(); + + // Find the close() method + Symbol.MethodSymbol closeMethod = null; + + for (ExecutableElement method : ElementFilter.methodsIn(elements.getAllMembers(exprElement))) { + if (method.getParameters().isEmpty() && method.getSimpleName().contentEquals("close")) { + closeMethod = (Symbol.MethodSymbol) method; + break; + } + } + + assert closeMethod != null + : "@AssumeAssertion(nullness): no close method declared for expression type"; + + JCTree.JCFieldAccess closeAccess = TreeUtils.Select(maker, autoCloseableExpr, closeMethod); + + return closeAccess; + } + /** * Builds an AST Tree to access the hasNext() method of an iterator. *