Merge pull request #267 from google/issue/265.sorted.set.clear

Fix SortedSetProperty clear bug

Author: alicederyn

src/main/java/org/inferred/freebuilder/processor/SortedSetProperty.java

@@ -421,7 +421,14 @@ private void addClear(SourceBuilder code, Metadata metadata) {
         .addLine("public %s %s() {", metadata.getBuilder(), clearMethod(property));
     if (code.feature(GUAVA).isAvailable()) {
       code.addLine("  if (%s instanceof %s) {", property.getField(), ImmutableSortedSet.class)
-          .addLine("    %s = %s.of();", property.getField(), ImmutableSortedSet.class)
+          .addLine("    if (%s.isEmpty()) {", property.getField())
+          .addLine("       // Do nothing")
+          .addLine("    } else if (%s.comparator() != null) {", property.getField())
+          .addLine("      %1$s = new %2$s<%3$s>(%1$s.comparator()).build();",
+              property.getField(), ImmutableSortedSet.Builder.class, elementType)
+          .addLine("    } else {")
+          .addLine("      %s = %s.of();", property.getField(), ImmutableSortedSet.class)
+          .addLine("    }")
           .add("  } else ");
     }
     code.addLine("  if (%s != null) {", property.getField())

src/test/java/org/inferred/freebuilder/processor/ElementFactory.java

@@ -1,19 +1,30 @@
 package org.inferred.freebuilder.processor;
 
 import static com.google.common.base.Preconditions.checkState;
+import static java.util.stream.Collectors.toList;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Ordering;
 
+import org.inferred.freebuilder.processor.testtype.AbstractNonComparable;
+import org.inferred.freebuilder.processor.testtype.NonComparable;
+import org.inferred.freebuilder.processor.testtype.OtherNonComparable;
 import org.inferred.freebuilder.processor.util.NameImpl;
 
+import java.util.Collection;
+import java.util.Comparator;
+import java.util.List;
+import java.util.Optional;
 import java.util.stream.Collectors;
 import java.util.stream.IntStream;
 
+import javax.annotation.Nullable;
+
 public enum ElementFactory {
   STRINGS(
+      String.class,
       "String",
-      "String",
+      null,
       CharSequence.class,
       "!element.isEmpty()",
       "!element.toString().isEmpty()",
@@ -28,8 +39,9 @@ public enum ElementFactory {
       "foxtrot",
       "golf"),
   INTEGERS(
-      "Integer",
+      Integer.class,
       "int",
+      null,
       Number.class,
       "element >= 0",
       "element.intValue() >= 0",
@@ -42,49 +54,72 @@ public enum ElementFactory {
       10,
       15,
       21,
-      28);
-
-  private final String type;
+      28),
+  NON_COMPARABLES(
+      NonComparable.class,
+      NonComparable.class.getName(),
+      AbstractNonComparable.ReverseIdComparator.class,
+      AbstractNonComparable.class,
+      "element.id() >= 0",
+      "element.id() >= 0",
+      "ID must be non-negative",
+      new NonComparable(-2, "broken"),
+      new OtherNonComparable(88, "other"),
+      new NonComparable(10, "alpha"),
+      new NonComparable(9, "cappa"),
+      new NonComparable(8, "echo"),
+      new NonComparable(7, "golf"),
+      new NonComparable(6, "beta"),
+      new NonComparable(5, "delta"),
+      new NonComparable(4, "foxtrot"));
+
+  private final Class<?> type;
   private final String unwrappedType;
+  @Nullable private final Class<?> comparator;
   private Class<?> supertype;
   private final String validation;
   private final String supertypeValidation;
   private final String errorMessage;
-  private final Comparable<?> invalidExample;
+  private final Object invalidExample;
   private final Object supertypeExample;
-  private final ImmutableList<Comparable<?>> examples;
+  private final ImmutableList<Object> examples;
 
   ElementFactory(
-      String type,
+      Class<?> type,
       String unwrappedType,
+      @Nullable Class<? extends Comparator<?>> comparator,
       Class<?> supertype,
       String validation,
       String supertypeValidation,
       String errorMessage,
-      Comparable<?> invalidExample,
+      Object invalidExample,
       Object supertypeExample,
-      Comparable<?>... examples) {
+      Object... examples) {
     this.type = type;
     this.unwrappedType = unwrappedType;
+    this.comparator = comparator;
     this.supertype = supertype;
     this.validation = validation;
     this.supertypeValidation = supertypeValidation;
     this.errorMessage = errorMessage;
     this.invalidExample = invalidExample;
     this.supertypeExample = supertypeExample;
     this.examples = ImmutableList.copyOf(examples);
-    checkState(Ordering.natural().isOrdered(this.examples),
-        "Examples must be in natural order (got %s)", this.examples);
+    checkExamplesOrdered(comparator, this.examples);
   }
 
-  public String type() {
+  public Class<?> type() {
     return type;
   }
 
   public String unwrappedType() {
     return unwrappedType;
   }
 
+  public Optional<Class<?>> comparator() {
+    return Optional.ofNullable(comparator);
+  }
+
   public Class<?> supertype() {
     return supertype;
   }
@@ -121,18 +156,47 @@ public String examples(int... ids) {
     return IntStream.of(ids).mapToObj(this::example).collect(Collectors.joining(", "));
   }
 
+  @Override
+  public String toString() {
+    return type.getSimpleName();
+  }
+
   private static String toSource(Object example) {
     if (example instanceof String) {
       return "\"" + example + "\"";
     } else if (example instanceof NameImpl) {
       return "new " + NameImpl.class.getName() + "(\"" + example + "\")";
+    } else if (example instanceof NonComparable) {
+      NonComparable nonComparable = (NonComparable) example;
+      return "new " + NonComparable.class.getName() + "(" + nonComparable.id() + ", \""
+          + nonComparable.name() + "\")";
+    } else if (example instanceof OtherNonComparable) {
+      OtherNonComparable otherNonComparable = (OtherNonComparable) example;
+      return "new " + OtherNonComparable.class.getName() + "(" + otherNonComparable.id() + ", \""
+          + otherNonComparable.name() + "\")";
     } else {
       return example.toString();
     }
   }
 
-  @Override
-  public String toString() {
-    return type;
+  private static void checkExamplesOrdered(
+      @Nullable Class<? extends Comparator<?>> comparator,
+      Collection<?> examples) {
+    if (comparator == null) {
+      List<Comparable<?>> comparables =
+          examples.stream().map(v -> (Comparable<?>) v).collect(toList());
+      checkState(Ordering.natural().isOrdered(comparables),
+          "Examples must be in natural order (got %s)", examples);
+    } else {
+      try {
+        @SuppressWarnings("rawtypes")
+        Ordering ordering = Ordering.from(comparator.newInstance());
+        @SuppressWarnings("unchecked")
+        boolean isOrdered = ordering.isOrdered(examples);
+        checkState(isOrdered);
+      } catch (ReflectiveOperationException e) {
+        throw new RuntimeException(e);
+      }
+    }
   }
 }

src/test/java/org/inferred/freebuilder/processor/ListMutateMethodTest.java

@@ -28,6 +28,7 @@
 import org.inferred.freebuilder.processor.util.testing.ParameterizedBehaviorTestFactory.Shared;
 import org.inferred.freebuilder.processor.util.testing.SourceBuilder;
 import org.inferred.freebuilder.processor.util.testing.TestBuilder;
+import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
@@ -114,7 +115,7 @@ public ListMutateMethodTest(
             elements.supertype());
     if (checked) {
       genericTypeBuilder
-          .addLine("    @Override public Builder addItems(T element) {")
+          .addLine("    @Override public Builder<T> addItems(T element) {")
           .addLine("      if (!(%s)) {", elements.supertypeValidation())
           .addLine("        throw new IllegalArgumentException(\"%s\");", elements.errorMessage())
           .addLine("      }")
@@ -145,6 +146,11 @@ public ListMutateMethodTest(
     }
   }
 
+  @Before
+  public void setUp() {
+    behaviorTester.withPermittedPackage(elements.type().getPackage());
+  }
+
   @Test
   public void mutateAndAddModifiesUnderlyingProperty() {
     behaviorTester

src/test/java/org/inferred/freebuilder/processor/ListPropertyTest.java

@@ -35,6 +35,7 @@
 import org.inferred.freebuilder.processor.util.testing.ParameterizedBehaviorTestFactory.Shared;
 import org.inferred.freebuilder.processor.util.testing.SourceBuilder;
 import org.inferred.freebuilder.processor.util.testing.TestBuilder;
+import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
@@ -117,6 +118,11 @@ public ListPropertyTest(
         .build();
   }
 
+  @Before
+  public void setUp() {
+    behaviorTester.withPermittedPackage(elements.type().getPackage());
+  }
+
   @Test
   public void testDefaultEmpty() {
     behaviorTester