Skip to content

Commit

Permalink
reflect/protoregistry: restore conflicting file names check
Browse files Browse the repository at this point in the history
There are inconsistencies between implementations on what happens when
a single program contains generated code for multiple files with the
same source path. At least one canonical implementation (C++) will fail
at link time. Others print warnings. Some silently resolve the registry
conflict in favor of one file or the other. The protobuf maintainers
agree, however, that the desired behavior is for this condition to be an
error.

Updates golang/protobuf#1122

Change-Id: I716708f16ef90210bdfceb0888691e47783df172
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/322729
Trust: Damien Neil <dneil@google.com>
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
  • Loading branch information
neild committed Jun 2, 2021
1 parent 426f20b commit 21e33cc
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 4 deletions.
7 changes: 6 additions & 1 deletion reflect/protoregistry/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,13 @@ func (r *Files) RegisterFile(file protoreflect.FileDescriptor) error {
r.filesByPath = make(map[string][]protoreflect.FileDescriptor)
}
path := file.Path()
if len(r.filesByPath[path]) > 0 {
if prev := r.filesByPath[path]; len(prev) > 0 {
r.checkGenProtoConflict(path)
err := errors.New("file %q is already registered", file.Path())
err = amendErrorWithCaller(err, prev[0], file)
if !(r == GlobalFiles && ignoreConflict(file, err)) {
return err
}
}

for name := file.Package(); name != ""; name = name.Parent() {
Expand Down
8 changes: 5 additions & 3 deletions reflect/protoregistry/registry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ func TestFiles(t *testing.T) {
files: []testFile{
{inFile: mustMakeFile(`syntax:"proto2" name:"test1.proto" package:"foo.bar"`)},
{inFile: mustMakeFile(`syntax:"proto2" name:"foo/bar/test.proto" package:"my.test"`)},
{inFile: mustMakeFile(`syntax:"proto2" name:"foo/bar/test.proto" package:"foo.bar.baz"`)},
{inFile: mustMakeFile(`syntax:"proto2" name:"foo/bar/test.proto" package:"foo.bar.baz"`), wantErr: "already registered"},
{inFile: mustMakeFile(`syntax:"proto2" name:"test2.proto" package:"my.test.package"`)},
{inFile: mustMakeFile(`syntax:"proto2" name:"weird" package:"foo.bar"`)},
{inFile: mustMakeFile(`syntax:"proto2" name:"foo/bar/baz/../test.proto" package:"my.test"`)},
Expand Down Expand Up @@ -112,8 +112,10 @@ func TestFiles(t *testing.T) {
{"weird", "foo.bar"},
},
}, {
inPath: "foo/bar/test.proto",
wantErr: `multiple files named "foo/bar/test.proto"`,
inPath: "foo/bar/test.proto",
wantFiles: []file{
{"foo/bar/test.proto", "my.test"},
},
}},
}, {
// Test when new enum conflicts with existing package.
Expand Down

0 comments on commit 21e33cc

Please sign in to comment.