From 069ce315cb874be3e49d56d8ac62825b98e8001f Mon Sep 17 00:00:00 2001 From: George Barnett Date: Tue, 19 Aug 2025 15:20:52 +0100 Subject: [PATCH 1/3] Only import SwiftProtobuf when WKTs are used as input/output Modification: The code generator imports SwiftProtobuf if the file being generated has a dependency on any of the protos bundled by SwiftProtobuf. This can yield unnecessary imports which may warn if used in conjunction with access levels on imports. The problem is that the grpc generator checks for dependencies on the FileDescriptor which may include messages defined in the same file as a service. If any of those messages depend on a well known type (WKT) then an import will be added even though the code being generated doesn't include message code. Modifications: - Only include a SwiftProtobuf import if an input or output type for an RPC is a bundled proto. - Fix a test where a WKT is used in a message but not as an input/output type for an RPC. Result: Fewer warnings --- .../ProtobufCodeGenParser.swift | 17 ++++++++++++++--- .../Generated/test-service.pb | Bin 6990 -> 6985 bytes .../ProtobufCodeGenParserTests.swift | 3 +-- dev/protos/local/test-service.proto | 2 +- 4 files changed, 16 insertions(+), 6 deletions(-) diff --git a/Sources/GRPCProtobufCodeGen/ProtobufCodeGenParser.swift b/Sources/GRPCProtobufCodeGen/ProtobufCodeGenParser.swift index dfddec6..00a2e90 100644 --- a/Sources/GRPCProtobufCodeGen/ProtobufCodeGenParser.swift +++ b/Sources/GRPCProtobufCodeGen/ProtobufCodeGenParser.swift @@ -110,15 +110,26 @@ extension ProtobufCodeGenParser { var codeDependencies: [Dependency] = [ Dependency(module: self.moduleNames.grpcProtobuf, accessLevel: .internal) ] + // If there's a dependency on a bundled proto then add the SwiftProtobuf import. // // Importing SwiftProtobuf unconditionally results in warnings in the generated // code if access-levels are used on imports and no bundled protos are used. - let dependsOnBundledProto = file.dependencies.contains { descriptor in - SwiftProtobufInfo.isBundledProto(file: descriptor) + // + // The import is only needed if a bundled proto is used as the input or output type + // for an RPC. The file may have a dependency on a bundled proto without requiring + // the SwiftProtobuf import (e.g. a message depending on a bundle proto may be + // defined in the same file as the service, the dependency will be in the '.pb.swift' + // along with the message code). + let needsProtobufImport = file.services.contains { service in + service.methods.contains { method in + let inputIsBundled = SwiftProtobufInfo.isBundledProto(file: method.inputType.file) + let outputIsBundled = SwiftProtobufInfo.isBundledProto(file: method.outputType.file) + return inputIsBundled || outputIsBundled + } } - if dependsOnBundledProto { + if needsProtobufImport { let dependency = Dependency( module: self.moduleNames.swiftProtobuf, accessLevel: self.accessLevel diff --git a/Tests/GRPCProtobufCodeGenTests/Generated/test-service.pb b/Tests/GRPCProtobufCodeGenTests/Generated/test-service.pb index 507373b47fe9052518e55ac2fcbe71e28bbc4bab..58866e2671ef15a9c126bc7aef3a9a9ea3fe9f6e 100644 GIT binary patch delta 105 zcmX?ScG7Hvhd9%J_RU`6d5nzhlXprO`uTG)F*67;voR=3xhRBthbUwg=c$({loqF^ zC}id-I0kqsq^IVk7A2Md1(GTi(t`q=brg#874q^GGII;^i%Nhx6jCb+Qj>uyHrq-H GG64XQG$4im delta 110 zcmX?UcFt^rhd5IU$7V0_JVwTz$vY(sgG0EOm>Gna*%*|iJQYHVGxO3F5*5O|Lln~T zi;`1|6%z9ll!D7M(@Fx0@=NlQO4F1SO7azwQWY|D3-XIfQd1N%^Ayrk^HPfvL7e3L Ll+?`@l7dVCIg}!^ diff --git a/Tests/GRPCProtobufCodeGenTests/ProtobufCodeGenParserTests.swift b/Tests/GRPCProtobufCodeGenTests/ProtobufCodeGenParserTests.swift index 4c31520..2df601d 100644 --- a/Tests/GRPCProtobufCodeGenTests/ProtobufCodeGenParserTests.swift +++ b/Tests/GRPCProtobufCodeGenTests/ProtobufCodeGenParserTests.swift @@ -66,8 +66,7 @@ struct ProtobufCodeGenParserTests { @available(gRPCSwiftProtobuf 2.0, *) func dependencies() throws { let expected: [GRPCCodeGen.Dependency] = [ - .init(module: "GRPCProtobuf", accessLevel: .internal), // Always an internal import - .init(module: "SwiftProtobuf", accessLevel: .internal), + .init(module: "GRPCProtobuf", accessLevel: .internal) // Always an internal import ] #expect(try self.codeGen.dependencies == expected) } diff --git a/dev/protos/local/test-service.proto b/dev/protos/local/test-service.proto index 2ff7430..aaf6a66 100644 --- a/dev/protos/local/test-service.proto +++ b/dev/protos/local/test-service.proto @@ -3,7 +3,7 @@ syntax = "proto3"; package test; -// Using a WKT forces an "SwiftProtobuf" to be imported in generated code. +// WKT isn't used in API generated by gRPC, so no import is expected. import "google/protobuf/any.proto"; // Service docs. From 8351d93f31f32820f718d16ecb54db00cd48a4a2 Mon Sep 17 00:00:00 2001 From: George Barnett Date: Tue, 19 Aug 2025 16:34:30 +0100 Subject: [PATCH 2/3] Fix module naming --- Sources/protoc-gen-grpc-swift-2/Options.swift | 29 ++++++++++++----- .../ProtobufCodeGenParserTests.swift | 32 +++++++++++++++++++ .../ProtobufCodeGeneratorTests.swift | 30 ----------------- 3 files changed, 53 insertions(+), 38 deletions(-) diff --git a/Sources/protoc-gen-grpc-swift-2/Options.swift b/Sources/protoc-gen-grpc-swift-2/Options.swift index a068a53..c0da484 100644 --- a/Sources/protoc-gen-grpc-swift-2/Options.swift +++ b/Sources/protoc-gen-grpc-swift-2/Options.swift @@ -62,6 +62,8 @@ struct GeneratorOptions { } init(pairs: [(key: String, value: String)]) throws { + var moduleMapPath: String? + for pair in pairs { switch pair.key { case "Visibility": @@ -87,14 +89,7 @@ struct GeneratorOptions { case "ProtoPathModuleMappings": if !pair.value.isEmpty { - do { - self.protoToModuleMappings = try ProtoFileToModuleMappings(path: pair.value) - } catch let e { - throw GenerationError.wrappedError( - message: "Parameter 'ProtoPathModuleMappings=\(pair.value)'", - error: e - ) - } + moduleMapPath = pair.value } case "FileNaming": @@ -164,6 +159,24 @@ struct GeneratorOptions { throw GenerationError.unknownParameter(name: pair.key) } } + + if let moduleMapPath = moduleMapPath { + do { + self.protoToModuleMappings = try ProtoFileToModuleMappings( + path: moduleMapPath, + swiftProtobufModuleName: self.config.moduleNames.swiftProtobuf + ) + } catch let e { + throw GenerationError.wrappedError( + message: "Parameter 'ProtoPathModuleMappings=\(moduleMapPath)'", + error: e + ) + } + } else { + self.protoToModuleMappings = ProtoFileToModuleMappings( + swiftProtobufModuleName: self.config.moduleNames.swiftProtobuf + ) + } } static func parseParameter(string: String?) -> [(key: String, value: String)] { diff --git a/Tests/GRPCProtobufCodeGenTests/ProtobufCodeGenParserTests.swift b/Tests/GRPCProtobufCodeGenTests/ProtobufCodeGenParserTests.swift index 2df601d..99ad2e9 100644 --- a/Tests/GRPCProtobufCodeGenTests/ProtobufCodeGenParserTests.swift +++ b/Tests/GRPCProtobufCodeGenTests/ProtobufCodeGenParserTests.swift @@ -265,5 +265,37 @@ struct ProtobufCodeGenParserTests { ] #expect(codeGen.dependencies == expected) } + + @Test("Generate with different module names") + @available(gRPCSwiftProtobuf 2.0, *) + func generateWithDifferentModuleNames() throws { + var config = ProtobufCodeGenerator.Config.defaults + let defaultNames = config.moduleNames + + config.accessLevel = .public + config.indentation = 2 + config.moduleNames.grpcCore = String(config.moduleNames.grpcCore.reversed()) + config.moduleNames.grpcProtobuf = String(config.moduleNames.grpcProtobuf.reversed()) + config.moduleNames.swiftProtobuf = String(config.moduleNames.swiftProtobuf.reversed()) + + let generator = ProtobufCodeGenerator(config: config) + let generated = try generator.generateCode( + fileDescriptor: Self.fileDescriptor, + protoFileModuleMappings: ProtoFileToModuleMappings( + swiftProtobufModuleName: config.moduleNames.swiftProtobuf + ), + extraModuleImports: [] + ) + + // Mustn't contain the default names. + #expect(!generated.contains(defaultNames.grpcCore)) + #expect(!generated.contains(defaultNames.grpcProtobuf)) + #expect(!generated.contains(defaultNames.swiftProtobuf)) + + // Must contain the configured names. + #expect(generated.contains(config.moduleNames.grpcCore)) + #expect(generated.contains(config.moduleNames.grpcProtobuf)) + #expect(generated.contains(config.moduleNames.swiftProtobuf)) + } } } diff --git a/Tests/GRPCProtobufCodeGenTests/ProtobufCodeGeneratorTests.swift b/Tests/GRPCProtobufCodeGenTests/ProtobufCodeGeneratorTests.swift index c4fdc62..1a85158 100644 --- a/Tests/GRPCProtobufCodeGenTests/ProtobufCodeGeneratorTests.swift +++ b/Tests/GRPCProtobufCodeGenTests/ProtobufCodeGeneratorTests.swift @@ -99,7 +99,6 @@ struct ProtobufCodeGeneratorTests { import GRPCCore import GRPCProtobuf - import SwiftProtobuf // MARK: - test.TestService @@ -1103,35 +1102,6 @@ struct ProtobufCodeGeneratorTests { #expect(generated == expected) } - @Test("Generate with different module names") - @available(gRPCSwiftProtobuf 2.0, *) - func generateWithDifferentModuleNames() throws { - var config = ProtobufCodeGenerator.Config.defaults - let defaultNames = config.moduleNames - - config.accessLevel = .public - config.indentation = 2 - config.moduleNames.grpcCore = String(config.moduleNames.grpcCore.reversed()) - config.moduleNames.grpcProtobuf = String(config.moduleNames.grpcProtobuf.reversed()) - config.moduleNames.swiftProtobuf = String(config.moduleNames.swiftProtobuf.reversed()) - - let generator = ProtobufCodeGenerator(config: config) - let generated = try generator.generateCode( - fileDescriptor: Self.fileDescriptor, - protoFileModuleMappings: ProtoFileToModuleMappings(), - extraModuleImports: [] - ) - - // Mustn't contain the default names. - #expect(!generated.contains(defaultNames.grpcCore)) - #expect(!generated.contains(defaultNames.grpcProtobuf)) - #expect(!generated.contains(defaultNames.swiftProtobuf)) - - // Must contain the configured names. - #expect(generated.contains(config.moduleNames.grpcCore)) - #expect(generated.contains(config.moduleNames.grpcProtobuf)) - #expect(generated.contains(config.moduleNames.swiftProtobuf)) - } } @Suite("File-without-services (foo-messages.proto)") From 3cee501c5800c40a29dc840da9fa5c6d40d7aed5 Mon Sep 17 00:00:00 2001 From: George Barnett Date: Tue, 19 Aug 2025 16:38:29 +0100 Subject: [PATCH 3/3] format --- Sources/protoc-gen-grpc-swift-2/Options.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/protoc-gen-grpc-swift-2/Options.swift b/Sources/protoc-gen-grpc-swift-2/Options.swift index c0da484..6c82334 100644 --- a/Sources/protoc-gen-grpc-swift-2/Options.swift +++ b/Sources/protoc-gen-grpc-swift-2/Options.swift @@ -89,7 +89,7 @@ struct GeneratorOptions { case "ProtoPathModuleMappings": if !pair.value.isEmpty { - moduleMapPath = pair.value + moduleMapPath = pair.value } case "FileNaming":