Dagger: Includes visibility validation is too strict

[JakeWharton]

The following component is valid:

@Component(modules = {
    ModuleIncludes.StringModule.class,
    ModuleIncludes.IntegerModule.class
})
interface ModuleIncludes {
  String string();

  @Module
  abstract class StringModule {
    @Provides static String string(Integer value) {
      return value.toString();
    }
  }

  @Module
  abstract class IntegerModule {
    @Provides static Integer value() {
      return 2;
    }
  }
}

But if I change it to

@Component(modules = ModuleIncludes.StringModule.class)
interface ModuleIncludes {
  String string();

  @Module(includes = IntegerModule.class)
  abstract class StringModule {
    @Provides static String string(Integer value) {
      return value.toString();
    }
  }

  @Module
  abstract class IntegerModule {
    @Provides static Integer value() {
      return 2;
    }
  }
}

Dagger will reject it:

src/main/java/com/example/ModuleIncludes.java:12: error: This module is public, but it includes non-public (or effectively non-public) modules. Either reduce the visibility of this module or make com.example.ModuleIncludes.IntegerModule public.
  abstract class StringModule {
           ^
src/main/java/com/example/ModuleIncludes.java:7: error: com.example.ModuleIncludes.StringModule has errors
@Component(modules = ModuleIncludes.StringModule.class)
                                                ^
2 errors

If the module isn't require to be public when included directly on the component, why is it required to be so when declared as a transitive module?

[tbroyer]

I think this is because nested classes in interfaces are public, but Dagger fails to resolve the "effective visibility" of the StringModule, treating it as public instead of package-private (whereas it correctly sees IntegerModule as package-private, hence the error).

[JakeWharton]

I thought it may be that, or it may see the enclosing interface being package-private and think it's non-public as a result (which is correct, but the other module isn't enforced to this restriction).

[ronshapiro]

We actually just addressed this in
bfdecadf4f2684a0b6e1818605d68390746faa59. Should be in the next release

On Sun, Mar 17, 2019, 9:09 AM Jake Wharton [e-mail] wrote:

I thought it may be that, or it may see the enclosing interface being
package-private and think it's non-public as a result (which is correct,
but the other module isn't enforced to this restriction).

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/google/dagger/issues/1445#issuecomment-473664220, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAwY3YJkpj-m7RvaqxwLgG6iKbQr5ZT7ks5vXj6ggaJpZM4b4K9W
.

[tbroyer]

While this would fix this example, I think it's only a side effect and does not fix the actual issue: to be confirmed (on my phone, don't have a computer at hand) but moduleVisibility in the code will be PUBLIC for the member class, which triggers the validation. It should be its effectiveVisibilityOfElement, which would be package-private, and wouldn't trigger the validation.

[ronshapiro]

@cgdecker

[cgdecker]

Yeah, that seems like a slightly different issue than what we fixed in bfdecad. We should only be doing that validation for modules that are effectively public, i.e. those that may actually be referenced outside their package. I'll get a fix in soon.