Add -AresourceLeakIgnoredExceptions option

[Calvin-L]

Fixes #6218.

This change is admittedly overwrought, but

• I want the ability to ignore all subtypes of an exception type and
• I want to preserve the current behavior where subtypes of the default ignored exceptions are not themselves ignored.

To meet those constraints I added a new data structure (SetOfTypes) and some funky syntax for specifying the set of ignored exceptions.

I'm not particularly attached to the syntax, and we should probably discuss improvements because it will be difficult to change later.

I picked the option name "resourceLeakIgnoredExceptions", but I'm happy to rename it if there is some sort of option naming pattern to follow.

[kelloggm]

This was a very surface-level review; my goal was just to understand the change well enough to comment on it.

Overall, I don't think this design is too over-wrought. If we want support for both ignoring specific exceptions (but not their subtypes) and ignoring all subtypes of an exception, we will need some kind of parsing like what you've implemented here. No simpler solution immediately comes to mind.

One alternative design would be to force (or permit?) the user to put the ignored exception types into a separate configuration file, which would simplify the command-line invocation. I don't think this is really any better, though, because in practice this option will be set within a build system's configuration anyway.

With respect to the naming of the option, my only concern is that resourceLeakIgnoredExceptions is very verbose. I'd suggest -AignoredExceptions, but I think that users are likely to be confused by the fact that an option with that name only impacts the RLC and not other checkers. This leaves us with your suggestion, which I think is fine.

[Calvin-L]

With respect to the naming of the option, my only concern is that resourceLeakIgnoredExceptions is very verbose.

There's precedent: the Called Methods Checker has options like CalledMethodsChecker_disableReturnsReceiver.

Actually, should the new option follow the same convention? It could be ResourceLeakChecker_ignoredExceptions.

[msridhar]

FYI I'm working through a review on this one

[msridhar]

Overall looks great! A couple comments / questions

[msridhar]

All the code changes look good here, just a minor point of discussion

[msridhar]

I guess our new syntax could be a bit awkward if your first ignored exception type is exact, since then you'll have to write -AresourceLeakIgnoredExceptions==exception.Type; not sure if this is a significant concern.

Thinking more, I think I was confused before. I think we only supported exact type matching before, and this PR adds support for matching all exception subtypes. Is that correct @Calvin-L? Maybe I was wrong about what would be a more common scenario...

[Calvin-L]

I think we only supported exact type matching before, and this PR adds support for matching all exception subtypes.

That's right.

Maybe I was wrong about what would be a more common scenario...

I actually think your first thought was right. I suspect that matching all subtypes will be the more common scenario, if only because it is easier to understand. Matching on exact type has very surprising semantics.

I haven't checked this recently, but if memory serves---

With no special options, the RLC allows this code:

r = alloc();
method();
r.close();

but does not allow this code:

r = alloc();
try {
  method();
} catch (IllegalStateException e) {
  throw e;
}
r.close();

The different handling is extremely confusing, because those snippets have essentially identical run-time behavior. The call to method() always had the option to throw IllegalStateException, regardless of whether we caught and re-threw it. Since IllegalStateException extends RuntimeException and RuntimeException is ignored by default, shouldn't the catch block in the second snippet be treated as unreachable?

For that reason, I think that matching all subtypes is more intuitive. With -AresourceLeakIgnoredExceptions=java.lang.Error,java.lang.RuntimeException the two snippets are both accepted.

[msridhar]

Hrm...interesting. By your argument, I think we should then also change how the default list of exceptions is handled (not based on exact name, but on any subtype).

But! When adding the default ignored exception functionality, in my mind I was primarily thinking about cases where the exceptional edge for the call went directly to the method exit. For your second example, I'm honestly not sure if we should be ignoring the potential exceptions from the call to method() or not. Somehow, the presence of a catch block to me implies that the developer cares more about the possibility of that exception being thrown; so maybe we should not ignore the possibility?

But in any case, this point is somewhat independent of this PR. Even now, writing this code, I think the exceptional possibility will be ignored by the RLC:

r = alloc();
try {
  method();
} catch (RuntimeException e) {
  throw e;
}
r.close();

So I think the question for this PR is whether the default list of ignored exceptions should also match all subtypes, and my instinct is to say yes. @kelloggm any thoughts on this?

[kelloggm]

I think the question for this PR is whether the default list of ignored exceptions should also match all subtypes, and my instinct is to say yes

Yes, I agree: the default list of ignored exceptions should match subtypes. That's a change to the RLC's externally-facing behavior, however, so this PR also needs a CHANGELOG entry.

Even now, writing this code, I think the exceptional possibility will be ignored by the RLC

I agree, I think this will be ignored. I agree with your intuition that this is weird: it would be surprising to me, as a user, that the RLC ignores the possibility of a catch block that I explicitly add. A separate PR to address this is a good idea (and I see you opened a separate issue, too).

[Calvin-L]

the default list of ignored exceptions should match subtypes

Do you want me to add that change to this PR? (In theory it's an orthogonal change, and I would opt to do it in a follow-up PR if given the option.)

[msridhar]

A follow-up PR is fine, good point. Thanks!

[msridhar]

We'll merge this one once the conflicts are fixed. Thanks again!