From d0ef25095dcf1eebc7824144ea6d350003461e08 Mon Sep 17 00:00:00 2001
From: Thomas Van Lenten <thomasvl@google.com>
Date: Fri, 5 Jan 2024 12:20:00 -0500
Subject: [PATCH] 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.
---
 Sources/protoc-gen-swift/FileGenerator.swift  | 26 +++++------------
 .../protoc-gen-swift/GeneratorOptions.swift   | 29 +++++++++++++++++++
 2 files changed, 37 insertions(+), 18 deletions(-)

diff --git a/Sources/protoc-gen-swift/FileGenerator.swift b/Sources/protoc-gen-swift/FileGenerator.swift
index 82b072ba9..e4055805a 100644
--- 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
index 6d875e2a6..b939e3458 100644
--- 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).
   }
 }