From 71d6df1979ddf38b4050482ef555b9f1dfead16b Mon Sep 17 00:00:00 2001 From: Guillaume George Date: Tue, 17 Mar 2026 16:15:04 +0900 Subject: [PATCH 1/2] Use relative imports for same-package pb2 modules When a twirp file imports a pb2 module from the same package directory, generate a relative import (from . import v1_pb2) instead of an absolute one (from crypto import v1_pb2). This fixes ModuleNotFoundError when generated files are nested under a different top-level package. Cross-package imports (e.g. google.protobuf) remain absolute. Co-Authored-By: Claude Opus 4.6 (1M context) --- protoc-gen-twirpy/generator/generator.go | 2 +- protoc-gen-twirpy/generator/generator_test.go | 14 ++++++++++---- protoc-gen-twirpy/generator/import_builder.go | 18 +++++++++++++++++- 3 files changed, 28 insertions(+), 6 deletions(-) diff --git a/protoc-gen-twirpy/generator/generator.go b/protoc-gen-twirpy/generator/generator.go index ccf1153..c8f6663 100644 --- a/protoc-gen-twirpy/generator/generator.go +++ b/protoc-gen-twirpy/generator/generator.go @@ -71,7 +71,7 @@ func buildTwirpServiceDescription(messagesToFiles map[string]string, fd *descrip FileName: name, } - imports := newImportBuilder(messagesToFiles) + imports := newImportBuilder(messagesToFiles, name) for _, service := range fd.GetService() { serviceURL := fmt.Sprintf("%s.%s", fd.GetPackage(), service.GetName()) diff --git a/protoc-gen-twirpy/generator/generator_test.go b/protoc-gen-twirpy/generator/generator_test.go index dac96ab..8077bf2 100644 --- a/protoc-gen-twirpy/generator/generator_test.go +++ b/protoc-gen-twirpy/generator/generator_test.go @@ -9,13 +9,14 @@ import ( ) func TestImportBuilder(t *testing.T) { + // The current file being generated is in twirp/twitch/example/ testImportBuilder := newImportBuilder(map[string]string{ "twirp.twitch.example.Hat": "twirp/twitch/example/haberdasher.proto", "twirp.twitch.example.Price": "twirp/twitch/example/haberdasher.proto", "twirp.twitch.example.Color": "twirp/twitch/example_folder/haberdasher.proto", "twirp.twitch.example.Size": "twirp/twitch/example/haberdasher_extension.proto", "google.protobuf.Empty": "google/protobuf/empty.proto", - }) + }, "twirp/twitch/example/service.proto") testCases := []struct { typeToImport string @@ -24,26 +25,29 @@ func TestImportBuilder(t *testing.T) { expectedImport *TwirpImport }{ { + // Same package → relative import typeToImport: "twirp.twitch.example.Hat", importKey: "twirp.twitch.example.haberdasher_pb2", qualifiedImport: "_haberdasher_pb2.Hat", expectedImport: &TwirpImport{ - From: "twirp.twitch.example", + From: ".", Import: "haberdasher_pb2", Alias: "_haberdasher_pb2", }, }, { + // Same package, same file → reuses existing import typeToImport: "twirp.twitch.example.Price", importKey: "twirp.twitch.example.haberdasher_pb2", qualifiedImport: "_haberdasher_pb2.Price", expectedImport: &TwirpImport{ - From: "twirp.twitch.example", + From: ".", Import: "haberdasher_pb2", Alias: "_haberdasher_pb2", }, }, { + // Different package → absolute import typeToImport: "twirp.twitch.example.Color", importKey: "twirp.twitch.example_folder.haberdasher_pb2", qualifiedImport: "_haberdasher_pb2_1.Color", @@ -54,16 +58,18 @@ func TestImportBuilder(t *testing.T) { }, }, { + // Same package, different file → relative import typeToImport: "twirp.twitch.example.Size", importKey: "twirp.twitch.example.haberdasher_extension_pb2", qualifiedImport: "_haberdasher_extension_pb2.Size", expectedImport: &TwirpImport{ - From: "twirp.twitch.example", + From: ".", Import: "haberdasher_extension_pb2", Alias: "_haberdasher_extension_pb2", }, }, { + // External package → absolute import typeToImport: "google.protobuf.Empty", importKey: "google.protobuf.empty_pb2", qualifiedImport: "_empty_pb2.Empty", diff --git a/protoc-gen-twirpy/generator/import_builder.go b/protoc-gen-twirpy/generator/import_builder.go index 714b0da..658b698 100644 --- a/protoc-gen-twirpy/generator/import_builder.go +++ b/protoc-gen-twirpy/generator/import_builder.go @@ -9,13 +9,25 @@ type importBuilder struct { seenAliases map[string]struct{} imports map[string]*TwirpImport messagesToFile map[string]string + currentFileDir string } -func newImportBuilder(messagesToFile map[string]string) *importBuilder { +func newImportBuilder(messagesToFile map[string]string, currentFileName string) *importBuilder { + // Extract directory from proto filename: "crypto/v1.proto" -> "crypto" + dir := "" + parts := strings.Split(currentFileName, "/") + if len(parts) > 1 { + dirParts := parts[:len(parts)-1] + for i, p := range dirParts { + dirParts[i] = strings.ReplaceAll(p, "-", "_") + } + dir = strings.Join(dirParts, ".") + } return &importBuilder{ messagesToFile: messagesToFile, seenAliases: make(map[string]struct{}), imports: make(map[string]*TwirpImport), + currentFileDir: dir, } } @@ -34,6 +46,10 @@ func (ib *importBuilder) addImportAndQualify(typeToImport string) (string, error moduleNameSlice := strings.Split(moduleName, ".") strippedModuleName := moduleNameSlice[len(moduleNameSlice)-1] modulePath := strings.Join(moduleNameSlice[:len(moduleNameSlice)-1], ".") + // Use relative import when the module is in the same package as the file being generated + if modulePath == ib.currentFileDir { + modulePath = "." + } alias := ib.generateAlias(strippedModuleName) ib.imports[moduleName] = &TwirpImport{ From: modulePath, From fa04008420214701261aec4f01ce5a7c83a7e405 Mon Sep 17 00:00:00 2001 From: Emilien Kenler Date: Wed, 18 Mar 2026 06:47:29 +0900 Subject: [PATCH 2/2] Fix "" == "" bug and add edge-case tests for relative imports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root-level proto files produced an empty currentFileDir. Any dependency also at the root would have an empty modulePath, causing `"" == ""` to incorrectly emit `from . import` instead of a plain absolute import. Guard the comparison with `modulePath != ""` so relative imports are only used when both sides name an actual package. Also add tests for: - root-level files (the bug case) - hyphenated directory names (hyphen→underscore normalisation) Co-Authored-By: Claude Sonnet 4.6 --- protoc-gen-twirpy/generator/generator_test.go | 36 +++++++++++++++++++ protoc-gen-twirpy/generator/import_builder.go | 6 ++-- 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/protoc-gen-twirpy/generator/generator_test.go b/protoc-gen-twirpy/generator/generator_test.go index 8077bf2..fb39493 100644 --- a/protoc-gen-twirpy/generator/generator_test.go +++ b/protoc-gen-twirpy/generator/generator_test.go @@ -90,3 +90,39 @@ func TestImportBuilder(t *testing.T) { }) } } + +func TestImportBuilderRootLevel(t *testing.T) { + // currentFileName has no directory component: currentFileDir must be "" + // and root-level dependencies must produce absolute imports, not "." + ib := newImportBuilder(map[string]string{ + "mypackage.Foo": "types.proto", + }, "service.proto") + + qualified, err := ib.addImportAndQualify("mypackage.Foo") + assert.NoError(t, err) + assert.Equal(t, "_types_pb2.Foo", qualified) + // From must be "" (absolute import), not "." (relative) + assert.Equal(t, &TwirpImport{ + From: "", + Import: "types_pb2", + Alias: "_types_pb2", + }, ib.imports["types_pb2"]) +} + +func TestImportBuilderHyphenatedDirectory(t *testing.T) { + // Hyphens in directory names are normalised to underscores so that the + // directory string matches the Python module path produced by getModuleName. + ib := newImportBuilder(map[string]string{ + "my_org.my_service.Request": "my-org/my-service/types.proto", + }, "my-org/my-service/service.proto") + + qualified, err := ib.addImportAndQualify("my_org.my_service.Request") + assert.NoError(t, err) + assert.Equal(t, "_types_pb2.Request", qualified) + // Same package after hyphen normalisation → relative import + assert.Equal(t, &TwirpImport{ + From: ".", + Import: "types_pb2", + Alias: "_types_pb2", + }, ib.imports["my_org.my_service.types_pb2"]) +} diff --git a/protoc-gen-twirpy/generator/import_builder.go b/protoc-gen-twirpy/generator/import_builder.go index 658b698..ae7e29c 100644 --- a/protoc-gen-twirpy/generator/import_builder.go +++ b/protoc-gen-twirpy/generator/import_builder.go @@ -46,8 +46,10 @@ func (ib *importBuilder) addImportAndQualify(typeToImport string) (string, error moduleNameSlice := strings.Split(moduleName, ".") strippedModuleName := moduleNameSlice[len(moduleNameSlice)-1] modulePath := strings.Join(moduleNameSlice[:len(moduleNameSlice)-1], ".") - // Use relative import when the module is in the same package as the file being generated - if modulePath == ib.currentFileDir { + // Use relative import when the module is in the same package as the file being generated. + // Require modulePath to be non-empty: an empty modulePath (root-level module) would + // otherwise match an empty currentFileDir and incorrectly emit a "." relative import. + if modulePath != "" && modulePath == ib.currentFileDir { modulePath = "." } alias := ib.generateAlias(strippedModuleName)