Fix missing try-catch-blocks in lambdas #1096
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Mhmm, looking at the failed CI it probably makes sense to check out the memory footprint 😬 I checked CPU performance and that seems okay, but maybe the additional caching of the access resolution really drives up the needed heap 🤔 Just need to figure out if this is because we were close to the limit and "those additional 50MB now broke it", or if it's like "now you need double the heap" 😬 |
codecholeric
force-pushed
the
fix-missing-try-blocks-in-lambdas
branch
from
cabd9da to
f62237e
Compare
While IntelliJ shows no problem when the Javadoc of a nested class references the constructor of an outer class in an unqualified way, the Javadoc tool complains about this. Using the qualified reference including the outer class's name fixes the warning. Signed-off-by: Peter Gafert <[email protected]>
This reduces the toplevel clutter in the package and is consistent with our usual pattern. Signed-off-by: Peter Gafert <[email protected]>
We really only need "something that has a code unit origin" to resolve to correct non-synthetic origin, it doesn't necessarily need to be a "code unit dependency". This is relevant, because we want to fix try-catch-blocks as well (i.e. if the try-catch-block is defined within a lambda), and such a try-catch-block doesn't really conform to the concept of a "code unit dependency" (the thrown type, etc., likely would, but that's not what we want to fix). Signed-off-by: Peter Gafert <[email protected]>
Usually we import a "raw" object that is stored within `ClassFileImportRecord`. For try-catch-blocks we somehow store the imported data within `ClassFileImportRecord` as a domain builder already instead of converting it to a domain builder later on. We now consolidate this to apply the same behavior when importing try-catch-blocks, i.e. store them as "raw" object and convert them to domain object builder during the domain object creation phase. Signed-off-by: Peter Gafert <[email protected]>
... since it adds no value beyond defining the nested class `TryCatchBlockCondition`. Signed-off-by: Peter Gafert <[email protected]>
For other imports, like instanceof-checks, we have a dedicated processing step to associate the respective element with the declaring code unit. We now adjust the import of try-catch-blocks to follow the same processing so that we can transparently add resolving synthetic origins like for the other types of imports. Signed-off-by: Peter Gafert <[email protected]>
To speed up performance we often use the identity equals and hashcode for objects like `AccessRecord`. This works i.g. quite well, because most objects are created uniquely only once, so doing a semantic equals (based on the properties) or a identity equals is in fact the same. When we fix the synthetic origins we need to make sure that we also only create each fixed `AccessRecord` once. At the moment, this is actually the case, because the `forEach...` methods that fix the origins are always only called once. But this is for once a very implicit requirement (that these methods should only be called once if we want to compare the `AccessRecord`s to each other), and secondly to fix synthetic accesses in try-catch-blocks we need to ensure that the `AccessRecord`s referenced within `RawTryCatchBlock` are in fact equal again to the `AccessRecord`s referenced outside. The easiest way to keep the identity consistent, and ensure that fixed records are also only created once under all circumstances, seems to be to cache the fixed `AccessRecord`s. Note, that it really makes a measurable difference for the necessary heap space if we store a singleton set with the original access for every non-synthetic access. Thus, we only cache the set of resolved accesses if a synthetic access has been resolved and use an empty set marker for all regular values. Signed-off-by: Peter Gafert <[email protected]>
We need to resolve synthetic origins for try-catch-blocks just like other objects declared in `JavaCodeUnit`, like `JavaAccess` or `InstanceofCheck`. Otherwise, a `TryCatchBlock` declared within a lambda will be imported as being declared within a synthetic method (like `lambda$abc$123`). But we throw out these synthetic methods in the end, because the origins should have been fixed during the import to point to the method declaring the lambda. Thus, before this change we lost all those try-catch-blocks together with these methods. Resolves: #1069 Signed-off-by: Peter Gafert <[email protected]>
codecholeric
force-pushed
the
fix-missing-try-blocks-in-lambdas
branch
from
f62237e to
7b34a1d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With #847 we have changed the way lambdas are handled. Before ArchUnit didn't have any special handling for lambdas which lead to synthetic methods (like
lambda$foo$123) being imported like regular methods and connections likecall() -> doSomething()in the following example would be lost:Afterwards we started to eliminate these synthetic methods and attached the call directly to the origin method. However, try-catch-blocks are also associated with methods as their owners and here we forgot to attach them to the correct declaring method. Thus, by throwing out synthetic lambda methods from the import we would completely lose try-catch-blocks declared within lambdas.
We now fix this by resolving the correct origin/declaring method for any try-catch-block, even if it's declared within a lambda.
Resolves: #1069