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 42ea0217c08b..fc232e29ac9a 100644 --- a/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java +++ b/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java @@ -239,20 +239,21 @@ private void handleInvocation(Set obligations, Node node) { /** * If node is an invocation of a this or super constructor that has a MustCallAlias return type - * and a MustCallAlias parameter, check if any variable in the current set of obligations is being - * passed to the other constructor. If so, remove it from the obligations. + * and some MustCallAlias parameter(s), check if any variable in the current set of obligations is + * being passed to the other constructor. If so, remove it from the obligations. * * @param obligations current obligations * @param node a super or this constructor invocation */ private void handleThisOrSuperConstructorMustCallAlias(Set obligations, Node node) { - Node mustCallAliasArgument = getMustCallAliasArgumentNode(node); + List mustCallAliasArguments = getMustCallAliasArgumentNodes(node); // If the MustCallAlias argument is also in the set of obligations, then remove it -- its - // obligation has been fulfilled by being passed on to the MustCallAlias constructor - // (because a this/super constructor call can only occur in the body of another - // constructor). - if (mustCallAliasArgument instanceof LocalVariableNode) { - removeObligationsContainingVar(obligations, (LocalVariableNode) mustCallAliasArgument); + // obligation has been fulfilled by being passed on to the MustCallAlias constructor (because a + // this/super constructor call can only occur in the body of another constructor). + for (Node mustCallAliasArgument : mustCallAliasArguments) { + if (mustCallAliasArgument instanceof LocalVariableNode) { + removeObligationsContainingVar(obligations, (LocalVariableNode) mustCallAliasArgument); + } } } @@ -409,8 +410,8 @@ private boolean representSame(JavaExpression target, @Nullable JavaExpression en /** * Given a node representing a method or constructor call, checks that if the result of the call * has a non-empty {@code @MustCall} type, then the result is pseudo-assigned to some location - * that can take ownership of the result. Searches for the set of same resources in {@code - * obligations} and adds the new resource alias to it if one exists. Otherwise creates a new set. + * that can take ownership of the result. Searches for the set(s) of same resources in {@code + * obligations} and adds the new resource alias to it if any exists. Otherwise creates a new set. * * @param obligations the currently-tracked obligations. This is always side-effected: an * obligation is either modified (by removing it from the obligation set and adding a new one) @@ -429,40 +430,45 @@ private void trackInvocationResult(Set obligations, Node node) { return; } - // `mustCallAlias` is the argument passed in the MustCallAlias position, if any exists, - // otherwise null. - Node mustCallAlias = getMustCallAliasArgumentNode(node); - // If mustCallAlias is null and call returns @This, set mustCallAlias to the receiver. - if (mustCallAlias == null - && node instanceof MethodInvocationNode + // mustCallAliases is a (possibly-empty) list of arguments passed in a MustCallAlias position + List mustCallAliases = getMustCallAliasArgumentNodes(node); + // If call returns @This, add the receiver to mustCallAliases. + if (node instanceof MethodInvocationNode && typeFactory.returnsThis((MethodInvocationTree) tree)) { - mustCallAlias = - removeCastsAndGetTmpVarIfPresent(((MethodInvocationNode) node).getTarget().getReceiver()); + mustCallAliases.add( + removeCastsAndGetTmpVarIfPresent( + ((MethodInvocationNode) node).getTarget().getReceiver())); } ResourceAlias tmpVarAsResourceAlias = new ResourceAlias(new LocalVariable(tmpVar), tree); - if (mustCallAlias instanceof FieldAccessNode) { - // Do not track the call result if the MustCallAlias argument is a field. Handling of - // @Owning fields is a completely separate check, and there is never a need to track an - // alias of a non-@Owning field, as by definition such a field does not have - // obligations! - } else if (mustCallAlias instanceof LocalVariableNode) { - Obligation obligationContainingMustCallAlias = - getObligationForVar(obligations, (LocalVariableNode) mustCallAlias); - // If mustCallAlias is a local variable already being tracked, add tmpVarAsResourceAlias - // to the set containing mustCallAlias. - if (obligationContainingMustCallAlias != null) { - Set newResourceAliasSet = - FluentIterable.from(obligationContainingMustCallAlias.resourceAliases) - .append(tmpVarAsResourceAlias) - .toSet(); - obligations.remove(obligationContainingMustCallAlias); - obligations.add(new Obligation(newResourceAliasSet)); - } - } else { - // If mustCallAlias is neither a field nor a local already in the set of obligations, - // add it to a new set. + if (mustCallAliases.isEmpty()) { + // If mustCallAliases is an empty List, add tmpVarAsResourceAlias to a new set. obligations.add(new Obligation(ImmutableSet.of(tmpVarAsResourceAlias))); + } else { + for (Node mustCallAlias : mustCallAliases) { + if (mustCallAlias instanceof FieldAccessNode) { + // Do not track the call result if the MustCallAlias argument is a field. Handling of + // @Owning fields is a completely separate check, and there is never a need to track an + // alias of a non-@Owning field, as by definition such a field does not have obligations! + } else { + if (!(mustCallAlias instanceof LocalVariableNode)) { + throw new TypeSystemError( + "unexpected node type for mustCallAlias: " + mustCallAlias.getClass()); + } + Obligation obligationContainingMustCallAlias = + getObligationForVar(obligations, (LocalVariableNode) mustCallAlias); + // If mustCallAlias is a local variable already being tracked, add tmpVarAsResourceAlias + // to the set containing mustCallAlias. + if (obligationContainingMustCallAlias != null) { + Set newResourceAliasSet = + FluentIterable.from(obligationContainingMustCallAlias.resourceAliases) + .append(tmpVarAsResourceAlias) + .toSet(); + obligations.remove(obligationContainingMustCallAlias); + obligations.add(new Obligation(newResourceAliasSet)); + } + } + } } } @@ -516,9 +522,13 @@ private boolean shouldTrackInvocationResult(Set obligations, Node no * field or a definitely non-owning pointer */ private boolean returnTypeIsMustCallAliasWithUntrackable(MethodInvocationNode node) { - Node mustCallAliasArgument = getMustCallAliasArgumentNode(node); - return mustCallAliasArgument instanceof FieldAccessNode - || mustCallAliasArgument instanceof ThisNode; + List mustCallAliasArguments = getMustCallAliasArgumentNodes(node); + for (Node mustCallAliasArg : mustCallAliasArguments) { + if (!(mustCallAliasArg instanceof FieldAccessNode || mustCallAliasArg instanceof ThisNode)) { + return false; + } + } + return !mustCallAliasArguments.isEmpty(); } /** @@ -991,38 +1001,38 @@ private String receiverAsString(FieldAccessNode fieldAccessNode) { } /** - * Finds the argument passed in the {@code @MustCallAlias} position for a call. + * Finds the arguments passed in the {@code @MustCallAlias} positions for a call. * * @param callNode callNode representing the call; must be {@link MethodInvocationNode} or {@link * ObjectCreationNode} * @return if {@code callNode} invokes a method with a {@code @MustCallAlias} annotation on some - * formal parameter (or the receiver), returns the result of calling {@link - * #removeCastsAndGetTmpVarIfPresent(Node)} on the argument passed in that position. - * Otherwise, returns {@code null}. + * formal parameter(s) (or the receiver), returns the result of calling {@link + * #removeCastsAndGetTmpVarIfPresent(Node)} on the argument(s) passed in corresponding + * position(s). Otherwise, returns an empty List. */ - private @Nullable Node getMustCallAliasArgumentNode(Node callNode) { + private List getMustCallAliasArgumentNodes(Node callNode) { Preconditions.checkArgument( callNode instanceof MethodInvocationNode || callNode instanceof ObjectCreationNode); + List result = new ArrayList<>(); if (!typeFactory.hasMustCallAlias(callNode.getTree())) { - return null; + return result; } - Node result = null; List args = getArgumentsOfInvocation(callNode); List parameters = getParametersOfInvocation(callNode); for (int i = 0; i < args.size(); i++) { if (typeFactory.hasMustCallAlias(parameters.get(i))) { - result = args.get(i); - break; + result.add(removeCastsAndGetTmpVarIfPresent(args.get(i))); } } // If none of the parameters were @MustCallAlias, it must be the receiver - if (result == null && callNode instanceof MethodInvocationNode) { - result = ((MethodInvocationNode) callNode).getTarget().getReceiver(); + if (result.isEmpty() && callNode instanceof MethodInvocationNode) { + result.add( + removeCastsAndGetTmpVarIfPresent( + ((MethodInvocationNode) callNode).getTarget().getReceiver())); } - result = removeCastsAndGetTmpVarIfPresent(result); return result; } diff --git a/checker/tests/resourceleak/MultipleMethodParamsMustCallAliasTest.java b/checker/tests/resourceleak/MultipleMethodParamsMustCallAliasTest.java new file mode 100644 index 000000000000..5337e576b44a --- /dev/null +++ b/checker/tests/resourceleak/MultipleMethodParamsMustCallAliasTest.java @@ -0,0 +1,89 @@ +import java.io.*; +import java.net.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; +import org.checkerframework.common.returnsreceiver.qual.*; + +class MultipleMethodParamsMustCallAliasTest { + + void testMultiMethodParamsCorrect1(@Owning InputStream in1, @Owning InputStream in2) + throws IOException { + + ReplicaInputStreams r = new ReplicaInputStreams(in1, in2); + + r.close(); + } + + void testMultiMethodParamsCorrect2(@Owning InputStream in1, @Owning InputStream in2) + throws IOException { + + ReplicaInputStreams r = new ReplicaInputStreams(in1, in2); + + try { + in1.close(); + } catch (IOException e) { + } finally { + in2.close(); + } + } + + // It's a FP, see: https://github.com/typetools/checker-framework/issues/4843 + // :: error: required.method.not.called + void testMultiMethodParamsCorrect3(@Owning InputStream in1, @Owning InputStream in2) + throws IOException { + + ReplicaInputStreams r = new ReplicaInputStreams(in1, in2); + + try { + in1.close(); + } finally { + in2.close(); + } + } + + // :: error: required.method.not.called + void testMultiMethodParamsWrong1(@Owning InputStream in1, @Owning InputStream in2) + throws IOException { + + ReplicaInputStreams r = new ReplicaInputStreams(in1, in2); + + in1.close(); + } + + // :: error: required.method.not.called + void testMultiMethodParamsWrong2(@Owning InputStream in1, @Owning InputStream in2) + throws IOException { + + ReplicaInputStreams r = new ReplicaInputStreams(in1, in2); + + in2.close(); + } + + // :: error: required.method.not.called + void testMultiMethodParamsWrong3(@Owning InputStream in1) throws IOException { + // :: error: required.method.not.called + Socket socket = new Socket("address", 12); + ReplicaInputStreams r = new ReplicaInputStreams(in1, socket.getInputStream()); + } + + class ReplicaInputStreams implements Closeable { + + private final @Owning InputStream in1; + private final @Owning InputStream in2; + + public @MustCallAlias ReplicaInputStreams( + @MustCallAlias InputStream i1, @MustCallAlias InputStream i2) { + this.in1 = i1; + this.in2 = i2; + } + + @Override + @EnsuresCalledMethods( + value = {"this.in1", "this.in2"}, + methods = {"close"}) + public void close() throws IOException { + in1.close(); + in2.close(); + } + } +}