From e4f5491018b939582cd9cbc2588471e5d297370e Mon Sep 17 00:00:00 2001 From: Thomas Van Lenten Date: Mon, 8 Jan 2024 14:10:51 -0500 Subject: [PATCH] Revise how modules imports are done to better support `import public` Historically Swift didn't have a good way to model `import public` in .proto files. But by using `@_exported import`, we can get the majority of the way there. This makes restructuring of proto files less likely to break Swift source code; meaning Swift is less of a "special case" compared to other languages. If generating with `public` or `package` visibility, then instead of importing the whole module of for an `import public`, explicitly re-export the imports of the types from that file. That will make them usable for the given source file but will also let the types continue to be used by someone just importing this module. This also means a file with no `import public` usages becomes must simpler as it can just rely on its direct dependencies to provide all the types that could be used. --- .../imports_a_publicly.pb.swift | 10 +- .../imports_imports_a_publicly.pb.swift | 12 +- .../MultiModule/Sources/ModuleA/a.pb.swift | 59 +++- .../MultiModule/Tests/Test1/test1.swift | 2 +- .../Tests/Test1/uses_a_transitively.pb.swift | 6 +- .../MultiModule/Tests/Test2/test2.swift | 2 +- .../Tests/Test2/uses_a_transitively2.pb.swift | 7 +- .../MultiModule/Sources/ModuleA/a.proto | 5 + .../imports_a_publicly.pb.swift | 10 +- .../imports_imports_a_publicly.pb.swift | 12 +- .../MultiModule/Sources/ModuleA/a.pb.swift | 59 +++- .../Tests/Test1/uses_a_transitively.pb.swift | 6 +- .../Tests/Test2/uses_a_transitively2.pb.swift | 7 +- .../ProtoFileToModuleMappings.swift | 8 + .../SwiftProtobufNamer.swift | 4 +- .../Descriptor+TestHelpers.swift | 9 +- .../Descriptor+Extensions.swift | 144 ++++++++++ Sources/protoc-gen-swift/FileGenerator.swift | 23 +- .../Test_DescriptorExtensions.swift | 269 ++++++++++++++++++ 19 files changed, 618 insertions(+), 36 deletions(-) diff --git a/CompileTests/MultiModule/Sources/ImportsAPublicly/imports_a_publicly.pb.swift b/CompileTests/MultiModule/Sources/ImportsAPublicly/imports_a_publicly.pb.swift index 556b2ed19c..99f4f3e881 100644 --- a/CompileTests/MultiModule/Sources/ImportsAPublicly/imports_a_publicly.pb.swift +++ b/CompileTests/MultiModule/Sources/ImportsAPublicly/imports_a_publicly.pb.swift @@ -10,7 +10,10 @@ import Foundation import SwiftProtobuf -import ModuleA +// Use of 'import public' causes re-exports: +@_exported import enum ModuleA.E +@_exported import let ModuleA.Extensions_ext_str +@_exported import struct ModuleA.A // If the compiler emits an error on this type, it is because this file // was generated by a version of the `protoc` Swift plug-in that is @@ -62,6 +65,11 @@ extension ImportsAPublicly: SwiftProtobuf.Message, SwiftProtobuf._MessageImpleme 12: .same(proto: "e"), ] + public var isInitialized: Bool { + if let v = self._a, !v.isInitialized {return false} + return true + } + public mutating func decodeMessage(decoder: inout D) throws { while let fieldNumber = try decoder.nextFieldNumber() { // The use of inline closures is to circumvent an issue where the compiler diff --git a/CompileTests/MultiModule/Sources/ImportsImportsAPublicly/imports_imports_a_publicly.pb.swift b/CompileTests/MultiModule/Sources/ImportsImportsAPublicly/imports_imports_a_publicly.pb.swift index 8a345f5d01..7e1ec87394 100644 --- a/CompileTests/MultiModule/Sources/ImportsImportsAPublicly/imports_imports_a_publicly.pb.swift +++ b/CompileTests/MultiModule/Sources/ImportsImportsAPublicly/imports_imports_a_publicly.pb.swift @@ -10,8 +10,11 @@ import Foundation import SwiftProtobuf -import ImportsAPublicly -import ModuleA +// Use of 'import public' causes re-exports: +@_exported import enum ModuleA.E +@_exported import let ModuleA.Extensions_ext_str +@_exported import struct ImportsAPublicly.ImportsAPublicly +@_exported import struct ModuleA.A // If the compiler emits an error on this type, it is because this file // was generated by a version of the `protoc` Swift plug-in that is @@ -63,6 +66,11 @@ extension ImportsImportsAPublicly: SwiftProtobuf.Message, SwiftProtobuf._Message 22: .same(proto: "e"), ] + public var isInitialized: Bool { + if let v = self._a, !v.isInitialized {return false} + return true + } + public mutating func decodeMessage(decoder: inout D) throws { while let fieldNumber = try decoder.nextFieldNumber() { // The use of inline closures is to circumvent an issue where the compiler diff --git a/CompileTests/MultiModule/Sources/ModuleA/a.pb.swift b/CompileTests/MultiModule/Sources/ModuleA/a.pb.swift index e456acc57e..165a10475b 100644 --- a/CompileTests/MultiModule/Sources/ModuleA/a.pb.swift +++ b/CompileTests/MultiModule/Sources/ModuleA/a.pb.swift @@ -49,7 +49,7 @@ public enum E: SwiftProtobuf.Enum { } -public struct A: Sendable { +public struct A: SwiftProtobuf.ExtensibleMessage, Sendable { // SwiftProtobuf.Message conformance is added in an extension below. See the // `Message` and `Message+*Additions` files in the SwiftProtobuf library for // methods supported on all messages. @@ -67,9 +67,57 @@ public struct A: Sendable { public init() {} + public var _protobuf_extensionFieldValues = SwiftProtobuf.ExtensionFieldValueSet() fileprivate var _e: E? = nil } +// MARK: - Extension support defined in a.proto. + +// MARK: - Extension Properties + +// Swift Extensions on the exteneded Messages to add easy access to the declared +// extension fields. The names are based on the extension field name from the proto +// declaration. To avoid naming collisions, the names are prefixed with the name of +// the scope where the extend directive occurs. + +extension A { + + public var extStr: String { + get {return getExtensionValue(ext: Extensions_ext_str) ?? String()} + set {setExtensionValue(ext: Extensions_ext_str, value: newValue)} + } + /// Returns true if extension `Extensions_ext_str` + /// has been explicitly set. + public var hasExtStr: Bool { + return hasExtensionValue(ext: Extensions_ext_str) + } + /// Clears the value of extension `Extensions_ext_str`. + /// Subsequent reads from it will return its default value. + public mutating func clearExtStr() { + clearExtensionValue(ext: Extensions_ext_str) + } + +} + +// MARK: - File's ExtensionMap: A_Extensions + +/// A `SwiftProtobuf.SimpleExtensionMap` that includes all of the extensions defined by +/// this .proto file. It can be used any place an `SwiftProtobuf.ExtensionMap` is needed +/// in parsing, or it can be combined with other `SwiftProtobuf.SimpleExtensionMap`s to create +/// a larger `SwiftProtobuf.SimpleExtensionMap`. +public let A_Extensions: SwiftProtobuf.SimpleExtensionMap = [ + Extensions_ext_str +] + +// Extension Objects - The only reason these might be needed is when manually +// constructing a `SimpleExtensionMap`, otherwise, use the above _Extension Properties_ +// accessors for the extension fields on the messages directly. + +public let Extensions_ext_str = SwiftProtobuf.MessageExtension, A>( + _protobuf_fieldNumber: 100, + fieldName: "ext_str" +) + // MARK: - Code below here is support for the SwiftProtobuf runtime. extension E: SwiftProtobuf._ProtoNameProviding { @@ -86,6 +134,11 @@ extension A: SwiftProtobuf.Message, SwiftProtobuf._MessageImplementationBase, Sw 1: .same(proto: "e"), ] + public var isInitialized: Bool { + if !_protobuf_extensionFieldValues.isInitialized {return false} + return true + } + public mutating func decodeMessage(decoder: inout D) throws { while let fieldNumber = try decoder.nextFieldNumber() { // The use of inline closures is to circumvent an issue where the compiler @@ -93,6 +146,8 @@ extension A: SwiftProtobuf.Message, SwiftProtobuf._MessageImplementationBase, Sw // enabled. https://github.com/apple/swift-protobuf/issues/1034 switch fieldNumber { case 1: try { try decoder.decodeSingularEnumField(value: &self._e) }() + case 100..<1001: + try { try decoder.decodeExtensionField(values: &_protobuf_extensionFieldValues, messageType: A.self, fieldNumber: fieldNumber) }() default: break } } @@ -106,12 +161,14 @@ extension A: SwiftProtobuf.Message, SwiftProtobuf._MessageImplementationBase, Sw try { if let v = self._e { try visitor.visitSingularEnumField(value: v, fieldNumber: 1) } }() + try visitor.visitExtensionFields(fields: _protobuf_extensionFieldValues, start: 100, end: 1001) try unknownFields.traverse(visitor: &visitor) } public static func ==(lhs: A, rhs: A) -> Bool { if lhs._e != rhs._e {return false} if lhs.unknownFields != rhs.unknownFields {return false} + if lhs._protobuf_extensionFieldValues != rhs._protobuf_extensionFieldValues {return false} return true } } diff --git a/CompileTests/MultiModule/Tests/Test1/test1.swift b/CompileTests/MultiModule/Tests/Test1/test1.swift index f186277bda..474a7c09cb 100644 --- a/CompileTests/MultiModule/Tests/Test1/test1.swift +++ b/CompileTests/MultiModule/Tests/Test1/test1.swift @@ -1,5 +1,5 @@ import ImportsAPublicly -import ModuleA // Needed because `import public` doesn't help Swift +// Don't need to import ModuleA because of the file being a `import public` import XCTest diff --git a/CompileTests/MultiModule/Tests/Test1/uses_a_transitively.pb.swift b/CompileTests/MultiModule/Tests/Test1/uses_a_transitively.pb.swift index 3bb4d6701f..e88624091a 100644 --- a/CompileTests/MultiModule/Tests/Test1/uses_a_transitively.pb.swift +++ b/CompileTests/MultiModule/Tests/Test1/uses_a_transitively.pb.swift @@ -11,7 +11,6 @@ import Foundation import SwiftProtobuf import ImportsAPublicly -import ModuleA // If the compiler emits an error on this type, it is because this file // was generated by a version of the `protoc` Swift plug-in that is @@ -63,6 +62,11 @@ extension UsesATransitively: SwiftProtobuf.Message, SwiftProtobuf._MessageImplem 102: .same(proto: "e"), ] + public var isInitialized: Bool { + if let v = self._a, !v.isInitialized {return false} + return true + } + public mutating func decodeMessage(decoder: inout D) throws { while let fieldNumber = try decoder.nextFieldNumber() { // The use of inline closures is to circumvent an issue where the compiler diff --git a/CompileTests/MultiModule/Tests/Test2/test2.swift b/CompileTests/MultiModule/Tests/Test2/test2.swift index e20ebd1303..45d62fbb55 100644 --- a/CompileTests/MultiModule/Tests/Test2/test2.swift +++ b/CompileTests/MultiModule/Tests/Test2/test2.swift @@ -1,5 +1,5 @@ import ImportsImportsAPublicly -import ModuleA // Needed because `import public` doesn't help Swift +// Don't need to import ModuleA because of the file being a `import public` import XCTest diff --git a/CompileTests/MultiModule/Tests/Test2/uses_a_transitively2.pb.swift b/CompileTests/MultiModule/Tests/Test2/uses_a_transitively2.pb.swift index 7dd0c210e4..6c056cb8a1 100644 --- a/CompileTests/MultiModule/Tests/Test2/uses_a_transitively2.pb.swift +++ b/CompileTests/MultiModule/Tests/Test2/uses_a_transitively2.pb.swift @@ -10,9 +10,7 @@ import Foundation import SwiftProtobuf -import ImportsAPublicly import ImportsImportsAPublicly -import ModuleA // If the compiler emits an error on this type, it is because this file // was generated by a version of the `protoc` Swift plug-in that is @@ -64,6 +62,11 @@ extension UsesATransitively2: SwiftProtobuf.Message, SwiftProtobuf._MessageImple 122: .same(proto: "e"), ] + public var isInitialized: Bool { + if let v = self._a, !v.isInitialized {return false} + return true + } + public mutating func decodeMessage(decoder: inout D) throws { while let fieldNumber = try decoder.nextFieldNumber() { // The use of inline closures is to circumvent an issue where the compiler diff --git a/Protos/CompileTests/MultiModule/Sources/ModuleA/a.proto b/Protos/CompileTests/MultiModule/Sources/ModuleA/a.proto index 7a2ef44962..75c1506a79 100644 --- a/Protos/CompileTests/MultiModule/Sources/ModuleA/a.proto +++ b/Protos/CompileTests/MultiModule/Sources/ModuleA/a.proto @@ -6,4 +6,9 @@ enum E { message A { optional E e = 1; + extensions 100 to 1000; +} + +extend A { + optional string ext_str = 100; } diff --git a/Reference/CompileTests/MultiModule/Sources/ImportsAPublicly/imports_a_publicly.pb.swift b/Reference/CompileTests/MultiModule/Sources/ImportsAPublicly/imports_a_publicly.pb.swift index 556b2ed19c..99f4f3e881 100644 --- a/Reference/CompileTests/MultiModule/Sources/ImportsAPublicly/imports_a_publicly.pb.swift +++ b/Reference/CompileTests/MultiModule/Sources/ImportsAPublicly/imports_a_publicly.pb.swift @@ -10,7 +10,10 @@ import Foundation import SwiftProtobuf -import ModuleA +// Use of 'import public' causes re-exports: +@_exported import enum ModuleA.E +@_exported import let ModuleA.Extensions_ext_str +@_exported import struct ModuleA.A // If the compiler emits an error on this type, it is because this file // was generated by a version of the `protoc` Swift plug-in that is @@ -62,6 +65,11 @@ extension ImportsAPublicly: SwiftProtobuf.Message, SwiftProtobuf._MessageImpleme 12: .same(proto: "e"), ] + public var isInitialized: Bool { + if let v = self._a, !v.isInitialized {return false} + return true + } + public mutating func decodeMessage(decoder: inout D) throws { while let fieldNumber = try decoder.nextFieldNumber() { // The use of inline closures is to circumvent an issue where the compiler diff --git a/Reference/CompileTests/MultiModule/Sources/ImportsImportsAPublicly/imports_imports_a_publicly.pb.swift b/Reference/CompileTests/MultiModule/Sources/ImportsImportsAPublicly/imports_imports_a_publicly.pb.swift index 8a345f5d01..7e1ec87394 100644 --- a/Reference/CompileTests/MultiModule/Sources/ImportsImportsAPublicly/imports_imports_a_publicly.pb.swift +++ b/Reference/CompileTests/MultiModule/Sources/ImportsImportsAPublicly/imports_imports_a_publicly.pb.swift @@ -10,8 +10,11 @@ import Foundation import SwiftProtobuf -import ImportsAPublicly -import ModuleA +// Use of 'import public' causes re-exports: +@_exported import enum ModuleA.E +@_exported import let ModuleA.Extensions_ext_str +@_exported import struct ImportsAPublicly.ImportsAPublicly +@_exported import struct ModuleA.A // If the compiler emits an error on this type, it is because this file // was generated by a version of the `protoc` Swift plug-in that is @@ -63,6 +66,11 @@ extension ImportsImportsAPublicly: SwiftProtobuf.Message, SwiftProtobuf._Message 22: .same(proto: "e"), ] + public var isInitialized: Bool { + if let v = self._a, !v.isInitialized {return false} + return true + } + public mutating func decodeMessage(decoder: inout D) throws { while let fieldNumber = try decoder.nextFieldNumber() { // The use of inline closures is to circumvent an issue where the compiler diff --git a/Reference/CompileTests/MultiModule/Sources/ModuleA/a.pb.swift b/Reference/CompileTests/MultiModule/Sources/ModuleA/a.pb.swift index e456acc57e..165a10475b 100644 --- a/Reference/CompileTests/MultiModule/Sources/ModuleA/a.pb.swift +++ b/Reference/CompileTests/MultiModule/Sources/ModuleA/a.pb.swift @@ -49,7 +49,7 @@ public enum E: SwiftProtobuf.Enum { } -public struct A: Sendable { +public struct A: SwiftProtobuf.ExtensibleMessage, Sendable { // SwiftProtobuf.Message conformance is added in an extension below. See the // `Message` and `Message+*Additions` files in the SwiftProtobuf library for // methods supported on all messages. @@ -67,9 +67,57 @@ public struct A: Sendable { public init() {} + public var _protobuf_extensionFieldValues = SwiftProtobuf.ExtensionFieldValueSet() fileprivate var _e: E? = nil } +// MARK: - Extension support defined in a.proto. + +// MARK: - Extension Properties + +// Swift Extensions on the exteneded Messages to add easy access to the declared +// extension fields. The names are based on the extension field name from the proto +// declaration. To avoid naming collisions, the names are prefixed with the name of +// the scope where the extend directive occurs. + +extension A { + + public var extStr: String { + get {return getExtensionValue(ext: Extensions_ext_str) ?? String()} + set {setExtensionValue(ext: Extensions_ext_str, value: newValue)} + } + /// Returns true if extension `Extensions_ext_str` + /// has been explicitly set. + public var hasExtStr: Bool { + return hasExtensionValue(ext: Extensions_ext_str) + } + /// Clears the value of extension `Extensions_ext_str`. + /// Subsequent reads from it will return its default value. + public mutating func clearExtStr() { + clearExtensionValue(ext: Extensions_ext_str) + } + +} + +// MARK: - File's ExtensionMap: A_Extensions + +/// A `SwiftProtobuf.SimpleExtensionMap` that includes all of the extensions defined by +/// this .proto file. It can be used any place an `SwiftProtobuf.ExtensionMap` is needed +/// in parsing, or it can be combined with other `SwiftProtobuf.SimpleExtensionMap`s to create +/// a larger `SwiftProtobuf.SimpleExtensionMap`. +public let A_Extensions: SwiftProtobuf.SimpleExtensionMap = [ + Extensions_ext_str +] + +// Extension Objects - The only reason these might be needed is when manually +// constructing a `SimpleExtensionMap`, otherwise, use the above _Extension Properties_ +// accessors for the extension fields on the messages directly. + +public let Extensions_ext_str = SwiftProtobuf.MessageExtension, A>( + _protobuf_fieldNumber: 100, + fieldName: "ext_str" +) + // MARK: - Code below here is support for the SwiftProtobuf runtime. extension E: SwiftProtobuf._ProtoNameProviding { @@ -86,6 +134,11 @@ extension A: SwiftProtobuf.Message, SwiftProtobuf._MessageImplementationBase, Sw 1: .same(proto: "e"), ] + public var isInitialized: Bool { + if !_protobuf_extensionFieldValues.isInitialized {return false} + return true + } + public mutating func decodeMessage(decoder: inout D) throws { while let fieldNumber = try decoder.nextFieldNumber() { // The use of inline closures is to circumvent an issue where the compiler @@ -93,6 +146,8 @@ extension A: SwiftProtobuf.Message, SwiftProtobuf._MessageImplementationBase, Sw // enabled. https://github.com/apple/swift-protobuf/issues/1034 switch fieldNumber { case 1: try { try decoder.decodeSingularEnumField(value: &self._e) }() + case 100..<1001: + try { try decoder.decodeExtensionField(values: &_protobuf_extensionFieldValues, messageType: A.self, fieldNumber: fieldNumber) }() default: break } } @@ -106,12 +161,14 @@ extension A: SwiftProtobuf.Message, SwiftProtobuf._MessageImplementationBase, Sw try { if let v = self._e { try visitor.visitSingularEnumField(value: v, fieldNumber: 1) } }() + try visitor.visitExtensionFields(fields: _protobuf_extensionFieldValues, start: 100, end: 1001) try unknownFields.traverse(visitor: &visitor) } public static func ==(lhs: A, rhs: A) -> Bool { if lhs._e != rhs._e {return false} if lhs.unknownFields != rhs.unknownFields {return false} + if lhs._protobuf_extensionFieldValues != rhs._protobuf_extensionFieldValues {return false} return true } } diff --git a/Reference/CompileTests/MultiModule/Tests/Test1/uses_a_transitively.pb.swift b/Reference/CompileTests/MultiModule/Tests/Test1/uses_a_transitively.pb.swift index 3bb4d6701f..e88624091a 100644 --- a/Reference/CompileTests/MultiModule/Tests/Test1/uses_a_transitively.pb.swift +++ b/Reference/CompileTests/MultiModule/Tests/Test1/uses_a_transitively.pb.swift @@ -11,7 +11,6 @@ import Foundation import SwiftProtobuf import ImportsAPublicly -import ModuleA // If the compiler emits an error on this type, it is because this file // was generated by a version of the `protoc` Swift plug-in that is @@ -63,6 +62,11 @@ extension UsesATransitively: SwiftProtobuf.Message, SwiftProtobuf._MessageImplem 102: .same(proto: "e"), ] + public var isInitialized: Bool { + if let v = self._a, !v.isInitialized {return false} + return true + } + public mutating func decodeMessage(decoder: inout D) throws { while let fieldNumber = try decoder.nextFieldNumber() { // The use of inline closures is to circumvent an issue where the compiler diff --git a/Reference/CompileTests/MultiModule/Tests/Test2/uses_a_transitively2.pb.swift b/Reference/CompileTests/MultiModule/Tests/Test2/uses_a_transitively2.pb.swift index 7dd0c210e4..6c056cb8a1 100644 --- a/Reference/CompileTests/MultiModule/Tests/Test2/uses_a_transitively2.pb.swift +++ b/Reference/CompileTests/MultiModule/Tests/Test2/uses_a_transitively2.pb.swift @@ -10,9 +10,7 @@ import Foundation import SwiftProtobuf -import ImportsAPublicly import ImportsImportsAPublicly -import ModuleA // If the compiler emits an error on this type, it is because this file // was generated by a version of the `protoc` Swift plug-in that is @@ -64,6 +62,11 @@ extension UsesATransitively2: SwiftProtobuf.Message, SwiftProtobuf._MessageImple 122: .same(proto: "e"), ] + public var isInitialized: Bool { + if let v = self._a, !v.isInitialized {return false} + return true + } + public mutating func decodeMessage(decoder: inout D) throws { while let fieldNumber = try decoder.nextFieldNumber() { // The use of inline closures is to circumvent an issue where the compiler diff --git a/Sources/SwiftProtobufPluginLibrary/ProtoFileToModuleMappings.swift b/Sources/SwiftProtobufPluginLibrary/ProtoFileToModuleMappings.swift index df21faf33d..5b8549521d 100644 --- a/Sources/SwiftProtobufPluginLibrary/ProtoFileToModuleMappings.swift +++ b/Sources/SwiftProtobufPluginLibrary/ProtoFileToModuleMappings.swift @@ -132,6 +132,14 @@ public struct ProtoFileToModuleMappings { } } + // NOTE: This api is only used by gRPC (or things like it), with + // `import public` now re-exporting things, this likely can go away or just + // be reduced just the above loop, without the need for special casing the + // `import public` cases. It will come down to what should expectations + // be for protobuf messages, enums, and extensions with repsect to something + // that generates on top if it. i.e. - should they re-export things or + // should only the generated proto code do it? + // Protocol Buffers has the concept of "public imports", these are imports // into a file that expose everything from within the file to the new // context. From the docs - diff --git a/Sources/SwiftProtobufPluginLibrary/SwiftProtobufNamer.swift b/Sources/SwiftProtobufPluginLibrary/SwiftProtobufNamer.swift index 83942d7b29..0dab8324c8 100644 --- a/Sources/SwiftProtobufPluginLibrary/SwiftProtobufNamer.swift +++ b/Sources/SwiftProtobufPluginLibrary/SwiftProtobufNamer.swift @@ -17,8 +17,8 @@ import Foundation public final class SwiftProtobufNamer { var filePrefixCache = [String:String]() var enumValueRelativeNameCache = [String:String]() - var mappings: ProtoFileToModuleMappings - var targetModule: String + public let mappings: ProtoFileToModuleMappings + public let targetModule: String public var swiftProtobufModuleName: String { return mappings.swiftProtobufModuleName } diff --git a/Sources/SwiftProtobufTestHelpers/Descriptor+TestHelpers.swift b/Sources/SwiftProtobufTestHelpers/Descriptor+TestHelpers.swift index 263e988db0..817b853d46 100644 --- a/Sources/SwiftProtobufTestHelpers/Descriptor+TestHelpers.swift +++ b/Sources/SwiftProtobufTestHelpers/Descriptor+TestHelpers.swift @@ -11,12 +11,13 @@ import SwiftProtobuf public extension Google_Protobuf_FileDescriptorProto { - init(name: String, dependencies: [String] = [], publicDependencies: [Int32] = []) { + init(name: String, dependencies: [String] = [], publicDependencies: [Int32] = [], package: String = "") { for idx in publicDependencies { precondition(Int(idx) <= dependencies.count) } self.init() self.name = name - dependency = dependencies - publicDependency = publicDependencies + self.dependency = dependencies + self.publicDependency = publicDependencies + self.package = package } init(textFormatStrings: [String]) throws { let s = textFormatStrings.joined(separator: "\n") + "\n" @@ -27,7 +28,7 @@ public extension Google_Protobuf_FileDescriptorProto { public extension Google_Protobuf_FileDescriptorSet { init(files: [Google_Protobuf_FileDescriptorProto]) { self.init() - file = files + self.file = files } init(file: Google_Protobuf_FileDescriptorProto) { self.init() diff --git a/Sources/protoc-gen-swift/Descriptor+Extensions.swift b/Sources/protoc-gen-swift/Descriptor+Extensions.swift index 0cb60effe5..1c1ab0d402 100644 --- a/Sources/protoc-gen-swift/Descriptor+Extensions.swift +++ b/Sources/protoc-gen-swift/Descriptor+Extensions.swift @@ -14,6 +14,150 @@ extension FileDescriptor { var isBundledProto: Bool { return SwiftProtobufInfo.isBundledProto(file: self) } + + // Returns a string of any import lines for the give file based on the file's + // imports. The string may include multiple lines. + // + // Protocol Buffers has the concept of "public imports", these are imports + // into a file that expose everything from within the file to the new + // context. From the docs - + // https://protobuf.dev/programming-guides/proto/#importing + // `import public` dependencies can be transitively relied upon by anyone + // importing the proto containing the import public statement. + // To properly expose the types for use, it means in each file, the public + // imports from the dependencies (recursively) have to be hoisted and + // reexported. This way someone importing a given module still sees the type + // even when moved. + // + // NOTE: There is a weakness for Swift with protobuf extensions. To make + // the protobuf extensions easier to use, a Swift extension is declared with + // field exposed as a property on the exteneded message. There is no way + // to reexport the Swift `extension` and/or added properties. But the raw + // types are re-exported to minimize the breaking of code if a type is moved + // between files/modules. + // + // `reexportPublicImports` will cause the `import public` types to be + // reexported to avoid breaking downstream code using a type that might have + // moved between .proto files. + // + // `asImplementationOnly` will cause all of the import directives to be + // marked as `@_implementationOnly`. It will also cause all of the `file`'s + // `publicDependencies` to instead be recursively pulled up as direct imports + // to ensure the generate file compiles, and no `import public` files are + // re-exported. + // + // Aside: This could be moved into the plugin library, but it doesn't seem + // like anyone else would need the logic. Swift GRPC support probably stick + // with the support for the module mappings. + public func computeImports( + namer: SwiftProtobufNamer, + reexportPublicImports: Bool, + asImplementationOnly: Bool + ) -> String { + // The namer should be configured with the module this file generated for. + assert(namer.targetModule == (namer.mappings.moduleName(forFile: self) ?? "")) + // Booth options can't be enabled. + assert(!reexportPublicImports || + !asImplementationOnly || + reexportPublicImports != asImplementationOnly) + + guard namer.mappings.hasMappings else { + // No module mappings? Everything must be the same module, so no Swift + // imports will be needed. + return "" + } + + if dependencies.isEmpty { + // No proto dependencies (imports), then no Swift imports will be needed. + return "" + } + + let directive = asImplementationOnly ? "@_implementationOnly import" : "import" + var imports = Set() + for dependency in dependencies { + if SwiftProtobufInfo.isBundledProto(file: dependency) { + continue // No import needed for the runtime, that's always added. + } + if reexportPublicImports && publicDependencies.contains(where: { $0 === dependency }) { + // When re-exporting, the `import public` types will be imported + // instead of importing the module. + continue + } + if let depModule = namer.mappings.moduleName(forFile: dependency), + depModule != namer.targetModule { + // Different module, import it. + imports.insert("\(directive) \(depModule)") + } + } + + // If not re-exporting imports, then there is nothing special needed for + // `import public` files, as any transitive `import public` directives + // would have already re-exported the types, so everything this file needs + // will be covered by the above imports. + let exportingImports: [String] = + reexportPublicImports ? computeSymbolReExports(namer: namer) : [String]() + + var result = imports.sorted().joined(separator: "\n") + if !exportingImports.isEmpty { + if !result.isEmpty { + result.append("\n") + } + result.append("// Use of 'import public' causes re-exports:\n") + result.append(exportingImports.sorted().joined(separator: "\n")) + } + return result + } + + // Internal helper to `computeImports(...)`. + private func computeSymbolReExports(namer: SwiftProtobufNamer) -> [String] { + var result = [String]() + + // To handle re-exporting, recuively walk all the `import public` files + // and make this module do a Swift exporting import of the specific + // symbols. That will keep any type that gets moved between .proto files + // still exposed from the same modules so as not to break developer + // authored code. + var toScan = publicDependencies + var visited = Set() + while let dependency = toScan.popLast() { + let dependencyName = dependency.name + if visited.contains(dependencyName) { continue } + visited.insert(dependencyName) + + if SwiftProtobufInfo.isBundledProto(file: dependency) { + continue // Bundlined file, nothing to do. + } + guard let depModule = namer.mappings.moduleName(forFile: dependency) else { + continue // No mapping, assume same module, nothing to do. + } + if depModule == namer.targetModule { + // Same module, nothing to do (that generated file will do any re-exports). + continue + } + + toScan.append(contentsOf: dependency.publicDependencies) + + // NOTE: This re-exports/imports from the module that defines the type. + // If Xcode/SwiftPM ever were to do some sort of "layering checks" to + // ensure there is a direct dependency on the thing being imported, this + // could be updated do the re-export/import from the middle step in + // chained imports. + + for m in dependency.messages { + result.append("@_exported import struct \(namer.fullName(message: m))") + } + for e in dependency.enums { + result.append("@_exported import enum \(namer.fullName(enum: e))") + } + // There is nothing we can do for the Swift extensions declared on the + // extended Messages, best we can do is expose the raw extenions + // themselves. + for e in dependency.extensions { + result.append("@_exported import let \(namer.fullName(extensionField: e))") + } + } + return result + } } extension Descriptor { diff --git a/Sources/protoc-gen-swift/FileGenerator.swift b/Sources/protoc-gen-swift/FileGenerator.swift index e4055805a8..18e46bbe37 100644 --- a/Sources/protoc-gen-swift/FileGenerator.swift +++ b/Sources/protoc-gen-swift/FileGenerator.swift @@ -90,25 +90,20 @@ class FileGenerator { p.print("\(comments)import Foundation") - // Import all other imports as @_implementationOnly if the option is - // set, to avoid exposing internal types to users. - let visibilityAnnotation: String - if self.generatorOptions.implementationOnlyImports { - precondition(self.generatorOptions.visibility == .internal) - visibilityAnnotation = "@_implementationOnly " - } else { - visibilityAnnotation = "" - } if fileDescriptor.isBundledProto { p.print("// 'import \(namer.swiftProtobufModuleName)' suppressed, this proto file is meant to be bundled in the runtime.") } else { - p.print("\(visibilityAnnotation)import \(namer.swiftProtobufModuleName)") + let directive = self.generatorOptions.implementationOnlyImports ? "@_implementationOnly import" : "import" + p.print("\(directive) \(namer.swiftProtobufModuleName)") } - if let neededImports = generatorOptions.protoToModuleMappings.neededModules(forFile: fileDescriptor) { + + let neededImports = fileDescriptor.computeImports( + namer: namer, + reexportPublicImports: self.generatorOptions.visibility != .internal, + asImplementationOnly: self.generatorOptions.implementationOnlyImports) + if !neededImports.isEmpty { p.print() - for i in neededImports { - p.print("\(visibilityAnnotation)import \(i)") - } + p.print(neededImports) } p.print() diff --git a/Tests/protoc-gen-swiftTests/Test_DescriptorExtensions.swift b/Tests/protoc-gen-swiftTests/Test_DescriptorExtensions.swift index 712070cb19..1c2a342eb9 100644 --- a/Tests/protoc-gen-swiftTests/Test_DescriptorExtensions.swift +++ b/Tests/protoc-gen-swiftTests/Test_DescriptorExtensions.swift @@ -13,6 +13,8 @@ import SwiftProtobuf import SwiftProtobufPluginLibrary @testable import protoc_gen_swift +fileprivate typealias FileDescriptorProto = Google_Protobuf_FileDescriptorProto + final class Test_DescriptorExtensions: XCTestCase { func testExtensionRanges() throws { @@ -194,4 +196,271 @@ final class Test_DescriptorExtensions: XCTestCase { XCTAssertIdentical(aliasInfo.original(of: e.values[5]), e.values[0]) } + func test_File_computeImports_noImportPublic() { + let configText = """ + mapping { module_name: "foo", proto_file_path: "file" } + mapping { module_name: "bar", proto_file_path: "dir1/file" } + mapping { module_name: "baz", proto_file_path: ["dir2/file","file4"] } + mapping { module_name: "foo", proto_file_path: "file5" } + """ + + let config = try! SwiftProtobuf_GenSwift_ModuleMappings(textFormatString: configText) + let mapper = try! ProtoFileToModuleMappings(moduleMappingsProto: config) + + let fileProtos = [ + FileDescriptorProto(name: "file"), + FileDescriptorProto(name: "google/protobuf/any.proto", package: "google.protobuf"), + FileDescriptorProto(name: "dir1/file", dependencies: ["file"]), + FileDescriptorProto(name: "dir2/file", dependencies: ["google/protobuf/any.proto"]), + FileDescriptorProto(name: "file4", dependencies: ["dir2/file", "dir1/file", "file"]), + FileDescriptorProto(name: "file5", dependencies: ["file"]), + ] + let descSet = DescriptorSet(protos: fileProtos) + + // ( filename, imports, implOnly imports ) + let tests: [(String, String, String)] = [ + ( "file", "", "" ), + ( "dir1/file", "import foo", "@_implementationOnly import foo" ), + ( "dir2/file", "", "" ), + ( "file4", "import bar\nimport foo", "@_implementationOnly import bar\n@_implementationOnly import foo" ), + ( "file5", "", "" ), + ] + + for (name, expected, expectedImplOnly) in tests { + let fileDesc = descSet.files.filter{ $0.name == name }.first! + do { // reexportPublicImports: false, asImplementationOnly: false + let namer = + SwiftProtobufNamer(currentFile: fileDesc, + protoFileToModuleMappings: mapper) + let result = fileDesc.computeImports(namer: namer, reexportPublicImports: false, asImplementationOnly: false) + XCTAssertEqual(result, expected, "Looking for \(name)") + } + do { // reexportPublicImports: true, asImplementationOnly: false - No `import publc`, same as previous + let namer = + SwiftProtobufNamer(currentFile: fileDesc, + protoFileToModuleMappings: mapper) + let result = fileDesc.computeImports(namer: namer, reexportPublicImports: true, asImplementationOnly: false) + XCTAssertEqual(result, expected, "Looking for \(name)") + } + do { // reexportPublicImports: false, asImplementationOnly: true + let namer = + SwiftProtobufNamer(currentFile: fileDesc, + protoFileToModuleMappings: mapper) + let result = fileDesc.computeImports(namer: namer, reexportPublicImports: false, asImplementationOnly: true) + XCTAssertEqual(result, expectedImplOnly, "Looking for \(name)") + } + } + } + + func test_File_computeImports_PublicImports() { + // See the notes on computeImports(namer:reexportPublicImports:asImplementationOnly:) + // about how public import complicate things. + + // Given: + // + // + File: a.proto + // message A {} + // + // enum E { + // E_UNSET = 0; + // E_A = 1; + // E_B = 2; + // } + // + // + File: imports_a_publicly.proto + // import public "a.proto"; + // + // message ImportsAPublicly { + // optional A a = 1; + // optional E e = 2; + // } + // + // + File: imports_imports_a_publicly.proto + // import public "imports_a_publicly.proto"; + // + // message ImportsImportsAPublicly { + // optional A a = 1; + // } + // + // + File: uses_a_transitively.proto + // import "imports_a_publicly.proto"; + // + // message UsesATransitively { + // optional A a = 1; + // } + // + // + File: uses_a_transitively2.proto + // import "imports_imports_a_publicly.proto"; + // + // message UsesATransitively2 { + // optional A a = 1; + // } + // + // With a mapping file of: + // + // mapping { + // module_name: "A" + // proto_file_path: "a.proto" + // } + // mapping { + // module_name: "ImportsAPublicly" + // proto_file_path: "imports_a_publicly.proto" + // } + // mapping { + // module_name: "ImportsImportsAPublicly" + // proto_file_path: "imports_imports_a_publicly.proto" + // } + + let configText = """ + mapping { module_name: "A", proto_file_path: "a.proto" } + mapping { module_name: "ImportsAPublicly", proto_file_path: "imports_a_publicly.proto" } + mapping { module_name: "ImportsImportsAPublicly", proto_file_path: "imports_imports_a_publicly.proto" } + """ + + let config = try! SwiftProtobuf_GenSwift_ModuleMappings(textFormatString: configText) + let mapper = try! ProtoFileToModuleMappings(moduleMappingsProto: config) + + let fileProtos = [ + try! FileDescriptorProto(textFormatString: """ + name: "a.proto" + message_type { + name: "A" + } + enum_type { + name: "E" + value { + name: "E_UNSET" + number: 0 + } + value { + name: "E_A" + number: 1 + } + value { + name: "E_B" + number: 2 + } + } + """), + try! FileDescriptorProto(textFormatString: """ + name: "imports_a_publicly.proto" + dependency: "a.proto" + message_type { + name: "ImportsAPublicly" + field { + name: "a" + number: 1 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".A" + json_name: "a" + } + field { + name: "e" + number: 2 + label: LABEL_OPTIONAL + type: TYPE_ENUM + type_name: ".E" + json_name: "e" + } + } + public_dependency: 0 + """), + try! FileDescriptorProto(textFormatString: """ + name: "imports_imports_a_publicly.proto" + dependency: "imports_a_publicly.proto" + message_type { + name: "ImportsImportsAPublicly" + field { + name: "a" + number: 1 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".A" + json_name: "a" + } + } + public_dependency: 0 + """), + try! FileDescriptorProto(textFormatString: """ + name: "uses_a_transitively.proto" + dependency: "imports_a_publicly.proto" + message_type { + name: "UsesATransitively" + field { + name: "a" + number: 1 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".A" + json_name: "a" + } + } + """), + try! FileDescriptorProto(textFormatString: """ + name: "uses_a_transitively2.proto" + dependency: "imports_imports_a_publicly.proto" + message_type { + name: "UsesATransitively2" + field { + name: "a" + number: 1 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".A" + json_name: "a" + } + } + """), + ] + let descSet = DescriptorSet(protos: fileProtos) + + // ( filename, imports, reExportPublicImports imports, implOnly imports ) + let tests: [(String, String, String, String)] = [ + ( "a.proto", + "", "", "" ), + ( "imports_a_publicly.proto", + "import A", + "// Use of 'import public' causes re-exports:\n@_exported import enum A.E\n@_exported import struct A.A", + "@_implementationOnly import A" ), + ( "imports_imports_a_publicly.proto", + "import ImportsAPublicly", + "// Use of 'import public' causes re-exports:\n@_exported import enum A.E\n@_exported import struct A.A\n@_exported import struct ImportsAPublicly.ImportsAPublicly", + "@_implementationOnly import ImportsAPublicly" ), + ( "uses_a_transitively.proto", + "import ImportsAPublicly", // this reexports A, so we don't directly pull in A. + "import ImportsAPublicly", // just a plain `import`, nothing to re-export. + "@_implementationOnly import ImportsAPublicly" ), + ( "uses_a_transitively2.proto", + "import ImportsImportsAPublicly", // this chain reexports A, so we don't directly pull in A. + "import ImportsImportsAPublicly", // just a plain `import`, nothing to re-export. + "@_implementationOnly import ImportsImportsAPublicly" ), + ] + + for (name, expected, expectedReExport, expectedImplOnly) in tests { + let fileDesc = descSet.files.filter{ $0.name == name }.first! + do { // reexportPublicImports: false, asImplementationOnly: false + let namer = + SwiftProtobufNamer(currentFile: fileDesc, + protoFileToModuleMappings: mapper) + let result = fileDesc.computeImports(namer: namer, reexportPublicImports: false, asImplementationOnly: false) + XCTAssertEqual(result, expected, "Looking for \(name)") + } + do { // reexportPublicImports: true, asImplementationOnly: false + let namer = + SwiftProtobufNamer(currentFile: fileDesc, + protoFileToModuleMappings: mapper) + let result = fileDesc.computeImports(namer: namer, reexportPublicImports: true, asImplementationOnly: false) + XCTAssertEqual(result, expectedReExport, "Looking for \(name)") + } + do { // reexportPublicImports: false, asImplementationOnly: true + let namer = + SwiftProtobufNamer(currentFile: fileDesc, + protoFileToModuleMappings: mapper) + let result = fileDesc.computeImports(namer: namer, reexportPublicImports: false, asImplementationOnly: true) + XCTAssertEqual(result, expectedImplOnly, "Looking for \(name)") + } + } + } + }