Add CompileTests for InternalImportsByDefault (#1709)

* Add CompileTests for InternalImportsByDefault

* Update Reference and Makefile

* Add missing headers

* Update Makefile

* Fix references

* Fix Makefile

* Fix Package.swift

* Fix build on 5.8

.gitignore

@@ -11,6 +11,8 @@ xcbaselines
 mined_words.txt
 /CompileTests/MultiModule/.build
 /CompileTests/MultiModule/.swiftpm
+/CompileTests/InternalImportsByDefault/.build
+/CompileTests/InternalImportsByDefault/.swiftpm
 /*DescriptorTestData.bin
 /Package.resolved
 /PluginLibEditionDefaults.bin

CompileTests/InternalImportsByDefault/Package.swift

@@ -0,0 +1,56 @@
+// swift-tools-version: 5.8
+
+// Package.swift
+//
+// Copyright (c) 2024 Apple Inc. and the project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See LICENSE.txt for license information:
+// https://github.com/apple/swift-protobuf/blob/main/LICENSE.txt
+
+import PackageDescription
+
+#if compiler(>=5.9)
+let package = Package(
+    name: "CompileTests",
+    dependencies: [
+        .package(path: "../..")
+    ],
+    targets: [
+        .executableTarget(
+            name: "InternalImportsByDefault",
+            dependencies: [
+                .product(name: "SwiftProtobuf", package: "swift-protobuf"),
+            ],
+            exclude: [
+                "Protos/SomeProtoWithBytes.proto",
+                "Protos/ServiceOnly.proto"
+            ],
+            swiftSettings: [
+                .enableExperimentalFeature("InternalImportsByDefault"),
+                .enableExperimentalFeature("AccessLevelOnImport"),
+                // Enable warnings as errors so the build fails if warnings are
+                // present in generated code.
+                    .unsafeFlags(["-warnings-as-errors"])
+            ],
+            plugins: [
+                .plugin(name: "SwiftProtobufPlugin", package: "swift-protobuf")
+            ]
+        ),
+    ]
+)
+#else
+let package = Package(
+    name: "CompileTests",
+    targets: [
+        .executableTarget(
+            name: "InternalImportsByDefault",
+            exclude: [
+                "swift-protobuf-config.json",
+                "Protos/SomeProtoWithBytes.proto",
+                "Protos/ServiceOnly.proto"
+            ]
+        )
+    ]
+)
+#endif

CompileTests/InternalImportsByDefault/README.md

@@ -0,0 +1,12 @@
+# CompileTests/InternalImportsByDefault
+
+This is a test case that ensures that generated code builds correctly when 
+`InternalImportsByDefault` is enabled and the code is generated with public
+visibility.
+
+When support for access level modifiers on imports was first added, an issue 
+was encountered where publicly-generated protos would generate build errors and 
+warnings when `InternalImportsByDefault` was enabled, as some dependencies were 
+imported without an explicit access level modifier (i.e. `Foundation`), and some
+where sometimes imported as `public` without actually being used in the 
+generated code at all (i.e. `Foundation` and `SwiftProtobuf`).

CompileTests/InternalImportsByDefault/Sources/InternalImportsByDefault/Protos/ServiceOnly.proto

@@ -0,0 +1,7 @@
+// This proto file should generate an empty file, since the plugin will ignore
+// service definitions.
+// This is here to make sure we don't import Foundation or SwiftProtobuf when
+// it's not necessary.
+
+service SomeService {
+}

CompileTests/InternalImportsByDefault/Sources/InternalImportsByDefault/Protos/SomeProtoWithBytes.proto

@@ -0,0 +1,12 @@
+// This proto will generate a Swift file that imports Foundation, because it
+// defines a bytes field.
+// Because InternalImportsByDefault is enabled on this module and we generate
+// protos with public visibility, the build will fail if the access level
+// modifier is missing (or wrong) since it will default the import to `internal`
+// and cause a conflict of access levels, since the `someBytes` property defined
+// on the message will be public.
+
+message SomeProtoWithBytes {
+  optional bytes someBytes = 2;
+  optional string ext_str = 100;
+}

CompileTests/InternalImportsByDefault/Sources/InternalImportsByDefault/main.swift

@@ -0,0 +1,26 @@
+// main.swift
+//
+// Copyright (c) 2024 Apple Inc. and the project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See LICENSE.txt for license information:
+// https://github.com/apple/swift-protobuf/blob/main/LICENSE.txt
+
+// This test only makes sense for Swift 5.9+ because 5.8 doesn't support access
+// level on imports.
+#if compiler(>=5.9)
+private import Foundation
+
+struct InternalImportsByDefault {
+    static func main() {
+        let protoWithBytes = SomeProtoWithBytes.with { proto in
+            proto.someBytes = Data()
+            proto.extStr = ""
+        }
+        blackhole(protoWithBytes)
+    }
+}
+
+@inline(never)
+func blackhole<T>(_: T) {}
+#endif

CompileTests/InternalImportsByDefault/Sources/InternalImportsByDefault/swift-protobuf-config.json

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

Makefile

@@ -79,13 +79,15 @@ PROTOS_DIRS=Conformance protoc-gen-swiftTests SwiftProtobuf SwiftProtobufPluginL
 	clean \
 	compile-tests \
 	compile-tests-multimodule \
+	compile-tests-internalimportsbydefault \
 	default \
 	docs \
 	install \
 	pod-lib-lint \
 	reference \
 	regenerate \
 	regenerate-compiletests-multimodule-protos \
+	copy-compiletests-internalimportsbydefault-protos \
 	regenerate-compiletests-protos \
 	regenerate-conformance-protos \
 	regenerate-fuzz-protos \
@@ -194,19 +196,33 @@ test-plugin: build ${PROTOC_GEN_SWIFT}
 		--tfiws_opt=ProtoPathModuleMappings=Protos/CompileTests/MultiModule/module_mappings.pbascii \
 		--tfiws_out=_test/CompileTests/MultiModule \
 		`(find Protos/CompileTests/MultiModule -type f -name "*.proto")`
+	@mkdir -p _test/CompileTests/InternalImportsByDefault
+	${GENERATE_SRCS} \
+	    -I Protos/CompileTests/InternalImportsByDefault \
+		--tfiws_opt=Visibility=Public \
+		--tfiws_opt=UseAccessLevelOnImports=true \
+		--tfiws_out=_test/CompileTests/InternalImportsByDefault \
+		`(find Protos/CompileTests/InternalImportsByDefault -type f -name "*.proto")`
 	diff -ru _test Reference
 
 # Test the SPM plugin.
 test-spm-plugin:
 	env PROTOC_PATH=$(shell realpath ${PROTOC}) ${SWIFT} test --package-path PluginExamples
 
-compile-tests: compile-tests-multimodule
+compile-tests: \
+	compile-tests-multimodule \
+	compile-tests-internalimportsbydefault
 
-# Test that ensure generating public into multiple modules with `import public`
+# Test that ensures generating public into multiple modules with `import public`
 # yields buildable code.
 compile-tests-multimodule:
 	${SWIFT} test --package-path CompileTests/MultiModule
 
+# Test that ensures that using access level modifiers on imports yields code that's buildable
+# when `InternalImportsByDefault` is enabled on the module.
+compile-tests-internalimportsbydefault:
+	env PROTOC_PATH=$(shell realpath ${PROTOC}) ${SWIFT} build --package-path CompileTests/InternalImportsByDefault
+
 
 # Rebuild the reference files by running the local version of protoc-gen-swift
 # against our menagerie of sample protos.
@@ -240,6 +256,13 @@ reference: build ${PROTOC_GEN_SWIFT}
 		--tfiws_opt=ProtoPathModuleMappings=Protos/CompileTests/MultiModule/module_mappings.pbascii \
 		--tfiws_out=Reference/CompileTests/MultiModule \
 		`(find Protos/CompileTests/MultiModule -type f -name "*.proto")`
+	@mkdir -p Reference/CompileTests/InternalImportsByDefault
+	${GENERATE_SRCS} \
+	    -I Protos/CompileTests/InternalImportsByDefault \
+		--tfiws_opt=Visibility=Public \
+		--tfiws_opt=UseAccessLevelOnImports=true \
+		--tfiws_out=Reference/CompileTests/InternalImportsByDefault \
+		`(find Protos/CompileTests/InternalImportsByDefault -type f -name "*.proto")`
 
 #
 # Rebuild the generated .pb.swift test files by running
@@ -494,10 +517,12 @@ regenerate-conformance-protos: build ${PROTOC_GEN_SWIFT}
 		`find Protos/Conformance -type f -name "*.proto"`
 
 # Rebuild just the protos used by the CompileTests.
-regenerate-compiletests-protos: regenerate-compiletests-multimodule-protos
+regenerate-compiletests-protos: \
+	regenerate-compiletests-multimodule-protos \
+	copy-compiletests-internalimportsbydefault-protos
 
 # Update the CompileTests/MultiModule files.
-# NOTE: Any changes here must be done of the "test-plugin" target so it
+# NOTE: Any changes here must also be done on the "test-plugin" target so it
 # generates in the same way.
 regenerate-compiletests-multimodule-protos: build ${PROTOC_GEN_SWIFT}
 	find CompileTests/MultiModule -name "*.pb.swift" -exec rm -f {} \;
@@ -508,6 +533,12 @@ regenerate-compiletests-multimodule-protos: build ${PROTOC_GEN_SWIFT}
 		--tfiws_out=CompileTests/MultiModule \
 		`(find Protos/CompileTests/MultiModule -type f -name "*.proto")`
 
+# We use the plugin for the InternalImportsByDefault test, so we don't actually need to regenerate
+# anything. However, to keep the protos centralised in a single place (the Protos directory),
+# this simply copies those files to the InternalImportsByDefault package in case they change.
+copy-compiletests-internalimportsbydefault-protos: 
+	@cp Protos/CompileTests/InternalImportsByDefault/* CompileTests/InternalImportsByDefault/Sources/InternalImportsByDefault/Protos
+
 # Helper to check if there is a protobuf checkout as expected.
 check-for-protobuf-checkout:
 	@if [ ! -d "${GOOGLE_PROTOBUF_CHECKOUT}/src/google/protobuf" ]; then \

Protos/CompileTests/InternalImportsByDefault/ServiceOnly.proto

@@ -0,0 +1,7 @@
+// This proto file should generate an empty file, since the plugin will ignore
+// service definitions.
+// This is here to make sure we don't import Foundation or SwiftProtobuf when
+// it's not necessary.
+
+service SomeService {
+}

Protos/CompileTests/InternalImportsByDefault/SomeProtoWithBytes.proto

@@ -0,0 +1,12 @@
+// This proto will generate a Swift file that imports Foundation, because it
+// defines a bytes field.
+// Because InternalImportsByDefault is enabled on this module and we generate
+// protos with public visibility, the build will fail if the access level
+// modifier is missing (or wrong) since it will default the import to `internal`
+// and cause a conflict of access levels, since the `someBytes` property defined
+// on the message will be public.
+
+message SomeProtoWithBytes {
+  optional bytes someBytes = 2;
+  optional string ext_str = 100;
+}

Reference/CompileTests/InternalImportsByDefault/ServiceOnly.pb.swift

@@ -0,0 +1,11 @@
+// DO NOT EDIT.
+// swift-format-ignore-file
+// swiftlint:disable all
+//
+// Generated by the Swift generator plugin for the protocol buffer compiler.
+// Source: ServiceOnly.proto
+//
+// For information on using the generated types, please see the documentation:
+//   https://github.com/apple/swift-protobuf/
+
+// This file contained no messages, enums, or extensions.

Reference/CompileTests/InternalImportsByDefault/SomeProtoWithBytes.pb.swift

@@ -0,0 +1,97 @@
+// DO NOT EDIT.
+// swift-format-ignore-file
+// swiftlint:disable all
+//
+// Generated by the Swift generator plugin for the protocol buffer compiler.
+// Source: SomeProtoWithBytes.proto
+//
+// For information on using the generated types, please see the documentation:
+//   https://github.com/apple/swift-protobuf/
+
+public import Foundation
+public import SwiftProtobuf
+
+// 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
+// incompatible with the version of SwiftProtobuf to which you are linking.
+// Please ensure that you are building against the same version of the API
+// that was used to generate this file.
+fileprivate struct _GeneratedWithProtocGenSwiftVersion: SwiftProtobuf.ProtobufAPIVersionCheck {
+  struct _2: SwiftProtobuf.ProtobufAPIVersion_2 {}
+  typealias Version = _2
+}
+
+public struct SomeProtoWithBytes: @unchecked 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.
+
+  public var someBytes: Data {
+    get {return _someBytes ?? Data()}
+    set {_someBytes = newValue}
+  }
+  /// Returns true if `someBytes` has been explicitly set.
+  public var hasSomeBytes: Bool {return self._someBytes != nil}
+  /// Clears the value of `someBytes`. Subsequent reads from it will return its default value.
+  public mutating func clearSomeBytes() {self._someBytes = nil}
+
+  public var extStr: String {
+    get {return _extStr ?? String()}
+    set {_extStr = newValue}
+  }
+  /// Returns true if `extStr` has been explicitly set.
+  public var hasExtStr: Bool {return self._extStr != nil}
+  /// Clears the value of `extStr`. Subsequent reads from it will return its default value.
+  public mutating func clearExtStr() {self._extStr = nil}
+
+  public var unknownFields = SwiftProtobuf.UnknownStorage()
+
+  public init() {}
+
+  fileprivate var _someBytes: Data? = nil
+  fileprivate var _extStr: String? = nil
+}
+
+// MARK: - Code below here is support for the SwiftProtobuf runtime.
+
+extension SomeProtoWithBytes: SwiftProtobuf.Message, SwiftProtobuf._MessageImplementationBase, SwiftProtobuf._ProtoNameProviding {
+  public static let protoMessageName: String = "SomeProtoWithBytes"
+  public static let _protobuf_nameMap: SwiftProtobuf._NameMap = [
+    2: .same(proto: "someBytes"),
+    100: .standard(proto: "ext_str"),
+  ]
+
+  public mutating func decodeMessage<D: SwiftProtobuf.Decoder>(decoder: inout D) throws {
+    while let fieldNumber = try decoder.nextFieldNumber() {
+      // The use of inline closures is to circumvent an issue where the compiler
+      // allocates stack space for every case branch when no optimizations are
+      // enabled. https://github.com/apple/swift-protobuf/issues/1034
+      switch fieldNumber {
+      case 2: try { try decoder.decodeSingularBytesField(value: &self._someBytes) }()
+      case 100: try { try decoder.decodeSingularStringField(value: &self._extStr) }()
+      default: break
+      }
+    }
+  }
+
+  public func traverse<V: SwiftProtobuf.Visitor>(visitor: inout V) throws {
+    // The use of inline closures is to circumvent an issue where the compiler
+    // allocates stack space for every if/case branch local when no optimizations
+    // are enabled. https://github.com/apple/swift-protobuf/issues/1034 and
+    // https://github.com/apple/swift-protobuf/issues/1182
+    try { if let v = self._someBytes {
+      try visitor.visitSingularBytesField(value: v, fieldNumber: 2)
+    } }()
+    try { if let v = self._extStr {
+      try visitor.visitSingularStringField(value: v, fieldNumber: 100)
+    } }()
+    try unknownFields.traverse(visitor: &visitor)
+  }
+
+  public static func ==(lhs: SomeProtoWithBytes, rhs: SomeProtoWithBytes) -> Bool {
+    if lhs._someBytes != rhs._someBytes {return false}
+    if lhs._extStr != rhs._extStr {return false}
+    if lhs.unknownFields != rhs.unknownFields {return false}
+    return true
+  }
+}