From be7c9983505853cc247fda3b92a17c0c6e816ed2 Mon Sep 17 00:00:00 2001 From: Martin Kellogg Date: Tue, 27 Dec 2022 18:47:46 -0500 Subject: [PATCH] Fix #5472 (#5476) The underlying problem is that `@NotOwning` and `@Owning` are owned by the Must Call Checker, not the Resource Leak Checker. So, only the Must Call Checker has access to such annotations if (and only if) they were written in stub files. I fixed both the problem described in #5472 and all other uses of `getDeclAnnotation(elt)` involving `Owning` or `NotOwning` in the consistency analyzer. --- checker/build.gradle | 2 ++ .../MustCallConsistencyAnalyzer.java | 33 ++++++++----------- .../ResourceLeakAnnotatedTypeFactory.java | 32 ++++++++++++++++++ checker/tests/resourceleak/JavaEETest.java | 7 ++++ 4 files changed, 55 insertions(+), 19 deletions(-) create mode 100644 checker/tests/resourceleak/JavaEETest.java diff --git a/checker/build.gradle b/checker/build.gradle index a6a7bf2268b..d09b9f95256 100644 --- a/checker/build.gradle +++ b/checker/build.gradle @@ -62,6 +62,8 @@ dependencies { testImplementation 'com.amazonaws:aws-java-sdk-kms' // The AWS SDK is used for testing the Called Methods Checker. testImplementation platform('com.amazonaws:aws-java-sdk-bom:1.12.293') + // For the Resource Leak Checker's support for JavaEE. + testImplementation 'javax.servlet:javax.servlet-api:3.1.0' testImplementation group: 'junit', name: 'junit', version: '4.13.2' testImplementation project(':framework-test') 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 ecf526952ea..4600c40ba1a 100644 --- a/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java +++ b/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java @@ -640,14 +640,14 @@ private boolean isValidCreatesMustCallForExpression( if (expression instanceof FieldAccess) { Element elt = ((FieldAccess) expression).getField(); if (!checker.hasOption(MustCallChecker.NO_LIGHTWEIGHT_OWNERSHIP) - && typeFactory.getDeclAnnotation(elt, Owning.class) != null) { + && typeFactory.hasOwning(elt)) { // The expression is an Owning field. This satisfies case 1. return true; } } else if (expression instanceof LocalVariable) { Element elt = ((LocalVariable) expression).getElement(); if (!checker.hasOption(MustCallChecker.NO_LIGHTWEIGHT_OWNERSHIP) - && typeFactory.getDeclAnnotation(elt, Owning.class) != null) { + && typeFactory.hasOwning(elt)) { // The expression is an Owning formal parameter. Note that this cannot actually // be a local variable (despite expressions's type being LocalVariable) because // the @Owning annotation can only be written on methods, parameters, and fields; @@ -956,19 +956,14 @@ private void removeObligationsAtOwnershipTransferToParameters( // check if parameter has an @Owning annotation VariableElement parameter = parameters.get(i); - Set annotationMirrors = typeFactory.getDeclAnnotations(parameter); - for (AnnotationMirror anno : annotationMirrors) { - if (AnnotationUtils.areSameByName( - anno, "org.checkerframework.checker.mustcall.qual.Owning")) { - Obligation localObligation = getObligationForVar(obligations, local); - // Passing to an owning parameter is not sufficient to resolve the - // obligation created from a MustCallAlias parameter, because the containing - // method must actually return the value. - if (!localObligation.derivedFromMustCallAlias()) { - // Transfer ownership! - obligations.remove(localObligation); - break; - } + if (typeFactory.hasOwning(parameter)) { + Obligation localObligation = getObligationForVar(obligations, local); + // Passing to an owning parameter is not sufficient to resolve the + // obligation created from a MustCallAlias parameter, because the containing + // method must actually return the value. + if (!localObligation.derivedFromMustCallAlias()) { + // Transfer ownership! + obligations.remove(localObligation); } } } @@ -1032,7 +1027,7 @@ private boolean isTransferOwnershipAtReturn(ControlFlowGraph cfg) { // not be transferred. MethodTree method = ((UnderlyingAST.CFGMethod) underlyingAST).getMethod(); ExecutableElement executableElement = TreeUtils.elementFromDeclaration(method); - return typeFactory.getDeclAnnotation(executableElement, NotOwning.class) == null; + return !typeFactory.hasNotOwning(executableElement); } return false; } @@ -1061,7 +1056,7 @@ private void updateObligationsForAssignment( if (lhsElement.getKind() == ElementKind.FIELD) { boolean isOwningField = !checker.hasOption(MustCallChecker.NO_LIGHTWEIGHT_OWNERSHIP) - && typeFactory.getDeclAnnotation(lhsElement, Owning.class) != null; + && typeFactory.hasOwning(lhsElement); // Check that the must-call obligations of the lhs have been satisfied, if the field is // non-final and owning. if (isOwningField @@ -1116,7 +1111,7 @@ private boolean hasAtMostOneOwningField(TypeElement element) { // Has an owning field already been encountered? boolean hasOwningField = false; for (VariableElement field : fields) { - if (typeFactory.getDeclAnnotation(field, Owning.class) != null) { + if (typeFactory.hasOwning(field)) { if (hasOwningField) { return false; } else { @@ -1635,7 +1630,7 @@ private boolean shouldTrackReturnType(MethodInvocationNode node) { return false; } // check for absence of @NotOwning annotation - return (typeFactory.getDeclAnnotation(executableElement, NotOwning.class) == null); + return !typeFactory.hasNotOwning(executableElement); } /** diff --git a/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakAnnotatedTypeFactory.java b/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakAnnotatedTypeFactory.java index c12f15993e6..0e65fabd9e3 100644 --- a/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakAnnotatedTypeFactory.java +++ b/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakAnnotatedTypeFactory.java @@ -23,6 +23,8 @@ import org.checkerframework.checker.mustcall.qual.CreatesMustCallFor; import org.checkerframework.checker.mustcall.qual.MustCall; import org.checkerframework.checker.mustcall.qual.MustCallAlias; +import org.checkerframework.checker.mustcall.qual.NotOwning; +import org.checkerframework.checker.mustcall.qual.Owning; import org.checkerframework.checker.nullness.qual.Nullable; import org.checkerframework.common.basetype.BaseTypeChecker; import org.checkerframework.dataflow.cfg.ControlFlowGraph; @@ -355,4 +357,34 @@ public ExecutableElement getCreatesMustCallForValueElement() { public ExecutableElement getCreatesMustCallForListValueElement() { return createsMustCallForListValueElement; } + + /** + * Does the given element have an {@code @NotOwning} annotation (including in stub files)? + * + *

Prefer this method to calling {@link #getDeclAnnotation(Element, Class)} on the type factory + * directly, which won't find this annotation in stub files (it only considers stub files loaded + * by this checker, not subcheckers). + * + * @param elt an element + * @return whether there is a NotOwning annotation on the given element + */ + public boolean hasNotOwning(Element elt) { + MustCallAnnotatedTypeFactory mcatf = getTypeFactoryOfSubchecker(MustCallChecker.class); + return mcatf.getDeclAnnotation(elt, NotOwning.class) != null; + } + + /** + * Does the given element have an {@code @Owning} annotation (including in stub files)? + * + *

Prefer this method to calling {@link #getDeclAnnotation(Element, Class)} on the type factory + * directly, which won't find this annotation in stub files (it only considers stub files loaded + * by this checker, not subcheckers). + * + * @param elt an element + * @return whether there is an Owning annotation on the given element + */ + public boolean hasOwning(Element elt) { + MustCallAnnotatedTypeFactory mcatf = getTypeFactoryOfSubchecker(MustCallChecker.class); + return mcatf.getDeclAnnotation(elt, Owning.class) != null; + } } diff --git a/checker/tests/resourceleak/JavaEETest.java b/checker/tests/resourceleak/JavaEETest.java new file mode 100644 index 00000000000..bb1f7431b49 --- /dev/null +++ b/checker/tests/resourceleak/JavaEETest.java @@ -0,0 +1,7 @@ +// Test for https://github.com/typetools/checker-framework/issues/5472 + +class JavaEETest { + static void foo(javax.servlet.ServletResponse s) throws java.io.IOException { + s.getWriter(); + } +}