Merge pull request #14 from pettermahlen/synthetic-constructors

Avoid synthetic constructors by default

Author: pettermahlen

README.md

@@ -252,6 +252,18 @@ if (message.isLogin()) {
   `Second_dataenum`, it must of course be public.
 
 
+## Configuration
+
+DataEnum currently has a single configurable setting determining the visibility of constructors in
+generated code. Generally speaking, `private` is best as it ensures there is a single way of creating
+case instances (the generated static factory methods like `MyMessages.login(String, String)` above).
+However, for Android development, you want to keep the method count down to a minimum, and private
+constructors lead to synthetic constructors being generated, increasing the method count. Since that
+is an important use case for us, we've chosen the package-private as the default. This is configurable
+through adding a
+[`@ConstructorAccess`](https://javadoc.io/page/com.spotify.dataenum/dataenum/latest/com/spotify/dataenum/ConstructorAccess.html)
+annotation to a `package-info.java` file. See the javadocs for more information.
+
 ## Known weaknesses of DataEnum
 
 - While the generated classes are immutable, they do not enforce that parameters are immutable. It is up to users of

dataenum-processor/src/main/java/com/spotify/dataenum/processor/AccessSelector.java

@@ -0,0 +1,121 @@
+/*
+ * -\-\-
+ * DataEnum
+ * --
+ * Copyright (c) 2017 Spotify AB
+ * --
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ * -/-/-
+ */
+package com.spotify.dataenum.processor;
+
+import com.spotify.dataenum.Access;
+import com.spotify.dataenum.ConstructorAccess;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
+import javax.lang.model.element.Element;
+import javax.lang.model.element.Modifier;
+import javax.lang.model.element.PackageElement;
+
+/**
+ * Determines the most appropriate access level based on package-level annotations. Uses a
+ * simplistic algorithm that should perform well given a small number of annotated packages, which
+ * seems like a reasonable assumption to make.
+ */
+class AccessSelector {
+
+  // sorted so that the longest names are first; this means the first hit is going to be the best
+  // one
+  private final List<PackageAndAccess> packages;
+
+  AccessSelector(Set<? extends Element> visibilityAnnotatedPackages) {
+    packages = parseAnnotatedPackages(visibilityAnnotatedPackages);
+  }
+
+  private List<PackageAndAccess> parseAnnotatedPackages(
+      Set<? extends Element> visibilityAnnotatedPackages) {
+    ArrayList<PackageAndAccess> result = new ArrayList<>(visibilityAnnotatedPackages.size());
+
+    for (Element element : visibilityAnnotatedPackages) {
+      if (!(element instanceof PackageElement)) {
+        throw new IllegalArgumentException(
+            "received a access annotated element that is not a package: " + element);
+      }
+
+      PackageElement packageElement = (PackageElement) element;
+
+      result.add(
+          new PackageAndAccess(
+              packageElement.getQualifiedName().toString(),
+              element.getAnnotation(ConstructorAccess.class).value()));
+    }
+
+    result.sort((o1, o2) -> o2.packageName.length() - o1.packageName.length());
+
+    return result;
+  }
+
+  public Optional<Modifier> accessModifierFor(String packageName) {
+    switch (accessFor(packageName)) {
+      case PACKAGE_PRIVATE:
+        return Optional.empty();
+      case PRIVATE:
+        return Optional.of(Modifier.PRIVATE);
+      case PROTECTED:
+        return Optional.of(Modifier.PROTECTED);
+      case PUBLIC:
+        return Optional.of(Modifier.PUBLIC);
+    }
+
+    throw new AssertionError("cannot get here");
+  }
+
+  private Access accessFor(String packageName) {
+    for (PackageAndAccess packageAndAccess : packages) {
+      if (isParentOf(packageAndAccess.packageName, packageName)) {
+        return packageAndAccess.access;
+      }
+    }
+
+    return Access.PACKAGE_PRIVATE;
+  }
+
+  private boolean isParentOf(String maybeParentPackage, String packageName) {
+    // check the easy case first
+    if (!packageName.startsWith(maybeParentPackage)) {
+      return false;
+    }
+
+    // even if they start with the same string, there might be a mismatch in the last package name
+    // in the possible parent. For instance, "com.spot" is not a parent package of "com.spotify".
+    String[] parentParts = maybeParentPackage.split("\\.");
+    String[] packageParts = packageName.split("\\.");
+
+    String lastParentPackagePart = parentParts[parentParts.length - 1];
+
+    return packageParts[parentParts.length - 1].equals(lastParentPackagePart);
+  }
+
+  private static class PackageAndAccess {
+
+    private final String packageName;
+    private final Access access;
+
+    private PackageAndAccess(String packageName, Access access) {
+      this.packageName = packageName;
+      this.access = access;
+    }
+  }
+}

dataenum-processor/src/main/java/com/spotify/dataenum/processor/DataEnumProcessor.java

@@ -19,6 +19,7 @@
  */
 package com.spotify.dataenum.processor;
 
+import com.spotify.dataenum.ConstructorAccess;
 import com.spotify.dataenum.DataEnum;
 import com.spotify.dataenum.DataenumUtils;
 import com.spotify.dataenum.processor.data.OutputSpec;
@@ -50,6 +51,9 @@ public boolean process(Set<? extends TypeElement> set, RoundEnvironment roundEnv
     Filer filer = processingEnv.getFiler();
     Messager messager = processingEnv.getMessager();
 
+    AccessSelector accessSelector =
+        new AccessSelector(roundEnvironment.getElementsAnnotatedWith(ConstructorAccess.class));
+
     for (Element element : roundEnvironment.getElementsAnnotatedWith(DataEnum.class)) {
       try {
 
@@ -59,7 +63,10 @@ public boolean process(Set<? extends TypeElement> set, RoundEnvironment roundEnv
         }
 
         OutputSpec outputSpec = OutputSpecFactory.create(spec);
-        TypeSpec outputTypeSpec = SpecTypeFactory.create(outputSpec);
+        TypeSpec outputTypeSpec =
+            SpecTypeFactory.create(
+                outputSpec,
+                accessSelector.accessModifierFor(outputSpec.outputClass().packageName()));
 
         JavaFile.Builder javaFileBuilder =
             JavaFile.builder(outputSpec.outputClass().packageName(), outputTypeSpec);

dataenum-processor/src/main/java/com/spotify/dataenum/processor/generator/spec/SpecTypeFactory.java

@@ -19,6 +19,8 @@
  */
 package com.spotify.dataenum.processor.generator.spec;
 
+import static com.spotify.dataenum.processor.util.Iterables.fromOptional;
+
 import com.spotify.dataenum.processor.DataEnumProcessor;
 import com.spotify.dataenum.processor.data.OutputSpec;
 import com.spotify.dataenum.processor.data.OutputValue;
@@ -34,14 +36,16 @@
 import com.squareup.javapoet.TypeSpec;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Optional;
 import javax.annotation.Generated;
 import javax.lang.model.element.Modifier;
 
 public final class SpecTypeFactory {
 
   private SpecTypeFactory() {}
 
-  public static TypeSpec create(OutputSpec spec) throws ParserException {
+  public static TypeSpec create(OutputSpec spec, Optional<Modifier> constructorAccessModifier)
+      throws ParserException {
     List<TypeSpec> valueTypes = new ArrayList<>();
     List<MethodSpec> factoryMethods = new ArrayList<>();
     List<MethodSpec> isMethods = new ArrayList<>();
@@ -51,7 +55,9 @@ public static TypeSpec create(OutputSpec spec) throws ParserException {
     MapMethods mapMethods = new MapMethods(spec.outputValues());
 
     for (OutputValue value : spec.outputValues()) {
-      valueTypes.add(ValueTypeFactory.create(value, spec, matchMethods, mapMethods));
+      valueTypes.add(
+          ValueTypeFactory.create(
+              value, spec, matchMethods, mapMethods, constructorAccessModifier));
 
       ValueMethods valueMethods = new ValueMethods(value);
       factoryMethods.add(valueMethods.createFactoryMethod(spec));
@@ -69,8 +75,11 @@ public static TypeSpec create(OutputSpec spec) throws ParserException {
             .addModifiers(Modifier.PUBLIC, Modifier.ABSTRACT)
             .addTypeVariables(spec.typeVariables());
 
-    // Hide the constructor
-    enumBuilder.addMethod(MethodSpec.constructorBuilder().addModifiers(Modifier.PRIVATE).build());
+    // add constructor with correct access
+    enumBuilder.addMethod(
+        MethodSpec.constructorBuilder()
+            .addModifiers(fromOptional(constructorAccessModifier))
+            .build());
 
     enumBuilder.addTypes(valueTypes);
     enumBuilder.addMethods(factoryMethods);

dataenum-processor/src/main/java/com/spotify/dataenum/processor/generator/value/ValueTypeFactory.java

@@ -19,6 +19,8 @@
  */
 package com.spotify.dataenum.processor.generator.value;
 
+import static com.spotify.dataenum.processor.util.Iterables.fromOptional;
+
 import com.spotify.dataenum.processor.data.OutputSpec;
 import com.spotify.dataenum.processor.data.OutputValue;
 import com.spotify.dataenum.processor.data.Parameter;
@@ -38,6 +40,7 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
+import java.util.Optional;
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 import javax.lang.model.element.Modifier;
@@ -50,7 +53,11 @@ public class ValueTypeFactory {
   private ValueTypeFactory() {}
 
   public static TypeSpec create(
-      OutputValue value, OutputSpec spec, MatchMethods matchMethods, MapMethods mapMethods)
+      OutputValue value,
+      OutputSpec spec,
+      MatchMethods matchMethods,
+      MapMethods mapMethods,
+      Optional<Modifier> constructorAccessModifier)
       throws ParserException {
 
     TypeSpec.Builder typeBuilder =
@@ -59,7 +66,7 @@ public static TypeSpec create(
             .superclass(getSuperclassForValue(value, spec))
             .addModifiers(Modifier.PUBLIC, Modifier.STATIC, Modifier.FINAL);
 
-    typeBuilder.addMethod(createConstructor(value));
+    typeBuilder.addMethod(createConstructor(value, constructorAccessModifier));
     typeBuilder.addFields(createFields(value));
     typeBuilder.addMethods(createGetters(value));
     typeBuilder.addMethod(createEquals(value));
@@ -102,8 +109,10 @@ private static TypeName getSuperclassForValue(OutputValue value, OutputSpec spec
         spec.outputClass(), superParameters.toArray(new TypeName[] {}));
   }
 
-  private static MethodSpec createConstructor(OutputValue value) {
-    MethodSpec.Builder constructor = MethodSpec.constructorBuilder().addModifiers(Modifier.PRIVATE);
+  private static MethodSpec createConstructor(
+      OutputValue value, Optional<Modifier> constructorAccessModifier) {
+    MethodSpec.Builder constructor =
+        MethodSpec.constructorBuilder().addModifiers(fromOptional(constructorAccessModifier));
     for (Parameter parameter : value.parameters()) {
       constructor.addParameter(parameter.type(), parameter.name());
 

dataenum-processor/src/main/java/com/spotify/dataenum/processor/parser/ValuesParser.java

@@ -48,7 +48,9 @@ static List<Value> parse(TypeElement enumElement, ProcessingEnvironment processi
             .getMessager()
             .printMessage(
                 Kind.ERROR,
-                "Duplicate case name '" + value.name().toLowerCase() + "' - lower-case case names must be unique.",
+                "Duplicate case name '"
+                    + value.name().toLowerCase()
+                    + "' - lower-case case names must be unique.",
                 valueElement);
       }
 

dataenum-processor/src/main/java/com/spotify/dataenum/processor/util/Iterables.java

@@ -21,7 +21,9 @@
 
 import java.lang.reflect.Array;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
+import java.util.Optional;
 
 public class Iterables {
   public static <T> boolean contains(Iterable<T> iterable, T value) {
@@ -53,4 +55,8 @@ public static <T> int sizeOf(Iterable<T> iterable) {
 
     return size;
   }
+
+  public static <T> Iterable<T> fromOptional(Optional<T> input) {
+    return input.map(Collections::singletonList).orElse(Collections.emptyList());
+  }
 }