Move option validation into GeneratorOptions.

This does it once rather than during each file generation. Add note about why there can't be validation between visibility and module mappings.
thomasvl · Jan 5, 2024 · d0ef250 · d0ef250
1 parent b8e2be8
commit d0ef250
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 18 deletions.
diff --git a/Sources/protoc-gen-swift/FileGenerator.swift b/Sources/protoc-gen-swift/FileGenerator.swift
@@ -90,25 +90,15 @@ class FileGenerator {
 
         p.print("\(comments)import Foundation")
 
-        if self.generatorOptions.implementationOnlyImports,
-           self.generatorOptions.visibility != .internal {
-            errorString = """
-                Cannot use @_implementationOnly imports when the proto visibility is public or package.
-                Either change the visibility to internal, or disable @_implementationOnly imports.
-            """
-            return
+        // 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 = ""
         }
-
-        // Import all other imports as @_implementationOnly if the visiblity is
-        // internal and the option is set, to avoid exposing internal types to users.
-        let visibilityAnnotation: String = {
-            if self.generatorOptions.implementationOnlyImports,
-               self.generatorOptions.visibility == .internal {
-                return "@_implementationOnly "
-            } else {
-                return ""
-            }
-        }()
         if fileDescriptor.isBundledProto {
             p.print("// 'import \(namer.swiftProtobufModuleName)' suppressed, this proto file is meant to be bundled in the runtime.")
         } else {

diff --git a/Sources/protoc-gen-swift/GeneratorOptions.swift b/Sources/protoc-gen-swift/GeneratorOptions.swift
@@ -130,5 +130,34 @@ class GeneratorOptions {
     }
 
     self.implementationOnlyImports = implementationOnlyImports
+
+    // ------------------------------------------------------------------------
+    // Now do "cross option" validations.
+
+    if self.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.
+        """)
+    }
+
+    // The majority case is that if `self.protoToModuleMappings.hasMappings` is
+    // true, then `self.visibility` should be either `.public` or `.package`.
+    // However, it is possible for someone to put top most proto files (ones
+    // not imported into other proto files) in a different module, and use
+    // internal visibility there. i.e. -
+    //
+    //    module One:
+    //    - foo.pb.swift from foo.proto generated with "public" visibility.
+    //    module Two:
+    //    - bar.pb.swift from bar.proto (which does `import foo.proto`)
+    //      generated with "internal" visiblity.
+    //
+    // Since this support is possible/valid, there's no good way a "bad" case
+    // (i.e. - if foo.pb.swift was generated with "internal" visiblity). So
+    // no options validation here, and instead developers would have to figure
+    // this out via the compiler errors around missing type (when bar.pb.swift
+    // gets unknown reference for thing that should be in module One via
+    // foo.pb.swift).
   }
 }