Support access level on import statements

[Skoti]

This PR brings in the possibility to define access level on import statements generated through ProtoPathModuleMappings plugin option.
Access level on import statements is a new feature of Swift 6, but it can already be enabled in Swift 5.9 builds.

See https://github.com/swiftlang/swift-evolution/blob/main/proposals/0409-access-level-on-imports.md

[thomasvl]

This feels like a lot of overlap with #1393 that added @_implementationOnly support, at a minimum it would need to be rationalize with that so one can't mix the options and get invalid Swift code.

I'm also not sure this can be done on a per module bases. If a given .proto file imports another one, then it's going to use the types for fields/adding extension fields/etc. So it seems like there would have to be some alignment between the imports and how the file being generated will declare things. i.e. - if the file is generating with public visibility, then can it really do internal or private on the import statements for modules for types likely needed?

@FranzBusch @gjcairo your thoughts in this space since I think you were the ones to look the most at the impl only support.

[Skoti]

if the file is generating with public visibility, then can it really do internal or private on the import statements for modules for types likely needed?

It can't as such Swift code wouldn't compile, however, to me this is simply a misconfiguration. But yeah I could add some validation steps if that's required.

This feels like a lot of overlap with #1393 that added @_implementationOnly support, at a minimum it would need to be rationalize with that so one can't mix the options and get invalid Swift code.

That's right. Basically, the access level you can use depends on the level of types you generate.
If the files are generated with public access you can only use @_exported public import or public import.
If the files are generated with package access (it's not possible right now in SwiftProtobuf), you can use the same as above + package import.
If the files are generated with internal access you can use the same as above + internal import.

We could go further and make ProtoPathModuleMappings and ImplementationOnlyImports options compatible.
What ImplementationOnlyImports does right now is that it globally adds @_implementationOnly which means it changes the default statement from import to @_implementationOnly import for all imports.

We could introduce DefaultImportAccessLevel option that would do the same but allow you to specify what will be the default access level (not only one hardcoded) for all imports, and the ProtoPathModuleMappings would allow you to override this behavior for the specified modules.

ImplementationOnlyImports would be deprecated and translated to DefaultImportAccessLevel with @_implementationOnly for backward compatibility.

Any configurations that would produce invalid Swift code could either be validated or left to fail in compile-time. The choice is yours.

[thomasvl]

I don't think I immediately see how this is how this is a per module decision then, seems like it would make more sense to module model it just a single option since it has to line up with the generated access level. Maybe even just a boolean to say if access level imports should be generated and they should then be inferred from the generated access level?

[Skoti]

I don't think I immediately see how this is how this is a per module decision then, seems like it would make more sense to module model it just a single option since it has to line up with the generated access level. Maybe even just a boolean to say if access level imports should be generated and they should then be inferred from the generated access level?

This would restrict what you can do with access level on imports.
It must be per module, because you may wish to expose each imported module differently.
For example, this should be possible in the generated pb.swift file if the access is internal for the generated proto:

@_exported public import ModuleWithReExportedProtos
public import ModuleWithProtosWithPublicVisibility
import InternalProtos

struct ProtoGeneratedAsinternal {
  var a: MessageFromReExportedModule
  var b: MessageFromPublicModule
  var c: MessageFromInternalModule
}

[FranzBusch]

I agree with @thomasvl here that I also don't fully see how this can be done on a per module level. In your last example why would you publicly import a module if the generated protos are internal? Can't we simply this and say that the generated access level should match the import access level? (Taking @_implementationOnly out of the question for a second)

[Skoti]

Can't we simply this and say that the generated access level should match the import access level? (Taking @_implementationOnly out of the question for a second)

We can't because the import access level means much more. You may still want to expose your transitive dependencies differently.
It's the same answer, doing this would restrict how you could use the access levels.

Imagine a scenario like this:

PublicProtos <- Main <- end-client

• A module named PublicProtos contains a set of protos that are shared publicly.
• A module named Main depends on PublicProtos - it has public APIs that can take/return protos from PublicProtos, and it contains internal protos (the generated ones in question) some of which rely on protos from PublicProtos.
• An end-client that uses Main and transitively PublicProtos

Now, because you're using PublicProtos in public APIs of Main you must import them at least with public import.
This means the internal protos you generate for the Main module also need to use public import, otherwise you'll get an error saying:
Ambiguous implicit access level for import of 'ProtoName'; it is imported as 'public' elsewhere

The same is if you additionally wish to re-export PublicProtos types from the Main module, so the end-client doesn't have to rely on importing PublicProtos as well, they can simply import Main.
In that case you would need to import PublicProtos inside Main module with @_exported public import, and the same kind of import must be used in the generated internal protos even though they're internal - otherwise you'll get the "ambiguous access level" error.

In the future the Main module may depend on another one named MyProtos, but this one will be used internally only, so you don't want to use public import.

So this is module-based, you decide how each dependency is exposed to the end-client.

[thomasvl]

Can you post a small SwiftPM package example showing the case you say is getting a warning? You should be able to do it with some trivial sources that don't involve protos, just basic Swift. It would be safer to make sure everyone is considering the same cases when discussion things.

Is the warning the same between Swift 5.x and Swift 6? It's possible the compiler has tweaked the behavior between versions.

I'm also sorta curing about the mention of implicit in that error message. It makes me wonder if an explicit internal for the generated proto sources would solve the problem.

[FranzBusch]

Now, because you're using PublicProtos in public APIs of Main you must import them at least with public import.
This means the internal protos you generate for the Main module also need to use public import, otherwise you'll get an error saying:
Ambiguous implicit access level for import of 'ProtoName'; it is imported as 'public' elsewhere

I think that's the key here and I wasn't aware that restriction existed. Now that makes total sense. The access level of imports is checked module wide and not on a per file basis.

[Skoti]

I'm also sorta curing about the mention of implicit in that error message. It makes me wonder if an explicit internal for the generated proto sources would solve the problem.

Oh, I have missed that, yes explicit internal solves this! Thanks, for pointing that out!
I wonder if this is the same in Swift 6. I'm using Swift 5.10 but I did enable InternalImportsByDefault 🤷‍♂️.

The access level of imports is checked module wide and not on a per file basis.

True, but I've just found out they're reducing it to the most visible declaration, and they only validate that declarations are not relying on imports with a lower access.

And I've found this in the proposal as a confirmation, and it works in the code too!

The same dependency can be imported with different access levels by different files of a same module. At the module level, we only take into account the most permissive access level. For example, if a dependency is imported as package and internal from two different files, we consider the dependency to be of package visibility at the module level.

Current type-checking enforces that declaration respect their respective access levels. It reports as errors when a more visible declaration refers to a less visible declaration.

Ok, I think we can go with a simpler approach here as was suggested. Would you like me to modify this PR?

[FranzBusch]

Ok, I think we can go with a simpler approach here as was suggested. Would you like me to modify this PR?

Yes please. I think we can just match the generated access level

[Skoti]

Maybe even just a boolean to say if access level imports should be generated and they should then be inferred from the generated access level?

Yes please. I think we can just match the generated access level

Done ✅
Please take a look 🙂

[thomasvl]

Can you squash the commits since I don't think we need the first one any more since it is a different approach?

I think you might be able to make some examples in https://github.com/apple/swift-protobuf/tree/main/PluginExamples by explicitly enabling the features in the Package.swift` files to actually have checked usages of this. Does it still work with 5.8 or does it need newer Swift versions to allow us to do that?

[Skoti]

Does it still work with 5.8 or does it need newer Swift versions to allow us to do that?

It's available since Swift 5.9, but you need to enable it manually in the hosting package/project.
Example for swift package:

            swiftSettings: [
                .enableExperimentalFeature("AccessLevelOnImport"),
                .enableUpcomingFeature("InternalImportsByDefault") // optionally this too
            ]

[gjcairo]

@Skoti thanks for this, looks great. Just left a small comment about adding some clarification around the options to the READMEs.

@thomasvl can you trigger CI on the PR? I don't seem to have the required privileges.

[Skoti]

I've updated and applied all your remarks, please take a look.

I think you might be able to make some examples in https://github.com/apple/swift-protobuf/tree/main/PluginExamples by explicitly enabling the features in the Package.swift` files to actually have checked usages of this.

I've also noticed that the Simple and Nested scenarios are the same, you may want to remove or alter one of them.

[Skoti]

I've missed fixing some switch expressions to compile for Swift 5.8, it should be all now.

[Skoti]

it now fails on

Run make test-spm-plugin PROTOC=../protobuf/cmake_build/protoc
env PROTOC_PATH=/__w/swift-protobuf/swift-protobuf/protobuf/cmake_build/protoc-29.0.0 swift test --package-path PluginExamples
error: 'pluginexamples': product 'SwiftProtobuf' required by package 'plugin examples' target 'Simple' not found in package 'swift-protobuf'.
make: *** [Makefile:201: test-spm-plugin] Error 1
Error: Process completed with exit code 2.

not sure why, the target is there 🤔

[gjcairo]

@thomasvl can we try re-triggering the CI? This works locally for me.

[thomasvl]

@thomasvl can we try re-triggering the CI? This works locally for me.

Triggered a rerun of the failed one and it failed again.

[thomasvl]

Thank you!