Merge pull request square#555 from square/jw/no-label-map

Forbid label in front of maps.
Faire · Jan 27, 2016 · 9b1f6db · 9b1f6db
2 parents 10f0fdd + 0e3d915
commit 9b1f6db
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 5 deletions.
diff --git a/wire-schema/src/main/java/com/squareup/wire/schema/internal/parser/ProtoParser.java b/wire-schema/src/main/java/com/squareup/wire/schema/internal/parser/ProtoParser.java
@@ -355,14 +355,17 @@ private Object readField(String documentation, Location location, String word) {
         break;
 
       default:
-        if (syntax != ProtoFile.Syntax.PROTO_3) {
+        if (syntax != ProtoFile.Syntax.PROTO_3 && (!word.equals("map") || peekChar() != '<')) {
           throw unexpected(location, "unexpected label: " + word);
         }
         label = null;
         type = readDataType(word);
         break;
     }
 
+    if (type.startsWith("map<") && label != null) {
+      throw unexpected(location, "'map' type cannot have label");
+    }
     if (type.equals("group")) {
       return readGroup(documentation, label);
     }

diff --git a/wire-schema/src/test/java/com/squareup/wire/schema/internal/parser/ProtoParserTest.java b/wire-schema/src/test/java/com/squareup/wire/schema/internal/parser/ProtoParserTest.java
@@ -56,8 +56,8 @@ public final class ProtoParserTest {
         + "  required string f14 = 14;\n"
         + "  required uint32 f15 = 15;\n"
         + "  required uint64 f16 = 16;\n"
-        + "  required map<string, bool> f17 = 17;\n"
-        + "  required map<arbitrary, nested.nested> f18 = 18;\n"
+        + "  map<string, bool> f17 = 17;\n"
+        + "  map<arbitrary, nested.nested> f18 = 18;\n"
         + "  required arbitrary f19 = 19;\n"
         + "  required nested.nested f20 = 20;\n"
         + "}\n";
@@ -163,13 +163,11 @@ public final class ProtoParserTest {
                         .tag(16)
                         .build(),
                     FieldElement.builder(location.at(18, 3))
-                        .label(REQUIRED)
                         .type("map<string, bool>")
                         .name("f17")
                         .tag(17)
                         .build(),
                     FieldElement.builder(location.at(19, 3))
-                        .label(REQUIRED)
                         .type("map<arbitrary, nested.nested>")
                         .name("f18")
                         .tag(18)
@@ -191,6 +189,30 @@ public final class ProtoParserTest {
     assertThat(ProtoParser.parse(location, proto)).isEqualTo(expected);
   }
 
+  @Test public void mapWithLabelThrows() {
+    try {
+      ProtoParser.parse(location, "message Hey { required map<string, string> a = 1; }");
+      fail();
+    } catch (IllegalStateException e) {
+      assertThat(e).hasMessage(
+          "Syntax error in file.proto at 1:15: 'map' type cannot have label");
+    }
+    try {
+      ProtoParser.parse(location, "message Hey { optional map<string, string> a = 1; }");
+      fail();
+    } catch (IllegalStateException e) {
+      assertThat(e).hasMessage(
+          "Syntax error in file.proto at 1:15: 'map' type cannot have label");
+    }
+    try {
+      ProtoParser.parse(location, "message Hey { repeated map<string, string> a = 1; }");
+      fail();
+    } catch (IllegalStateException e) {
+      assertThat(e).hasMessage(
+          "Syntax error in file.proto at 1:15: 'map' type cannot have label");
+    }
+  }
+
   /** It looks like an option, but 'default' is special. It's missing from descriptor.proto! */
   @Test public void defaultFieldOptionIsSpecial() {
     String proto = ""