-
Notifications
You must be signed in to change notification settings - Fork 329
add support for dependencies from caught exceptions #1555
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
hankem
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for this contribution, which I think is a very nice addtion to ArchUnit!
On a first look, I could only spot very minor code style improvements.
archunit/src/test/java/com/tngtech/archunit/core/domain/testobjects/AhavingMembersOfTypeB.java
Outdated
Show resolved
Hide resolved
...st/java/com/tngtech/archunit/core/domain/testobjects/ClassWithDependencyOnTryCatchBlock.java
Outdated
Show resolved
Hide resolved
...st/java/com/tngtech/archunit/core/domain/testobjects/ClassWithDependencyOnTryCatchBlock.java
Outdated
Show resolved
Hide resolved
|
Glad you like it! I pushed the new changes as a separate commit; tell me if I should squash everything before you merge. |
hankem
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a closer look, but still nothing but minor comments.
Feel free to already rebase and squash your commits, thank you.
| return $$( | ||
| $(javaClass.getStaticInitializer().get(), 9), | ||
| $(javaClass.getConstructor(), 16), | ||
| $(javaClass.getMethod("simpleCatchMethod"), 23) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
9
16
23
I wonder whether we'd want to note somewhere™ (in the commit message? in the PR? It might be too detailed for the JavaDoc of a general Dependency...) that the line numbers for dependencies on caught exceptions currently actually refer to the first access in the try block.
(We should have the line number of each catch block in
the byte code
void complexCatchMethod();
descriptor: ()V
flags: (0x0000)
Code:
stack=2, locals=2, args_size=1
0: new #7 // class java/io/IOException
3: dup
4: invokespecial #9 // Method java/io/IOException."<init>":()V
7: athrow
8: astore_1
9: return
Exception table:
from to target type
0 8 8 Class java/lang/IllegalStateException
0 8 8 Class java/io/IOException
LineNumberTable:
line 34: 0
line 35: 8
line 37: 9
but I think it's beyond the scope of this PR to make them accessible.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether we'd want to note somewhere™ (…)
Yes, we should. I'm going with an @implNote on Dependency.getSourceCodeLocation(). I don't know whether Javadoc will actually include that in the generated docs, but even if it doesn't, it's better than nothing. Certainly better than a non-Javadoc comment buried somewhere nobody will ever find it.
We should have the line number of each catch block in but I think it's beyond the scope of this PR to make them accessible.
Agree with both. I would maybe even work on that next or at some point because I'm (mildly) annoyed by the wrong line number reporting. No promises, though.
What we (you? I?) could already do is create an issue for that which aims to fix the problem both for TryCatchBlock itself (maybe add something like getCatchClauses()?) and for Dependency.
.../java/com/tngtech/archunit/core/domain/testobjects/ClassWithDependencyOnCaughtException.java
Outdated
Show resolved
Hide resolved
archunit/src/test/java/com/tngtech/archunit/core/domain/testobjects/AhavingMembersOfTypeB.java
Show resolved
Hide resolved
archunit/src/test/java/com/tngtech/archunit/core/importer/ClassFileImporterMembersTest.java
Show resolved
Hide resolved
60ba41c to
d25c629
Compare
Referencing an exception class in the `catch` clause of a try-catch block can certainly be considered a dependency on that class, similar to a `throws` clause or an `instanceof` check: if the referenced class is renamed or deleted, the dependent code will break. To prevent this from happening, one could write architecture rules that e.g. limit the use of a certain third-party library to a specific layer, or allow-list only certain "stable" or "vetted" classes. Unfortunately, such rules will not notice when exception classes are referenced in `catch` clauses of try-catch blocks. The reason is that so far, ArchUnit does not generate a `Dependency` for them (it only does that once a member of the exception class is used, e.g. calling a method). This behavior defeats the purpose of implementing such a rule in the first place. Thus, we change ArchUnit so that it considers each exception type referenced in `catch` clauses of try-catch blocks to be a dependency on that type. As the information is already available in the model via `TryCatchBlock.caughtThrowables`, all it takes is wiring it up to creation and handling of `Dependency` instances. Resolves: TNG#1232 Signed-off-by: Jens Bannmann <jens.b@web.de>
d25c629 to
ef9f3bc
Compare
|
I totally forgot about the "rebase" part last time; have now done this. Let me know if there's anything else I should do! |
Referencing an exception class in the
catchclause of a try-catch block can certainly be considered a dependency on that class, similar to athrowsclause or aninstanceofcheck: if the referenced class is renamed or deleted, the dependent code will break.To prevent this from happening, one could write architecture rules that e.g. limit the use of a certain third-party library to a specific layer, or allow-list only certain "stable" or "vetted" classes.
Unfortunately, such rules will not notice when exception classes are referenced in
catchclauses of try-catch blocks. The reason is that so far, ArchUnit does not generate aDependencyfor them (it only does that once a member of the exception class is used, e.g. calling a method). This behavior defeats the purpose of implementing such a rule in the first place.Thus, we change ArchUnit so that it considers each exception type referenced in
catchclauses of try-catch blocks to be a dependency on that type. As the information is already available in the model viaTryCatchBlock.caughtThrowables, all it takes is wiring it up to creation and handling ofDependencyinstances.Resolves: #1232