Skip to content
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

Fix Foundation import's access level in generated code #1701

Merged
merged 1 commit into from
Aug 28, 2024

Conversation

gjcairo
Copy link
Contributor

@gjcairo gjcairo commented Aug 28, 2024

#1683 added support for access level modifiers on imports in the generated code.

Foundation (which is needed because Data is used extensively in generated protos) is imported by manually writing to the file instead of using the "compute imports" logic we use to deal with all other imports. This is because other imports are all imports coming from other generated proto files, whereas Foundation is "pure Swift" dependency.

We forgot to add the access level modifier to this Foundation import.
This causes issues when enabling the InternalImportsByDefault flag on modules that use generated protos (even when swift-protobuf's UseAccessLevelOnImports option is enabled), because Foundation will be imported as internal, while we expose public properties of type Data in the generated code.

This PR adds the right import Foundation statement based on the import directive.

@thomasvl
Copy link
Collaborator

@gjcairo can you also test this option with a public import file? Glancing back the way we re-export things it seems like that might not be right, as won't it put public on things where things would otherwise be package or internal and is that the correct net result?

@gjcairo
Copy link
Contributor Author

gjcairo commented Aug 28, 2024

@thomasvl according to the proposal, @_exported imports must be followed by public if using access level modifiers:

With this proposal, @_exported is accepted only on public import declarations, both with the modifier or the default public visibility in current language modes.

So I'd say this is the correct behaviour.

@thomasvl thomasvl merged commit be11d18 into apple:main Aug 28, 2024
10 checks passed
@gjcairo gjcairo deleted the fix-import-foundation-access-level branch August 28, 2024 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants