Stop generating the repeated field view of maps

PiperOrigin-RevId: 342350461
google · Nov 13, 2020 · 4416f25 · 4416f25
1 parent 848aa2d
commit 4416f25
Show file tree

Hide file tree

Showing 11 changed files with 71 additions and 9,875 deletions.
diff --git a/java/com/google/protobuf/contrib/immutablejs/generator/TemplateFieldDescriptor.java b/java/com/google/protobuf/contrib/immutablejs/generator/TemplateFieldDescriptor.java
@@ -60,8 +60,7 @@ public String getFieldNumberName() {
   }
 
   public boolean isRepeated() {
-    // TODO(b/171708241): consider repeated and maps distinct types.
-    return protoFieldDescriptor().isRepeated();
+    return protoFieldDescriptor().isRepeated() && !isMap();
   }
 
   public boolean isMap() {
@@ -179,6 +178,16 @@ private static String calculateDefaultLongValue(long defaultValue) {
   }
 
   public String getJsDoc() {
+    // Map fields are typed dramatically different as they cannot be repeated or participate in
+    // extensions. Handle them specially first.
+    if (isMap()) {
+      return String.format(
+          "!%s<!%s, !%s>",
+          TypeDescriptor.MAP_VIEW.createReference(enclosingType()).getExpression(),
+          getMapKey().getType().getExpression(),
+          getMapValue().getType().getExpression());
+    }
+
     StringBuilder builder = new StringBuilder();
     if (protoFieldDescriptor().isExtension()) {
       builder.append("!").append(createExtensionReference().getExpression());
@@ -200,17 +209,6 @@ public String getJsDoc() {
     return builder.toString();
   }
 
-  public String getMapJsDoc() {
-    // TODO(b/171708241): Collapse this into getJsDoc when we don't need to support both the map
-    // view and repeated field view.
-    checkState(isMap());
-    return String.format(
-        "!%s<!%s, !%s>",
-        TypeDescriptor.MAP_VIEW.createReference(enclosingType()).getExpression(),
-        getMapKey().getType().getExpression(),
-        getMapValue().getType().getExpression());
-  }
-
   public String getElementJsDoc() {
     checkState(isRepeated());
     return "!" + getType().getExpression();

diff --git a/java/com/google/protobuf/contrib/immutablejs/generator/TemplateMessageDescriptor.java b/java/com/google/protobuf/contrib/immutablejs/generator/TemplateMessageDescriptor.java
@@ -14,6 +14,7 @@
 package com.google.protobuf.contrib.immutablejs.generator;
 
 import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.base.Predicates.not;
 
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableList;
@@ -46,7 +47,7 @@ public ImmutableList<TemplateMessageDescriptor> getAllMessages() {
     checkState(getType().isTopLevel());
     return getAllMessagesDescriptors(descriptor())
         .map(TemplateMessageDescriptor::create)
-        // TODO(b/171708241): Filter out MapEntry submessages.
+        .filter(not(TemplateMessageDescriptor::isMapEntry))
         .collect(ImmutableList.toImmutableList());
   }
 
@@ -113,4 +114,8 @@ public String getProtoFileComments() {
   public boolean isDeprecated() {
     return descriptor().getOptions().getDeprecated();
   }
+
+  private boolean isMapEntry() {
+    return descriptor().getOptions().getMapEntry();
+  }
 }
diff --git a/java/com/google/protobuf/contrib/immutablejs/generator/templates/getter_map.vm b/java/com/google/protobuf/contrib/immutablejs/generator/templates/getter_map.vm
@@ -1,7 +1,4 @@
-## TODO(b/171708241): Stop generating the repeated field view of map fields.
-#parse('getter_repeated.vm')
-
-#set ($mapType = $field.mapJsDoc)
+#set ($mapType = $field.jsDoc)
 #set ($key = $field.mapKey)
 #set ($value = $field.mapValue)
 
@@ -18,18 +15,16 @@ $!bodyContent
 #end
 #end
 
-## TODO(b/171708241): Generate the Map count method once we stop generating the
-## repeated field version.
-##  /**
-##   * $!field.protoFileComments
-##   * @return {number}
-###if ($field.isDeprecated())
-##   * @deprecated
-###end
-##   */
-##  get${field.name}Count() {
-##    return this.internal_getMap${key.stem}Key${value.stem}ValueEntriesCount(${field.number}${lastParameter});
-##  }
+ /**
+  * $!field.protoFileComments
+  * @return {number}
+#if ($field.isDeprecated())
+  * @deprecated
+#end
+  */
+ get${field.name}Count() {
+   return this.internal_getMap${key.stem}Key${value.stem}ValueEntriesCount(${field.number}${lastParameter});
+ }
 
   /**
    * $!field.protoFileComments

diff --git a/java/com/google/protobuf/contrib/immutablejs/generator/templates/message.vm b/java/com/google/protobuf/contrib/immutablejs/generator/templates/message.vm
@@ -125,7 +125,7 @@ ${messageName}.Builder = class extends proto_im_InternalMessage.Builder {
 
 #foreach($field in $message.fields)
 #if ($field.isMap())
-#set ($mapViewType = $field.mapJsDoc)
+#set ($mapViewType = $field.jsDoc)
 #set ($key = $field.mapKey)
 #set ($value = $field.mapValue)
 
@@ -143,19 +143,16 @@ ${messageName}.Builder = class extends proto_im_InternalMessage.Builder {
 #set ($putAllType = "!Map<${key.jsDoc}, ${value.jsDoc}>|${mapViewType}")
 #set ($internalPutAllType = "!Map<${key.jsDoc},${internalMapValueType}>|!proto_im_MapView<${key.jsDoc},${internalMapValueType}>")
 
-
-## TODO(b/171708241): Generate the Map count method once we stop generating the
-## repeated field version.
-##  /**
-##   * $!field.protoFileComments
-##   * @return {number}
-###if ($field.isDeprecated())
-##   * @deprecated
-###end
-##   */
-##  get${field.name}Count() {
-##    return this.message.get${field.name}Count();
-##  }
+ /**
+  * $!field.protoFileComments
+  * @return {number}
+#if ($field.isDeprecated())
+  * @deprecated
+#end
+  */
+ get${field.name}Count() {
+   return this.message.get${field.name}Count();
+ }
 
   /**
    * $!field.protoFileComments
@@ -248,9 +245,7 @@ ${messageName}.Builder = class extends proto_im_InternalMessage.Builder {
     return this;
   }
 
-## TODO(b/171708241): This should be an else-if.
-#end
-#if ($field.isRepeated())
+#elseif ($field.isRepeated())
 #set ($listType = $field.jsDoc)
 #set ($elementType = $field.elementJsDoc)
 #set ($elementSetType = "#if($field.isMessage())$elementType|${elementType}.Builder#{else}$elementType#end")

diff --git a/java/com/google/protobuf/contrib/j2cl/generator/TemplateFieldDescriptor.java b/java/com/google/protobuf/contrib/j2cl/generator/TemplateFieldDescriptor.java
@@ -79,8 +79,7 @@ private static TemplateFieldDescriptor calculateType(FieldDescriptor fieldDescri
   public abstract String getUnboxedType();
 
   public boolean isRepeated() {
-    // TODO(b/171708241): consider repeated and maps distinct types.
-    return fieldDescriptor().isRepeated();
+    return fieldDescriptor().isRepeated() && !isMap();
   }
 
   public boolean isMap() {

diff --git a/java/com/google/protobuf/contrib/j2cl/generator/TemplateFileDescriptor.java b/java/com/google/protobuf/contrib/j2cl/generator/TemplateFileDescriptor.java
@@ -13,6 +13,8 @@
  */
 package com.google.protobuf.contrib.j2cl.generator;
 
+import static com.google.common.base.Predicates.not;
+
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableList;
 import com.google.common.io.Files;
@@ -53,7 +55,7 @@ public String getJsName() {
   public List<TemplateMessageDescriptor> getMessages() {
     return fileDescriptor().getMessageTypes().stream()
         .map(TemplateMessageDescriptor::create)
-        // TODO(b/171708241): Filter out MapEntry submessages.
+        .filter(not(TemplateMessageDescriptor::isMapEntry))
         .collect(ImmutableList.toImmutableList());
   }
 

diff --git a/java/com/google/protobuf/contrib/j2cl/generator/TemplateMessageDescriptor.java b/java/com/google/protobuf/contrib/j2cl/generator/TemplateMessageDescriptor.java
@@ -13,6 +13,8 @@
  */
 package com.google.protobuf.contrib.j2cl.generator;
 
+import static com.google.common.base.Predicates.not;
+
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableList;
 import com.google.protobuf.Descriptors.Descriptor;
@@ -33,6 +35,10 @@ Descriptor getContainingType() {
     return descriptor().getContainingType();
   }
 
+  boolean isMapEntry() {
+    return descriptor().getOptions().getMapEntry();
+  }
+
   public boolean isTopLevelMessage() {
     return descriptor().getContainingType() == null;
   }
@@ -62,7 +68,7 @@ public List<TemplateEnumDescriptor> getEnums() {
   public List<TemplateMessageDescriptor> getMessages() {
     return descriptor().getNestedTypes().stream()
         .map(TemplateMessageDescriptor::create)
-        // TODO(b/171708241): Filter out MapEntry submessages.
+        .filter(not(TemplateMessageDescriptor::isMapEntry))
         .collect(ImmutableList.toImmutableList());
   }
 

diff --git a/java/com/google/protobuf/contrib/j2cl/generator/templates/builder_map.vm b/java/com/google/protobuf/contrib/j2cl/generator/templates/builder_map.vm
@@ -1,6 +1,3 @@
-## TODO(b/171708241): Stop generating the repeated field view of map fields.
-#parse ('builder_repeated.vm')
-
 #set ($keyField = $field.keyField);
 #set ($valueField = $field.valueField);
 

diff --git a/java/com/google/protobuf/contrib/j2cl/generator/templates/getter_map.vm b/java/com/google/protobuf/contrib/j2cl/generator/templates/getter_map.vm
@@ -1,17 +1,12 @@
-## TODO(b/171708241): Stop generating the repeated field view of map fields.
-#parse ('getter_repeated.vm')
-
 #set ($keyField = $field.keyField)
 #set ($valueField = $field.valueField)
 #set ($valueFieldJsType = ${valueField.unboxedType})
 #if ($valueField.isEnum())
 #set ($valueFieldJsType = "${valueFieldJsType}.Internal_ClosureEnum")
 #end
 
-## TODO(b/171708241): Generate the Map count method once we stop generating the
-## repeated field version.
-##@jsinterop.annotations.JsMethod(name = "get${field.jsName}Count")
-##public native int get${field.name}Count();
+@jsinterop.annotations.JsMethod(name = "get${field.jsName}Count")
+public native int get${field.name}Count();
 
 @jsinterop.annotations.JsMethod(name = "contains${field.jsName}")
 public native boolean contains${field.name}(${keyField.unboxedType} key);

diff --git a/javatests/com/google/protobuf/contrib/immutablejs/integration/maps_test.js b/javatests/com/google/protobuf/contrib/immutablejs/integration/maps_test.js
@@ -23,8 +23,7 @@ class MapsTest {
     const serialized = `[null,null,null,null,null,[[1,"foo"],[1,"bar"]]]`;
     const immutable = ImmutableProto.parse(serialized);
 
-    // TODO(b/171708241): The count should be 1 when using the map count method.
-    assertEquals(2, immutable.getInt32KeyStringValueCount());
+    assertEquals(1, immutable.getInt32KeyStringValueCount());
     assertEquals('bar', immutable.getInt32KeyStringValueOrThrow(1));
     assertEquals('bar', immutable.getInt32KeyStringValueMap().get(1));
   }