Skip to content

Enable InternalImportsByDefault in remaining modules #2042

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Sep 6, 2024
Merged

Enable InternalImportsByDefault in remaining modules #2042

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
419a6f0
Enable access level on imports in remaining modules
gjcairo Aug 28, 2024
d99ca92
Regenerate protos
gjcairo Sep 3, 2024
dd2df75
Formatting
gjcairo Sep 3, 2024
ef7e527
PR fixes
gjcairo Sep 5, 2024
4a51a89
PR Fix
gjcairo Sep 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions Package@swift-6.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -550,7 +550,8 @@ extension Target {
],
swiftSettings: [
.swiftLanguageMode(.v6),
.enableUpcomingFeature("ExistentialAny")
.enableUpcomingFeature("ExistentialAny"),
.enableUpcomingFeature("InternalImportsByDefault")
]
)
}
Expand Down Expand Up @@ -950,7 +951,8 @@ extension Target {
path: "Sources/Services/Health",
swiftSettings: [
.swiftLanguageMode(.v6),
.enableUpcomingFeature("ExistentialAny")
.enableUpcomingFeature("ExistentialAny"),
.enableUpcomingFeature("InternalImportsByDefault")
]
)
}
Expand Down
12 changes: 6 additions & 6 deletions Protos/generate.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ bin_path=$(swift build -c release --show-bin-path)
protoc_gen_swift="$bin_path/protoc-gen-swift"
protoc_generate_grpc_swift="$bin_path/protoc-gen-grpc-swift"

# Genreates gRPC by invoking protoc with the gRPC Swift plugin.
# Generates gRPC by invoking protoc with the gRPC Swift plugin.
# Parameters:
# - $1: .proto file
# - $2: proto path
Expand All @@ -46,7 +46,7 @@ function generate_grpc {
invoke_protoc "${args[@]}" "$proto"
}

# Genreates messages by invoking protoc with the Swift plugin.
# Generates messages by invoking protoc with the Swift plugin.
# Parameters:
# - $1: .proto file
# - $2: proto path
Expand Down Expand Up @@ -228,8 +228,8 @@ function generate_service_messages_interop_tests {
local output="$root/Sources/InteroperabilityTests/Generated"

for proto in "${protos[@]}"; do
generate_message "$proto" "$here/tests/interoperability" "$output" "Visibility=Public" "FileNaming=DropPath"
generate_grpc "$proto" "$here/tests/interoperability" "$output" "Visibility=Public" "Server=true" "_V2=true" "FileNaming=DropPath"
generate_message "$proto" "$here/tests/interoperability" "$output" "Visibility=Public" "FileNaming=DropPath" "UseAccessLevelOnImports=true"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this the default for protobuf?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the default only if compiling with Swift 6 (it defaults to false on <6). Since we use this only in the v2 modules I could get rid of it, but I thought it was clearer. I can get rid of it though, I don't think it matters.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. I think we should be consistent with generate_grpc, so either explicitly set both or set neither.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, protobuf uses Swift 6 language mode:

https://github.com/apple/swift-protobuf/blob/beeb4142e775f40fa4162b52af32ed29ade4db1e/Sources/protoc-gen-swift/GeneratorOptions.swift#L82-L86

We default to true but ignore it in v1:

grpc-swift/Sources/protoc-gen-grpc-swift/options.swift

Line 74 in 63168d0

private(set) var useAccessLevelOnImports = true

This means if you use gRPC Swift v2 with a Swift 6 compiler but in Swift 5 language mode you get imports for gRPC but not Protobuf (unless you specify otherwise). Would you mind changing our options to match Protobuf?

Copy link
Collaborator Author

@gjcairo gjcairo Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good catch! Fixed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Half fixed 🙃

Can we have the args to generate_message and generate_grpc be consistent? Either both explicitly set "UseAccessLevelOnImports=true" or have neither of them set it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so I've been playing around with this and I think we've got a bit of a problem.
Because the compiler flag dance swift-protobuf did (and we copied) is on the language version, it doesn't matter what compiler we're using: we'll default this option to false because both have their language modes set to 5 for the plugin target.

We could get around this on gRPC and get the right default if, in the Package@swift-6.swift definition of the protoc target, we enable v6 language mode. However, this doesn't affect the swift-protobuf generation, because language mode is still v5 for swift-protobuf's plugin.

I'm starting to think we should just change the option defaults to use #if compiler instead of #if swift both here and on protobuf. I've made this change on gRPC, but for now I'll also explicitly set the options for both in generate.sh because we'll need this to change in swift-protobuf too.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I think the behaviour we really want is dependent on the language mode of the module into which the code is being generated which isn't possible for us to know.

Failing that I think doing it based on the compiler version is reasonable.

generate_grpc "$proto" "$here/tests/interoperability" "$output" "Visibility=Public" "Server=true" "_V2=true" "FileNaming=DropPath" "UseAccessLevelOnImports=true"
done
}

Expand Down Expand Up @@ -260,8 +260,8 @@ function generate_health_service {
local proto="$here/upstream/grpc/health/v1/health.proto"
local output="$root/Sources/Services/Health/Generated"

generate_message "$proto" "$(dirname "$proto")" "$output" "Visibility=Package"
generate_grpc "$proto" "$(dirname "$proto")" "$output" "Visibility=Package" "Client=true" "Server=true" "_V2=true"
generate_message "$proto" "$(dirname "$proto")" "$output" "Visibility=Package" "UseAccessLevelOnImports=true"
generate_grpc "$proto" "$(dirname "$proto")" "$output" "Visibility=Package" "Client=true" "Server=true" "_V2=true" "UseAccessLevelOnImports=true"
}

#------------------------------------------------------------------------------
Expand Down
2 changes: 1 addition & 1 deletion Sources/InteroperabilityTests/Generated/empty.pb.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

import SwiftProtobuf
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
Expand Down
4 changes: 2 additions & 2 deletions Sources/InteroperabilityTests/Generated/messages.pb.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@

// Message definitions to be used by integration test service definitions.

import Foundation
import SwiftProtobuf
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
Expand Down
2 changes: 1 addition & 1 deletion Sources/InteroperabilityTests/InteroperabilityTestCase.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
import GRPCCore
public import GRPCCore

@available(macOS 15.0, iOS 18.0, watchOS 11.0, tvOS 18.0, visionOS 2.0, *)
public protocol InteroperabilityTest {
Expand Down
4 changes: 2 additions & 2 deletions Sources/InteroperabilityTests/InteroperabilityTestCases.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@
* limitations under the License.
*/

import GRPCCore
internal import GRPCCore

import struct Foundation.Data
private import struct Foundation.Data

/// This test verifies that implementations support zero-size messages. Ideally, client
/// implementations would verify that the request and response were zero bytes serialized, but
Expand Down
4 changes: 2 additions & 2 deletions Sources/InteroperabilityTests/TestService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@
* limitations under the License.
*/

import Foundation
import GRPCCore
private import Foundation
public import GRPCCore

@available(macOS 15.0, iOS 18.0, watchOS 11.0, tvOS 18.0, visionOS 2.0, *)
public struct TestService: Grpc_Testing_TestService.ServiceProtocol {
Expand Down
2 changes: 1 addition & 1 deletion Sources/Services/Health/Generated/health.pb.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
// The canonical version of this proto can be found at
// https://github.com/grpc/grpc-proto/blob/master/grpc/health/v1/health.proto

import SwiftProtobuf
package 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
Expand Down
6 changes: 6 additions & 0 deletions Sources/protoc-gen-grpc-swift/Options.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,16 @@ struct GeneratorOptions {
private(set) var gRPCModuleName = "GRPC"
private(set) var swiftProtobufModuleName = "SwiftProtobuf"
private(set) var generateReflectionData = false

#if compiler(>=6.0)
private(set) var v2 = false
#endif

#if compiler(>=6.0)
private(set) var useAccessLevelOnImports = true
#else
private(set) var useAccessLevelOnImports = false
#endif

init(parameter: any CodeGeneratorParameter) throws {
try self.init(pairs: parameter.parsedPairs)
Expand Down
Loading