Merge pull request #1683 from Skoti/feature/access-level-on-imports

Support access level on import statements

Documentation/PLUGIN.md

@@ -182,6 +182,26 @@ exposed via public API, so even if `ImplementationOnlyImports` is set to `true`,
 this will only work if the `Visibility` is set to `internal`.
 
 
+##### Generation Option: `UseAccessLevelOnImports` - imports preceded by a visibility modifier (`public`, `package`, `internal`)
+
+The default behavior depends on the Swift version the plugin is compiled with. 
+For Swift versions below 6.0 the default is `false` and the code generator does not precede any imports with a visibility modifier. 
+You can change this by explicitly setting the `UseAccessLevelOnImports` option:
+
+```
+$ protoc --swift_opt=UseAccessLevelOnImports=[value] --swift_out=. foo/bar/*.proto mumble/*.proto
+```
+
+The possible values for `UseAccessLevelOnImports` are:
+
+* `false`: Generates plain import directives without a visibility modifier.
+* `true`: Imports of internal dependencies and any modules defined in the module
+mappings will be preceded by a visibility modifier corresponding to the visibility of the generated types - see `Visibility` option. 
+
+**Important:** It is strongly encouraged to use `internal` imports instead of `@_implementationOnly` imports. 
+Hence `UseAccessLevelOnImports` and `ImplementationOnlyImports` options exclude each other. 
+
+
 ### Building your project
 
 After copying the `.pb.swift` files into your project, you will need

PluginExamples/Package.swift

@@ -7,14 +7,22 @@ let package = Package(
     dependencies: [
         .package(path: "../")
     ],
-    targets: [
+    targets: targets()
+)
+
+private func targets() -> [Target] {
+    var testDependencies: [Target.Dependency] = [
+        .target(name: "Simple"),
+        .target(name: "Nested"),
+        .target(name: "Import"),
+    ]
+#if compiler(>=5.9)
+    testDependencies.append(.target(name: "AccessLevelOnImport"))
+#endif
+    var targets: [Target] = [
         .testTarget(
             name: "ExampleTests",
-            dependencies: [
-                .target(name: "Simple"),
-                .target(name: "Nested"),
-                .target(name: "Import"),
-            ]
+            dependencies: testDependencies
         ),
         .target(
             name: "Simple",
@@ -44,4 +52,21 @@ let package = Package(
             ]
         ),
     ]
-)
+#if compiler(>=5.9)
+    targets.append(
+        .target(
+            name: "AccessLevelOnImport",
+            dependencies: [
+                .product(name: "SwiftProtobuf", package: "swift-protobuf"),
+            ],
+            swiftSettings: [
+                .enableExperimentalFeature("AccessLevelOnImport"),
+            ],
+            plugins: [
+                .plugin(name: "SwiftProtobufPlugin", package: "swift-protobuf")
+            ]
+        )
+    )
+#endif
+    return targets
+}

PluginExamples/Sources/AccessLevelOnImport/AccessLevelOnImport/AccessLevelOnImport.proto

@@ -0,0 +1,7 @@
+syntax = "proto3";
+
+import "Dependency/Dependency.proto";
+
+message AccessLevelOnImport {
+  Dependency dependency = 1;
+}

PluginExamples/Sources/AccessLevelOnImport/Dependency/Dependency.proto

@@ -0,0 +1,5 @@
+syntax = "proto3";
+
+message Dependency {
+  string name = 1;
+}

PluginExamples/Sources/AccessLevelOnImport/empty.swift

@@ -0,0 +1,3 @@
+/// DO NOT DELETE.
+///
+/// We need to keep this file otherwise the plugin is not running.

PluginExamples/Sources/AccessLevelOnImport/swift-protobuf-config.json

@@ -0,0 +1,12 @@
+{
+    "invocations": [
+        {
+            "protoFiles": [
+                "AccessLevelOnImport/AccessLevelOnImport.proto",
+                "Dependency/Dependency.proto",
+            ],
+            "visibility": "public",
+            "useAccessLevelOnImports": true
+        }
+    ]
+}

PluginExamples/Sources/ExampleTests/AccessLevelOnImportTests.swift

@@ -0,0 +1,13 @@
+#if compiler(>=5.9)
+
+import AccessLevelOnImport
+import XCTest
+
+final class AccessLevelOnImportTests: XCTestCase {
+    func testAccessLevelOnImport() {
+        let access = AccessLevelOnImport.with { $0.dependency = .with { $0.name = "Dependency" } }
+        XCTAssertEqual(access.dependency.name, "Dependency")
+    }
+}
+
+#endif

Plugins/SwiftProtobufPlugin/plugin.swift

@@ -85,6 +85,8 @@ struct SwiftProtobufPlugin {
             var fileNaming: FileNaming?
             /// Whether internal imports should be annotated as `@_implementationOnly`.
             var implementationOnlyImports: Bool?
+            /// Whether import statements should be preceded with visibility.
+            var useAccessLevelOnImports: Bool?
         }
 
         /// The path to the `protoc` binary.
@@ -188,6 +190,11 @@ struct SwiftProtobufPlugin {
             protocArgs.append("--swift_opt=ImplementationOnlyImports=\(implementationOnlyImports)")
         }
 
+        // Add the useAccessLevelOnImports only imports flag if it was set
+        if let useAccessLevelOnImports = invocation.useAccessLevelOnImports {
+            protocArgs.append("--swift_opt=UseAccessLevelOnImports=\(useAccessLevelOnImports)")
+        }
+
         var inputFiles = [Path]()
         var outputFiles = [Path]()
 

Sources/SwiftProtobufPluginLibrary/ProtoFileToModuleMappings.swift

@@ -20,7 +20,7 @@ private let defaultSwiftProtobufModuleName = "SwiftProtobuf"
 public struct ProtoFileToModuleMappings {
 
   /// Errors raised from parsing mappings
-  public enum LoadError: Error {
+  public enum LoadError: Error, Equatable {
     /// Raised if the path wasn't found.
     case failToOpen(path: String)
     /// Raised if an mapping entry in the protobuf doesn't have a module name.

Sources/protoc-gen-swift/Descriptor+Extensions.swift

@@ -49,17 +49,15 @@ extension FileDescriptor {
   // 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(
+  func computeImports(
     namer: SwiftProtobufNamer,
-    reexportPublicImports: Bool,
-    asImplementationOnly: Bool
+    directive: GeneratorOptions.ImportDirective,
+    reexportPublicImports: Bool
   ) -> String {
     // The namer should be configured with the module this file generated for.
     assert(namer.targetModule == (namer.mappings.moduleName(forFile: self) ?? ""))
     // Both options can't be enabled.
-    assert(!reexportPublicImports ||
-           !asImplementationOnly ||
-           reexportPublicImports != asImplementationOnly)
+    assert(!reexportPublicImports || directive != .implementationOnly)
 
     guard namer.mappings.hasMappings else {
       // No module mappings? Everything must be the same module, so no Swift
@@ -72,7 +70,7 @@ extension FileDescriptor {
       return ""
     }
 
-    let directive = asImplementationOnly ? "@_implementationOnly import" : "import"
+    let importSnippet = directive.snippet
     var imports = Set<String>()
     for dependency in dependencies {
       if SwiftProtobufInfo.isBundledProto(file: dependency) {
@@ -86,16 +84,19 @@ extension FileDescriptor {
       if let depModule = namer.mappings.moduleName(forFile: dependency),
          depModule != namer.targetModule {
         // Different module, import it.
-        imports.insert("\(directive) \(depModule)")
+        imports.insert("\(importSnippet) \(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]()
+    let exportingImports: [String] = reexportPublicImports
+      ? computeSymbolReExports(
+        namer: namer,
+        useAccessLevelOnImports: directive.isAccessLevel)
+      : [String]()
 
     var result = imports.sorted().joined(separator: "\n")
     if !exportingImports.isEmpty {
@@ -109,7 +110,7 @@ extension FileDescriptor {
   }
 
   // Internal helper to `computeImports(...)`.
-  private func computeSymbolReExports(namer: SwiftProtobufNamer) -> [String] {
+  private func computeSymbolReExports(namer: SwiftProtobufNamer, useAccessLevelOnImports: Bool) -> [String] {
     var result = [String]()
 
     // To handle re-exporting, recursively walk all the `import public` files
@@ -119,6 +120,7 @@ extension FileDescriptor {
     // authored code.
     var toScan = publicDependencies
     var visited = Set<String>()
+    let exportedImportDirective = "@_exported\(useAccessLevelOnImports ? " public" : "") import"
     while let dependency = toScan.popLast() {
       let dependencyName = dependency.name
       if visited.contains(dependencyName) { continue }
@@ -144,16 +146,16 @@ extension FileDescriptor {
       // chained imports.
 
       for m in dependency.messages {
-        result.append("@_exported import struct \(namer.fullName(message: m))")
+        result.append("\(exportedImportDirective) struct \(namer.fullName(message: m))")
       }
       for e in dependency.enums {
-        result.append("@_exported import enum \(namer.fullName(enum: e))")
+        result.append("\(exportedImportDirective) 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 extensions
       // themselves.
       for e in dependency.extensions {
-        result.append("@_exported import let \(namer.fullName(extensionField: e))")
+        result.append("\(exportedImportDirective) let \(namer.fullName(extensionField: e))")
       }
     }
     return result

Sources/protoc-gen-swift/Docs.docc/index.md

@@ -188,6 +188,26 @@ exposed via public API, so even if `ImplementationOnlyImports` is set to `true`,
 this will only work if the `Visibility` is set to `internal`. 
 
 
+##### Generation Option: `UseAccessLevelOnImports` - imports preceded by a visibility modifier (`public`, `package`, `internal`)
+
+The default behavior depends on the Swift version the plugin is compiled with. 
+For Swift versions below 6.0 the default is `false` and the code generator does not precede any imports with a visibility modifier. 
+You can change this by explicitly setting the `UseAccessLevelOnImports` option:
+
+```
+$ protoc --swift_opt=UseAccessLevelOnImports=[value] --swift_out=. foo/bar/*.proto mumble/*.proto
+```
+
+The possible values for `UseAccessLevelOnImports` are:
+
+* `false`: Generates plain import directives without a visibility modifier.
+* `true`: Imports of internal dependencies and any modules defined in the module
+mappings will be preceded by a visibility modifier corresponding to the visibility of the generated types - see `Visibility` option. 
+
+**Important:** It is strongly encouraged to use `internal` imports instead of `@_implementationOnly` imports. 
+Hence `UseAccessLevelOnImports` and `ImplementationOnlyImports` options exclude each other. 
+
+
 ### Building your project
 
 After copying the `.pb.swift` files into your project, you will need

Sources/protoc-gen-swift/FileGenerator.swift

@@ -100,14 +100,13 @@ class FileGenerator {
         if fileDescriptor.isBundledProto {
             p.print("// 'import \(namer.swiftProtobufModuleName)' suppressed, this proto file is meant to be bundled in the runtime.")
         } else {
-            let directive = self.generatorOptions.implementationOnlyImports ? "@_implementationOnly import" : "import"
-            p.print("\(directive) \(namer.swiftProtobufModuleName)")
+            p.print("\(generatorOptions.importDirective.snippet) \(namer.swiftProtobufModuleName)")
         }
 
         let neededImports = fileDescriptor.computeImports(
           namer: namer,
-          reexportPublicImports: self.generatorOptions.visibility != .internal,
-          asImplementationOnly: self.generatorOptions.implementationOnlyImports)
+          directive: generatorOptions.importDirective,
+          reexportPublicImports: generatorOptions.visibility != .internal)
         if !neededImports.isEmpty {
             p.print()
             p.print(neededImports)

Sources/protoc-gen-swift/GeneratorOptions.swift

@@ -30,29 +30,44 @@ class GeneratorOptions {
     }
   }
 
-  enum Visibility {
+  enum Visibility: String {
     case `internal`
     case `public`
     case `package`
 
     init?(flag: String) {
-      switch flag.lowercased() {
-      case "internal":
-        self = .internal
-      case "public":
-        self = .public
-      case "package":
-        self = .package
-      default:
-        return nil
+      self.init(rawValue: flag.lowercased())
+    }
+  }
+
+  enum ImportDirective: Equatable {
+    case accessLevel(Visibility)
+    case plain
+    case implementationOnly
+
+    var isAccessLevel: Bool {
+        switch self {
+        case .accessLevel: return true
+        default: return false
+        }
+    }
+
+    var snippet: String {
+      switch self {
+      case let .accessLevel(visibility):
+        return "\(visibility) import"
+      case .plain:
+        return "import"
+      case .implementationOnly:
+        return "@_implementationOnly import"
       }
     }
   }
 
   let outputNaming: OutputNaming
   let protoToModuleMappings: ProtoFileToModuleMappings
   let visibility: Visibility
-  let implementationOnlyImports: Bool
+  let importDirective: ImportDirective
   let experimentalStripNonfunctionalCodegen: Bool
 
   /// A string snippet to insert for the visibility
@@ -64,6 +79,11 @@ class GeneratorOptions {
     var visibility: Visibility = .internal
     var swiftProtobufModuleName: String? = nil
     var implementationOnlyImports: Bool = false
+#if swift(>=6.0)
+    var useAccessLevelOnImports = true
+#else
+    var useAccessLevelOnImports = false
+#endif
     var experimentalStripNonfunctionalCodegen: Bool = false
 
     for pair in parameter.parsedPairs {
@@ -102,6 +122,13 @@ class GeneratorOptions {
           throw GenerationError.invalidParameterValue(name: pair.key,
                                                       value: pair.value)
         }
+      case "UseAccessLevelOnImports":
+        if let value = Bool(pair.value) {
+          useAccessLevelOnImports = value
+        } else {
+          throw GenerationError.invalidParameterValue(name: pair.key,
+                                                      value: pair.value)
+        }
       case "experimental_strip_nonfunctional_codegen":
         if pair.value.isEmpty {  // Also support option without any value.
           experimentalStripNonfunctionalCodegen = true
@@ -140,13 +167,22 @@ class GeneratorOptions {
       visibilitySourceSnippet = "package "
     }
 
-    self.implementationOnlyImports = implementationOnlyImports
     self.experimentalStripNonfunctionalCodegen = experimentalStripNonfunctionalCodegen
 
+    switch (implementationOnlyImports, useAccessLevelOnImports) {
+    case (false, false): self.importDirective = .plain
+    case (false, true): self.importDirective = .accessLevel(visibility)
+    case (true, false): self.importDirective = .implementationOnly
+    case (true, true): throw GenerationError.message(message: """
+      When using access levels on imports the @_implementationOnly option is unnecessary.
+      Disable @_implementationOnly imports.
+      """)
+    }
+
     // ------------------------------------------------------------------------
     // Now do "cross option" validations.
 
-    if self.implementationOnlyImports && self.visibility != .internal {
+    if implementationOnlyImports && self.visibility != .internal {
       throw GenerationError.message(message: """
         Cannot use @_implementationOnly imports when the proto visibility is public or package.
         Either change the visibility to internal, or disable @_implementationOnly imports.