Resolve call targets like Reflection API #722
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
KorSin
reviewed
Nov 26, 2021
.../core/importer/testexamples/methodresolution/DeterminesMethodAnalogouslyToReflectionApi.java
Show resolved
Hide resolved
.../core/importer/testexamples/methodresolution/DeterminesMethodAnalogouslyToReflectionApi.java
Show resolved
Hide resolved
archunit/src/main/java/com/tngtech/archunit/core/importer/AccessRecord.java
Show resolved
Hide resolved
By mapping the `types` to `rawTypes` we would hide any wrong assumptions that this should always be in sync. I think we should rather find out, if this is the case. Also, meanwhile I found cases, where the `rawTypes` and `types` don't even match 1-2-1 (e.g. generically typed private inner class constructor). So I think it is better to keep exactly what we import from the bytecode and analyze it, if we run into strange cases. Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
Originally I assumed the order of the interfaces would always be irrelevant for typical architecture tests. However for certain aspects it actually is relevant, e.g. when determining which method a method call should resolve to. Guava's `ImmutableSet` preserves the order, so we do not need to break the API (and in this case a `Set` that keeps the order is equivalent to a `List` since no element can appear more than once anyway). Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
Instead of having a complicated `JavaMethodCallTarget.resolve()` API that returns a `Set<JavaMethod>` we can follow the same logic as the Reflection API, which always offers one `Method` matching a certain signature. This should make the vast majority of cases a lot simpler, and for diamond scenarios we are following an established precedence logic. Thus, the argument why method x is chosen in a specific case is a lot simpler ("we just follow the established precedence of the Java Reflection API").
Note that this also fixes a bug in the field resolution logic. Earlier accesses of constants in interfaces of parent classes were not resolved correctly (i.e. if we find a field in an interface it should have precedence).
Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
codecholeric
force-pushed
the
resolve-call-targets-like-Reflection-API
branch
from
4e5a726
to
4c2144d
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.
Instead of having a complicated
JavaMethodCallTarget.resolve()API that returns aSet<JavaMethod>we can follow the same logic as the Reflection API, which always offers oneMethodmatching a certain signature. This should make the vast majority of cases a lot simpler, and for diamond scenarios we are following an established precedence logic. Thus, the argument why method x is chosen in a specific case is a lot simpler ("we just follow the established precedence of the Java Reflection API").Note that this also fixes a bug in the field resolution logic. Earlier accesses of constants in interfaces of parent classes were not resolved correctly (i.e. if we find a field in an interface it should have precedence).