bufbuild/protocompile

Panic in MessageFieldNode

Alfus opened this issue · 8 comments

When the separator is missing, the NewMessageFieldNode panics.

panic: val is nil

goroutine 21 [running]:
github.com/bufbuild/protocompile/ast.NewMessageFieldNode(0x103bf82b0?, 0x140006252c0?, {0x0?, 0x0?})
	/Users/alfred/go/pkg/mod/github.com/bufbuild/protocompile@v0.6.1-0.20231108163138-146b831231f7/ast/values.go:541 +0x280
github.com/bufbuild/protocompile/parser.(*protoParserImpl).Parse(0x1400055f000, {0x103bf82b0?, 0x140006252c0?})
	/Users/alfred/go/pkg/mod/github.com/bufbuild/protocompile@v0.6.1-0.20231108163138-146b831231f7/parser/proto.y.go:1556 +0x3b30
github.com/bufbuild/protocompile/parser.protoParse(...)
	/Users/alfred/go/pkg/mod/github.com/bufbuild/protocompile@v0.6.1-0.20231108163138-146b831231f7/parser/proto.y.go:997
github.com/bufbuild/protocompile/parser.Parse({0x0, 0x0}, {0x103bf4de0?, 0x1400062bb20?}, 0x20?)
	/Users/alfred/go/pkg/mod/github.com/bufbuild/protocompile@v0.6.1-0.20231108163138-146b831231f7/parser/parser.go:98 +0x70
github.com/bufbuild/buf-lsp/internal/buflsp.parseFile({0x0, 0x0}, {0x103bf4de0, 0x1400062bb20})

can repro with:

message TestNestedMessage {
  TestNestedEnum test_nested_enum = 3 [(buf.validate.field) = {
    enum: {
      defined_only: true,
      not_in: [
        1
        2
      ]
    }
  }];
}

The protobuf languages does not allow the separator to be missing. When I try this, I get a proper error:

test.proto:7:9: syntax error: unexpected int literal, expecting ',' or ']'

I think this must be coming about because you have a branch where you've changed the grammar, in which case the fix is to update these constructors to match your relaxed grammar 😛

Hmm, I tried to make extra sure I was using the most recent main version of protocompile. I am on: github.com/bufbuild/protocompile v0.6.1-0.20231108163138-146b831231f7

I just checked out that version (146b831) and get the same error I pasted above.

I tested it by applying this patch. The test case succeeds, no panics.

diff --git a/parser/validate_test.go b/parser/validate_test.go
index 4a4a0e0..24c384c 100644
--- a/parser/validate_test.go
+++ b/parser/validate_test.go
@@ -44,6 +44,20 @@ func TestBasicValidation(t *testing.T) {
                expectedErr            string
                expectedDiffWithProtoc bool
        }{
+               "failure_missing_separator": {
+                       contents: `message TestNestedMessage {
+  TestNestedEnum test_nested_enum = 3 [(buf.validate.field) = {
+    enum: {
+      defined_only: true,
+      not_in: [
+        1
+        2
+      ]
+    }
+  }];
+}`,
+                       expectedErr: `test.proto:7:9: syntax error: unexpected int literal, expecting ',' or ']'`,
+               },
                "success_large_negative_integer": {
                        contents: `message Foo { optional double bar = 1 [default = -18446744073709551615]; }`,
                },

image
is looks like where my nil is coming from.

so odd:

protoVAL.v = ast.NewArrayLiteralNode(protoDollar[1].b, nil, nil, protoDollar[3].b)

Okay, I think I see why I couldn't repro in that simple test case. Let me play with it a little more.

Okay. I got to the bottom of it. This is a dup of #196 and was already fixed in #197. That commit you're on is a bit too old. The bug was fixed in literally the very next commit on main (same day even): db37c4a