Add support to test caught exceptions (fixes #591)

[crizzis]

This will add TryCatchBlocks to the ArchUnit core and introduce JavaCodeUnit.getTryCatchBlocks() to examine try-catch blocks that have been parsed from the bytecode of a method or constructor. We also add an extension JavaAccess.getContainingTryBlocks() to make it easy to verify that certain accesses in code are wrapped into certain try-catch blocks (e.g. "whenever method x is called there should be a try-catch block to handle exception case y").

Resolves: #591

Signed-off-by: Krzysztof Sierszeń krzysztof.sierszen@digitalnewagency.com

[crizzis]

In this minimal version, only the exception type is retained in CaughtThrowable

This is a pretty large PR as it is, I'd suggest that any enhancements be added in a separate PR

[crizzis]

Quite a lot of layers to punch through :)

I didn't quite get the idea behind RawAccessRecord/RawXXXRecordProcessed. Am I wrapping JavaClassDescriptor into CaughtThrowable in the right layer?

[codecholeric]

I can explain the idea quickly: RawAccessRecord represents the raw data as collected during bytecode analysis (i.e. origin and target are just strings or type descriptors, etc.). RawAccessRecordProcessed represents the next step, when this raw data has been resolved to domain objects (e.g. the target is now not a string anymore, but a JavaMethodCallTarget which has a JavaClass as its owner, the origin is a JavaCodeUnit, etc.).

[crizzis]

I wish there was some metamodel tool to generate those automatically :|

[codecholeric]

Well, the user doesn't have one either 😉

[codecholeric]

Thank you so much for all your hard work!! 😃 I'm really sorry, but I feel we should have discussed it more ahead though, because now looking deeper into it there are a lot of questions coming to my mind where things don't seem as clear as I thought on first sight 🤔 Because somehow the discussion how to model this just stopped and we never really came to a final conclusion (I just threw in that I would prefer CaughtException over JavaClass, but I didn't think about how to model it deeper).

But still, let's start discussing this a little though, because as I said there are some basic points for me that influence everything. The most important one for me is the model. @hankem was asking if it should be something like JavaCall.getCaughtThrowables(), as you modeled it, but looking at it now this looks confusing to me. Because the "call" hasn't caught the throwable, no? The method has caught the throwable around the call, hasn't it? So naively I would expect JavaCodeUnit.getCaughtThrowables() where I would get the throwables the method/constructor has caught. But then CaughtThrowable would need the info what JavaCalls were covered, which would seem really weird on a type with that name. So looking at it now, I tend to think that what we really want is something like a TryCatchBlock, very similar to what you have created internally anyway to track the boundaries of the try-catch-block 🤔

The current model in the PR is very close to the issue, i.e. the concrete issue would be super easy to solve with this model. But I'm not sure it's the best domain model in general. Because, if for example I would want to write a rule on all the caught exception types. Then I would now have to track down every single call, gather all the exception types and deduplicate, because multiple calls in the same block all have the same CaughtThrowables attached 🤔 Also, as I said, JavaCall.getCaughtThrowables() doesn't feel intuitive to me from a semantic point of view, i.e. not knowing about the API I would not expect to find this on JavaCall 🤔

I'm wondering if the following wouldn't be a more intuitive model:

class TryCatchBlock {
  int tryBlockStartLineNumber;
  int tryBlockEndLineNumber;
  Set<JavaClass> caughtThrowableTypes;

  boolean tryBlockContains(JavaCall call) {
    // compare line numbers to check that the call is contained? Or should we keep references to all `JavaCall` instead?
  }
}

class JavaCodeUnit {
  // ...
  Set<TryCatchBlock> getTryCatchBlocks() {..}
}

This model would make it a little more tedious to solve the concrete issue (I would now have to check all the try-catch-blocks to see if the relevant call is contained and the caught throwables match), but to me it feels better as a generic model to also implement other rules users might have for try-catch-blocks 🤔 (but I might be wrong 🤷‍♂️) But it would also be possible to e.g. add JavaCall.getContainingTryCatchBlocks() or something like that to make it easier to solve this issue but still have a more generic model 🤔

What do you think about these points?

[crizzis]

@codecholeric Your comment raises a couple of different points, so let me try and unpack it step by step.

@hankem was asking if it should be something like JavaCall.getCaughtThrowables(), as you modeled it, but looking at it now this looks confusing to me. Because the "call" hasn't caught the throwable, no? The method has caught the throwable around the call, hasn't it?

I had a hard time coming up with an appropriate name, and I agree getCaughtThrowables is not the best one. Perhaps getThrowablesHandledByCaller would be better?

So naively I would expect JavaCodeUnit.getCaughtThrowables() where I would get the throwables the method/constructor has caught. (...) Because, if for example I would want to write a rule on all the caught exception types. Then I would now have to track down every single call, gather all the exception types and deduplicate, because multiple calls in the same block all have the same CaughtThrowables attached 🤔

I believe we are discussing two separate use cases. My PR focused on covering @cpollet's use case and would not be easily solved by your proposal. Your use case, in turn, also sounds completely valid and the call for having TryCatch blocks listed under JavaCodeUnit seems perfectly justifiable.

Perhaps then, our proposed solutions are both correct, and the association between JavaCalls and TryCatchBlocks should simply be a bidirectional one (I believe that's what you're suggesting by getContainingTryCatchBlocks)?

What I would suggest now, is to keep this solution as it is, and add another PR that would in addition create a TryCatchBlock entity, with a set of CaughtThrowable attached. I believe some of the functionality provided in this PR will be useful in implementing that other scenario.

compare line numbers to check that the call is contained? Or should we keep references to all JavaCall instead?

I'd rather not rely on line numbers here, since both try and catch blocks (and even multiple try .. catch blocks) can be placed on the same line. Worse still, a method call could be placed inside the catch block in such a scenario.

EDIT Just to be clear, I'm proposing:

class TryCatchBlock {
  int tryBlockStartLineNumber;
  int tryBlockEndLineNumber;
  Set<CaughtThrowable> caughtThrowableTypes;
  Set<JavaCall> javaCalls;
}

class JavaCodeUnit {
  // ...
  Set<TryCatchBlock> tryCatchBlocks;
}

class JavaCall {
  Set<CaughtThrowable> throwablesHandledByCaller; // or perhaps just Throwable?
}

WDYT?

[codecholeric]

Yes, a bidirectional association was what I meant by adding JavaCall.getContainingTryCatchBlocks(). I think it is very nice to be able to get the containing catch-blocks of a JavaCall. I'm just not sure we really need TryCatchBlock and CaughtThrowable then. Because how would you use CaughtThrowable? You would retrieve it, then retrieve the Throwable type, right? But TryCatchBlock can offer exactly the same, you could get all caught exception types and TryCatchBlock could simply hold a Set<JavaClass> or throwable types then (I only voted for something like CaughtException around it before, because Set<JavaClass> as a top-level type to model this can't be extended with any domain logic)

I fully agree about your argument with the line numbers, it would likely work most of the time, but it would be shady. Because in theory you can also do crazy one-line things like this:

try { call(); } catch(Wohoo e) {} call(); try { call(); } catch(Yahoo e) {}

Maybe the line numbers of start and end could still be interesting, but I think we can leave them out and see if users really want them 👍

Coming back to how to model the types, as mentioned, I think one domain object would be good enough. I think the best would be to record the necessary info (like labels, etc.) on import, then construct

class JavaCodeUnit {
  // ...
  Set<TryCatchBlock> tryCatchBlocks;
}

class TryCatchBlock {
  Set<JavaCall> containedCalls; // (or Set<JavaAccess> containedAccesses? see below)
  Set<JavaClass> caughtThrowableTypes;
  // ...
}

class JavaCall {
  // ...
  Set<TryCatchBlock> getContainingTryCatchBlocks() {
    Set<TryCatchBlock> allTryCatchBlocks = getOwner().getTryCatchBlocks();
    return filter(allTryCatchBlocks, contains(this));
  }
}

This would offer a convenient API. (any better name than getContainingTryCatchBlocks()? Should it be TryBlocks, because it's only the try-block we're interested in? But that might be confusing? Or getTryCatchBlocksWhereThisCodeUnitIsContainedWithinTheTryBlock() 😂 )

One thing I'm wondering about is if we should just make it JavaAccess instead of JavaCall 🤔 Because any JavaAccess could be contained within a try-catch-block, right? Just that for a field access I can't imagine a use case for the info out of my head 🤔 But the implementation effort would certainly not be more and maybe it would make the model simpler if we do it for all JavaAccess? 🤔

[hankem]

Maybe the line numbers of start [...] could still be interesting, but [...] leave them out and see if users really want them

For conditions based on TryCatchBlocks, it would certainly be nice if this domain object implemented HasSourceCodeLocation.

class TryCatchBlock {
Set<JavaCall> containedCalls; // (or Set<JavaAccess> containedAccesses? see below)

Shouldn't this Set be calls(or accesses... 🤷‍♂️)ContainedInTryBlock? I wonder whether we might want to extend TryCatchBlock to also include calls(or accesses... 🤷‍♂️)ContainedInCatchBlock at some point.

P.S. I'm very sorry that #591 (comment) might have led in a wrong direction!

[codecholeric]

True, it could implement HasSourceCodeLocation giving the line number of the try-block-start 👍 And yes, I agree that we should always be careful about "contained in try-catch-block", because that's ambiguous, if it's the try- or the catch-block (or even the finally block? Is that also in there? 🤔)

[codecholeric]

Also no worries about the comment @hankem, you only asked a question and we never finished the discussion. I should probably be more careful about slapping the help-wanted label onto such issues and only do it, when the issue is fully analysed. Also sorry for that, I'm often on such a tight schedule that I'm lacking the time to refine each issue before I know if somebody actually has time to tackle it 😞

[codecholeric]

BTW @crizzis, we have some code style files for IntelliJ and Eclipse checked in at https://github.com/TNG/ArchUnit/tree/main/develop. You can use those to make sure the formatting is consistent and the imports don't change unnecessarily!

[crizzis]

@codecholeric Fixed the code style, converted CaughtThrowable into TryCatchBlock, and made it implement HasSourceCodeLocation.

Is it okay with you for the JavaCodeUnit -> TryCatchBlock to be implemented in another PR?

(Also, I guess it makes sense for TryCatchBlock to then implement HasOwner as well, right?)

[codecholeric]

Sorry that I haven't gotten back to you on this for so long!! I read through the code, then decided I should support you more (since obviously this whole back and forth wasn't super motivating for you, sorry about that), then got sick for a while 😬 Anyway, I invested quite some time now to pretty much bend it into the direction of having try-catch-blocks on the JavaCodeUnit instead of on the JavaCall. The reason is that I don't think it makes sense to add it in a way that we need to refactor it completely again to go for the other vision. I want to share some feedback with you, so that you hopefully can also get something out of it 🙂

First of all the code was very nice to read 👍 😃 And also respect that you managed to bite through all those layers, understand and handle the ASM visitor API and then implement it correctly, that was certainly not easy! I'll try to provide some constructive feedback about things I noticed:

• The most important part for the future is to watch out for other qualitative properties besides functionality. Because, while your code was written very cleanly and did what was necessary on the functional level, it was also quite inefficient. I tried to do a performance test (running com.tngtech.archunit.core.importer.ClassFileImporterSlowTest#creates_JavaPackages) and what took 4 seconds on the main branch now crashed with an OutOfMemoryError after 6 minutes. I think the reason was excessive filtering + copying of collections (in TryCatchTracker and co) and the massive duplication of TryCatchBlocks by adding them as separate objects to every JavaCall. JavaCall is a high volume object, so it is very easy to add something there that makes the heap explode. This was one reason I wanted to have exactly one TryCatchBlock for each try-catch-block in the code and add it to JavaCodeUnit. In my refactoring I had to watch out quite some bit to achieve a similar performance than before, because this is just a high-volume processing where we need to look at all the labels, so it's very easy to add a bottleneck there. I added things like the active flag simply to shave of some processing time 😬
• Before you create a PR you should run the full build once locally (that would have shown you that there are some errors as well as that the build now suddenly takes a very long time with your change)
• Another point is about structuring commits. It is fine to refactor some code, like move these assertions from inner classes to toplevel, but you should always do such things in a preparatory separate commit ahead. Because it makes it massively harder to see what you really added to implement the feature versus parts that were just moved. Also, sometimes it might be worth it to ask about it ahead. For example some of the inner classes in the main code are a conscious trade-off to group them somehow (while unfortunately not being able to use packages because of visibility 😞). Anyway, for now I extracted those pure preparatory refactorings into a separate commit for you...
• The class LabelSpan would not have belonged to the domain part, since that is for the ArchUnit domain model that users use. But LabelSpan was a purely internal class. On the other hand TryCatchBlock would have belonged there, because JavaCall has a reference and in general domain should not have dependencies on import
• There was some functional part missing. If a try-catch-block contained an inner return statement the block would not have been completed correctly (unfortunately there was no test for that, I found it by accident when doing performance tests with some additional assertions in place)
• Domain objects like TryCatchBlock should never return JavaClassDescriptor (which was the return type for the caughtThrowables), because that class is @Internal and thus not to be used by users. Instead it has to be JavaClass for these cases.

In any case, I would be happy if you could look at my proposed changes and tell me what you think about them! Maybe it can also be made clearer in some parts or written more efficiently. I'm in particular not super happy that we now have to pass the TryCatchBlockBuilders to all the access creation, but I couldn't find any cleaner way to associate the completed accesses to the TryCatchBlocks 😞 If you want to compare performance between main and that branch, that might also be good (since I only used my own local machine for a limited number of cases)
If everything looks good to you I would suggest you just squash my review changes onto yours to create one clean commit that adds the support for try-catch-blocks.

[crizzis]

Sorry that I haven't gotten back to you on this for so long!! I read through the code, then decided I should support you more (since obviously this whole back and forth wasn't super motivating for you, sorry about that), then got sick for a while 😬

No worries, life happens, hope things are better now ;)

Unfortunately, life happens to me too, and while I promise to have a look at the suggestions, what I can't promise is that it will happen anytime soon, as I will have much less time now to contribute to open source stuff.

The ultimate goal is of course to ship new, shiny functionality to users, and most of the proposed changes sound very reasonable, so if you feel the feature can't wait, I wouldn't like to make my lack of time a showstopper. I really appreciate the constructive feedback, and I can always revisit the PR even after it is merged, the code is not set in stone, after all.

Otherwise I will have to ask you for some patience, but either way, it's probably not a good idea to leave a PR alive for too long lest it becomes hopelessly conflicted with the main branch. I'm leaving the decision up to you.

Cheers!

[codecholeric]

Okay, don't stress about it! I still have to look at your other PR as well, which I couldn't find the time yet 🙈

[hankem]

I haven't yet played with the new feature for myself, and I didn't understand the one test with the 3 try-catch-blocks, but other than that, the code looks good to me!

[hankem]

@codecholeric, why would we need ? extends Class?

[codecholeric]

That is part of the Java Generics variance behavior. At least AFAIS without the ? extends you can't use it for

Formatters.formatNamesOf(ImmutableList.of(String.class, Serializable.class));

while you can use it for

Formatters.formatNamesOf(ImmutableList.of(String.class, Object.class));

The problem seems to be that in the former case the inferred type is List<Class<? extends Serializable>> which seems to conflict with Iterable<Class<?>>. So to make the API more convenient I changed the signature to allow subtypes of Class<?>, because then the former case compiles without the need to explicitly fix the type by ImmutableList.<Class<?>>. Should maybe add a test though and split out the commit 😉 (because I saw that the final state actually compiles without this change, so I think I added it at some point when I stumbled over it, but the reason afterwards wasn't necessary to be added to the PR after all 🤷‍♂️)