Skip to content
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

support method and constructor parameter annotations #701

Merged
merged 7 commits into from
Oct 27, 2021

Conversation

codecholeric
Copy link
Collaborator

@codecholeric codecholeric commented Oct 24, 2021

This will extend the domain model of ArchUnit by providing

  • List<Parameter> JavaCodeUnit.getParameters()
  • List<List<JavaAnnotation<Parameter>> JavaCodeUnit.getParameterAnnotations()

where Parameter also offers getParameterType(), getRawParameterType() and getAnnotations().

Furthermore parameter annotations are now part of JavaClass.directDependencies{From/To}Self.

On the contrary to the Java Reflection API the JavaCodeUnit.getParameters() will exactly mirror the (possibly generic) signature and not the raw types. This means that for generic signatures synthetic raw type parameter types (like name and ordinal for an enum constructor) will not be visible in the List<Parameter>. I think for ArchUnit this makes sense, as there is limited interest in synthetic parts of the code and users are only interested in the parameters that have really been introduced by source code. Unfortunately there are many cases where the bytecode does not contain the signature attribute (whenever the signature is non-generic, i.e. does not contain any parameterized types, type variables, etc.). In these cases the parameters will still mirror the raw types (including possible synthetic parameters), because there is no other source of information where the parameters could be derived from. It is also not i.g. possible to derive which parameters are synthetic and which are not, so ArchUnit does not attempt this at all at the moment.

Resolves: #404
Resolves: #373
Resolves: #113

@codecholeric codecholeric requested a review from hankem October 24, 2021 15:48
Copy link
Member

@hankem hankem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't read through ClassFileImporterAnnotationsTest and the last commit yet, but I thought that I'd definitely agree with your fixup! commits (both their content, and your intention to fixup!) until I read about your example

class Outer {
  private class InnerWithoutGenerics {
    InnerWithoutGenerics(Object param) {
      // will have 2 Parameters (first synthetic)
    }
    
    InnerWithGenerics(List<String> param) {
      // will have 1 Parameter
    }
  }
}

It's unfortunately too late for me to give a good statement on that.

This was implemented a little brittle and sure enough does not work correctly if we introduce `JavaAnnotation<Parameter>`. I think what we really want to do is finding the closest owner that implements `CanBeAnnotated`. Changing this also still has all tests passing, so effectively it is the same behavior with the current code state, but it will also work for future additions that implement `CanBeAnnotated` (and that should always be the case for the parameter of `JavaAnnotation`, since an "owner" of a `JavaAnnotation` should implement `CanBeAnnotated`).

Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
Originally this was supposed to apply the interface segregation to only pass on what is really needed for the following builders. However annotations already need more to build and thus need the full `ImportContext`. I feel that `ImportedClasses` is still okay as an interface, since the scope is restricted to the import process. On the other hand we can then also refer to `ImportedClasses` when building annotations in a following step.

Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
While other builders previously used the very narrow `ClassesByTypeName` interface the annotation builders depended on the full `ImportContext`. Since they only needed one extra method from `ImportContext` I decided to move this into `ImportedClasses` by delegating back to the original method. It is a little convoluted but decouples domain builders from the full import context and makes it easier in the next step to integrate parameter annotation construction, since we already have `ImportedClasses` in a fitting scope.

Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
We can put type and raw type info there for now and enrich it with annotations in the future.

Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
Same as the Reflection API we can offer a more powerful `Parameter` object, which can tell its type as well as its annotations in the future. Having a nice consistent view on a single parameter to easily match type and annotations makes sense for writing ArchRules.
Note that there are some cases where we cannot safely determine the parameters. These are cases where the compiler adds some synthetic parameters. We can resolve it for common cases (like constructors of inner classes have the outer class as first raw parameter, even though it is missing from the generic signature). But in general there is no way to do this safely, because it can even differ from compiler to compiler, and I could not find any way to resolve the flag SYNTHETIC of a parameter with ASM.
 For example local classes will have parameters from the outer context appended to the constructor signature, so we can also not assume synthetic parameters are always in the beginning, nor a fixed number. Altogether, there is just no safe way to do it for all corner cases, so I decided to compensate the two known common cases (inner class constructor and enum constructors) and otherwise fall back to creating parameters just from the raw parameter types. This way they will at least be consistent and even the Reflection API has to do this, as it is also not possible to determine this in all cases with Reflection (compare `java.lang.reflect.Executable.getParameters()`).

Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
@codecholeric codecholeric force-pushed the introduce-code-unit-parameter-annotations branch from 5cb8034 to 9e4490b Compare October 27, 2021 05:37
@codecholeric
Copy link
Collaborator Author

@hankem thanks for the extensive review! 😄 Too bad that these synthetic parameters cause such a hassle. The only comfort is, that there is a vast amount of benevolent signatures out there where it doesn't make a difference 🤷‍♂️ (namely all method signatures AFAIS and all non-inner/non-enum constructors. And for enums it looks at least like the compiler always adds the signature, but that might just be my version 🤷‍♂️)
I wish there would be a compiler option like -parameters that could be added to always include the signature. Then users could decide to encode this additional information and every JavaCodeUnit would simply have the correct parameters parsed from the signature. (And I don't think this additional signature really makes a big difference in all typical modern scenarios, where the size of the class files is no real issue) But I couldn't find any such option 😞

@codecholeric codecholeric marked this pull request as ready for review October 27, 2021 05:49
Since we only take into account the generic parameter types to create `JavaCodeUnit.Parameters` the `index` of the parameter seem to match exactly without any further special case handling (like visiting `annotatableParameterCount` in the `MethodProcessor`).

Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
@codecholeric codecholeric force-pushed the introduce-code-unit-parameter-annotations branch from 9e4490b to 98787a0 Compare October 27, 2021 05:49
@codecholeric codecholeric merged commit ec51005 into main Oct 27, 2021
@codecholeric codecholeric deleted the introduce-code-unit-parameter-annotations branch October 27, 2021 06:49
@hankem
Copy link
Member

hankem commented Oct 27, 2021

Yes, you're right; I also figured that the annoying inconsistency might effectively not hurt so much as it's limited to inner class constructors and enums. (Even a compiler option wouldn't save you entirely, as the annoying byte code might be in a library you can't compile yourself.)

@hankem
Copy link
Member

hankem commented Oct 27, 2021

@codecholeric , if you want to reuse the PR description for the release notes, you may want to add a tiny disclaimer to this:

contrary to the Java Reflection API the JavaCodeUnit.getParameters() will exactly mirror the (possibly generic) signature and not the raw types

(Maybe replace exactly with usually?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants