Support inheritance across modules and incremental annotation processing #1466

stephanenicolas · 2019-02-09T22:34:43Z

The PR is now ready for review: @JakeWharton @ZacSweers.
I did my best to comment it and help your review. I remain available to address your comments and answer your questions. I would appreciate our exchanges to be respectful and open.
Thx in advance.

This solves both:

support incremetal annotation processing: Support incremental annotation processing #1225
support inheritance across modules support inheritance across modules. Issue #808. #1045

Both issues are linked: to support incremental changes, we have to support the case where a subclass only is recompiled and the super class is not passed to the annotation processor. The same occurs for inheritance across modules.

In this case, we want to avoid doing some work that we have already done, we don't want to re-scan the annotations of the super class (it would have been possible to access it via annotation processing). To avoid this duplicate processing, we reuse the generated code of the super class in the subclass. I will comment more inside the PR on how this is currently achevied.

stephanenicolas · 2019-02-09T22:35:50Z

butterknife-compiler/src/main/java/butterknife/compiler/ButterKnifeProcessor.java

  private Filer filer;
  private @Nullable Trees trees;

  private int sdk = 1;
  private boolean debuggable = true;

  private final RScanner rScanner = new RScanner();
+  private HashMap<String, List<Element>> mapGeneratedFileToOriginatingElements
+      = new LinkedHashMap<>();


This is used only for testing. I will talk more about tests below.

stephanenicolas · 2019-02-09T22:48:38Z

butterknife-compiler/src/main/java/butterknife/compiler/ButterKnifeProcessor.java

+    } catch (Throwable t) {
+      try {
+        trees = Trees.instance(processingEnv);
+      } catch (Throwable ignored) {


This whole block here is meant to access the ProcessingEnvironment instance passed by gradle. Gradle wraps the ProcessingEnvironment internally so that it can control the filer passed to annotation processors.

The wrapper class provided by gradle is IncrementalProcessingEnvironment.

This class is not exposed by Gradle. The idea is to access this class and the original wrapped ProcessingEnvironment. We need this vanilla instance code because Trees.getInstance(..) checks that it receives the original processing environment. If you look at the code of the Trees class, you will see that it first checks the class name of the instance. I tried to bypass this check and call the method getJavacTrees that the instance method is calling internally. But there are additional checks later on the road. I resorted to use reflection to access the original processing environment in the non exposed gradle wrapper class.

I have opened an issue in gradle's code base on this in: gradle/gradle#8393
The PR has been rejected has gradle doesn't want to expose this class. As you can see in this issue, we discuss the safety of accessing Trees in the BK processor during incremental annotation processing. This practice is a corner case when incremental processing is used and it is generally not advised to turn annotation processors to incremental if they use this API. Though, I explained why I believe there is no issue in doing so in our case.

Also, at the end of the discussion in the gradle PR, gradle says that the processing environment is always wrapped. It was not my experience when testing: I received the vanilla processing environment when I was developing. I didn't follow up with gradle on this. But for this PR, I don't think it should matter as the case was already handled by just swallowing the exception. In practice, the PR has been tested at an industrial scale in 2 companies that I know of and all works well. This code has fixed a bug in the first version of the fork: stephanenicolas#2.

stephanenicolas · 2019-02-09T22:56:10Z

butterknife-compiler/src/main/java/butterknife/compiler/ButterKnifeProcessor.java

@@ -201,6 +217,10 @@
    return false;
  }

+  public HashMap<String, List<Element>> getMapGeneratedFileToOriginatingElements() {


This method is also always visible for testing. Of course, we can change the visibility accordingly.

stephanenicolas · 2019-02-09T23:04:46Z

butterknife-compiler/src/main/java/butterknife/compiler/ButterKnifeProcessor.java

@@ -1273,7 +1326,7 @@ private static boolean isTypeEqual(TypeMirror typeMirror, String otherType) {
        return null;
      }
      typeElement = (TypeElement) ((DeclaredType) type).asElement();
-      if (parents.contains(typeElement)) {
+      if (parents.contains(typeElement) || hasViewBinder(typeElement)) {


This comment is about all the changes above from line 381 to here. The idea is that we want to take into account the situation where a sub class is being processed and not its super class (it applies both when we are in an incremental build and when the super class is in a different module).

Even though the super class is not passed to the annotation processor, we can know if butterknife has generated some code for it by testing the presence of the generated class in the classpath. If such a __Binder exists for the super class, we create a StubBindingSet for it: we do re-scan its annotations here, only because we want to know which constructor of the super class to invoke later when we generate the __Binder of the sub class. (The constructor of a __Binder can have 2 forms depending on whether the class uses@BindView(s) annotations.)

Of course, I am open to amend the way of doing things if there is a better solution. We could have a better way to handle the case, by adding some information to the generated code of the super class for instance. I preferred to limit the scope of changes in the PR.

stephanenicolas · 2019-02-09T23:11:42Z

butterknife-runtime/src/test/java/butterknife/BindViewTest.java

+    assertEquals("test/Test_ViewBinding.java", entry.getKey());
+    List<Element> elements = entry.getValue();
+    assertEquals(1, elements.size());
+    assertEquals("test.Test", elements.get(0).asType().toString());


In this test, I use the method and the map I have added to the BK processor.

There are different options here:

unit testing: we can test that we pass the right originating element to the filer. This is a healthy practice, idependently of gradle's incremental solution.

integration testing. I've seen other projects using integration tests with gradle, with fixtures, and I am not very familiar with this technology. Such tests can fully reproduce an incremental build.

I decided to go with the first testing strategy as it limits the impact of the changes here, I also think this test and the tested behavior is just "healthier" for the compiler. But we can also prefer to test more directly that incremental annotation processing is working as expected via an integration test, which would be better to ensure there is no regression at this level. It would probably bring more value for maintenance.
If we decide to go this way though, I will need some help to write such a test.

stephanenicolas · 2019-02-09T23:14:12Z

butterknife-runtime/src/test/java/butterknife/InheritanceTest.java

+        .and()
+        .generatesSources(bindingTestSource);
+  }
+}


I have separted the test to check if we support inheritance of a precompiled class. It covers both incremental compilation and support of inheritance across modules.

The idea is that we now have a new class (right below in the current PR) that is part of the test code itself. Hence it will be compiled, processed by butterknife too and passed on the classpath of the tests when they are run. It mimics the fact that we are relying on a compiled super class, and not to a super class passed to the processor.

stephanenicolas · 2019-02-09T23:15:14Z

sample/base-library/build.gradle

@@ -0,0 +1,27 @@
+buildscript {


I have decomposed the library of the sample into a base library and a library itself so that the sample also covers the case of inheritance across module. Feel free to revert if you want.

stephanenicolas · 2019-02-09T23:17:16Z

sample/library/build.gradle

+  //TODO this change is just to demonstrate that the new compiler works well
+  //but should be reversed when the new version is released
+  //annotationProcessor deps.release.compiler
+  annotationProcessor project(':butterknife-compiler')


I can revert that as soon as you want, I just use it for debugging purposes. I will probably open a ticket soon on the BK repo: in another project we now use composite builds to manage samples and it works great. It allows to have a clean sample and also to use the current modules being developed for testing purposes.

oldergod

cool

oldergod · 2019-02-10T03:47:11Z

butterknife-compiler/src/main/java/butterknife/compiler/ButterKnifeProcessor.java

    filer = env.getFiler();
    try {
-      trees = Trees.instance(processingEnv);
-    } catch (IllegalArgumentException ignored) {
+      //reflection won't be necessary after https://github.com/gradle/gradle/pull/8393


The issue is closed, is the comment still relevant?

I think it still provides some context. Let me know if you want me to remove it.

If the issue isn't getting fixed, maybe we can change it. What would future dev would like to see here?

oldergod · 2019-02-10T03:48:18Z

butterknife-compiler/src/main/java/butterknife/compiler/ButterKnifeProcessor.java

+    BindingSet.Builder parentBuilder = BindingSet.newBuilder(parentType);
+    if (hasViewBindings(parentType)) {
+      //add a fake field to the parent class so that it will indicate it has a view bindings.
+      //this is required for the subclass to generate a proper view binder


https://nedbatchelder.com/blog/201401/comments_should_be_sentences.html

I try to do that too. You mean it should start with upper case and end with a . ?

oldergod · 2019-02-10T03:49:41Z

butterknife-runtime/src/test/java/butterknife/BindViewTest.java

+import static butterknife.TestStubs.ANDROIDX_CONTEXT_COMPAT;
+import java.util.List;
+import java.util.Map;
+


not sure imports order is right, and no need to add empty lines between them

What do you think of applying the spotless plugin in ButterKnife ? I can file a ticket for that.

Jake can decide, I don't know. My point is rather that whatever rule was applied before is the correct way to organize imports.

oldergod · 2019-02-10T03:52:10Z

sample/library/build.gradle

+
+  //TODO this change is just to demonstrate that the new compiler works well
+  //but should be reversed when the new version is released
+  //annotationProcessor deps.release.compiler


https://nedbatchelder.com/blog/201401/comments_should_be_sentences.html

stephanenicolas · 2019-02-10T18:40:12Z

I get your points Benoit. I am open to change my comments. But I am waiting to hear from maintainers about the core stuff of this PR. Or are these the main changes the bk team is asking before merging it ? Thx for the review. Le dim. 10 févr. 2019 à 10:35, Benoît Quenaudon <[email protected]> a écrit :

…

***@***.**** commented on this pull request. ------------------------------ In butterknife-compiler/src/main/java/butterknife/compiler/ButterKnifeProcessor.java <#1466 (comment)> : > filer = env.getFiler(); try { - trees = Trees.instance(processingEnv); - } catch (IllegalArgumentException ignored) { + //reflection won't be necessary after gradle/gradle#8393 If the issue isn't getting fixed, maybe we can change it. What would future dev would like to see here? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1466 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABv33Y9O-lgS3yjzeTbT_qzroqVjIqksks5vMGZogaJpZM4ayxtG> .

oldergod · 2019-02-10T22:49:17Z

Tout est possible à celui qui croit and I believe in Jack Wharton.

Alonexx · 2019-05-10T16:19:28Z

This change is not compatible with kapt, since Trees via reflection became invalid again. https://github.com/JetBrains/kotlin/blob/master/plugins/kapt3/kapt3-base/src/org/jetbrains/kotlin/kapt3/base/incremental/incrementalProcessors.kt

dhruvj · 2019-05-18T09:14:53Z

When will this be merged? As Google is recommending everyone to move towards "modular" architecture, I think this problem is only going to get more frequently reported.

stephanenicolas · 2019-05-18T09:22:26Z

@alone I couldn't find documentation about kapt making it deprecated to access trees. It is officially already the case in Java with Gradle. But the changes to make BK incremental do work well, it's a edge case but everything works fine. I would assume it also works pretty well with kapt.

…

On Sat., May 18, 2019, 11:15 Dhruv, ***@***.***> wrote: When will this be merged? As Google is recommending everyone to move towards "modular" architecture, I think this problem is only going to get more frequently reported. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1466?email_source=notifications&email_token=AAN7PXKJTI2HG6GFGXYRKW3PV7CMJA5CNFSM4GWLDNDKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVWK4XI#issuecomment-493661789>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAN7PXOSMZZASDFYLSTD6YLPV7CMJANCNFSM4GWLDNDA> .

aritrobanerjee93 · 2019-09-03T21:18:14Z

When will this be merged?

Support inheritance across modules and incremental annotation processing

eca7927

stephanenicolas commented Feb 9, 2019

View reviewed changes

This was referenced Feb 9, 2019

Support incremental annotation processing #1225

Closed

Support incremental annotation processing. #1430

Closed

Support incremental annotation processing. #1432

Closed

oldergod reviewed Feb 10, 2019

View reviewed changes

gavra0 mentioned this pull request Sep 4, 2019

Add support for finding parent bindings in classpath #1545

Merged

JakeWharton closed this in #1545 Sep 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support inheritance across modules and incremental annotation processing #1466

Support inheritance across modules and incremental annotation processing #1466

stephanenicolas commented Feb 9, 2019 •

edited

Loading

stephanenicolas Feb 9, 2019 •

edited

Loading

stephanenicolas Feb 9, 2019 •

edited

Loading

stephanenicolas Feb 9, 2019

stephanenicolas Feb 9, 2019 •

edited

Loading

stephanenicolas Feb 9, 2019 •

edited

Loading

stephanenicolas Feb 9, 2019

stephanenicolas Feb 9, 2019 •

edited

Loading

stephanenicolas Feb 9, 2019 •

edited

Loading

oldergod left a comment

oldergod Feb 10, 2019

stephanenicolas Feb 10, 2019

oldergod Feb 10, 2019

oldergod Feb 10, 2019

stephanenicolas Feb 10, 2019

oldergod Feb 10, 2019

oldergod Feb 10, 2019

stephanenicolas Feb 10, 2019

oldergod Feb 10, 2019

oldergod Feb 10, 2019

stephanenicolas commented Feb 10, 2019 via email •

edited

Loading

oldergod commented Feb 10, 2019

Alonexx commented May 10, 2019

dhruvj commented May 18, 2019

stephanenicolas commented May 18, 2019 via email

aritrobanerjee93 commented Sep 3, 2019

Support inheritance across modules and incremental annotation processing #1466

Support inheritance across modules and incremental annotation processing #1466

Conversation

stephanenicolas commented Feb 9, 2019 • edited Loading

stephanenicolas Feb 9, 2019 • edited Loading

Choose a reason for hiding this comment

stephanenicolas Feb 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephanenicolas Feb 9, 2019 • edited Loading

Choose a reason for hiding this comment

stephanenicolas Feb 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephanenicolas Feb 9, 2019 • edited Loading

Choose a reason for hiding this comment

stephanenicolas Feb 9, 2019 • edited Loading

Choose a reason for hiding this comment

oldergod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephanenicolas commented Feb 10, 2019 via email • edited Loading

oldergod commented Feb 10, 2019

Alonexx commented May 10, 2019

dhruvj commented May 18, 2019

stephanenicolas commented May 18, 2019 via email

aritrobanerjee93 commented Sep 3, 2019

stephanenicolas commented Feb 9, 2019 •

edited

Loading

stephanenicolas Feb 9, 2019 •

edited

Loading

stephanenicolas Feb 9, 2019 •

edited

Loading

stephanenicolas Feb 9, 2019 •

edited

Loading

stephanenicolas Feb 9, 2019 •

edited

Loading

stephanenicolas Feb 9, 2019 •

edited

Loading

stephanenicolas Feb 9, 2019 •

edited

Loading

stephanenicolas commented Feb 10, 2019 via email •

edited

Loading