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

Gradle incremental annotation rocessing #373

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bric3
Copy link
Contributor

@bric3 bric3 commented Feb 7, 2021

Hi @doanduyhai

I'd liek to propose the following enhancement: Gradle supports incremental annotation processing, meaning the annotation processor won't have to run on every new compilation.

For that there is a simple guide to follow there to make sure the annotation processor is eligible to incremental annotation processing: https://docs.gradle.org/current/userguide/java_plugin.html#sec:incremental_annotation_processing

If I'm not mistaken achilles core is ok with these points :

  • They must generate their files using the Filer API. Writing files any other way will result in silent failures later on, as these files won’t be cleaned up correctly. If your processor does this, it cannot be incremental.

  • They must not depend on compiler-specific APIs like com.sun.source.util.Trees. Gradle wraps the processing APIs, so attempts to cast to compiler-specific types will fail. If your processor does this, it cannot be incremental, unless you have some fallback mechanism.

  • If they use Filer#createResource, the location argument must be one of these values from StandardLocation: CLASS_OUTPUT, SOURCE_OUTPUT, or NATIVE_HEADER_OUTPUT. Any other argument will disable incremental processing.

I believe this annotation processor falls aggregatig category as it reaches to other elements like codecs or compiler registry. I followed the specific constraints :

"Aggregating" processors have the following limitations:

  • They can only read CLASS or RUNTIME retention annotations
  • They can only read parameter names if the user passes the -parameters compiler argument.

The changes looks to be minimal.

If possible it would be great to backport this change to the 5.3.x line.

Thank you in advance for the consideration.

bric3 added 2 commits February 7, 2021 14:13
There's a few constraints that are mentioned on the gradle doc
where annotation processors must abide in order to profit from
incremental annotation processing, thanks to the current code
and best practice the current processor seems to be within the
supported scope.

https://docs.gradle.org/current/userguide/java_plugin.html#sec:incremental_annotation_processing
@doanduyhai
Copy link
Owner

Thanks for the PR @bric3

Unfortunately, Achilles processor cannot be incremental for several reasons:

  1. It does not use the Filer API. Instead it is using com.squareup.javapoet.JavaFile to generate the source classes: https://github.com/doanduyhai/Achilles/blob/master/achilles-core/src/main/java/info/archinnov/achilles/internals/apt/processors/meta/AchillesProcessor.java#L118-L162

  2. It does relies on internal Sun compiler classes to parse nested structures like Map<String, @Frozen MyUDT>:

    } else if (isJavaCompiler(varElm)) {
    final Map<Class<? extends Annotation>, TypedMap> annotationInfo = varElm.getAnnotationMirrors()
    .stream()
    .filter(x ->
    areSameByClass(x, JSON.class) ||
    areSameByClass(x, EmptyCollectionIfNull.class) ||
    areSameByClass(x, Enumerated.class) ||
    areSameByClass(x, Frozen.class) ||
    areSameByClass(x, Computed.class) ||
    areSameByClass(x, Counter.class) ||
    areSameByClass(x, TimeUUID.class) ||
    areSameByClass(x, ASCII.class) ||
    areSameByClass(x, Codec.class) ||
    areSameByClass(x, RuntimeCodec.class) ||
    areSameByClass(x, Index.class) ||
    areSameByClass(x, SASI.class) ||
    areSameByClass(x, DSE_Search.class) ||
    areSameByClass(x, PartitionKey.class) ||
    areSameByClass(x, ClusteringColumn.class)
    )
    .map(x -> (AnnotationMirror) x)
    .collect(Collectors.toMap(x -> toAnnotation_Javac(aptUtils, x),
    x -> inspectSupportedAnnotation_Javac(aptUtils, currentType, x)));
    final AnnotationTree annotationTree = new AnnotationTree(currentType, annotationInfo, 1);
    final SymbolMetadata metadata = ((Symbol.VarSymbol) varElm).getMetadata();

At that time, this was the only solution to parse nested annotations inside generic types. I don't know if since then things have improved on the front of Java annotation processor to avoid using internal classes. If so I'll be happy to refactor this part of the code to get rid of the Sun internal classes

@bric3
Copy link
Contributor Author

bric3 commented Feb 7, 2021

Oh I missed those 😅.

I have seen other annotation processors using JavaPoet and activating Gradle's incremental processing. In preliminary tests this worked either in isolating and aggregating modes, but the model don't rely on UDT. Maybe I was mislead.

Thanks for the quick feedback and the consideration of this feature!

@doanduyhai
Copy link
Owner

My bad. The code https://github.com/doanduyhai/Achilles/blob/master/achilles-core/src/main/java/info/archinnov/achilles/internals/apt/processors/meta/AchillesProcessor.java#L118-L162 still uses the Filer API in the end. It is just handled by com.squareup.javapoet.JavaFile class itself: https://github.com/square/javapoet/blob/8ebce31cd655cf181ebdde59dcf04c50cbda7264/src/main/java/com/squareup/javapoet/JavaFile.java#L164

However the 2nd issue still remains, if I stop using Sun javac internal class, it will break all support of nested annotations

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

Successfully merging this pull request may close these issues.

2 participants