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

Non-static providers inside abstract modules cause runtime crashes #621

Closed
AttwellBrian opened this issue Mar 8, 2017 · 12 comments
Closed

Non-static providers inside abstract modules cause runtime crashes #621

AttwellBrian opened this issue Mar 8, 2017 · 12 comments

Comments

@AttwellBrian
Copy link

@AttwellBrian AttwellBrian commented Mar 8, 2017

If you forget to add static to an abstract module your app will crash at runtime. This should be prevented at compile time.

Example:
Suppose you have a nice abstract module such as the following example:

class SomeOuterClass {
  @dagger.Module
  public abstract static class Module {
      @NonNull
      @OnlineScope
      @Provides
      static A a() {
          return new A();
      }
  }

  @OnlineScope
  @dagger.Component(modules = Module.class, dependencies = ParentComponent.class)
  interface Component {
      @dagger.Component.Builder
      interface Builder {
          @BindsInstance
          Builder interactor(OnlineInteractor interactor);

          @BindsInstance
          Builder view(Observable<Optional<OnlineView>> view);

          Builder parentComponent(ParentComponent component);
          Component build();
      }
  }
}

Next, lets add a new provider to the Module.

@dagger.Module
public abstract static class Module {
    @NonNull
    @OnlineScope
    @Provides
    static A a() {
        return new A();
       }
   }

    @NonNull
    @OnlineScope
    @Provides
    B b() {
        return new B();
    }
}

Oops! We forgot to add static to the second provider. This example now crashes at runtime.

@ronshapiro

This comment has been minimized.

Copy link

@ronshapiro ronshapiro commented Mar 8, 2017

Are you saying the issue is that A and B aren't used in the component?

@AttwellBrian

This comment has been minimized.

Copy link
Author

@AttwellBrian AttwellBrian commented Mar 8, 2017

The issue is that dagger will no longer treat the module as abstract. So we will see a java.lang.IllegalStateException: OnlineBuilder.Module must be set when building the component.

@naturalwarren

This comment has been minimized.

Copy link

@naturalwarren naturalwarren commented Mar 9, 2017

In @AttwellBrian's example engineers add a new @Provides instance method to an already existing static, abstract module that has all static providers, and the app crashes at runtime because an instance of the module is now required.

We could forbid instance methods from being used in abstract modules to catch this kind of thing at compile time. If a module is abstract you can't provide an instance of it because you can't instantiate it.

@ronshapiro

This comment has been minimized.

Copy link

@ronshapiro ronshapiro commented Mar 10, 2017

Can you clarify for me:

  • is it a compiler error, or a runtime error? if there's a stack trace, can you provide the entire stack trace?
  • Can you provide the failing case (in a repo ideally), and make a comment over the method that should be removed to get it all working?

static modules should be treated like any top level module.

It seems to me that the issue is that the @Component.Builder that you defined should have errored out in the compiler becuase you didn't have a setter method for the abstract module. If the module is not needed by the component implementation and there's a bug in our consistency of that, then we should fix that.

@naturalwarren

This comment has been minimized.

Copy link

@naturalwarren naturalwarren commented Mar 11, 2017

is it a compiler error, or a runtime error? if there's a stack trace, can you provide the entire stack trace?

This is a runtime error. Entire stack trace included below:

03-10 21:00:34.758 21551-21551/com.uber.moduleruntimebug E/AndroidRuntime: FATAL EXCEPTION: main Process: com.uber.moduleruntimebug, PID: 21551                                                                         java.lang.RuntimeException: Unable to start activity
ComponentInfo{com.uber.moduleruntimebug/com.uber.moduleruntimebug.MainActivity}:
java.lang.IllegalStateException: com.uber.moduleruntimebug.MainActivity.MainActivityModule must be set                                                                             
   at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2665)                                                                             
   at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2726)                                                                             
   at android.app.ActivityThread.-wrap12(ActivityThread.java)                                                                             
   at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1477)                                                                             
   at android.os.Handler.dispatchMessage(Handler.java:102)                                                                             
   at android.os.Looper.loop(Looper.java:154)                                                                             
   at android.app.ActivityThread.main(ActivityThread.java:6119)                                                                             
   at java.lang.reflect.Method.invoke(Native Method)                                                                             
   at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:886)                                                                             
   at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:776)                                                                           

Caused by: java.lang.IllegalStateException: com.uber.moduleruntimebug.MainActivity.MainActivityModule must be set at
com.uber.moduleruntimebug.DaggerMainActivity_MainActivityComponent$Builder.build(DaggerManActivity_MainActivityComponent.java:45)                                                                             
   at com.uber.moduleruntimebug.MainActivity.onCreate(MainActivity.java:24)                                                                             
   at android.app.Activity.performCreate(Activity.java:6679)                                                                             
   at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1118)                                                                             
   at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2618)                                                                             
   at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2726)                                                                              
   at android.app.ActivityThread.-wrap12(ActivityThread.java)                                                                              
   at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1477)                                                                              
   at android.os.Handler.dispatchMessage(Handler.java:102)                                                                              
   at android.os.Looper.loop(Looper.java:154)                                                                              
   at android.app.ActivityThread.main(ActivityThread.java:6119)                                                                              
   at java.lang.reflect.Method.invoke(Native Method)                                                                              
   at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:886)                                                                              
   at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:776)

Can you provide the failing case (in a repo ideally), and make a comment over the method that should be removed to get it all working?

This can be found at https://github.com/naturalwarren/DaggerModuleBug

It seems to me that the issue is that the @Component.Builder that you defined should have errored out in the compiler becuase you didn't have a setter method for the abstract module.

After building the sample above I realized that if a module is abstract it can't be instantiated so an instance can't be provided regardless of whether you define a setter method in your component builder.

I'd suggest we just disallow instance methods in abstract modules at compile time.

@pyricau

This comment has been minimized.

Copy link

@pyricau pyricau commented Aug 7, 2017

We've been running into the same issue. Technically, this is actually not a Dagger bug. However this behavior leads to incorrect usage.

There are 3 kind of modules:

  • Modules for which Dagger needs instances, and can create on its own:
@Module
class MyModule {
  @Provides Thing foo() { return new Thing(); }
}
  • Modules for which Dagger does not need instances:
@Module
abstract class MyModule {
  @Provides static Thing foo() { return new Thing(); }
}
  • Modules for which Dagger needs instances, but cannot create on its own:
@Module
class MyModule {
  Thing thing;

  MyModule(Thing thing) { this.thing = thing; }

  @Provides Thing foo() { return thing; }
}

For the latter modules, the build() method of Builders will throw an exception if a module instance has not been set manually on the component builder.

The problem appears with this type of module:

@Module
abstract class MyModule {
  @Provides static Thing foo() { return new Thing(); }

  @Provides OtherThing bar() { return new OtherThing(); }
}

This latter module has a second method that isn't static. So Dagger needs a module instance. However that module is abstract and therefore Dagger cannot create instances. So the generated code expects the module instance to be provided.

This case could maybe be a compiler error instead. However, there might be legitimate cases for abstract modules with real implementations that aren't known to Dagger.

@tasomaniac

This comment has been minimized.

Copy link

@tasomaniac tasomaniac commented Aug 8, 2017

We had the same problem too while migrating all the modules to be abstract. We basically missed one method and it was a runtime crash.
@pyricau 's explanation is perfect and 100% the same what we have. This can easily be a compile time validation by dagger.

@ronshapiro

This comment has been minimized.

Copy link

@ronshapiro ronshapiro commented Aug 11, 2017

I talked this over with @netdpb and we think the best path forward is to not allow abstract modules that have instance @Provides methods. The error will say to either make the methods static, or use a subclass of the module (where it's clear whether or not an instance is required). How does that sound?

@naturalwarren

This comment has been minimized.

Copy link

@naturalwarren naturalwarren commented Aug 11, 2017

That sounds great.

ronshapiro added a commit that referenced this issue Aug 14, 2017
Fixes #621

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=165177293
ronshapiro added a commit that referenced this issue Aug 14, 2017
Fixes #621

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=165177293
ronshapiro added a commit that referenced this issue Aug 14, 2017
Fixes #621

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=165177293
@pyricau

This comment has been minimized.

Copy link

@pyricau pyricau commented Sep 28, 2017

Thanks! This is great!

@ltpquang

This comment has been minimized.

Copy link

@ltpquang ltpquang commented May 17, 2018

@pyricau thanks so much, totally saved my day. But how did you get to know those "kinds of module"? Are those documented anywhere?

@pyricau

This comment has been minimized.

Copy link

@pyricau pyricau commented May 30, 2018

abstract modules?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.