protoc-gen-go: fix up generation of package names (#576)

An earlier change inadvertendly made the import_path flag stop setting
the package name. In addition, we intentionally removed the behavior where
a go_package option in one file would set the package name for other
generated files, in anticipation of allowing a single compilation action
to generate code for many packages.

This change restores the import_path behavior. It also permits a go_package
option in one file to affect other files *in the same package*. (Every
source file should include a go_package option, which makes this case moot,
but this minimizes the amount of breaking change that we're introducing.)

Also tweak assignment of import paths to allow the import_path flag to
set the import path for all generated files.

protoc-gen-go/generator/generator.go

@@ -270,8 +270,9 @@ type FileDescriptor struct {
 	// This is used for supporting public imports.
 	exported map[Object][]symbol
 
-	fingerprint string       // Fingerprint of this file's contents.
-	importPath  GoImportPath // Import path of this file's package.
+	fingerprint string        // Fingerprint of this file's contents.
+	importPath  GoImportPath  // Import path of this file's package.
+	packageName GoPackageName // Name of this file's Go package.
 
 	proto3 bool // whether to generate proto3 code for this file
 }
@@ -306,25 +307,6 @@ func (d *FileDescriptor) goPackageOption() (impPath GoImportPath, pkg GoPackageN
 	return "", cleanPackageName(opt), true
 }
 
-// goPackageName returns the Go package name to use in the
-// generated Go file.  The result explicit reports whether the name
-// came from an option go_package statement.  If explicit is false,
-// the name was derived from the protocol buffer's package statement
-// or the input file name.
-func (d *FileDescriptor) goPackageName() (name GoPackageName, explicit bool) {
-	// Does the file have a "go_package" option?
-	if _, pkg, ok := d.goPackageOption(); ok {
-		return pkg, true
-	}
-
-	// Does the file have a package clause?
-	if p := d.GetPackage(); p != "" {
-		return cleanPackageName(p), false
-	}
-	// Use the file base name.
-	return cleanPackageName(baseName(d.GetName())), false
-}
-
 // goFileName returns the output name for the generated Go file.
 func (d *FileDescriptor) goFileName(pathType pathType) string {
 	name := *d.Name
@@ -773,15 +755,45 @@ func (g *Generator) defaultGoPackage() GoPackageName {
 func (g *Generator) SetPackageNames() {
 	g.outputImportPath = g.genFiles[0].importPath
 
+	defaultPackageNames := make(map[GoImportPath]GoPackageName)
+	for _, f := range g.genFiles {
+		if _, p, ok := f.goPackageOption(); ok {
+			defaultPackageNames[f.importPath] = p
+		}
+	}
+	for _, f := range g.genFiles {
+		if _, p, ok := f.goPackageOption(); ok {
+			// Source file: option go_package = "quux/bar";
+			f.packageName = p
+		} else if p, ok := defaultPackageNames[f.importPath]; ok {
+			// A go_package option in another file in the same package.
+			//
+			// This is a poor choice in general, since every source file should
+			// contain a go_package option. Supported mainly for historical
+			// compatibility.
+			f.packageName = p
+		} else if p := g.defaultGoPackage(); p != "" {
+			// Command-line: import_path=quux/bar.
+			//
+			// The import_path flag sets a package name for files which don't
+			// contain a go_package option.
+			f.packageName = p
+		} else if p := f.GetPackage(); p != "" {
+			// Source file: package quux.bar;
+			f.packageName = cleanPackageName(p)
+		} else {
+			// Source filename.
+			f.packageName = cleanPackageName(baseName(f.GetName()))
+		}
+	}
+
 	// Check that all files have a consistent package name and import path.
-	pkg, _ := g.genFiles[0].goPackageName()
 	for _, f := range g.genFiles[1:] {
 		if a, b := g.genFiles[0].importPath, f.importPath; a != b {
-			g.Fail(fmt.Sprint("inconsistent package import paths: ", a, b))
+			g.Fail(fmt.Sprintf("inconsistent package import paths: %v, %v", a, b))
 		}
-		thisPkg, _ := f.goPackageName()
-		if pkg != thisPkg {
-			g.Fail(fmt.Sprint("inconsistent package names: ", thisPkg, pkg))
+		if a, b := g.genFiles[0].packageName, f.packageName; a != b {
+			g.Fail(fmt.Sprintf("inconsistent package names: %v, %v", a, b))
 		}
 	}
 
@@ -800,17 +812,37 @@ func (g *Generator) SetPackageNames() {
 func (g *Generator) WrapTypes() {
 	g.allFiles = make([]*FileDescriptor, 0, len(g.Request.ProtoFile))
 	g.allFilesByName = make(map[string]*FileDescriptor, len(g.allFiles))
+	genFileNames := make(map[string]bool)
+	for _, n := range g.Request.FileToGenerate {
+		genFileNames[n] = true
+	}
 	for _, f := range g.Request.ProtoFile {
 		fd := &FileDescriptor{
 			FileDescriptorProto: f,
 			exported:            make(map[Object][]symbol),
 			proto3:              fileIsProto3(f),
 		}
+		// The import path may be set in a number of ways.
 		if substitution, ok := g.ImportMap[f.GetName()]; ok {
+			// Command-line: M=foo.proto=quux/bar.
+			//
+			// Explicit mapping of source file to import path.
 			fd.importPath = GoImportPath(substitution)
+		} else if genFileNames[f.GetName()] && g.PackageImportPath != "" {
+			// Command-line: import_path=quux/bar.
+			//
+			// The import_path flag sets the import path for every file that
+			// we generate code for.
+			fd.importPath = GoImportPath(g.PackageImportPath)
 		} else if p, _, _ := fd.goPackageOption(); p != "" {
+			// Source file: option go_package = "quux/bar";
+			//
+			// The go_package option sets the import path. Most users should use this.
 			fd.importPath = p
 		} else {
+			// Source filename.
+			//
+			// Last resort when nothing else is available.
 			fd.importPath = GoImportPath(path.Dir(f.GetName()))
 		}
 		// We must wrap the descriptors before we wrap the enums
@@ -1335,12 +1367,11 @@ func (g *Generator) generateHeader() {
 	}
 	g.P()
 
-	name, _ := g.file.goPackageName()
 	importPath, _, _ := g.file.goPackageOption()
 	if importPath == "" {
-		g.P("package ", name)
+		g.P("package ", g.file.packageName)
 	} else {
-		g.P("package ", name, " // import ", GoImportPath(g.ImportPrefix)+importPath)
+		g.P("package ", g.file.packageName, " // import ", GoImportPath(g.ImportPrefix)+importPath)
 	}
 	g.P()
 

protoc-gen-go/golden_test.go

@@ -115,14 +115,14 @@ var fdescRE = regexp.MustCompile(`(?ms)^var fileDescriptor.*}`)
 const (
 	aProto = `
 syntax = "proto3";
-package alpha;
+package test.alpha;
 option go_package = "package/alpha";
 import "beta/b.proto";
-message M { beta.M field = 1; }`
+message M { test.beta.M field = 1; }`
 
 	bProto = `
 syntax = "proto3";
-package beta;
+package test.beta;
 // no go_package option
 message M {}`
 )
@@ -132,12 +132,16 @@ func TestParameters(t *testing.T) {
 		parameters   string
 		wantFiles    map[string]bool
 		wantImportsA map[string]bool
+		wantPackageA string
+		wantPackageB string
 	}{{
 		parameters: "",
 		wantFiles: map[string]bool{
 			"package/alpha/a.pb.go": true,
 			"beta/b.pb.go":          true,
 		},
+		wantPackageA: "alpha",
+		wantPackageB: "test_beta",
 		wantImportsA: map[string]bool{
 			"github.com/golang/protobuf/proto": true,
 			"beta": true,
@@ -148,6 +152,8 @@ func TestParameters(t *testing.T) {
 			"package/alpha/a.pb.go": true,
 			"beta/b.pb.go":          true,
 		},
+		wantPackageA: "alpha",
+		wantPackageB: "test_beta",
 		wantImportsA: map[string]bool{
 			// This really doesn't seem like useful behavior.
 			"prefixgithub.com/golang/protobuf/proto": true,
@@ -155,7 +161,9 @@ func TestParameters(t *testing.T) {
 		},
 	}, {
 		// import_path only affects the 'package' line.
-		parameters: "import_path=import/path/of/pkg",
+		parameters:   "import_path=import/path/of/pkg",
+		wantPackageA: "alpha",
+		wantPackageB: "pkg",
 		wantFiles: map[string]bool{
 			"package/alpha/a.pb.go": true,
 			"beta/b.pb.go":          true,
@@ -166,6 +174,8 @@ func TestParameters(t *testing.T) {
 			"package/alpha/a.pb.go": true,
 			"beta/b.pb.go":          true,
 		},
+		wantPackageA: "alpha",
+		wantPackageB: "test_beta",
 		wantImportsA: map[string]bool{
 			"github.com/golang/protobuf/proto": true,
 			// Rewritten by the M parameter.
@@ -177,6 +187,8 @@ func TestParameters(t *testing.T) {
 			"package/alpha/a.pb.go": true,
 			"beta/b.pb.go":          true,
 		},
+		wantPackageA: "alpha",
+		wantPackageB: "test_beta",
 		wantImportsA: map[string]bool{
 			// import_prefix applies after M.
 			"prefixpackage/gamma": true,
@@ -187,6 +199,8 @@ func TestParameters(t *testing.T) {
 			"alpha/a.pb.go": true,
 			"beta/b.pb.go":  true,
 		},
+		wantPackageA: "alpha",
+		wantPackageB: "test_beta",
 	}, {
 		parameters: "paths=source_relative,import_prefix=prefix",
 		wantFiles: map[string]bool{
@@ -194,6 +208,8 @@ func TestParameters(t *testing.T) {
 			"alpha/a.pb.go": true,
 			"beta/b.pb.go":  true,
 		},
+		wantPackageA: "alpha",
+		wantPackageB: "test_beta",
 	}} {
 		name := test.parameters
 		if name == "" {
@@ -232,19 +248,20 @@ func TestParameters(t *testing.T) {
 			filepath.Join(workdir, "beta", "b.proto"),
 		})
 
-		var aGen string
+		contents := make(map[string]string)
 		gotFiles := make(map[string]bool)
 		outdir := filepath.Join(workdir, "out")
 		filepath.Walk(outdir, func(p string, info os.FileInfo, _ error) error {
 			if info.IsDir() {
 				return nil
 			}
-			if filepath.Base(p) == "a.pb.go" {
+			base := filepath.Base(p)
+			if base == "a.pb.go" || base == "b.pb.go" {
 				b, err := ioutil.ReadFile(p)
 				if err != nil {
 					t.Fatal(err)
 				}
-				aGen = string(b)
+				contents[base] = string(b)
 			}
 			relPath, _ := filepath.Rel(outdir, p)
 			gotFiles[relPath] = true
@@ -266,10 +283,20 @@ func TestParameters(t *testing.T) {
 				t.Errorf("missing output file:    %v", want)
 			}
 		}
-		gotImports, err := parseImports(aGen)
+		gotPackageA, gotImports, err := parseFile(contents["a.pb.go"])
 		if err != nil {
 			t.Fatal(err)
 		}
+		gotPackageB, _, err := parseFile(contents["b.pb.go"])
+		if err != nil {
+			t.Fatal(err)
+		}
+		if got, want := gotPackageA, test.wantPackageA; want != got {
+			t.Errorf("output file a.pb.go is package %q, want %q", got, want)
+		}
+		if got, want := gotPackageB, test.wantPackageB; want != got {
+			t.Errorf("output file b.pb.go is package %q, want %q", got, want)
+		}
 		missingImport := false
 	WantImport:
 		for want := range test.wantImportsA {
@@ -348,18 +375,17 @@ func TestPackageComment(t *testing.T) {
 	}
 }
 
-// parseImports returns a list of all packages imported by a file.
-func parseImports(source string) ([]string, error) {
+// parseFile returns a file's package name and a list of all packages it imports.
+func parseFile(source string) (packageName string, imports []string, err error) {
 	fset := token.NewFileSet()
 	f, err := parser.ParseFile(fset, "<source>", source, parser.ImportsOnly)
 	if err != nil {
-		return nil, err
+		return "", nil, err
 	}
-	var imports []string
 	for _, imp := range f.Imports {
 		imports = append(imports, imp.Path.Value)
 	}
-	return imports, nil
+	return f.Name.Name, imports, nil
 }
 
 func protoc(t *testing.T, args []string) {