Fix parser handling of message field values and group-in-oneof legalization

experimental/parser/legalize_def.go

@@ -37,7 +37,7 @@ var validDefParents = [...]taxa.Set{
 	ast.DefKindExtend:    taxa.NewSet(taxa.TopLevel, taxa.Message, taxa.Group),
 	ast.DefKindField:     taxa.NewSet(taxa.Message, taxa.Group, taxa.Extend, taxa.Oneof),
 	ast.DefKindOneof:     taxa.NewSet(taxa.Message, taxa.Group),
-	ast.DefKindGroup:     taxa.NewSet(taxa.Message, taxa.Group, taxa.Extend),
+	ast.DefKindGroup:     taxa.NewSet(taxa.Message, taxa.Group, taxa.Extend, taxa.Oneof),
 	ast.DefKindEnumValue: taxa.NewSet(taxa.Enum),
 	ast.DefKindMethod:    taxa.NewSet(taxa.Service),
 	ast.DefKindOption: taxa.NewSet(

experimental/parser/parse_expr.go

@@ -46,6 +46,20 @@ func parseExprInfix(p *parser, c *token.Cursor, where taxa.Place, lhs ast.ExprAn
 	}
 
 	next := peekTokenExpr(p, c)
+	// If the next token is an identifier, we check two tokens ahead for validaton that this
+	// is an infix expression, either using ":" and "=" or colon-less assignments.
+	//
+	// If it is valid, then we return the left-hand side. Otherwise, we continue to parse
+	// the infix expression.
+	if next.Kind() == token.Ident {
+		clone := c.Clone()
+		clone.Next()
+		switch clone.Peek().Keyword() {
+		case keyword.Assign, keyword.Colon, keyword.Braces, keyword.Lt, keyword.Brackets:
+			return lhs
+		}
+	}
+
 	switch prec {
 	case 0:
 		if where.Subject() == taxa.Array || where.Subject() == taxa.Dict {
@@ -69,9 +83,6 @@ func parseExprInfix(p *parser, c *token.Cursor, where taxa.Place, lhs ast.ExprAn
 
 			case keyword.Braces, keyword.Lt, keyword.Brackets:
 				// This is for colon-less, array or dict-valued fields.
-				if next.IsLeaf() {
-					break
-				}
 
 				// The previous expression cannot also be a key-value pair, since
 				// this messes with parsing of dicts, which are not comma-separated.
@@ -91,16 +102,10 @@ func parseExprInfix(p *parser, c *token.Cursor, where taxa.Place, lhs ast.ExprAn
 					break
 				}
 
+				// Same rationale as the colon case above: field values are
+				// not infix expressions, so use parseExprPrefix.
 				return p.NewExprField(ast.ExprFieldArgs{
-					Key: lhs,
-					// Why not call parseExprSolo? Suppose the following
-					// (invalid) production:
-					//
-					// foo { ... } to { ... }
-					//
-					// Calling parseExprInfix will cause this to be parsed
-					// as a range expression, which will be diagnosed when
-					// we legalize.
+					Key:   lhs,
 					Value: parseExprInfix(p, c, where, ast.ExprAny{}, prec+1),
 				}).AsAny()
 			}

experimental/parser/testdata/parser/field/group.proto

@@ -31,4 +31,8 @@ message Foo {
     }
 
     required group lowercase = 4 {}
+
+    oneof O {
+        group Baz = 5 {}
+    }
 }

experimental/parser/testdata/parser/field/group.proto.stderr.txt

@@ -40,4 +40,11 @@ error: group names must start with an uppercase letter
 33 |     required group lowercase = 4 {}
    |                    ^^^^^^^^^
 
-encountered 1 error and 5 warnings
+warning: group syntax is deprecated
+  --> testdata/parser/field/group.proto:36:9
+   |
+36 |         group Baz = 5 {}
+   |         ^^^^^
+   = note: group syntax is not available in proto3 or editions
+
+encountered 1 error and 6 warnings

experimental/parser/testdata/parser/field/group.proto.yaml

@@ -64,3 +64,13 @@ decls:
           type.path.components: [{ ident: "group" }]
         value.literal.int_value: 4
         body: {}
+    - def:
+        kind: KIND_ONEOF
+        name.components: [{ ident: "O" }]
+        body.decls:
+        - def:
+            kind: KIND_GROUP
+            name.components: [{ ident: "Baz" }]
+            type.path.components: [{ ident: "group" }]
+            value.literal.int_value: 5
+            body: {}

experimental/parser/testdata/parser/option/values.proto

@@ -70,6 +70,20 @@ option x = {
     x: <a: 42>
     x: <a: <a: 42>>
 
+    x {}
+    x {a: 42}
+    x <a: 42>
+    x <a: 42, b: 43>
+
+    x1: 55 to {}
+    x2: {} to {}
+    x3 17 to {}
+    x4 {} to 91
+    x1: to
+
+    reserved: true
+    to: true
+
     "ident": 42
     "???": 42
     42: 42

experimental/parser/testdata/parser/option/values.proto.stderr.txt

@@ -193,76 +193,124 @@ warning: using `<...>` for message expression is not recommended
    = help: `<...>` message expressions are an obscure feature and not
            recommended
 
+warning: using `<...>` for message expression is not recommended
+  --> testdata/parser/option/values.proto:75:7
+   |
+75 |     x <a: 42>
+   |       ^^^^^^^
+  help: use `{...}` instead
+   |
+75 | -     x <a: 42>
+75 | +     x {a: 42}
+   |
+   = note: `<...>` are only permitted for sub-messages within a message
+           expression, but as top-level option values
+   = help: `<...>` message expressions are an obscure feature and not
+           recommended
+
+warning: using `<...>` for message expression is not recommended
+  --> testdata/parser/option/values.proto:76:7
+   |
+76 |     x <a: 42, b: 43>
+   |       ^^^^^^^^^^^^^^
+  help: use `{...}` instead
+   |
+76 | -     x <a: 42, b: 43>
+76 | +     x {a: 42, b: 43}
+   |
+   = note: `<...>` are only permitted for sub-messages within a message
+           expression, but as top-level option values
+   = help: `<...>` message expressions are an obscure feature and not
+           recommended
+
+error: unexpected identifier in message expression
+  --> testdata/parser/option/values.proto:80:5
+   |
+80 |     x3 17 to {}
+   |     ^^ expected message field value
+
+error: unexpected integer literal in message expression
+  --> testdata/parser/option/values.proto:80:8
+   |
+80 |     x3 17 to {}
+   |        ^^ expected message field value
+
+error: unexpected range expression in option setting value
+  --> testdata/parser/option/values.proto:81:8
+   |
+81 |     x4 {} to 91
+   |        ^^^^^^^^
+
 error: cannot name extension field using `(...)` in message expression
-  --> testdata/parser/option/values.proto:77:5
+  --> testdata/parser/option/values.proto:91:5
    |
-77 |     (x.y): 42
+91 |     (x.y): 42
    |     ^^^^^ expected this to be wrapped in `[...]` instead
    |
   help: replace the `(...)` with `[...]`
    |
-77 | -     (x.y): 42
-77 | +     [x.y]: 42
+91 | -     (x.y): 42
+91 | +     [x.y]: 42
    |
 
 error: unexpected absolute path in extension name
-  --> testdata/parser/option/values.proto:82:6
+  --> testdata/parser/option/values.proto:96:6
    |
-82 |     [.x.y]: 42
+96 |     [.x.y]: 42
    |      ^^^^ expected a path without a leading `.`
    |
   help: remove the leading `.`
    |
-82 | -     [.x.y]: 42
-82 | +     [x.y]: 42
+96 | -     [.x.y]: 42
+96 | +     [x.y]: 42
    |
 
 error: unexpected array expression in message field value
-  --> testdata/parser/option/values.proto:83:5
+  --> testdata/parser/option/values.proto:97:5
    |
-83 |     [x, y, z]: 42
+97 |     [x, y, z]: 42
    |     ^^^^^^^^^
    |      |
    |      expected message field name, extension name, or `Any` type URL
 
 error: unexpected array expression in message field value
-  --> testdata/parser/option/values.proto:84:5
+  --> testdata/parser/option/values.proto:98:5
    |
-84 |     []: 42
+98 |     []: 42
    |     ^^ expected message field name, extension name, or `Any` type URL
 
 error: type URL can only contain a single `/`
-  --> testdata/parser/option/values.proto:86:17
-   |
-86 |     [buf.build/x/y]: 42
-   |               - ^
-   |               |
-   |               first one is here
+   --> testdata/parser/option/values.proto:100:17
+    |
+100 |     [buf.build/x/y]: 42
+    |               - ^
+    |               |
+    |               first one is here
 
 error: unexpected integer literal in array expression
-  --> testdata/parser/option/values.proto:88:16
-   |
-88 |     x [{x: 5}, 1, <x: 5>, 2, 3],
-   |     -          ^ expected message expression
-   |     |
-   |     because this message field value is missing a `:`
-   |
-   = note: the `:` can be omitted in a message field value, but only if the
-           value is a message expression or a array expression of them
+   --> testdata/parser/option/values.proto:102:16
+    |
+102 |     x [{x: 5}, 1, <x: 5>, 2, 3],
+    |     -          ^ expected message expression
+    |     |
+    |     because this message field value is missing a `:`
+    |
+    = note: the `:` can be omitted in a message field value, but only if the
+            value is a message expression or a array expression of them
 
 warning: using `<...>` for message expression is not recommended
-  --> testdata/parser/option/values.proto:88:19
-   |
-88 |     x [{x: 5}, 1, <x: 5>, 2, 3],
-   |                   ^^^^^^
-  help: use `{...}` instead
-   |
-88 | -     x [{x: 5}, 1, <x: 5>, 2, 3],
-88 | +     x [{x: 5}, 1, {x: 5}, 2, 3],
-   |
-   = note: `<...>` are only permitted for sub-messages within a message
-           expression, but as top-level option values
-   = help: `<...>` message expressions are an obscure feature and not
-           recommended
+   --> testdata/parser/option/values.proto:102:19
+    |
+102 |     x [{x: 5}, 1, <x: 5>, 2, 3],
+    |                   ^^^^^^
+   help: use `{...}` instead
+    |
+102 | -     x [{x: 5}, 1, <x: 5>, 2, 3],
+102 | +     x [{x: 5}, 1, {x: 5}, 2, 3],
+    |
+    = note: `<...>` are only permitted for sub-messages within a message
+            expression, but as top-level option values
+    = help: `<...>` message expressions are an obscure feature and not
+            recommended
 
-encountered 19 errors and 6 warnings
+encountered 22 errors and 8 warnings

experimental/parser/testdata/parser/option/values.proto.yaml

@@ -162,6 +162,32 @@ decls:
       - key.path.components: [{ ident: "a" }]
         value.dict.entries:
         - { key.path.components: [{ ident: "a" }], value.literal.int_value: 42 }
+    - { key.path.components: [{ ident: "x" }], value.dict: {} }
+    - key.path.components: [{ ident: "x" }]
+      value.dict.entries:
+      - { key.path.components: [{ ident: "a" }], value.literal.int_value: 42 }
+    - key.path.components: [{ ident: "x" }]
+      value.dict.entries:
+      - { key.path.components: [{ ident: "a" }], value.literal.int_value: 42 }
+    - key.path.components: [{ ident: "x" }]
+      value.dict.entries:
+      - { key.path.components: [{ ident: "a" }], value.literal.int_value: 42 }
+      - { key.path.components: [{ ident: "b" }], value.literal.int_value: 43 }
+    - { key.path.components: [{ ident: "x1" }], value.literal.int_value: 55 }
+    - { key.path.components: [{ ident: "to" }], value.dict: {} }
+    - { key.path.components: [{ ident: "x2" }], value.dict: {} }
+    - { key.path.components: [{ ident: "to" }], value.dict: {} }
+    - value.path.components: [{ ident: "x3" }]
+    - value.literal.int_value: 17
+    - { key.path.components: [{ ident: "to" }], value.dict: {} }
+    - key.path.components: [{ ident: "x4" }]
+      value.range: { start.dict: {}, end.literal.int_value: 91 }
+    - key.path.components: [{ ident: "x1" }]
+      value.path.components: [{ ident: "to" }]
+    - key.path.components: [{ ident: "reserved" }]
+      value.path.components: [{ ident: "true" }]
+    - key.path.components: [{ ident: "to" }]
+      value.path.components: [{ ident: "true" }]
     - { key.literal.string_value: "ident", value.literal.int_value: 42 }
     - { key.literal.string_value: "???", value.literal.int_value: 42 }
     - { key.literal.int_value: 42, value.literal.int_value: 42 }