Skip to content

Commit

Permalink
Tolerate trailing ',' in compact options (#221)
Browse files Browse the repository at this point in the history 
While still producing an error, see
#200.
  • Loading branch information
@Alfus
Alfus authored Dec 8, 2023
1 parent f2f1254 commit 508b83b
Show file tree
Hide file tree
Showing 4 changed files with 634 additions and 573 deletions.
28 changes: 18 additions & 10 deletions ast/options.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -343,22 +343,30 @@ func NewCompactOptionsNode(openBracket *RuneNode, opts []*OptionNode, commas []*
if len(opts) == 0 && len(commas) != 0 {
panic("opts is empty but commas is not")
}
if len(opts) > 0 && len(commas) != len(opts)-1 {
if len(opts) != len(commas) && len(opts) != len(commas)+1 {
panic(fmt.Sprintf("%d opts requires %d commas, not %d", len(opts), len(opts)-1, len(commas)))
}
children := make([]Node, 0, len(opts)*2+1)
children := make([]Node, 0, len(opts)+len(commas)+2)
children = append(children, openBracket)
for i, opt := range opts {
if i > 0 {
if commas[i-1] == nil {
panic(fmt.Sprintf("commas[%d] is nil", i-1))
if len(opts) > 0 {
for i, opt := range opts {
if i > 0 {
if commas[i-1] == nil {
panic(fmt.Sprintf("commas[%d] is nil", i-1))
}
children = append(children, commas[i-1])
}
if opt == nil {
panic(fmt.Sprintf("opts[%d] is nil", i))
}
children = append(children, commas[i-1])
children = append(children, opt)
}
if opt == nil {
panic(fmt.Sprintf("opts[%d] is nil", i))
if len(opts) == len(commas) { // Add the erroneous, but tolerated trailing comma.
if commas[len(commas)-1] == nil {
panic(fmt.Sprintf("commas[%d] is nil", len(commas)-1))
}
children = append(children, commas[len(commas)-1])
}
children = append(children, opt)
}
children = append(children, closeBracket)

Expand Down
89 changes: 42 additions & 47 deletions parser/parser_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,12 +82,29 @@ func TestJunkParse(t *testing.T) {
}
}

type parseErrorTestCase struct {
Error string
NoError string
}

func runParseErrorTestCases(t *testing.T, testCases map[string]parseErrorTestCase, expected string) {
for name, testCase := range testCases {
name, testCase := name, testCase
t.Run(name, func(t *testing.T) {
t.Parallel()
errHandler := reporter.NewHandler(nil)
protoName := fmt.Sprintf("%s.proto", name)
_, err := Parse(protoName, strings.NewReader(testCase.NoError), errHandler)
require.NoError(t, err)
_, err = Parse(protoName, strings.NewReader(testCase.Error), errHandler)
require.ErrorContains(t, err, expected)
})
}
}

func TestLenientParse_SemicolonLess(t *testing.T) {
t.Parallel()
inputs := map[string]struct {
Error string
NoError string
}{
inputs := map[string]parseErrorTestCase{
"package": {
Error: `syntax = "proto3";
package foo
Expand Down Expand Up @@ -353,26 +370,12 @@ func TestLenientParse_SemicolonLess(t *testing.T) {
}`,
},
}
for name, input := range inputs {
name, input := name, input
t.Run(name, func(t *testing.T) {
t.Parallel()
errHandler := reporter.NewHandler(nil)
protoName := fmt.Sprintf("%s.proto", name)
_, err := Parse(protoName, strings.NewReader(input.NoError), errHandler)
require.NoError(t, err)
_, err = Parse(protoName, strings.NewReader(input.Error), errHandler)
require.ErrorContains(t, err, "expected ';'")
})
}
runParseErrorTestCases(t, inputs, "expected ';'")
}

func TestLenientParse_EmptyCompactOptions(t *testing.T) {
t.Parallel()
inputs := map[string]struct {
Error string
NoError string
}{
inputs := map[string]parseErrorTestCase{
"field-options": {
Error: `syntax = "proto3";
message Foo {
Expand All @@ -394,26 +397,12 @@ func TestLenientParse_EmptyCompactOptions(t *testing.T) {
}`,
},
}
for name, input := range inputs {
name, input := name, input
t.Run(name, func(t *testing.T) {
t.Parallel()
errHandler := reporter.NewHandler(nil)
protoName := fmt.Sprintf("%s.proto", name)
_, err := Parse(protoName, strings.NewReader(input.NoError), errHandler)
require.NoError(t, err)
_, err = Parse(protoName, strings.NewReader(input.Error), errHandler)
require.ErrorContains(t, err, "compact options must have at least one option")
})
}
runParseErrorTestCases(t, inputs, "compact options must have at least one option")
}

func TestLenientParse_EmptyCompactValue(t *testing.T) {
t.Parallel()
inputs := map[string]struct {
Error string
NoError string
}{
inputs := map[string]parseErrorTestCase{
"field-options": {
Error: `syntax = "proto2";
message Foo {
Expand All @@ -435,18 +424,24 @@ func TestLenientParse_EmptyCompactValue(t *testing.T) {
}`,
},
}
for name, input := range inputs {
name, input := name, input
t.Run(name, func(t *testing.T) {
t.Parallel()
errHandler := reporter.NewHandler(nil)
protoName := fmt.Sprintf("%s.proto", name)
_, err := Parse(protoName, strings.NewReader(input.NoError), errHandler)
require.NoError(t, err)
_, err = Parse(protoName, strings.NewReader(input.Error), errHandler)
require.ErrorContains(t, err, "compact option must have a value")
})
runParseErrorTestCases(t, inputs, "compact option must have a value")
}

func TestLenientParse_OptionsTrailingComma(t *testing.T) {
t.Parallel()
inputs := map[string]parseErrorTestCase{
"field-options": {
Error: `syntax = "proto3";
message Foo {
int32 bar = 1 [default=1,];
}`,
NoError: `syntax = "proto3";
message Foo {
int32 bar = 1 [default=1];
}`,
},
}
runParseErrorTestCases(t, inputs, "unexpected ','")
}

func TestSimpleParse(t *testing.T) {
Expand Down
35 changes: 28 additions & 7 deletions parser/proto.y
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,8 @@ import (
%type <imprt> importDecl
%type <pkg> packageDecl
%type <optRaw> compactOption oneofOptionDecl
%type <opt> optionDecl
%type <opts> compactOptionDecls
%type <opt> optionDecl compactOptionEntry compactOptionFinal
%type <opts> compactOptionDecls compactOptionLeadingDecls
%type <ref> extensionName messageLiteralFieldName
%type <optNms> optionName
%type <cmpctOpts> compactOptions
Expand Down Expand Up @@ -604,15 +604,36 @@ compactOptions : '[' compactOptionDecls ']' {
$$ = ast.NewCompactOptionsNode($1, nil, nil, $2)
}

compactOptionDecls : compactOption {
$$ = &compactOptionSlices{options: []*ast.OptionNode{$1}}
compactOptionDecls : compactOptionFinal {
$$ = &compactOptionSlices{options: []*ast.OptionNode{$1.Node}, commas: $1.Runes}
}
| compactOptionDecls ',' compactOption {
$1.options = append($1.options, $3)
$1.commas = append($1.commas, $2)
| compactOptionLeadingDecls compactOptionFinal {
$1.options = append($1.options, $2.Node)
$1.commas = append($1.commas, $2.Runes...)
$$ = $1
}

compactOptionLeadingDecls : compactOptionEntry {
$$ = &compactOptionSlices{options: []*ast.OptionNode{$1.Node}, commas: $1.Runes}
}
| compactOptionLeadingDecls compactOptionEntry {
$1.options = append($1.options, $2.Node)
$1.commas = append($1.commas, $2.Runes...)
$$ = $1
}

compactOptionFinal : compactOption {
$$ = newNodeWithRunes($1)
}
| compactOptionEntry {
protolex.(*protoLex).Error("unexpected ','")
$$ = $1
}

compactOptionEntry : compactOption ',' {
$$ = newNodeWithRunes($1, $2)
}

compactOption : optionName '=' optionValue {
optName := ast.NewOptionNameNode($1.refs, $1.dots)
$$ = ast.NewCompactOptionNode(optName, $2, $3)
Expand Down
Loading

0 comments on commit 508b83b

Please sign in to comment.