Skip to content

Conversation

@dsyer
Copy link
Contributor

@dsyer dsyer commented Sep 26, 2025

Apparently the trailing comma is optional in blockLit, see
https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/descriptor.proto#L1026
for example. There are other examples in that file that use trailing commas
as list delimiters.

I haven't seen this comma in the wild in any proto3 files, but that doesn't mean it's not legal I suppose (I haven't changed that grammar for now).

I think this change is probably also required by #4631 to be fully correct,
but it is somewhat independent of that issue.

Apparently the trailing comma is optional in blockLit, see
https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/descriptor.proto#L1026
for example. There are other examples in that file that use trailing commas
as list delimiters.

I think this change is probably also required by antlr#4631 to be fully correct,
but it is somewhat independent of that issue.
dsyer added a commit to dsyer/grammars-v4 that referenced this pull request Sep 26, 2025
Duplicates one of the changes from antlr#4632
@kaby76
Copy link
Contributor

kaby76 commented Sep 26, 2025

In this PR title, you specified the grammar as [protobuf]. It is not! It is protobuf2. Please change this.

In addition, this PR has a grammar change without a single example added.

@dsyer dsyer changed the title [protobuf] Fix blockLit to allow trailing comma [protobuf2] Fix blockLit to allow trailing comma Sep 26, 2025
@dsyer
Copy link
Contributor Author

dsyer commented Sep 26, 2025

Is it my fault that the current grammar is broken without any supporting examples? I think you need to be a bit kinder to people who spend their time fixing your code. An apology will be acceptable.

@kaby76
Copy link
Contributor

kaby76 commented Sep 26, 2025

Is it my fault that the current grammar is broken without any supporting examples? I think you need to be a bit kinder to people who spend their time fixing your code. An apology will be acceptable.

Sorry, it is not your fault. The person who did the original PR had just one test case. e33d351

However, you need to add one or more examples that test the specific rules that you changed in the grammar.

@dsyer
Copy link
Contributor Author

dsyer commented Sep 26, 2025

I added an example supporting this change. The protobuf3 examples are a mess though - hardly any of them actually are parseable, and the tests don't seem to care (the <exampleFiles>/</exampleFiles> is empty so I don't think they do anything, but happy to learn otherwise). I can raise another issue to maybe deal with that, but I won't be able to add any samples for protobuf3 (in other PRs) unless someone can fix this or explain how to fix it.

@kaby76
Copy link
Contributor

kaby76 commented Sep 26, 2025

The protobuf3 examples are a mess though - hardly any of them actually are parseable

I entered the tests carefully taking into consideration whether they are really protobuf3 input. They ones that I test are defined by the desc.xml: <inputs>examples/*.proto</inputs>. I did not say <inputs>examples/**/*.proto</inputs>; the .proto files underneath. Yes, while protoc examples/google/protobuf/*.proto --csharp_out=foobar works, the protobuf3 grammar does not parse protobuf2, so it fails to parse ../examples/google/protobuf/descriptor.proto and ../examples/google/protobuf/compiler/plugin.proto. That's been logged in another issue (#4554).

All the other files parse:

$ ./bin/Debug/net8.0/Test.exe `find ../examples -name '*.proto' | grep -v descriptor.proto | grep -v plugin.proto`
CSharp 0 ../examples/1.proto success 0.0394949
CSharp 1 ../examples/1r.proto success 0.0002494
CSharp 2 ../examples/2.proto success 0.0001787
CSharp 3 ../examples/3.proto success 0.0011074
CSharp 4 ../examples/4.proto success 0.0002232
CSharp 5 ../examples/6.proto success 0.0030082
CSharp 6 ../examples/6r.proto success 0.000161
CSharp 7 ../examples/7.proto success 0.0001421
CSharp 8 ../examples/7r.proto success 0.0001432
CSharp 9 ../examples/addressbook.proto success 0.003751
CSharp 10 ../examples/conformance.proto success 0.0168829
CSharp 11 ../examples/example.proto success 0.0088368
CSharp 12 ../examples/google/protobuf/any.proto success 0.0009125
CSharp 13 ../examples/google/protobuf/api.proto success 0.0020325
CSharp 14 ../examples/google/protobuf/duration.proto success 0.0007471
CSharp 15 ../examples/google/protobuf/empty.proto success 0.0004448
CSharp 16 ../examples/google/protobuf/field_mask.proto success 0.0012575
CSharp 17 ../examples/google/protobuf/source_context.proto success 0.0004184
CSharp 18 ../examples/google/protobuf/struct.proto success 0.0009124
CSharp 19 ../examples/google/protobuf/timestamp.proto success 0.000816
CSharp 20 ../examples/google/protobuf/type.proto success 0.0023619
CSharp 21 ../examples/google/protobuf/wrappers.proto success 0.0009136
CSharp 22 ../examples/helloworld.proto success 0.000314
CSharp 23 ../examples/helloworldreserved.proto success 0.001308
CSharp 24 ../examples/keywordasid.proto success 8.2E-05
Total Time: 0.1562078
09/26-06:57:09 ~/issues/g4-current/protobuf/protobuf3/Generated-CSharp

It looks like the pom.xml does not have any tests. I'll fix that right now.

Copy link
Contributor

@kaby76 kaby76 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works fine. @teverett Please start the workflows for this PR. Ty.

09/26-08:11:36 ~/issues/g4-4632/protobuf/protobuf2
$ bash ../../_scripts/test.sh
Adding protobuf/protobuf2
Number of grammars before sorting and making unique: 1
Number of grammars after sorting and making unique: 1
Grammars to do are: protobuf/protobuf2
grammars = protobuf/protobuf2
targets = Antlr4ng CSharp Cpp Dart Go Java JavaScript Python3 TypeScript
order = grammars
filter = all
Tests now protobuf/protobuf2,Antlr4ng protobuf/protobuf2,CSharp protobuf/protobuf2,Cpp protobuf/protobuf2,Dart protobuf/protobuf2,Go protobuf/protobuf2,Java protobuf/protobuf2,JavaScript protobuf/protobuf2,Python3 protobuf/protobuf2,TypeScript

protobuf/protobuf2,Antlr4ng:
 Build 00:00:15
 Test 00:00:03
 Succeeded.

protobuf/protobuf2,CSharp:
 Build 00:00:07
 Test 00:00:03
 Succeeded.

protobuf/protobuf2,Cpp:
 Build 00:00:27
 Test 00:00:02
 Succeeded.

protobuf/protobuf2,Dart:
 Build 00:00:14
 Test 00:00:02
 Succeeded.

protobuf/protobuf2,Go:
 Build 00:00:19
 Test 00:00:02
 Succeeded.

protobuf/protobuf2,Java:
 Build 00:00:06
 Test 00:00:02
 Succeeded.

protobuf/protobuf2,JavaScript:
 Build 00:00:04
 Test 00:00:02
 Succeeded.

protobuf/protobuf2,Python3:
 Build 00:00:06
 Test 00:00:04
 Succeeded.

protobuf/protobuf2,TypeScript:
 Build 00:00:10
 Test 00:00:04
 Succeeded.
09/26-08:14:22 ~/issues/g4-4632/protobuf/protobuf2
$

@teverett
Copy link
Member

@dsyer thanks

@teverett teverett merged commit 5fbc437 into antlr:master Sep 29, 2025
32 checks passed
dsyer added a commit to dsyer/grammars-v4 that referenced this pull request Sep 29, 2025
Duplicates one of the changes from antlr#4632
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants