-
Notifications
You must be signed in to change notification settings - Fork 460
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add CompileTests for InternalImportsByDefault #1709
Merged
thomasvl
merged 8 commits into
apple:main
from
gjcairo:internal-imports-by-default-compile-tests
Sep 5, 2024
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
911e931
Add CompileTests for InternalImportsByDefault
gjcairo 6da847a
Update Reference and Makefile
gjcairo 785879e
Add missing headers
gjcairo d11ea32
Update Makefile
gjcairo 8b2c219
Fix references
gjcairo d05504c
Fix Makefile
gjcairo dad56cb
Fix Package.swift
gjcairo 6323349
Fix build on 5.8
gjcairo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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`). |
7 changes: 7 additions & 0 deletions
7
...eTests/InternalImportsByDefault/Sources/InternalImportsByDefault/Protos/ServiceOnly.proto
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 { | ||
} |
12 changes: 12 additions & 0 deletions
12
...InternalImportsByDefault/Sources/InternalImportsByDefault/Protos/SomeProtoWithBytes.proto
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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; | ||
} |
26 changes: 26 additions & 0 deletions
26
CompileTests/InternalImportsByDefault/Sources/InternalImportsByDefault/main.swift
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
12 changes: 12 additions & 0 deletions
12
...ests/InternalImportsByDefault/Sources/InternalImportsByDefault/swift-protobuf-config.json
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
{ | ||
"invocations": [ | ||
{ | ||
"protoFiles": [ | ||
"Protos/SomeProtoWithBytes.proto", | ||
"Protos/ServiceOnly.proto", | ||
], | ||
"visibility": "public", | ||
"useAccessLevelOnImports": true | ||
} | ||
] | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7 changes: 7 additions & 0 deletions
7
Protos/CompileTests/InternalImportsByDefault/ServiceOnly.proto
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 { | ||
} |
12 changes: 12 additions & 0 deletions
12
Protos/CompileTests/InternalImportsByDefault/SomeProtoWithBytes.proto
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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; | ||
} |
11 changes: 11 additions & 0 deletions
11
Reference/CompileTests/InternalImportsByDefault/ServiceOnly.pb.swift
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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. |
97 changes: 97 additions & 0 deletions
97
Reference/CompileTests/InternalImportsByDefault/SomeProtoWithBytes.pb.swift
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to use
AccessLevelOnImport
and not useInternalImportsByDefault
, the original issue with Foundation was because we didn't have an access level, so we don't want any level by default, but to ensure we're always providing level and to enable the support on these 5.x compilers.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I forgot to enable
AccessLevelOnImport
too. IfInternalImportsByDefault
is not enabled, we still get some default: it defaults topublic
instead. I came across this bug when having both features enabled.There were also some issues with 5.8 not understanding these flags so I had to add some compiler guards but now finally CI is happy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What imports are still defaulting wrong? Do we need to fix those?