Incremental compilation support seems implemented incorrectly to me

[bejibx]

For proper support of incremental compilation you should provide originating elements when creating source files. See Filer#createSourceFile(CharSequence, Element...) documentation. When using JavaPoet you should specify originating elements using TypeSpec.Builder#addOriginatingElement(Element) method. I can't spot a single call to this method in this repo, which can lead to missing files during build, some generated files not being rebuild when needed and other problems.

Another problem I spot is you declare your annotation processor both using AutoService library and META-INF.services file, which is kinda strange because AutoService is generating this file for you.

Also you can optimize incap support by splitting MvpCompiler into two - one for generating MoxyReflector (this one is aggregating) and one for other stuff (isolating).

I did so in my PR with incap support to original repo. I can work on migrating my changes into this repo, but I don't want to do so while processor tests are broken. I created another issue about it - #23.

[aasitnikov]

Hi! Thanks for creating this issue.

First of all, we decided to ditch MoxyReflector in favor of actual reflection, as Class.forName() is as fast, as using generated code. This will take away the burden of declaring @RegisterMoxyReflectorPackages and moxyReflectorPackage option for users.

Also we tried to split processor into aggregating and isolating, but this idea was rejected. You cannot declare originating element in ViewStateClassGenerator, as it breaks rules for isolating processors(no annotations in View interfaces).

I think declaring originatingElement takes no effect in aggregating processors, but this supposition have to be checked.

I think i will go through your original PR to cherry-pick some changes, if you don't mind.

[bejibx]

You cannot declare originating element in ViewStateClassGenerator, as it breaks rules for isolating processors(no annotations in View interfaces).

I didn't hear about that. Based on gradle incremental annotation processing documentation isolating processors have following limitations:

• 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.
• They must make all decisions (code generation, validation messages) for an annotated type based on information reachable from its AST. This means you can analyze the types' super-class, method return types, annotations etc., even transitively. But you cannot make decisions based on unrelated elements in the RoundEnvironment. Doing so will result in silent failures because too few files will be recompiled later. If your processor needs to make decisions based on a combination of otherwise unrelated elements, mark it as "aggregating" instead.
• They must provide exactly one originating element for each file generated with the Filer API. If zero or many originating elements are provided, Gradle will recompile all source files.

Where did you find this info?

Anyway, If you are going to remove MoxyReflector, you can just make MvpCompiler plain isolating I think.

I think declaring originatingElement takes no effect in aggregating processors, but this supposition have to be checked.

Hmm, actually I don't know if this is true. Gradle documentation says nothing about originating elements in aggregating processor, so it can be true.

I think i will go through your original PR to cherry-pick some changes, if you don't mind.

Sure, I wanted to do it by myself, but broken tests really makes me feel uncomfortable because I can break something.

[aasitnikov]

Where did you find this info?

Based on this item:

• They must make all decisions (code generation, validation messages) for an annotated type based on information reachable from its AST. This means you can analyze the types' super-class, method return types, annotations etc., even transitively. But you cannot make decisions based on unrelated elements in the RoundEnvironment. Doing so will result in silent failures because too few files will be recompiled later. If your processor needs to make decisions based on a combination of otherwise unrelated elements, mark it as "aggregating" instead.

ViewStates are generated for View interfaces, that are type parameters of MvpPresenters, or parameters of @InjectPresenter annotation. This is what contradicts referenced item - MvpPresenter is unrelated for View interface. If you change View interface, isolating annotation processor won't be able to restart processing to generate ViewState, cause there's no annotation on interface.

[bejibx]

Sorry, I'm still can't get it. So if we ignore MoxyReflector, MvpCompiler is generating 3 other things: ViewState, PresentersBinder and ViewStateProvider.

Lets say we have following classes:

class ExampleFragment : ExampleView {

    @InjectPresenter
    lateinit var presenter: ExamplePresenter
}

class ExampleView: MvpView {

    @StateStrategyType(AddToEndSingleStateStrategy::class)
    fun showNumber(number: Long)
}

@InjectViewState
class ExamplePresenter : MvpPresenter<ExampleView> {
}

ViewState is based only on view interface, so originating element should be ExampleView.

PresentersBinder originating element is ExampleFragment.

ViewStateProvider originating element is ExamplePresenter. ViewStateProvider should only know about ViewState type which is based on view interface used by presenter.

I think I missing something important but can't figure out what?

[bejibx]

or parameters of @InjectPresenter annotation

What does this mean? Can we somehow use @InjectPresenter annotation to specify view type? I can't see how to do that based on current source code:

@Target(ElementType.FIELD)
@Retention(RetentionPolicy.RUNTIME)
public @interface InjectPresenter {
	String EMPTY = "";

	String tag() default EMPTY;

	PresenterType type() default PresenterType.LOCAL;

	String presenterId() default EMPTY;
}

[bejibx]

Oh, I think I get it. You are worry because we are not annotating view interface itself, right? I think this is ok because view interface is part of presenter AST, which means if we change it, this will trigger @InjectViewState annotation processing on presenter, which will also recreate ViewState.

[aasitnikov]

Yes, but what would you set as originating element for generated ViewState?
If you choose view interface, than gradle won't be able to restart annotation proccessing for it, as there's no annotations on it.
If you choose presenter, than changes in interface won't trigger incremental annotation processing at all.
If you choose both, then gradle will fallback to non-incremental processing, as each element must have exactly one originating element.

I've tested all of the above, when i tried to make it isolating, and gradle consistently failed.

[bejibx]

Hmm, this is strange because it worked fine for me. Probably my tests was incorrect, I'll try again more carefully.

[bejibx]

Can you please tell me how did you test this? I'm not able to reproduce this behavior.

What I'm trying is:

• I'm using this branch which is based on original Moxy develop branch plus these PR's: Fix inherited method being written twice inside generated ViewState when using modifiers Arello-Mobile/Moxy#259, Support gradle incremental compilation Arello-Mobile/Moxy#256.
• I add kapt.incremental.apt=true to gradle.properties
• I started with ./gradlew clean moxy-androidx-sapmle:assembleDebug
• Then I add method to MainView interface:

interface MainView : MvpView {
	fun printLog(msg: String)

	fun showNumber(i: Long) // this method
}

• Building again using ./gradlew moxy-androidx-sapmle:assembleDebug --info
• After building MainView$$State looks ok:

package example.com.moxy_androidx_sapmle;

import com.arellomobile.mvp.viewstate.MvpViewState;
import com.arellomobile.mvp.viewstate.ViewCommand;
import com.arellomobile.mvp.viewstate.strategy.AddToEndStrategy;
import java.lang.Override;
import java.lang.String;

public class MainView$$State extends MvpViewState<MainView> implements MainView {
	@Override
	public void printLog(String msg) {
		PrintLogCommand printLogCommand = new PrintLogCommand(msg);
		mViewCommands.beforeApply(printLogCommand);

		if (mViews == null || mViews.isEmpty()) {
			return;
		}

		for (MainView view : mViews) {
			view.printLog(msg);
		}

		mViewCommands.afterApply(printLogCommand);
	}

	@Override
	public void showNumber(long i) {
		ShowNumberCommand showNumberCommand = new ShowNumberCommand(i);
		mViewCommands.beforeApply(showNumberCommand);

		if (mViews == null || mViews.isEmpty()) {
			return;
		}

		for (MainView view : mViews) {
			view.showNumber(i);
		}

		mViewCommands.afterApply(showNumberCommand);
	}

	public class PrintLogCommand extends ViewCommand<MainView> {
		public final String msg;

		PrintLogCommand(String msg) {
			super("printLog", AddToEndStrategy.class);

			this.msg = msg;
		}

		@Override
		public void apply(MainView mvpView) {
			mvpView.printLog(msg);
		}
	}

	public class ShowNumberCommand extends ViewCommand<MainView> {
		public final long i;

		ShowNumberCommand(long i) {
			super("showNumber", AddToEndStrategy.class);

			this.i = i;
		}

		@Override
		public void apply(MainView mvpView) {
			mvpView.showNumber(i);
		}
	}
}

Build logs: info.txt

Am I missing something?

[aasitnikov]

Gradle supports IntAP starting from 4.7, your project is built with gradle 4.6
Kotlin supports incremental kapt from 1.3.30, your version is 1.3.10
And i suggest you to use java instead of kotlin, it produces more useful logs.

Also i've run some tests, seems like change in MainView triggers MainPresenter invalidation, which in turn triggers annotation processing on MainPresenter, which causes MainView$$State to be generated again. It`s kinda strange, but i will take a closer look at it.

[bejibx]

Gradle supports IntAP starting from 4.7, your project is built with gradle 4.6
Kotlin supports incremental kapt from 1.3.30, your version is 1.3.10

Damn, how can I forgot about it 😞 Anyway, I updated dependencies and everything still works.

Also i've run some tests, seems like change in MainView triggers MainPresenter invalidation, which in turn triggers annotation processing on MainPresenter, which causes MainView$$State to be generated again. It`s kinda strange, but i will take a closer look at it.

That what I'm saying! Let's consider this item again:

They must make all decisions (code generation, validation messages) for an annotated type based on information reachable from its AST. This means you can analyze the types' super-class, method return types, annotations etc., even transitively. But you cannot make decisions based on unrelated elements in the RoundEnvironment. Doing so will result in silent failures because too few files will be recompiled later. If your processor needs to make decisions based on a combination of otherwise unrelated elements, mark it as "aggregating" instead.

What this mean is changing view interface should trigger annotation processing on presenter because processor can make decisions about generated code based on information from view interface accessed transitively. And this is what triggers ViewState generation because MvpCompiler creates both ViewState and ViewStateProvider during @InjectViewState annotation processing.