add support for accesses from lambdas #847
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
|
Thanks a lot!! 😄 I just had time to take a quick look, but just so we don't forget it: We also need to assert that the methods of imported classes now don't contain any |
codecholeric
changed the title
|
Rebased it on |
hankem
reviewed
May 23, 2022
...rc/jdk9test/java/com/tngtech/archunit/core/importer/ClassFileImporterLambdaAccessesTest.java
Outdated
Show resolved
Hide resolved
archunit/src/main/java/com/tngtech/archunit/core/importer/JavaClassProcessor.java
Outdated
Show resolved
Hide resolved
...rc/jdk9test/java/com/tngtech/archunit/core/importer/ClassFileImporterLambdaAccessesTest.java
Outdated
Show resolved
Hide resolved
Two accesses should not be equal if origin, target and line number match. Because it is legal to write something like
```
void call() {
callSomeMethod.chainSameMethod().chainSameMethod().chainSameMethod();
}
```
In such a case origin, target and line number are identical. Thus, when we return a `Set<JavaCall<?>>` we will only contain one element with the old implementation, which is not correct. Since only ArchUnit's importer creates domain objects we can simply use the identity `equals` and `hashCode`. Because in the end each access is unique by itself and users never create these objects themselves to compare them against imported objects.
Signed-off-by: Peter Gafert <[email protected]>
There are some clean-ups with regard to the previous Java 8 migration (e.g. functional patterns for `Optional`, redundant type information, etc.). Also there were some unused methods that were overlooked when the dependency resolution process was improved. Signed-off-by: Peter Gafert <[email protected]>
Most tests were meanwhile residing in the source set `jdk9test` because it was the next higher source set that supported Java 8 language level. Since we now support Java 8 language level everywhere we can merge these tests back into the normal `test` source set. Signed-off-by: Peter Gafert <[email protected]>
hankem
approved these changes
May 29, 2022
archunit/src/main/java/com/tngtech/archunit/core/importer/DomainBuilders.java
Show resolved
Hide resolved
archunit/src/test/java/com/tngtech/archunit/library/freeze/ViolationLineMatcherFactoryTest.java
Outdated
Show resolved
Hide resolved
...it/src/test/java/com/tngtech/archunit/core/importer/ClassFileImporterLambdaAccessesTest.java
Show resolved
Hide resolved
For lambdas in Java code (e.g. `() -> "some value"`) the compiler adds a synthetic static method to the Java class that follows the naming pattern `lambda$declaringMethod$42`. It also adds an `invokeDynamic` instruction that creates the link from the method that declares the lambda to this static method.
So far we ignore those `invokeDynamic` instructions for lambda cases. On the other hand we treat the synthetic lambda method like an ordinary method. This can lead to confusing behavior, e.g.
```
Supplier<SomeObject> call() {
return () -> new SomeObject();
}
```
will now create a `JavaConstructorCall` that originates from some method `lambda$call$0()`, while there will be no trace about any constructor call from within the method `call()`.
We decided that for a typical user the simplest and likely most appropriate behavior will be to treat this constructor call from within the lambda as a `JavaConstructorCall` from `call()` to `SomeObject.<init>()` (even though technically it is something different, since the call to `new SomeObject()` will in this case not happen on invocation time of the method, but on invocation time of the return value). From an architecture test point of view the relevant information will likely be, that there is a dependency from `call()` to `new SomeObject()`, which is exactly what this way of treating the call from the lambda will provide.
Thus, to improve this we will track all `invokeDynamic` instructions and then "merge" these with the accesses from synthetic lambda methods into more appropriate accesses from the code unit declaring the lambda to the target that the synthetic lambda method targets. For cases where users actually want to distinguish a "direct" access and one that is declared in the context of a lambda we add the boolean flag `declaredInLambda` to `JavaAccess`. This way the information can still be retrieved if it should be relevant in a certain context.
Signed-off-by: Hannes Oberprantacher <[email protected]>
Signed-off-by: Peter Gafert <[email protected]>
Unfortunately, the `gradle-build-action` cache seems to be corrupt and there is no way to manually invalidate the cache. All we can do is to reconfigure the cache prefix or disable the cache for 7 days, which is the default period until caches are invalidated automatically. Since disabling the cache is the simpler solution, and it is only for 7 days, I decided to go with this way. Signed-off-by: Peter Gafert <[email protected]>
This was referenced Oct 4, 2022
snuyanzin
added a commit
to snuyanzin/flink
that referenced
this pull request
Nov 7, 2022
snuyanzin
added a commit
to snuyanzin/flink
that referenced
this pull request
Dec 11, 2022
snuyanzin
added a commit
to snuyanzin/flink
that referenced
this pull request
Dec 12, 2022
codecholeric
added a commit
that referenced
this pull request
Apr 15, 2023
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 like `call() -> doSomething()` in the following example would be lost: ``` void call() { inLambda(() -> doSomething()); } ``` 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
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.
For lambdas in Java code (e.g.
() -> "some value") the compiler adds a synthetic static method to the Java class that follows the naming patternlambda$declaringMethod$42. It also adds aninvokeDynamicinstruction that creates the link from the method that declares the lambda to this static method.So far we ignore those
invokeDynamicinstructions for lambda cases. On the other hand we treat the synthetic lambda method like an ordinary method. This can lead to confusing behavior, e.g.will now create a
JavaConstructorCallthat originates from some methodlambda$call$0(), while there will be no trace about any constructor call from within the methodcall().We decided that for a typical user the simplest and likely most appropriate behavior will be to treat this constructor call from within the lambda as a
JavaConstructorCallfromcall()toSomeObject.<init>()(even though technically it is something different, since the call tonew SomeObject()will in this case not happen on invocation time of the method, but on invocation time of the return value). From an architecture test point of view the relevant information will likely be, that there is a dependency fromcall()tonew SomeObject(), which is exactly what this way of treating the call from the lambda will provide.Thus, to improve this we will track all
invokeDynamicinstructions and then "merge" these with the accesses from synthetic lambda methods into more appropriate accesses from the code unit declaring the lambda to the target that the synthetic lambda method targets.