Dagger: File name too long when nesting subcomponents

[pyricau]

I'm going through the final steps to fully convert our app to Dagger 2, tying the hierarchy of subcomponents together. We have a deep hierarchy of sub components. This means the subcomponent names are too long:

error while writing DaggerDevRegisterAppComponent.DevLoggedInComponentImpl.DevRootActivityComponentImpl.SellerFlow_MobileComponentImpl.HomeScreen_MobileComponentImpl.TenderPath_ComponentImpl.AbstractGiftCardBalancePath_ComponentImpl.GiftCardBalanceInputScreen_ComponentImpl: 
./DaggerDevRegisterAppComponent$DevLoggedInComponentImpl$DevRootActivityComponentImpl$SellerFlow_MobileComponentImpl$HomeScreen_MobileComponentImpl$TenderPath_ComponentImpl$AbstractGiftCardBalancePath_ComponentImpl$GiftCardBalanceInputScreen_ComponentImpl.class (File name too long)

[pyricau]

Maybe Dagger should chance its strategy for naming the impls, and add a numbering scheme for duplicates?

Not sure what I can do in the meantime.

[gk5885]

A few things here…

• I think the current naming scheme is pretty ideal for keeping things clear and straightforward. I can't think of any change that we'd want to just apply across the board that would be shorter while being clearer or even as clear.
• We _could_ use an alternate scheme for whenever we think the names are getting too long. Unfortunately, "too long" can be hard to predict: https://en.wikipedia.org/wiki/Comparison_of_file_systems#Limits
• Adding javadoc has long been on our list of things to do and if we do change the names, at least javadoc @links back to the component would soften the blow.

A quick strawman for something that might help would be to use initialisms to try to shorten some of these names. Here's an example if we just did it for any enclosing classes:
DaggerDevRegisterAppComponent.DevLoggedInComponentImpl.DevRootActivityComponentImpl.SF_MobileComponentImpl.HS_MobileComponentImpl.TP_ComponentImpl.AGCBP_ComponentImpl.GCBIS_ComponentImpl

[valeriyo]

Instead of changing the filename generation scheme wholesale, how about providing a class annotation so that folks who encounter this issue could override the Dagger-generated name part? For example, for class DevRegisterAppComponent --> @DaggerName(name = "DDevRegAppCmp"). This would still keep easy-to-read filenames for everyone, but allow teams with complex hierarchies to mitigate the long filename problem locally.

Or even better, add "name" parameter to @Component annotation.

[loganj]

Or just turn on a "deep component hierarchy" flag and switch to a flat class naming scheme. <ComponentName>_<UUID>.java or similar.

[ronshapiro]

It would be pretty lame if you had to know that you needed to pass that flag just in order to have your code compile.

@gk5885 what if all subcomponent types were nested directly within the generated component? They'd all still be inaccessible from without the codegen, and we could pass references directly instead of implicitly. Then as long as "Dagger" + component_name + longest_subcomponent_name < N you'd be fine.

[pyricau]

Now, please make fun of me, this is my temporary fix for our internal fork:

--- a/compiler/src/main/java/dagger/internal/codegen/ComponentWriter.java
+++ b/compiler/src/main/java/dagger/internal/codegen/ComponentWriter.java
@@ -44,12 +44,15 @@ import javax.lang.model.element.Name;
 import javax.lang.model.element.TypeElement;
 import javax.lang.model.util.Elements;
 import javax.lang.model.util.Types;
+import java.util.concurrent.atomic.AtomicInteger;

 /**
  * Creates the implementation class for a component.
  */
 final class ComponentWriter extends AbstractComponentWriter {

+  private static final AtomicInteger count = new AtomicInteger(0);
+
   ComponentWriter(
       Types types,
       Elements elements,
@@ -108,23 +111,10 @@ final class ComponentWriter extends AbstractComponentWriter {

     private ImmutableBiMap<ComponentDescriptor, String> disambiguateConflictingSimpleNames(
         Collection<ComponentDescriptor> components) {
+      // See https://github.com/google/dagger/issues/421
       Map<String, ComponentDescriptor> generatedSimpleNames = new LinkedHashMap<>();
-      // The ending condition is when there is a unique simple name generated for every element
-      // in components. The sizes should be equivalent (with one generated name per component).
-      for (int levels = 0; generatedSimpleNames.size() != components.size(); levels++) {
-        generatedSimpleNames.clear();
-        for (ComponentDescriptor component : components) {
-          List<String> pieces = componentQualifiedNamePieces.get(component);
-          String simpleName =
-              QUALIFIED_NAME_JOINER.join(
-                  pieces.subList(Math.max(0, pieces.size() - levels - 1), pieces.size()));
-          ComponentDescriptor conflict = generatedSimpleNames.put(simpleName, component);
-          if (conflict != null) {
-            // if the map previously contained an entry for the same simple name, stop early since
-            // 2+ subcomponent descriptors will have the same simple name
-            break;
-          }
-        }
+      for (ComponentDescriptor component : components) {
+        generatedSimpleNames.put("C" + count.incrementAndGet(), component);
       }
       return ImmutableBiMap.copyOf(generatedSimpleNames).inverse();
     }

[pyricau]

Which generates things like com.squareup.DaggerAppComponent.C2Impl.C279Impl.C281Impl.C282Impl.C63Impl.C64Impl

[ronshapiro]

FYI, the processor is single threaded so the atomic integer isn't necessary (unless you plan on maintaining this fork for a while ;)

[gk5885]

@valeriyo, I'd rather not add an API for tweaking something that is _entirely_ an implementation detail.

@loganj, if by "flag naming scheme" you mean using top-level classes rather than nested classes, that would be an option, but means that we'd have to generate components very differently. We would either have to make a lot of fields accessible or pass around a bunch of state (probably more than the max number of cxtor args).

@ronshapiro, that's a variant of flattening the component hierarchy that might dodge some accessibility issues, but does complicate the generated code because you can no longer use the implicit this pointer. We'd have to start keeping track of some number of parent references and traverse the correct number of them to get to some Provider. E.g. this.parent.parent.parent.applicationProvider. Ick. It'd work but it seems like it'd be a little gross.

OK, time to make fun of @py. Not really, but that is _exactly_ the type of thing we don't want to show up in a stack trace. We might as well call them $$$FastClassByGuice$$. :P

@ronshapiro, I'd stick with AtomicInterger b/c incrementAndGet() is useful even if you don't care about concurrency.

So, here's what I'm thinking we can do…

• Let's use 255 characters as the heuristic. It's not exactly right, but should cover most cases correctly.
• Collapse names w/ some version of initialisms, but only when we have to. I.e. walk the whole tree of subcomponents, figure out the name size and just shorten the ones that are required to make them and all of their children fit.

Sound like a reasonable enough plan?

[pyricau]

Yes! You might still want to use a variant of my strategy for the degenerate case where shortening still creates conflicts?

[pyricau]

For the record:

The actual fix will be deeply appreciated :) .

[py]

Why are you making fun of me? )c:

On Wednesday, August 3, 2016, Gregory Kick [email protected] wrote:

@valeriyo https://github.com/valeriyo, I'd rather not add an API for
tweaking something that is _entirely_ an implementation detail.

@loganj https://github.com/loganj, if by "flag naming scheme" you mean
using top-level classes rather than nested classes, that would be an
option, but means that we'd have to generate components very differently.
We would either have to make a lot of fields accessible or pass around a
bunch of state (probably more than the max number of cxtor args).

@ronshapiro https://github.com/ronshapiro, that's a variant of
flattening the component hierarchy that might dodge some accessibility
issues, but does complicate the generated code because you can no longer
use the implicit this pointer. We'd have to start keeping track of some
number of parent references and traverse the correct number of them to
get to some Provider. E.g. this.parent.parent.parent.applicationProvider.
Ick. It'd work but it seems like it'd be a little gross.

OK, time to make fun of @py https://github.com/py. Not really, but that
is _exactly_ the type of thing we don't want to show up in a stack trace.
We might as well call them $$$FastClassByGuice$$. :P

@ronshapiro https://github.com/ronshapiro, I'd stick with AtomicInterger
b/c incrementAndGet() is useful even if you don't care about concurrency.

So, here's what I'm thinking we can do…

• Let's use 255 characters as the heuristic. It's not exactly right,
  but should cover most cases correctly.
• Collapse names w/ some version of initialisms, but only when we have
  to. I.e. walk the whole tree of subcomponents, figure out the name size and
  just shorten the ones that are required to make them and all of their
  children fit.

Sound like a reasonable enough plan?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/google/dagger/issues/421#issuecomment-237394827, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAOB0hcM1_U-t6o-t2bo3F4RiBIza_Oiks5qcRXzgaJpZM4Ja9NX
.

[gk5885]

Sorry, @py. Let me invite you to join in making fun of @pyricau.

[gk5885]

@pyricau, I'm actually at a conference this week and not going to be able to look at this for quite a while. Do you have any interest in taking this on? Otherwise, it might be a while before we get a fix.

[pyricau]

Just saw this. I don't have any bandwidth at the moment. I'll pass the word around, maybe someone in my team will get pissed and just do it.

[vanniktech]

This was not mentioned properly in the Github release notes - https://github.com/google/dagger/releases/tag/dagger-2.11-rc2

[TWiStErRob]

@pyricau @gk5885 this problem came up for 8 nesting levels, shouldn't the names be kept for 2-3 levels to cover most common usage with nice names, and then start abbreviating only at deeper levels?