Merge pull request #18 from pettermahlen/generated-code-fb

Ensure generated code doesn't cause FindBugs/SpotBugs violations

Author: pettermahlen

dataenum-integration-test/pom.xml

@@ -1,5 +1,7 @@
 <?xml version="1.0" encoding="UTF-8"?>
-<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+  xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+  xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
   <parent>
     <artifactId>dataenum-parent</artifactId>
     <groupId>com.spotify.dataenum</groupId>
@@ -45,6 +47,25 @@
         <groupId>com.github.siom79.japicmp</groupId>
         <artifactId>japicmp-maven-plugin</artifactId>
       </plugin>
+      <plugin>
+        <groupId>com.github.spotbugs</groupId>
+        <artifactId>spotbugs-maven-plugin</artifactId>
+        <executions>
+          <execution>
+            <phase>verify</phase>
+            <goals><goal>check</goal></goals>
+          </execution>
+        </executions>
+        <configuration>
+          <plugins>
+            <plugin>
+              <groupId>com.mebigfatguy.fb-contrib</groupId>
+              <artifactId>fb-contrib</artifactId>
+              <version>7.4.0</version>
+            </plugin>
+          </plugins>
+        </configuration>
+      </plugin>
     </plugins>
   </build>
 

dataenum-processor/src/main/java/com/spotify/dataenum/processor/data/Parameter.java

@@ -26,12 +26,14 @@ public class Parameter {
   private final TypeName type;
   private final boolean nullable;
   private final boolean redacted;
+  private final boolean isEnum;
 
-  public Parameter(String name, TypeName type, boolean nullable, boolean redacted) {
+  public Parameter(String name, TypeName type, boolean nullable, boolean redacted, boolean isEnum) {
     this.name = name;
     this.type = type;
     this.nullable = nullable;
     this.redacted = redacted;
+    this.isEnum = isEnum;
   }
 
   public String name() {
@@ -49,4 +51,8 @@ public boolean canBeNull() {
   public boolean redacted() {
     return redacted;
   }
+
+  public boolean isEnum() {
+    return isEnum;
+  }
 }

dataenum-processor/src/main/java/com/spotify/dataenum/processor/generator/data/OutputSpecFactory.java

@@ -25,7 +25,6 @@
 import com.spotify.dataenum.processor.data.Value;
 import com.spotify.dataenum.processor.parser.ParserException;
 import com.squareup.javapoet.ClassName;
-import com.squareup.javapoet.TypeName;
 import java.util.ArrayList;
 import java.util.List;
 
@@ -48,8 +47,7 @@ public static OutputSpec create(Spec spec) throws ParserException {
     return new OutputSpec(spec, outputClass, values);
   }
 
-  public static ClassName toOutputClass(TypeName dataEnumType) throws ParserException {
-    ClassName dataEnumClass = (ClassName) dataEnumType;
+  static ClassName toOutputClass(ClassName dataEnumClass) throws ParserException {
 
     String packageName = dataEnumClass.packageName();
     String name = dataEnumClass.simpleName();

dataenum-processor/src/main/java/com/spotify/dataenum/processor/generator/data/OutputValueFactory.java

@@ -51,10 +51,14 @@ static OutputValue create(Value value, ClassName specOutputClass, Spec spec)
 
       if (isDataEnumParameter(rawParamType)) {
         TypeName paramOutputType =
-            withParametersFromOther(toOutputClass(rawParamType), parameter.type());
+            withParametersFromOther(toOutputClass((ClassName) rawParamType), parameter.type());
         parameters.add(
             new Parameter(
-                parameter.name(), paramOutputType, parameter.canBeNull(), parameter.redacted()));
+                parameter.name(),
+                paramOutputType,
+                parameter.canBeNull(),
+                parameter.redacted(),
+                false));
       } else {
         parameters.add(parameter);
       }
@@ -64,7 +68,8 @@ static OutputValue create(Value value, ClassName specOutputClass, Spec spec)
   }
 
   private static boolean isDataEnumParameter(TypeName rawParamType) {
-    return rawParamType.toString().endsWith(OutputSpecFactory.SUFFIX);
+    return rawParamType.toString().endsWith(OutputSpecFactory.SUFFIX)
+        && rawParamType instanceof ClassName;
   }
 
   private static Iterable<TypeVariableName> getTypeVariables(

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

@@ -182,30 +182,54 @@ private static MethodSpec createEquals(OutputValue value) throws ParserException
     TypeName wildCardTypeName = withWildCardTypeParameters(value);
     result.addStatement("$1T o = ($1T) other", wildCardTypeName);
     result.addCode("$[return ");
-    boolean first = true;
+
+    List<Parameter> usingReferenceEquality = new ArrayList<>();
+    List<Parameter> usingEquals = new ArrayList<>();
+
     for (Parameter parameter : value.parameters()) {
+      if (useReferenceEquality(parameter)) {
+        usingReferenceEquality.add(parameter);
+      } else {
+        usingEquals.add(parameter);
+      }
+    }
+
+    boolean first = true;
+    for (Parameter parameter : usingReferenceEquality) {
       if (first) {
         first = false;
       } else {
         result.addCode("\n&& ");
       }
 
       String fieldName = parameter.name();
-      if (parameter.type().isPrimitive()) {
-        result.addCode("o.$1L == $1L", fieldName);
+      result.addCode("o.$1L == $1L", fieldName);
+    }
+
+    for (Parameter parameter : usingEquals) {
+      if (first) {
+        first = false;
       } else {
-        if (parameter.canBeNull()) {
-          result.addCode("equal(o.$1L, this.$1L)", fieldName);
-        } else {
-          result.addCode("o.$1L.equals(this.$1L)", fieldName);
-        }
+        result.addCode("\n&& ");
+      }
+
+      String fieldName = parameter.name();
+      if (parameter.canBeNull()) {
+        result.addCode("equal(o.$1L, this.$1L)", fieldName);
+      } else {
+        result.addCode("o.$1L.equals(this.$1L)", fieldName);
       }
     }
+
     result.addCode(";\n$]");
 
     return result.build();
   }
 
+  private static boolean useReferenceEquality(Parameter parameter) {
+    return parameter.isEnum() || parameter.type().isPrimitive();
+  }
+
   private static TypeName withWildCardTypeParameters(OutputValue value) {
     if (!value.hasTypeVariables()) {
       return value.outputClass();
@@ -231,9 +255,19 @@ private static MethodSpec createHashCode(OutputValue value) {
     }
 
     result.addStatement("int result = 0");
+
+    int parameterCount = Iterables.sizeOf(value.parameters());
+    int parameterIndex = 0;
+
     for (Parameter parameter : value.parameters()) {
       String fieldName = parameter.name();
-      result.addCode("result = result * 31 + ");
+      parameterIndex++;
+
+      if (parameterIndex == parameterCount) {
+        result.addCode("return result * 31 + ");
+      } else {
+        result.addCode("result = result * 31 + ");
+      }
       if (parameter.type().isPrimitive()) {
         TypeName boxedType = parameter.type().box();
         result.addStatement("$T.valueOf($L).hashCode()", boxedType, fieldName);
@@ -245,7 +279,6 @@ private static MethodSpec createHashCode(OutputValue value) {
         }
       }
     }
-    result.addStatement("return result");
     return result.build();
   }
 

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

@@ -92,8 +92,12 @@ static Value parse(Element element, ProcessingEnvironment processingEnv) {
       boolean nullable = isAnnotationPresent(parameterElement, ValueParser::isNullableAnnotation);
       boolean redacted =
           isAnnotationPresent(parameterElement, ValueParser.isRedactedAnnotation(processingEnv));
+      Element parameterTypeElement =
+          processingEnv.getTypeUtils().asElement(parameterElement.asType());
+      boolean isEnum =
+          parameterTypeElement != null && parameterTypeElement.getKind() == ElementKind.ENUM;
 
-      parameters.add(new Parameter(parameterName, parameterType, nullable, redacted));
+      parameters.add(new Parameter(parameterName, parameterType, nullable, redacted, isEnum));
     }
 
     String valueSimpleName = methodElement.getSimpleName().toString();

dataenum-processor/src/test/java/com/spotify/dataenum/processor/IntegrationTest.java

@@ -92,6 +92,11 @@ public void privateConstructors() throws Exception {
     assertThatEnumGeneratedMatchingFile("hide/ctors/PrivateConstructors", "hide/package-info.java");
   }
 
+  @Test
+  public void efficientEquals() throws Exception {
+    assertThatEnumGeneratedMatchingFile("EfficientEquals", "MyEnum.java");
+  }
+
   @Test
   public void genericValuesGeneratedCodeCompiles() throws Exception {
     Compilation compilation =

dataenum-processor/src/test/java/com/spotify/dataenum/processor/generator/data/OutputSpecFactoryTest.java

@@ -25,8 +25,6 @@
 
 import com.spotify.dataenum.processor.parser.ParserException;
 import com.squareup.javapoet.ClassName;
-import com.squareup.javapoet.TypeName;
-import org.assertj.core.api.ThrowableAssert.ThrowingCallable;
 import org.junit.Test;
 
 public class OutputSpecFactoryTest {
@@ -40,26 +38,8 @@ public void shouldRemoveDataEnumInterfaceSuffix() throws Exception {
 
   @Test
   public void shouldThrowForNonDataenumClassName() throws Exception {
-    assertThatThrownBy(
-            new ThrowingCallable() {
-              @Override
-              public void call() throws Throwable {
-                OutputSpecFactory.toOutputClass(ClassName.get("com.spotify", "My"));
-              }
-            })
+    assertThatThrownBy(() -> OutputSpecFactory.toOutputClass(ClassName.get("com.spotify", "My")))
         .isInstanceOf(ParserException.class)
         .hasMessageContaining("Bad name");
   }
-
-  @Test
-  public void shouldThrowForNonClassName() throws Exception {
-    assertThatThrownBy(
-            new ThrowingCallable() {
-              @Override
-              public void call() throws Throwable {
-                OutputSpecFactory.toOutputClass(TypeName.BOOLEAN);
-              }
-            })
-        .isInstanceOf(ClassCastException.class);
-  }
 }

dataenum-processor/src/test/resources/EfficientEquals.java

@@ -0,0 +1,129 @@
+/*
+ * -\-\-
+ * 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.
+ * -/-/-
+ */
+
+import static com.spotify.dataenum.DataenumUtils.checkNotNull;
+
+import com.spotify.dataenum.function.Consumer;
+import com.spotify.dataenum.function.Function;
+import java.lang.Double;
+import java.lang.Integer;
+import java.lang.Object;
+import java.lang.Override;
+import java.lang.String;
+import java.lang.StringBuilder;
+import javax.annotation.Generated;
+import javax.annotation.Nonnull;
+
+@Generated("com.spotify.dataenum.processor.DataEnumProcessor")
+public abstract class EfficientEquals {
+  EfficientEquals() {
+  }
+
+  public static EfficientEquals value(int param1, @Nonnull String param2, @Nonnull MyEnum param3, double param4) {
+    return new Value(param1, param2, param3, param4);
+  }
+
+  public final boolean isValue() {
+    return (this instanceof Value);
+  }
+
+  public final Value asValue() {
+    return (Value) this;
+  }
+
+  public abstract void match(@Nonnull Consumer<Value> value);
+
+  public abstract <R_> R_ map(@Nonnull Function<Value, R_> value);
+
+  public static final class Value extends EfficientEquals {
+    private final int param1;
+
+    private final String param2;
+
+    private final MyEnum param3;
+
+    private final double param4;
+
+    Value(int param1, String param2, MyEnum param3, double param4) {
+      this.param1 = param1;
+      this.param2 = checkNotNull(param2);
+      this.param3 = checkNotNull(param3);
+      this.param4 = param4;
+    }
+
+    public final int param1() {
+      return param1;
+    }
+
+    @Nonnull
+    public final String param2() {
+      return param2;
+    }
+
+    @Nonnull
+    public final MyEnum param3() {
+      return param3;
+    }
+
+    public final double param4() {
+      return param4;
+    }
+
+    @Override
+    public boolean equals(Object other) {
+      if (other == this) return true;
+      if (!(other instanceof Value)) return false;
+      Value o = (Value) other;
+      return o.param1 == param1
+          && o.param3 == param3
+          && o.param4 == param4
+          && o.param2.equals(this.param2);
+    }
+
+    @Override
+    public int hashCode() {
+      int result = 0;
+      result = result * 31 + Integer.valueOf(param1).hashCode();
+      result = result * 31 + param2.hashCode();
+      result = result * 31 + param3.hashCode();
+      return result * 31 + Double.valueOf(param4).hashCode();
+    }
+
+    @Override
+    public String toString() {
+      StringBuilder builder = new StringBuilder();
+      builder.append("Value{param1=").append(param1);
+      builder.append(", param2=").append(param2);
+      builder.append(", param3=").append(param3);
+      builder.append(", param4=").append(param4);
+      return builder.append('}').toString();
+    }
+
+    @Override
+    public final void match(@Nonnull Consumer<Value> value) {
+      value.accept(this);
+    }
+
+    @Override
+    public final <R_> R_ map(@Nonnull Function<Value, R_> value) {
+      return value.apply(this);
+    }
+  }
+}