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 inheritance across modules and incremental annotation processing #1466

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.util.Map;
import java.util.Set;
import javax.annotation.Nullable;
import javax.lang.model.element.Element;
import javax.lang.model.element.Modifier;
import javax.lang.model.element.TypeElement;
import javax.lang.model.type.TypeMirror;
Expand Down Expand Up @@ -84,14 +85,14 @@ private BindingSet(TypeName targetTypeName, ClassName bindingClassName, boolean
this.parentBinding = parentBinding;
}

JavaFile brewJava(int sdk, boolean debuggable) {
TypeSpec bindingConfiguration = createType(sdk, debuggable);
JavaFile brewJava(int sdk, boolean debuggable, Element originatingElement) {
TypeSpec bindingConfiguration = createType(sdk, debuggable, originatingElement);
return JavaFile.builder(bindingClassName.packageName(), bindingConfiguration)
.addFileComment("Generated code from Butter Knife. Do not modify!")
.build();
}

private TypeSpec createType(int sdk, boolean debuggable) {
private TypeSpec createType(int sdk, boolean debuggable, Element originatingElement) {
TypeSpec.Builder result = TypeSpec.classBuilder(bindingClassName.simpleName())
.addModifiers(PUBLIC);
if (isFinal) {
Expand Down Expand Up @@ -125,6 +126,8 @@ private TypeSpec createType(int sdk, boolean debuggable) {
result.addMethod(createBindingUnbindMethod(result));
}

result.addOriginatingElement(originatingElement);

return result.build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
import java.util.Arrays;
import java.util.BitSet;
import java.util.Deque;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.List;
Expand All @@ -72,6 +73,8 @@
import javax.lang.model.type.TypeKind;
import javax.lang.model.type.TypeMirror;
import javax.lang.model.type.TypeVariable;
import javax.lang.model.util.ElementFilter;
import javax.lang.model.util.Elements;
import javax.lang.model.util.Types;
import javax.tools.Diagnostic.Kind;

Expand Down Expand Up @@ -117,13 +120,16 @@ public final class ButterKnifeProcessor extends AbstractProcessor {
);

private Types typeUtils;
private Elements elementUtils;
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<>();
Copy link
Contributor Author

@stephanenicolas stephanenicolas Feb 9, 2019

Choose a reason for hiding this comment

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

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


@Override public synchronized void init(ProcessingEnvironment env) {
super.init(env);
Expand All @@ -143,10 +149,18 @@ public final class ButterKnifeProcessor extends AbstractProcessor {
debuggable = !"false".equals(env.getOptions().get(OPTION_DEBUGGABLE));

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

Choose a reason for hiding this comment

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

The issue is closed, is the comment still relevant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

java.lang.reflect.Field delegateField = processingEnv.getClass().getDeclaredField("delegate");
delegateField.setAccessible(true);
trees = Trees.instance((ProcessingEnvironment) delegateField.get(processingEnv));
} catch (Throwable t) {
try {
trees = Trees.instance(processingEnv);
} catch (Throwable ignored) {
Copy link
Contributor Author

@stephanenicolas stephanenicolas Feb 9, 2019

Choose a reason for hiding this comment

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

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.

}
}
}

Expand Down Expand Up @@ -190,9 +204,11 @@ private Set<Class<? extends Annotation>> getSupportedAnnotations() {
TypeElement typeElement = entry.getKey();
BindingSet binding = entry.getValue();

JavaFile javaFile = binding.brewJava(sdk, debuggable);
JavaFile javaFile = binding.brewJava(sdk, debuggable, typeElement);
try {
javaFile.writeTo(filer);
mapGeneratedFileToOriginatingElements.put(javaFile.toJavaFileObject().getName(),
javaFile.typeSpec.originatingElements);
} catch (IOException e) {
error(typeElement, "Unable to write binding for type %s: %s", typeElement, e.getMessage());
}
Expand All @@ -201,6 +217,10 @@ private Set<Class<? extends Annotation>> getSupportedAnnotations() {
return false;
}

public HashMap<String, List<Element>> getMapGeneratedFileToOriginatingElements() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

return mapGeneratedFileToOriginatingElements;
}

private Map<TypeElement, BindingSet> findAndParseTargets(RoundEnvironment env) {
Map<TypeElement, BindingSet.Builder> builderMap = new LinkedHashMap<>();
Set<TypeElement> erasedTargetNames = new LinkedHashSet<>();
Expand Down Expand Up @@ -358,6 +378,12 @@ private Map<TypeElement, BindingSet> findAndParseTargets(RoundEnvironment env) {
bindingMap.put(type, builder.build());
} else {
BindingSet parentBinding = bindingMap.get(parentType);

// parent binding is null, let's try to find a previouly generated binding
if (parentBinding == null && hasViewBinder(parentType)) {
parentBinding = createStubBindingSet(parentType);
}

if (parentBinding != null) {
builder.setParent(parentBinding);
bindingMap.put(type, builder.build());
Expand All @@ -371,6 +397,33 @@ private Map<TypeElement, BindingSet> findAndParseTargets(RoundEnvironment env) {
return bindingMap;
}

private BindingSet createStubBindingSet(TypeElement parentType) {
BindingSet parentBinding;
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

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

yes

parentBuilder.addField(new Id(-1), new FieldViewBinding("", null, false));
}
parentBinding = parentBuilder.build();
return parentBinding;
}

private boolean hasViewBindings(TypeElement parentType) {
for (VariableElement fieldElement : ElementFilter.fieldsIn(parentType.getEnclosedElements())) {
if (fieldElement.getAnnotation(BindView.class) != null
|| fieldElement.getAnnotation(BindViews.class) != null) {
return true;
}
}
return false;
}

private boolean hasViewBinder(TypeElement typeElement) {
final String viewBindingClassName = typeElement.getQualifiedName().toString() + "_ViewBinding";
return elementUtils.getTypeElement(viewBindingClassName) != null;
}

private void logParsingError(Element element, Class<? extends Annotation> annotation,
Exception e) {
StringWriter stackTrace = new StringWriter();
Expand Down Expand Up @@ -1273,7 +1326,7 @@ private BindingSet.Builder getOrCreateBindingBuilder(
return null;
}
typeElement = (TypeElement) ((DeclaredType) type).asElement();
if (parents.contains(typeElement)) {
if (parents.contains(typeElement) || hasViewBinder(typeElement)) {
Copy link
Contributor Author

@stephanenicolas stephanenicolas Feb 9, 2019

Choose a reason for hiding this comment

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

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.

return typeElement;
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
butterknife.compiler.ButterKnifeProcessor,isolating
19 changes: 18 additions & 1 deletion butterknife-runtime/src/test/java/butterknife/BindViewTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,22 @@
import butterknife.compiler.ButterKnifeProcessor;
import com.google.common.collect.ImmutableList;
import com.google.testing.compile.JavaFileObjects;

import javax.lang.model.element.Element;
import javax.tools.JavaFileObject;
import javax.tools.StandardLocation;

import org.junit.Test;

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

import static com.google.common.truth.Truth.assertAbout;
import static com.google.testing.compile.JavaSourceSubjectFactory.javaSource;
import static com.google.testing.compile.JavaSourcesSubjectFactory.javaSources;
import static java.util.Arrays.asList;
import static junit.framework.Assert.assertEquals;

public class BindViewTest {
@Test public void bindingViewNonDebuggable() {
Expand Down Expand Up @@ -49,12 +57,21 @@ public class BindViewTest {
+ "}"
);

ButterKnifeProcessor butterKnifeProcessor = new ButterKnifeProcessor();
assertAbout(javaSource()).that(source)
.withCompilerOptions("-Xlint:-processing", "-Abutterknife.debuggable=false")
.processedWith(new ButterKnifeProcessor())
.processedWith(butterKnifeProcessor)
.compilesWithoutWarnings()
.and()
.generatesSources(bindingSource);

Map<String, List<Element>> map = butterKnifeProcessor.getMapGeneratedFileToOriginatingElements();
assertEquals(1, map.size());
Map.Entry<String, List<Element>> entry = map.entrySet().iterator().next();
assertEquals("test/Test_ViewBinding.java", entry.getKey());
List<Element> elements = entry.getValue();
assertEquals(1, elements.size());
assertEquals("test.Test", elements.get(0).asType().toString());
Copy link
Contributor Author

@stephanenicolas stephanenicolas Feb 9, 2019

Choose a reason for hiding this comment

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

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

There are different options here:

  1. 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.
  2. 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.

}

@Test public void bindingViewSubclassNonDebuggable() {
Expand Down
62 changes: 62 additions & 0 deletions butterknife-runtime/src/test/java/butterknife/InheritanceTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
package butterknife;

import butterknife.compiler.ButterKnifeProcessor;
import com.google.common.collect.ImmutableList;
import com.google.testing.compile.JavaFileObjects;
import javax.tools.JavaFileObject;
import javax.tools.StandardLocation;
import org.junit.Test;

import static com.google.common.truth.Truth.assertAbout;
import static com.google.testing.compile.JavaSourceSubjectFactory.javaSource;
import static com.google.testing.compile.JavaSourcesSubjectFactory.javaSources;
import static java.util.Arrays.asList;

public class InheritanceTest {

@Test public void bindingViewFinalClassWithBaseClassAlreadyCompiledInDifferentModule() {
JavaFileObject testSource = JavaFileObjects.forSourceString("test.Test", ""
+ "package test;\n"
+ "import android.view.View;\n"
+ "import butterknife.BindView;\n"
+ "import butterknife.precompiled.Base;\n"
+ "public final class Test extends Base {\n"
+ " @BindView(1) View thing;\n"
+ "}"
);

JavaFileObject bindingTestSource = JavaFileObjects.forSourceString("test/Test_ViewBinding", ""
+ "package test;\n"
+ "import android.view.View;\n"
+ "import androidx.annotation.UiThread;\n"
+ "import butterknife.internal.Utils;\n"
+ "import butterknife.precompiled.Base_ViewBinding;\n"
+ "import java.lang.IllegalStateException;\n"
+ "import java.lang.Override;\n"
+ "public final class Test_ViewBinding extends Base_ViewBinding {\n"
+ " private Test target;\n"
+ " @UiThread\n"
+ " public Test_ViewBinding(Test target, View source) {\n"
+ " super(target, source);\n"
+ " this.target = target;\n"
+ " target.thing = Utils.findRequiredView(source, 1, \"field 'thing'\");\n"
+ " }\n"
+ " @Override\n"
+ " public void unbind() {\n"
+ " Test target = this.target;\n"
+ " if (target == null) throw new IllegalStateException(\"Bindings already cleared.\");\n"
+ " this.target = null\n"
+ " target.thing = null;\n"
+ " super.unbind();\n"
+ " }\n"
+ "}"
);

assertAbout(javaSources()).that(asList(testSource))
.withCompilerOptions("-Xlint:-processing")
.processedWith(new ButterKnifeProcessor())
.compilesWithoutWarnings()
.and()
.generatesSources(bindingTestSource);
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package butterknife.precompiled;

import android.view.View;
import butterknife.BindView;

public class Base {
@BindView(1) View thing;
}
27 changes: 27 additions & 0 deletions sample/base-library/build.gradle
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
buildscript {
Copy link
Contributor Author

@stephanenicolas stephanenicolas Feb 9, 2019

Choose a reason for hiding this comment

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

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.

repositories {
mavenCentral()
jcenter()
}

dependencies {
classpath "com.jakewharton:butterknife-gradle-plugin:${versions.release}"
}
}

apply plugin: 'com.android.library'
apply plugin: 'com.jakewharton.butterknife'

android {
compileSdkVersion versions.compileSdk
buildToolsVersion versions.buildTools

defaultConfig {
minSdkVersion versions.minSdk
}
}

dependencies {
compile deps.release.runtime
annotationProcessor deps.release.compiler
}
1 change: 1 addition & 0 deletions sample/base-library/src/main/AndroidManifest.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<manifest package="com.example.butterknife.baselibrary"/>
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package com.example.butterknife.baselibrary;

import android.app.Activity;
import butterknife.BindString;

public class BaseActivity extends Activity {
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@

<resources>
<string name="app_name">Butter Knife</string>
</resources>
</resources>
7 changes: 6 additions & 1 deletion sample/library/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,12 @@ android {

dependencies {
implementation deps.release.runtime
annotationProcessor deps.release.compiler
implementation project(':sample:base-library')

//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

Choose a reason for hiding this comment

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

annotationProcessor project(':butterknife-compiler')
Copy link
Contributor Author

@stephanenicolas stephanenicolas Feb 9, 2019

Choose a reason for hiding this comment

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

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.


testImplementation deps.junit
testImplementation deps.truth
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package com.example.butterknife.library;

import android.annotation.SuppressLint;
import android.app.Activity;
import android.os.Bundle;
import androidx.annotation.NonNull;
import android.view.View;
Expand All @@ -16,11 +15,12 @@
import butterknife.OnClick;
import butterknife.OnItemClick;
import butterknife.OnLongClick;
import com.example.butterknife.baselibrary.BaseActivity;
import java.util.List;

import static android.widget.Toast.LENGTH_SHORT;

public class SimpleActivity extends Activity {
public class SimpleActivity extends BaseActivity {
private static final ButterKnife.Action<View> ALPHA_FADE = new ButterKnife.Action<View>() {
@Override public void apply(@NonNull View view, int index) {
AlphaAnimation alphaAnimation = new AlphaAnimation(0, 1);
Expand Down
1 change: 1 addition & 0 deletions settings.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ include ':butterknife-reflect'
include ':butterknife-runtime'

//include ':sample:app'
//include ':sample:base-library'
//include ':sample:library'

rootProject.name = 'butterknife-parent'