diff --git a/checker/src/main/java/org/checkerframework/checker/calledmethods/CalledMethodsAnalysis.java b/checker/src/main/java/org/checkerframework/checker/calledmethods/CalledMethodsAnalysis.java index b1de3289a20..607067af8ed 100644 --- a/checker/src/main/java/org/checkerframework/checker/calledmethods/CalledMethodsAnalysis.java +++ b/checker/src/main/java/org/checkerframework/checker/calledmethods/CalledMethodsAnalysis.java @@ -2,9 +2,9 @@ import com.google.common.collect.ImmutableSet; import com.sun.tools.javac.code.Type; -import java.util.HashSet; import java.util.Set; import javax.lang.model.type.TypeMirror; +import org.checkerframework.checker.signature.qual.CanonicalName; import org.checkerframework.common.accumulation.AccumulationAnalysis; import org.checkerframework.common.basetype.BaseTypeChecker; @@ -18,17 +18,16 @@ public class CalledMethodsAnalysis extends AccumulationAnalysis { * The fully-qualified names of the exception types that are ignored by this checker when * computing dataflow stores. */ - protected static final Set ignoredExceptionTypes = - new HashSet<>( - ImmutableSet.of( - // Use the Nullness Checker instead. - NullPointerException.class.getCanonicalName(), - // Ignore run-time errors, which cannot be predicted statically. Doing - // so is unsound in the sense that they could still occur - e.g., the - // program could run out of memory - but if they did, the checker's - // results would be useless anyway. - Error.class.getCanonicalName(), - RuntimeException.class.getCanonicalName())); + protected static final Set<@CanonicalName String> ignoredExceptionTypes = + ImmutableSet.of( + // Use the Nullness Checker instead. + NullPointerException.class.getCanonicalName(), + // Ignore run-time errors, which cannot be predicted statically. Doing + // so is unsound in the sense that they could still occur - e.g., the + // program could run out of memory - but if they did, the checker's + // results would be useless anyway. + Error.class.getCanonicalName(), + RuntimeException.class.getCanonicalName()); /** * Creates a new {@code CalledMethodsAnalysis}. 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 7e2170d270f..f3685970254 100644 --- a/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java +++ b/checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java @@ -10,8 +10,6 @@ import com.sun.source.tree.Tree; import com.sun.source.tree.VariableTree; import com.sun.source.util.TreePath; -import com.sun.tools.javac.code.Type; -import java.io.UnsupportedEncodingException; import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Collection; @@ -34,7 +32,6 @@ import javax.lang.model.element.Element; import javax.lang.model.element.ElementKind; import javax.lang.model.element.ExecutableElement; -import javax.lang.model.element.Name; import javax.lang.model.element.TypeElement; import javax.lang.model.element.VariableElement; import javax.lang.model.type.TypeKind; @@ -48,8 +45,6 @@ import org.checkerframework.checker.mustcall.qual.NotOwning; import org.checkerframework.checker.mustcall.qual.Owning; import org.checkerframework.checker.nullness.qual.Nullable; -import org.checkerframework.checker.signature.qual.FullyQualifiedName; -import org.checkerframework.common.accumulation.AccumulationAnalysis; import org.checkerframework.common.accumulation.AccumulationStore; import org.checkerframework.common.accumulation.AccumulationValue; import org.checkerframework.dataflow.cfg.ControlFlowGraph; @@ -182,7 +177,7 @@ class MustCallConsistencyAnalyzer { private final ResourceLeakChecker checker; /** The analysis from the Resource Leak Checker, used to get input stores based on CFG blocks. */ - private final AccumulationAnalysis analysis; + private final ResourceLeakAnalysis analysis; /** True if -AnoLightweightOwnership was passed on the command line. */ private final boolean noLightweightOwnership; @@ -561,7 +556,7 @@ public String stringForErrorMessage() { * so this constructor cannot get it directly. */ /*package-private*/ MustCallConsistencyAnalyzer( - ResourceLeakAnnotatedTypeFactory typeFactory, AccumulationAnalysis analysis) { + ResourceLeakAnnotatedTypeFactory typeFactory, ResourceLeakAnalysis analysis) { this.typeFactory = typeFactory; this.checker = (ResourceLeakChecker) typeFactory.getChecker(); this.analysis = analysis; @@ -1906,8 +1901,8 @@ private boolean shouldTrackReturnType(MethodInvocationNode node) { /** * Get all successor blocks for some block, except for those corresponding to ignored exception - * types. See {@link #ignoredExceptionTypes}. Each exceptional successor is paired with the type - * of exception that leads to it, for use in error messages. + * types. See {@link ResourceLeakAnalysis#isIgnoredExceptionType(TypeMirror)}. Each exceptional + * successor is paired with the type of exception that leads to it, for use in error messages. * * @param block input block * @return set of pairs (b, t), where b is a successor block, and t is the type of exception for @@ -1927,7 +1922,7 @@ private boolean shouldTrackReturnType(MethodInvocationNode node) { Map> exceptionalSuccessors = excBlock.getExceptionalSuccessors(); for (Map.Entry> entry : exceptionalSuccessors.entrySet()) { TypeMirror exceptionType = entry.getKey(); - if (!isIgnoredExceptionType(((Type) exceptionType).tsym.getQualifiedName())) { + if (!analysis.isIgnoredExceptionType(exceptionType)) { for (Block exSucc : entry.getValue()) { result.add(IPair.of(exSucc, exceptionType)); } @@ -2477,58 +2472,6 @@ private void incrementMustCallImpl(TypeMirror type) { .isSubtypeQualifiersOnly(cmAnno, cmAnnoForMustCallMethods); } - /** - * The exception types in this set are ignored in the CFG when determining if a resource leaks - * along an exceptional path. These kinds of errors fall into a few categories: runtime errors, - * errors that the JVM can issue on any statement, and errors that can be prevented by running - * some other CF checker. - * - *

Package-private to permit access from {@link ResourceLeakAnalysis}. - */ - /*package-private*/ static final Set ignoredExceptionTypes = - new HashSet<>( - ImmutableSet.of( - // Any method call has a CFG edge for Throwable/RuntimeException/Error - // to represent run-time misbehavior. Ignore it. - Throwable.class.getCanonicalName(), - Error.class.getCanonicalName(), - RuntimeException.class.getCanonicalName(), - // Use the Nullness Checker to prove this won't happen. - NullPointerException.class.getCanonicalName(), - // These errors can't be predicted statically, so ignore them and assume - // they won't happen. - ClassCircularityError.class.getCanonicalName(), - ClassFormatError.class.getCanonicalName(), - NoClassDefFoundError.class.getCanonicalName(), - OutOfMemoryError.class.getCanonicalName(), - // It's not our problem if the Java type system is wrong. - ClassCastException.class.getCanonicalName(), - // It's not our problem if the code is going to divide by zero. - ArithmeticException.class.getCanonicalName(), - // Use the Index Checker to prevent these errors. - ArrayIndexOutOfBoundsException.class.getCanonicalName(), - NegativeArraySizeException.class.getCanonicalName(), - // Most of the time, this exception is infeasible, as the charset used - // is guaranteed to be present by the Java spec (e.g., "UTF-8"). - // Eventually, this exclusion could be refined by looking at the charset - // being requested. - UnsupportedEncodingException.class.getCanonicalName())); - - /** - * Is {@code exceptionClassName} an exception type the checker ignores, to avoid excessive false - * positives? For now the checker ignores most runtime exceptions (especially the runtime - * exceptions that can occur at any point during the program due to something going wrong in the - * JVM, like OutOfMemoryError and ClassCircularityError) and exceptions that can be proved to - * never occur by another Checker Framework built-in checker, such as null-pointer dereferences - * (the Nullness Checker) and out-of-bounds array indexing (the Index Checker). - * - * @param exceptionClassName the fully-qualified name of the exception - * @return true if the given exception class should be ignored - */ - private static boolean isIgnoredExceptionType(@FullyQualifiedName Name exceptionClassName) { - return ignoredExceptionTypes.contains(exceptionClassName.toString()); - } - /** * If the input {@code state} has not been visited yet, add it to {@code visited} and {@code * worklist}. diff --git a/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakAnalysis.java b/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakAnalysis.java index 8de3ec4142b..1dab92a1da8 100644 --- a/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakAnalysis.java +++ b/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakAnalysis.java @@ -1,15 +1,22 @@ package org.checkerframework.checker.resourceleak; +import javax.lang.model.type.TypeMirror; import org.checkerframework.checker.calledmethods.CalledMethodsAnalysis; import org.checkerframework.checker.calledmethods.CalledMethodsAnnotatedTypeFactory; -import org.checkerframework.common.basetype.BaseTypeChecker; /** - * This variant of CFAnalysis extends the set of ignored exception types to include all those - * ignored by the {@link MustCallConsistencyAnalyzer}. See {@link - * MustCallConsistencyAnalyzer#ignoredExceptionTypes}. + * This variant of CFAnalysis extends the set of ignored exception types. + * + * @see ResourceLeakChecker#getIgnoredExceptions() */ public class ResourceLeakAnalysis extends CalledMethodsAnalysis { + + /** + * The set of exceptions to ignore, cached from {@link + * ResourceLeakChecker#getIgnoredExceptions()}. + */ + private final SetOfTypes ignoredExceptions; + /** * Creates a new {@code CalledMethodsAnalysis}. * @@ -17,8 +24,13 @@ public class ResourceLeakAnalysis extends CalledMethodsAnalysis { * @param factory the factory */ protected ResourceLeakAnalysis( - BaseTypeChecker checker, CalledMethodsAnnotatedTypeFactory factory) { + ResourceLeakChecker checker, CalledMethodsAnnotatedTypeFactory factory) { super(checker, factory); - ignoredExceptionTypes.addAll(MustCallConsistencyAnalyzer.ignoredExceptionTypes); + this.ignoredExceptions = checker.getIgnoredExceptions(); + } + + @Override + public boolean isIgnoredExceptionType(TypeMirror exceptionType) { + return ignoredExceptions.contains(getTypes(), exceptionType); } } 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 adf5f107a66..e8b16023e6a 100644 --- a/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakAnnotatedTypeFactory.java +++ b/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakAnnotatedTypeFactory.java @@ -131,7 +131,7 @@ public AnnotationMirror createCalledMethods(String... val) { @Override public void postAnalyze(ControlFlowGraph cfg) { MustCallConsistencyAnalyzer mustCallConsistencyAnalyzer = - new MustCallConsistencyAnalyzer(this, this.analysis); + new MustCallConsistencyAnalyzer(this, (ResourceLeakAnalysis) this.analysis); mustCallConsistencyAnalyzer.analyze(cfg); // Inferring owning annotations for @Owning fields/parameters, @EnsuresCalledMethods for @@ -148,7 +148,7 @@ public void postAnalyze(ControlFlowGraph cfg) { @Override protected ResourceLeakAnalysis createFlowAnalysis() { - return new ResourceLeakAnalysis(checker, this); + return new ResourceLeakAnalysis((ResourceLeakChecker) checker, this); } /** diff --git a/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakChecker.java b/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakChecker.java index ec55e6d6712..d26819bfd4a 100644 --- a/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakChecker.java +++ b/checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakChecker.java @@ -1,11 +1,20 @@ package org.checkerframework.checker.resourceleak; +import com.google.common.collect.ImmutableSet; +import java.io.UnsupportedEncodingException; +import java.util.ArrayList; +import java.util.List; import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import javax.lang.model.element.TypeElement; +import javax.lang.model.type.TypeMirror; import javax.tools.Diagnostic; import org.checkerframework.checker.calledmethods.CalledMethodsChecker; import org.checkerframework.checker.compilermsgs.qual.CompilerMessageKey; import org.checkerframework.checker.mustcall.MustCallChecker; import org.checkerframework.checker.mustcall.MustCallNoCreatesMustCallForChecker; +import org.checkerframework.checker.nullness.qual.MonotonicNonNull; import org.checkerframework.checker.nullness.qual.Nullable; import org.checkerframework.common.basetype.BaseTypeChecker; import org.checkerframework.common.basetype.BaseTypeVisitor; @@ -21,6 +30,7 @@ "permitStaticOwning", "permitInitializationLeak", ResourceLeakChecker.COUNT_MUST_CALL, + ResourceLeakChecker.IGNORED_EXCEPTIONS, MustCallChecker.NO_CREATES_MUSTCALLFOR, MustCallChecker.NO_LIGHTWEIGHT_OWNERSHIP, MustCallChecker.NO_RESOURCE_ALIASES, @@ -39,6 +49,63 @@ public ResourceLeakChecker() {} */ public static final String COUNT_MUST_CALL = "countMustCall"; + /** + * The exception types in this set are ignored in the CFG when determining if a resource leaks + * along an exceptional path. These kinds of errors fall into a few categories: runtime errors, + * errors that the JVM can issue on any statement, and errors that can be prevented by running + * some other CF checker. + */ + private static final SetOfTypes DEFAULT_IGNORED_EXCEPTIONS = + SetOfTypes.anyOfTheseNames( + ImmutableSet.of( + // Any method call has a CFG edge for Throwable/RuntimeException/Error + // to represent run-time misbehavior. Ignore it. + Throwable.class.getCanonicalName(), + Error.class.getCanonicalName(), + RuntimeException.class.getCanonicalName(), + // Use the Nullness Checker to prove this won't happen. + NullPointerException.class.getCanonicalName(), + // These errors can't be predicted statically, so ignore them and assume + // they won't happen. + ClassCircularityError.class.getCanonicalName(), + ClassFormatError.class.getCanonicalName(), + NoClassDefFoundError.class.getCanonicalName(), + OutOfMemoryError.class.getCanonicalName(), + // It's not our problem if the Java type system is wrong. + ClassCastException.class.getCanonicalName(), + // It's not our problem if the code is going to divide by zero. + ArithmeticException.class.getCanonicalName(), + // Use the Index Checker to prevent these errors. + ArrayIndexOutOfBoundsException.class.getCanonicalName(), + NegativeArraySizeException.class.getCanonicalName(), + // Most of the time, this exception is infeasible, as the charset used + // is guaranteed to be present by the Java spec (e.g., "UTF-8"). + // Eventually, this exclusion could be refined by looking at the charset + // being requested. + UnsupportedEncodingException.class.getCanonicalName())); + + /** + * Command-line option for controlling which exceptions are ignored. + * + * @see #DEFAULT_IGNORED_EXCEPTIONS + * @see #getIgnoredExceptions() + */ + public static final String IGNORED_EXCEPTIONS = "resourceLeakIgnoredExceptions"; + + /** + * A pattern that matches one or more consecutive commas, optionally preceded and followed by + * whitespace. + */ + private static final Pattern COMMAS = Pattern.compile("\\s*(?:" + Pattern.quote(",") + "\\s*)+"); + + /** + * A pattern that matches an exception specifier for the {@link #IGNORED_EXCEPTIONS} option: an + * optional "=" followed by a qualified name. The whole thing can be padded with whitespace. + */ + private static final Pattern EXCEPTION_SPECIFIER = + Pattern.compile( + "^\\s*" + "(" + Pattern.quote("=") + "\\s*" + ")?" + "(\\w+(?:\\.\\w+)*)" + "\\s*$"); + /** * Ordinarily, when the -Ainfer flag is used, whole-program inference is run for every checker and * sub-checker. However, the Resource Leak Checker is different. The -Ainfer flag enables the @@ -60,6 +127,14 @@ public ResourceLeakChecker() {} */ private int numMustCallFailed = 0; + /** + * The cached set of ignored exceptions parsed from {@link #IGNORED_EXCEPTIONS}. Caching this + * field prevents the checker from issuing duplicate warnings about missing exception types. + * + * @see #getIgnoredExceptions() + */ + private @MonotonicNonNull SetOfTypes ignoredExceptions = null; + @Override protected Set> getImmediateSubcheckerClasses() { Set> checkers = super.getImmediateSubcheckerClasses(); @@ -103,4 +178,114 @@ public void typeProcessingOver() { } super.typeProcessingOver(); } + + /** + * Get the set of exceptions that should be ignored. This set comes from the {@link + * #IGNORED_EXCEPTIONS} option if it was provided, or {@link #DEFAULT_IGNORED_EXCEPTIONS} if not. + * + * @return the set of exceptions to ignore + */ + public SetOfTypes getIgnoredExceptions() { + SetOfTypes result = ignoredExceptions; + if (result == null) { + String ignoredExceptionsOptionValue = getOption(IGNORED_EXCEPTIONS); + result = + ignoredExceptionsOptionValue == null + ? DEFAULT_IGNORED_EXCEPTIONS + : parseIgnoredExceptions(ignoredExceptionsOptionValue); + ignoredExceptions = result; + } + return result; + } + + /** + * Parse the argument given for the {@link #IGNORED_EXCEPTIONS} option. Warnings will be issued + * for any problems in the argument, for instance if any of the named exceptions cannot be found. + * + * @param ignoredExceptionsOptionValue the value given for {@link #IGNORED_EXCEPTIONS} + * @return the set of ignored exceptions + */ + protected SetOfTypes parseIgnoredExceptions(String ignoredExceptionsOptionValue) { + String[] exceptions = COMMAS.split(ignoredExceptionsOptionValue); + List sets = new ArrayList<>(); + for (String e : exceptions) { + SetOfTypes set = parseExceptionSpecifier(e, ignoredExceptionsOptionValue); + if (set != null) { + sets.add(set); + } + } + return SetOfTypes.union(sets.toArray(new SetOfTypes[0])); + } + + /** + * Parse a single exception specifier from the {@link #IGNORED_EXCEPTIONS} option and issue + * warnings if it does not parse. See {@link #EXCEPTION_SPECIFIER} for a description of the + * syntax. + * + * @param exceptionSpecifier the exception specifier to parse + * @param ignoredExceptionsOptionValue the whole value of the {@link #IGNORED_EXCEPTIONS} option; + * only used for error reporting + * @return the parsed set of types, or null if the value does not parse + */ + @SuppressWarnings({ + // user input might not be a legal @CanonicalName, but it should be safe to pass to + // `SetOfTypes.anyOfTheseNames` + "signature:argument", + }) + protected @Nullable SetOfTypes parseExceptionSpecifier( + String exceptionSpecifier, String ignoredExceptionsOptionValue) { + Matcher m = EXCEPTION_SPECIFIER.matcher(exceptionSpecifier); + if (m.matches()) { + @Nullable String equalsSign = m.group(1); + String qualifiedName = m.group(2); + + if (qualifiedName.equalsIgnoreCase("default")) { + return DEFAULT_IGNORED_EXCEPTIONS; + } + TypeMirror type = checkCanonicalName(qualifiedName); + if (type == null) { + // There is a chance that the user named a real type, but the class is not accessible for + // some reason. We'll issue a warning (in case this was a typo) but add the type as + // ignored anyway (in case it's just an inaccessible type). + // + // Note that if the user asked to ignore subtypes of this exception, this code won't do it + // because we can't know what those subtypes are. We have to treat this as if it were + // "=qualifiedName" even if no equals sign was provided. + message( + Diagnostic.Kind.WARNING, + "The exception '%s' appears in the -A%s=%s option, but it does not seem to exist", + exceptionSpecifier, + IGNORED_EXCEPTIONS, + ignoredExceptionsOptionValue); + return SetOfTypes.anyOfTheseNames(ImmutableSet.of(qualifiedName)); + } else { + return equalsSign == null ? SetOfTypes.allSubtypes(type) : SetOfTypes.singleton(type); + } + } else if (!exceptionSpecifier.trim().isEmpty()) { + message( + Diagnostic.Kind.WARNING, + "The string '%s' appears in the -A%s=%s option, but it is not a legal exception specifier", + exceptionSpecifier, + IGNORED_EXCEPTIONS, + ignoredExceptionsOptionValue); + } + return null; + } + + /** + * Check if the given String refers to an actual type. + * + * @param s any string + * @return the referenced type, or null if it does not exist + */ + @SuppressWarnings({ + "signature:argument", // `s` is not a qualified name, but we pass it to getTypeElement anyway + }) + protected @Nullable TypeMirror checkCanonicalName(String s) { + TypeElement elem = getProcessingEnvironment().getElementUtils().getTypeElement(s); + if (elem == null) { + return null; + } + return types.getDeclaredType(elem); + } } diff --git a/checker/src/main/java/org/checkerframework/checker/resourceleak/SetOfTypes.java b/checker/src/main/java/org/checkerframework/checker/resourceleak/SetOfTypes.java new file mode 100644 index 00000000000..4187adfc506 --- /dev/null +++ b/checker/src/main/java/org/checkerframework/checker/resourceleak/SetOfTypes.java @@ -0,0 +1,97 @@ +package org.checkerframework.checker.resourceleak; + +import com.google.common.collect.ImmutableSet; +import com.sun.tools.javac.code.Type; +import javax.lang.model.type.TypeMirror; +import javax.lang.model.util.Types; +import org.checkerframework.checker.signature.qual.CanonicalName; +import org.checkerframework.dataflow.qual.Pure; + +/** + * A set of types. + * + *

Important properties of this class: + * + *

    + *
  • No defined equality: in general, equality between these sets is prohibitively difficult to + * compute, and therefore this class uses reference equality. + *
  • Unknown size: it is not possible to know the true size of a set like {@link + * #allSubtypes(TypeMirror)}. By extension, it is not possible to iterate over a {@code + * SetOfTypes}. + *
  • Immutable: instances of this class can be created but not modified. + *
+ */ +public interface SetOfTypes { + + /** + * Test whether this set contains the given type. + * + * @param typeUtils a {@code Types} object for computing the relationships between types + * @param type the type in question + * @return true if this set contains {@code type}, or false otherwise + */ + @Pure + boolean contains(Types typeUtils, TypeMirror type); + + /** An empty set of types. */ + SetOfTypes EMPTY = (typeUtils, type) -> false; + + /** + * Create a set containing exactly the given type, but not its subtypes. + * + * @param t the type + * @return a set containing only {@code t} + */ + @Pure + static SetOfTypes singleton(TypeMirror t) { + return (typeUtils, u) -> typeUtils.isSameType(t, u); + } + + /** + * Create a set containing the given type and all of its subtypes. + * + * @param t the type + * @return a set containing {@code t} and its subtypes + */ + @Pure + static SetOfTypes allSubtypes(TypeMirror t) { + return (typeUtils, u) -> typeUtils.isSubtype(u, t); + } + + /** + * Create a set containing exactly the types with the given names, but not their subtypes. + * + * @param names the type names + * @return a set containing only the named types + */ + @Pure + static SetOfTypes anyOfTheseNames(ImmutableSet<@CanonicalName String> names) { + return (typeUtils, u) -> + u instanceof Type && names.contains(((Type) u).tsym.getQualifiedName().toString()); + } + + /** + * Create a set representing the union of all the given sets. + * + * @param typeSets an array of sets + * @return the union of the given sets + */ + @Pure + static SetOfTypes union(SetOfTypes... typeSets) { + switch (typeSets.length) { + case 0: + return EMPTY; + case 1: + return typeSets[0]; + default: + return (typeUtils, type) -> { + for (SetOfTypes set : typeSets) { + if (set.contains(typeUtils, type)) { + return true; + } + } + return false; + }; + } + } +} diff --git a/checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakCustomIgnoredExceptionsTest.java b/checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakCustomIgnoredExceptionsTest.java new file mode 100644 index 00000000000..df6911b0620 --- /dev/null +++ b/checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakCustomIgnoredExceptionsTest.java @@ -0,0 +1,25 @@ +package org.checkerframework.checker.test.junit; + +import java.io.File; +import java.util.List; +import org.checkerframework.checker.resourceleak.ResourceLeakChecker; +import org.checkerframework.framework.test.CheckerFrameworkPerDirectoryTest; +import org.junit.runners.Parameterized; + +public class ResourceLeakCustomIgnoredExceptionsTest extends CheckerFrameworkPerDirectoryTest { + public ResourceLeakCustomIgnoredExceptionsTest(List testFiles) { + super( + testFiles, + ResourceLeakChecker.class, + "resourceleak-customignoredexceptions", + "-AresourceLeakIgnoredExceptions=java.lang.Error, =java.lang.NullPointerException", + "-AwarnUnneededSuppressions", + "-encoding", + "UTF-8"); + } + + @Parameterized.Parameters + public static String[] getTestDirs() { + return new String[] {"resourceleak-customignoredexceptions"}; + } +} diff --git a/checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakExtraIgnoredExceptionsTest.java b/checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakExtraIgnoredExceptionsTest.java new file mode 100644 index 00000000000..9a69eac7c02 --- /dev/null +++ b/checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakExtraIgnoredExceptionsTest.java @@ -0,0 +1,25 @@ +package org.checkerframework.checker.test.junit; + +import java.io.File; +import java.util.List; +import org.checkerframework.checker.resourceleak.ResourceLeakChecker; +import org.checkerframework.framework.test.CheckerFrameworkPerDirectoryTest; +import org.junit.runners.Parameterized; + +public class ResourceLeakExtraIgnoredExceptionsTest extends CheckerFrameworkPerDirectoryTest { + public ResourceLeakExtraIgnoredExceptionsTest(List testFiles) { + super( + testFiles, + ResourceLeakChecker.class, + "resourceleak-extraignoredexceptions", + "-AresourceLeakIgnoredExceptions=default,java.lang.IllegalStateException", + "-AwarnUnneededSuppressions", + "-encoding", + "UTF-8"); + } + + @Parameterized.Parameters + public static String[] getTestDirs() { + return new String[] {"resourceleak-extraignoredexceptions"}; + } +} diff --git a/checker/tests/resourceleak-customignoredexceptions/BasicTest.java b/checker/tests/resourceleak-customignoredexceptions/BasicTest.java new file mode 100644 index 00000000000..e6f41bcd264 --- /dev/null +++ b/checker/tests/resourceleak-customignoredexceptions/BasicTest.java @@ -0,0 +1,59 @@ +import java.io.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +abstract class BasicTest { + + abstract Closeable alloc(); + + abstract void method(); + + public void runtimeExceptionManuallyThrown() throws IOException { + // this code is obviously wrong + // ::error: (required.method.not.called) + Closeable r = alloc(); + if (true) { + throw new RuntimeException(); + } + r.close(); + } + + public void runtimeExceptionFromMethod() throws IOException { + // method() may throw RuntimeException, so this code is not OK + // ::error: (required.method.not.called) + Closeable r = alloc(); + method(); + r.close(); + } + + // Note that even just constructing an instance of NullPointerException can throw all kinds + // of exceptions: ClassCircularityError, OutOfMemoryError, etc. Even RuntimeException is + // possible in theory. So, to really test what we're trying to test, we have to isolate + // the construction of the exception out here. + static final NullPointerException NPE = new NullPointerException(); + + public void ignoreNPE() throws IOException { + // this code is obviously wrong, but it is allowed because our ignored exceptions list + // includes NullPointerException + Closeable r = alloc(); + if (true) { + throw NPE; + } + r.close(); + } + + static class CustomNPESubtype extends NullPointerException { + static final CustomNPESubtype INSTANCE = new CustomNPESubtype(); + } + + public void doNotIgnoreNPESubtype() throws IOException { + // Only NullPointerException should be ignored, not its subtypes, since the options + // specified "=java.lang.NullPointerException". + // ::error: (required.method.not.called) + Closeable r = alloc(); + if (true) { + throw CustomNPESubtype.INSTANCE; + } + r.close(); + } +} diff --git a/checker/tests/resourceleak-extraignoredexceptions/BasicTest.java b/checker/tests/resourceleak-extraignoredexceptions/BasicTest.java new file mode 100644 index 00000000000..d1a3324910e --- /dev/null +++ b/checker/tests/resourceleak-extraignoredexceptions/BasicTest.java @@ -0,0 +1,36 @@ +import java.io.*; +import org.checkerframework.checker.calledmethods.qual.*; +import org.checkerframework.checker.mustcall.qual.*; + +abstract class BasicTest { + + abstract Closeable alloc(); + + abstract void method(); + + public void runtimeExceptionManuallyThrown() throws IOException { + // this code is obviously wrong, but RuntimeException is ignored by default + Closeable r = alloc(); + if (true) { + throw new RuntimeException(); + } + r.close(); + } + + public void runtimeExceptionFromMethod() throws IOException { + // method() may throw RuntimeException, but RuntimeException is ignored by default + Closeable r = alloc(); + method(); + r.close(); + } + + public void ignoreIllegalStateException() throws IOException { + // this code is obviously wrong, but it is allowed because our ignored exceptions list + // includes IllegalStateException + Closeable r = alloc(); + if (true) { + throw new IllegalStateException(); + } + r.close(); + } +} diff --git a/docs/manual/resource-leak-checker.tex b/docs/manual/resource-leak-checker.tex index 2f6dfb3c1dc..f7214791c01 100644 --- a/docs/manual/resource-leak-checker.tex +++ b/docs/manual/resource-leak-checker.tex @@ -51,11 +51,13 @@ The Resource Leak Checker supports all the command-line arguments listed in Section~\ref{called-methods-run-checker} for -the Called Methods Checker, plus two others: +the Called Methods Checker, plus three others: \begin{description} \item[\<-ApermitStaticOwning>] See Section~\ref{resource-leak-owning-fields}. +\item[\<-AresourceLeakIgnoredExceptions=...>] + See Section~\ref{resource-leak-ignored-exceptions}. \item[\<-ApermitInitializationLeak>] See Section~\ref{resource-leak-field-initialization}. %% TODO: Uncomment when the feature is ready to be publicized. @@ -488,12 +490,14 @@ \begin{itemize} \item -The Resource Leak Checker ignores run-time errors that can occur +By default the Resource Leak Checker ignores run-time errors that can occur unpredictably at most points in the program. For example, the JVM can throw an \ on any allocation. Similarly, \, \, and \ may occur at any reference to a class. Such exceptions usually terminate the program, and in that case unclosed resources do not matter. +Furthermore, any method can throw \, and the Checker +Framework pessimistically assumes one can be thrown at every call site. Accounting for such exceptions would lead to vast numbers of false positive warnings, so the Resource Leak Checker assumes they are never thrown. Strictly speaking, this is an unsoundness: it can lead to @@ -511,6 +515,25 @@ verification tool. \end{itemize} +The set of ignored exception types can be controlled with the option +\<-AresourceLeakIgnoredExceptions=...>. The option takes a comma-separated list of +fully-qualified exception types. A type can be prefixed with \<=> to ignore exactly +that type and not its subclasses. For example, for a very pedantic set of ignored +exceptions use: + +\begin{verbatim} + -AresourceLeakIgnoredExceptions=java.lang.Error, =java.lang.NullPointerException +\end{verbatim} + +which ignores \ (and all its subclasses) as well as +\ (but not its subclasses). + +The keyword \ will expand to the default set of ignored exceptions. So, +to add an additional exception to the set of ignored exceptions, use: + +\begin{verbatim} + -AresourceLeakIgnoredExceptions=default,package.MyCustomException +\end{verbatim} \sectionAndLabel{Errors about field initialization}{resource-leak-field-initialization}