spotify / dataenum

Algebraic data types in Java.
Apache License 2.0
166 stars 16 forks source link

Remove synthetic accessor from generated code #13

Closed bangarharshit closed 6 years ago

bangarharshit commented 6 years ago

The currently generated code is generating synthetic accessors for constructors for inner classes. It can be a big saving if the identifier is changed from private to package protected.

import java.lang.Boolean;
import java.lang.Integer;
import java.lang.Object;
import java.lang.Override;
import java.lang.String;
import java.lang.StringBuilder;
import java.util.function.Consumer;
import java.util.function.Function;
import javax.annotation.Generated;
import javax.annotation.Nonnull;

public abstract class MultipleValues {
  private MultipleValues() {
  }

  public static MultipleValues value1(int param1, boolean param2) {
    return new Value1(param1, param2);
  }

  public static MultipleValues value2(int param1, boolean param2) {
    return new Value2(param1, param2);
  }

  public final boolean isValue1() {
    return (this instanceof Value1);
  }

  public final boolean isValue2() {
    return (this instanceof Value2);
  }

  public final Value1 asValue1() {
    return (Value1) this;
  }

  public final Value2 asValue2() {
    return (Value2) this;
  }

  public abstract void match(@Nonnull Consumer<Value1> value1, @Nonnull Consumer<Value2> value2);

  public abstract <R_> R_ map(@Nonnull Function<Value1, R_> value1,
      @Nonnull Function<Value2, R_> value2);

  public static final class Value1 extends MultipleValues {
    private final int param1;

    private final boolean param2;

    private Value1(int param1, boolean param2) {
      this.param1 = param1;
      this.param2 = param2;
    }

    public final int param1() {
      return param1;
    }

    public final boolean param2() {
      return param2;
    }

    @Override
    public boolean equals(Object other) {
      if (other == this) return true;
      if (!(other instanceof Value1)) return false;
      Value1 o = (Value1) other;
      return o.param1 == param1
          && o.param2 == param2;
    }

    @Override
    public int hashCode() {
      int result = 0;
      result = result * 31 + Integer.valueOf(param1).hashCode();
      result = result * 31 + Boolean.valueOf(param2).hashCode();
      return result;
    }

    @Override
    public String toString() {
      StringBuilder builder = new StringBuilder();
      builder.append("Value1{param1=").append(param1);
      builder.append(", param2=").append(param2);
      return builder.append('}').toString();
    }

    @Override
    public final void match(@Nonnull Consumer<Value1> value1, @Nonnull Consumer<Value2> value2) {
      value1.accept(this);
    }

    @Override
    public final <R_> R_ map(@Nonnull Function<Value1, R_> value1,
        @Nonnull Function<Value2, R_> value2) {
      return value1.apply(this);
    }
  }

  public static final class Value2 extends MultipleValues {
    private final int param1;

    private final boolean param2;

    private Value2(int param1, boolean param2) {
      this.param1 = param1;
      this.param2 = param2;
    }

    public final int param1() {
      return param1;
    }

    public final boolean param2() {
      return param2;
    }

    @Override
    public boolean equals(Object other) {
      if (other == this) return true;
      if (!(other instanceof Value2)) return false;
      Value2 o = (Value2) other;
      return o.param1 == param1
          && o.param2 == param2;
    }

    @Override
    public int hashCode() {
      int result = 0;
      result = result * 31 + Integer.valueOf(param1).hashCode();
      result = result * 31 + Boolean.valueOf(param2).hashCode();
      return result;
    }

    @Override
    public String toString() {
      StringBuilder builder = new StringBuilder();
      builder.append("Value2{param1=").append(param1);
      builder.append(", param2=").append(param2);
      return builder.append('}').toString();
    }

    @Override
    public final void match(@Nonnull Consumer<Value1> value1, @Nonnull Consumer<Value2> value2) {
      value2.accept(this);
    }

    @Override
    public final <R_> R_ map(@Nonnull Function<Value1, R_> value1,
        @Nonnull Function<Value2, R_> value2) {
      return value2.apply(this);
    }
  }
}
pettermahlen commented 6 years ago

Thanks for the report! Could you quantify/clarify 'big saving'? It seems like the penalty comes in the form of a) an extra method for each dataenum_case, and b) a layer of indirection (which should be inlined by JIT compilation if hot, I would think). On the other hand, making access package-private leads to opening up a slightly less readable API/two ways of constructing case instances. I find it hard to see an example of when the penalties for the synthetic methods would be large, but I'd be happy to be corrected.

pettermahlen commented 6 years ago

I've done some digging into the generated classes, and I can confirm that there is an extra synthetic constructor generated for each case. Here's some hacky code, added as a test case to the OuterIntegrationTest class:

  @Test
  public void printMethodsInGeneratedCode() {
    printMethods(Testing.class);
    printMethods(One.class);
    printMethods(Two.class);
  }

  private void printMethods(Class<?> oneClass) {
    System.out.println();
    System.out.println("Methods in class: " + oneClass.getCanonicalName());
    System.out.println("declared methods:");
    for (Method m : oneClass.getDeclaredMethods()) {
      System.out.println(
          Modifier.toString(m.getModifiers())
              + ((m.getModifiers() & 0x00001000) != 0 ? "synthetic" : "")
              + " "
              + m.getName()
              + "("
              + Arrays.toString(m.getParameters())
              + ")");
    }

    System.out.println("===================");
    System.out.println("constructors:");
    for (Constructor c : oneClass.getDeclaredConstructors()) {
      System.out.println(
          Modifier.toString(c.getModifiers())
              + ((c.getModifiers() & 0x00001000) != 0 ? "synthetic" : "")
              + " "
              + c.getName()
              + "("
              + Arrays.toString(c.getParameters())
              + ")");
    }
  }

The output looks like:

Methods in class: com.spotify.dataenum.it.Testing.One
declared methods:
public equals([java.lang.Object arg0])
public toString([])
public hashCode([])
public final match([com.spotify.dataenum.function.Consumer<com.spotify.dataenum.it.Testing$One> arg0, com.spotify.dataenum.function.Consumer<com.spotify.dataenum.it.Testing$Two> arg1, com.spotify.dataenum.function.Consumer<com.spotify.dataenum.it.Testing$Three> arg2])
public final map([com.spotify.dataenum.function.Function<com.spotify.dataenum.it.Testing$One, R_> arg0, com.spotify.dataenum.function.Function<com.spotify.dataenum.it.Testing$Two, R_> arg1, com.spotify.dataenum.function.Function<com.spotify.dataenum.it.Testing$Three, R_> arg2])
private staticsynthetic $$$reportNull$$$0([int arg0])
public final i([])
===================
constructors:
private com.spotify.dataenum.it.Testing$One([int arg0])
synthetic com.spotify.dataenum.it.Testing$One([int arg0, com.spotify.dataenum.it.Testing$1 arg1])

The second constructor goes away if the generation code is changed to use package-private rather than private constructors. I'm not sure this is a tradeoff you always want to make, given that this code lives in a user package. It could be a reason to start introducing configuration parameters to the code generation.

bangarharshit commented 6 years ago

Agreed, config is the way to go. In android synthetic accessors are an issue but they are ok for other environments. Most of the Android libraries have removed synthetic accessor. Some of the static code analyzers are able to detect those.

pettermahlen commented 6 years ago

Doing a little thinking about how you might configure this behaviour, I've come up with some options:

  1. Locally, using a field in the @DataEnum annotation
  2. Locally, using a separate annotation (along the lines of @ConstructorVisibility(PRIVATE))
  3. Globally, using the -A javac option. (javac -ACONSTRUCTOR_VISIBILITY=PACKAGE_PRIVATE ...)
  4. Globally, using an environment variable/system property (CONSTRUCTOR_VISIBILITY=PRIVATE javac ...).

I think the way you'd like to configure this is globally. There seems to be very little value in doing it locally, because the one reason to avoid synthetic constructors is to reduce the method count for Android. And the ways of configuring the annotation processor behaviour are quite obscure and hard to use (from build files), meaning that most people are unlikely to use them.

So that makes me not like the configurability option so much. I guess a large Android app using DataEnum with Mobius might have several hundred DataEnum cases in it with associated synthetic methods. Not a whole lot, but also not quite ignorable. I'm leaning towards changing this and make the case constructors package-private, without making it configurable, or perhaps, making it possible to make constructors private through a specific annotation (could even be applied to a (parent) package via package-info.java to make it nearly global).