Skip to content

Commit

Permalink
update to protocompile v0.7.1; fix some incompatibilities with it (#586)
Browse files Browse the repository at this point in the history
  • Loading branch information
@jhump
jhump committed Dec 7, 2023
1 parent dd8b737 commit f139a6d
Show file tree
Hide file tree
Showing 40 changed files with 2,610 additions and 175 deletions.
9 changes: 7 additions & 2 deletions desc/protoparse/errors.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,12 @@ type ErrorWithPos = reporter.ErrorWithPos
//
// SourcePos should always be set and never nil.
type ErrorWithSourcePos struct {
// These fields are present and exported for backwards-compatibility
// with v1.4 and earlier.
Underlying error
Pos *SourcePos

reporter.ErrorWithPos
}

// Error implements the error interface
Expand Down Expand Up @@ -92,8 +96,9 @@ var _ ErrorWithPos = ErrorWithSourcePos{}
func toErrorWithSourcePos(err ErrorWithPos) ErrorWithPos {
pos := err.GetPosition()
return ErrorWithSourcePos{
Underlying: err.Unwrap(),
Pos: &pos,
ErrorWithPos: err,
Underlying: err.Unwrap(),
Pos: &pos,
}
}

Expand Down
53 changes: 34 additions & 19 deletions desc/protoparse/parser.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,11 +147,11 @@ func (p Parser) ParseFiles(filenames ...string) ([]*desc.FileDescriptor, error)
srcInfoMode = protocompile.SourceInfoExtraComments
}
rep := newReporter(p.ErrorReporter, p.WarningReporter)
res, srcPosAddr := p.getResolver(filenames)
res, srcSpanAddr := p.getResolver(filenames)

if p.InferImportPaths {
// we must first compile everything to protos
results, err := parseToProtosRecursive(res, filenames, reporter.NewHandler(rep), srcPosAddr)
results, err := parseToProtosRecursive(res, filenames, reporter.NewHandler(rep), srcSpanAddr)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -375,17 +375,17 @@ func parseToProtos(res protocompile.Resolver, filenames []string, rep *reporter.
return results, nil
}

func parseToProtosRecursive(res protocompile.Resolver, filenames []string, rep *reporter.Handler, srcPosAddr *SourcePos) (map[string]parser.Result, error) {
func parseToProtosRecursive(res protocompile.Resolver, filenames []string, rep *reporter.Handler, srcSpanAddr *ast2.SourceSpan) (map[string]parser.Result, error) {
results := make(map[string]parser.Result, len(filenames))
for _, filename := range filenames {
if err := parseToProtoRecursive(res, filename, rep, srcPosAddr, results); err != nil {
if err := parseToProtoRecursive(res, filename, rep, srcSpanAddr, results); err != nil {
return results, err
}
}
return results, rep.Error()
}

func parseToProtoRecursive(res protocompile.Resolver, filename string, rep *reporter.Handler, srcPosAddr *SourcePos, results map[string]parser.Result) error {
func parseToProtoRecursive(res protocompile.Resolver, filename string, rep *reporter.Handler, srcSpanAddr *ast2.SourceSpan, results map[string]parser.Result) error {
if _, ok := results[filename]; ok {
// already processed this one
return nil
Expand All @@ -412,13 +412,13 @@ func parseToProtoRecursive(res protocompile.Resolver, filename string, rep *repo
continue
}
err := func() error {
orig := *srcPosAddr
*srcPosAddr = astRoot.NodeInfo(imp.Name).Start()
orig := *srcSpanAddr
*srcSpanAddr = astRoot.NodeInfo(imp.Name)
defer func() {
*srcPosAddr = orig
*srcSpanAddr = orig
}()

return parseToProtoRecursive(res, imp.Name.AsString(), rep, srcPosAddr, results)
return parseToProtoRecursive(res, imp.Name.AsString(), rep, srcSpanAddr, results)
}()
if err != nil {
return err
Expand All @@ -433,27 +433,42 @@ func parseToProtoRecursive(res protocompile.Resolver, filename string, rep *repo
for i, dep := range fd.Dependency {
path := []int32{internal.File_dependencyTag, int32(i)}
err := func() error {
orig := *srcPosAddr
orig := *srcSpanAddr
found := false
for _, loc := range fd.GetSourceCodeInfo().GetLocation() {
if pathsEqual(loc.Path, path) {
*srcPosAddr = SourcePos{
start := SourcePos{
Filename: dep,
Line: int(loc.Span[0]),
Col: int(loc.Span[1]),
}
var end SourcePos
if len(loc.Span) > 3 {
end = SourcePos{
Filename: dep,
Line: int(loc.Span[2]),
Col: int(loc.Span[3]),
}
} else {
end = SourcePos{
Filename: dep,
Line: int(loc.Span[0]),
Col: int(loc.Span[2]),
}
}
*srcSpanAddr = ast2.NewSourceSpan(start, end)
found = true
break
}
}
if !found {
*srcPosAddr = *ast.UnknownPos(dep)
*srcSpanAddr = ast2.UnknownSpan(dep)
}
defer func() {
*srcPosAddr = orig
*srcSpanAddr = orig
}()

return parseToProtoRecursive(res, dep, rep, srcPosAddr, results)
return parseToProtoRecursive(res, dep, rep, srcSpanAddr, results)
}()
if err != nil {
return err
Expand Down Expand Up @@ -496,8 +511,8 @@ func newReporter(errRep ErrorReporter, warnRep WarningReporter) reporter.Reporte
return reporter.NewReporter(errRep, warnRep)
}

func (p Parser) getResolver(filenames []string) (protocompile.Resolver, *SourcePos) {
var srcPos SourcePos
func (p Parser) getResolver(filenames []string) (protocompile.Resolver, *ast2.SourceSpan) {
var srcSpan ast2.SourceSpan
accessor := p.Accessor
if accessor == nil {
accessor = func(name string) (io.ReadCloser, error) {
Expand All @@ -512,8 +527,8 @@ func (p Parser) getResolver(filenames []string) (protocompile.Resolver, *SourceP
// errors that don't include the filename that failed are no bueno
err = errorWithFilename{filename: filename, underlying: err}
}
if srcPos.Filename != "" {
err = reporter.Error(srcPos, err)
if srcSpan != nil {
err = reporter.Error(srcSpan, err)
}
}
return in, err
Expand Down Expand Up @@ -552,7 +567,7 @@ func (p Parser) getResolver(filenames []string) (protocompile.Resolver, *SourceP
}
return backupResolver.FindFileByPath(path)
}),
}, &srcPos
}, &srcSpan
}

func fixupFilenames(protos map[string]parser.Result) (revisedProtos map[string]parser.Result, rewrittenPaths map[string]string) {
Expand Down
6 changes: 3 additions & 3 deletions desc/protoparse/reporting_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func TestErrorReporting(t *testing.T) {
`,
},
expectedErrs: []string{
"test.proto:5:41: syntax error: unexpected \"enum\", expecting ';' or '.'",
"test.proto:5:41: expected ';'",
"test.proto:5:69: syntax error: unexpected ';', expecting '='",
"test.proto:7:53: syntax error: unexpected '='",
},
Expand Down Expand Up @@ -132,14 +132,14 @@ func TestErrorReporting(t *testing.T) {
`,
},
expectedErrs: []string{
"test2.proto:7:49: syntax error: unexpected identifier, expecting \"option\" or \"rpc\" or ';' or '}'",
"test2.proto:7:49: syntax error: unexpected identifier, expecting \"option\" or \"rpc\" or '}'",
"test1.proto:5:62: syntax error: unexpected '-', expecting int literal",
"test1.proto:8:62: syntax error: unexpected ';', expecting \"returns\"",
},
expectedErrsAlt: []string{
"test1.proto:5:62: syntax error: unexpected '-', expecting int literal",
"test1.proto:8:62: syntax error: unexpected ';', expecting \"returns\"",
"test2.proto:7:49: syntax error: unexpected identifier, expecting \"option\" or \"rpc\" or ';' or '}'",
"test2.proto:7:49: syntax error: unexpected identifier, expecting \"option\" or \"rpc\" or '}'",
},
},
{
Expand Down
18 changes: 9 additions & 9 deletions desc/protoparse/validate_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,11 +107,11 @@ func TestBasicValidation(t *testing.T) {
},
{
contents: `message Foo { option map_entry = true; }`,
errMsg: `test.proto:1:34: message Foo: map_entry option should not be set explicitly; use map type instead`,
errMsg: `test.proto:1:22: message Foo: map_entry option should not be set explicitly; use map type instead`,
},
{
contents: `message Foo { option map_entry = false; }`,
succeeds: true, // okay if explicit setting is false
errMsg: `test.proto:1:22: message Foo: map_entry option should not be set explicitly; use map type instead`,
},
{
contents: `syntax = "proto2"; message Foo { string s = 1; }`,
Expand All @@ -131,15 +131,15 @@ func TestBasicValidation(t *testing.T) {
},
{
contents: `syntax = "proto3"; message Foo { required string s = 1; }`,
errMsg: `test.proto:1:34: field Foo.s: label 'required' is not allowed in proto3`,
errMsg: `test.proto:1:34: field Foo.s: label 'required' is not allowed in proto3 or editions`,
},
{
contents: `message Foo { extensions 1 to max; } extend Foo { required string sss = 100; }`,
errMsg: `test.proto:1:51: field sss: extension fields cannot be 'required'`,
errMsg: `test.proto:1:51: extension sss: extension fields cannot be 'required'`,
},
{
contents: `syntax = "proto3"; message Foo { optional group Grp = 1 { } }`,
errMsg: `test.proto:1:43: field Foo.grp: groups are not allowed in proto3`,
errMsg: `test.proto:1:43: field Foo.grp: groups are not allowed in proto3 or editions`,
},
{
contents: `syntax = "proto3"; message Foo { extensions 1 to max; }`,
Expand Down Expand Up @@ -247,11 +247,11 @@ func TestBasicValidation(t *testing.T) {
},
{
contents: `message Foo { reserved "foo", "foo"; }`,
errMsg: `test.proto:1:31: name "foo" is reserved multiple times`,
errMsg: `test.proto:1:31: name "foo" is already reserved at test.proto:1:24`,
},
{
contents: `message Foo { reserved "foo"; reserved "foo"; }`,
errMsg: `test.proto:1:40: name "foo" is reserved multiple times`,
errMsg: `test.proto:1:40: name "foo" is already reserved at test.proto:1:24`,
},
{
contents: `message Foo { reserved "foo"; optional string foo = 1; }`,
Expand Down Expand Up @@ -279,7 +279,7 @@ func TestBasicValidation(t *testing.T) {
},
{
contents: `enum Foo { reserved = 1; }`,
errMsg: `test.proto:1:21: syntax error: unexpected '=', expecting string literal or int literal or '-'`,
errMsg: `test.proto:1:21: syntax error: unexpected '='`,
},
{
contents: `syntax = "proto3"; enum message { unset = 0; } message Foo { message bar = 1; }`,
Expand All @@ -291,7 +291,7 @@ func TestBasicValidation(t *testing.T) {
},
{
contents: `syntax = "proto3"; enum reserved { unset = 0; } message Foo { reserved bar = 1; }`,
errMsg: `test.proto:1:72: syntax error: unexpected identifier, expecting string literal or int literal`,
errMsg: `test.proto:1:76: expected ';'`,
},
{
contents: `syntax = "proto3"; enum extend { unset = 0; } message Foo { extend bar = 1; }`,
Expand Down

0 comments on commit f139a6d

Please sign in to comment.