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

Default UseAccessLevelOnImports to false #2047

Merged
merged 5 commits into from
Sep 9, 2024

Conversation

gjcairo
Copy link
Collaborator

@gjcairo gjcairo commented Sep 6, 2024

Motivation

After the discussion on #2042 (comment), I played a bit more with InternalImportsByDefault and the different generator options, got to the following conclusions:

  • If you add import Foo to some file, where Foo is also being imported with an explicit internal access-level modifier in the generated code, it will work as long as InternalImportsByDefault is enabled.
  • If you disable InternalImportsByDefault for the corresponding target in Package.swift, you get an "Ambiguous implicit access level for import of 'Foo'; it is imported as 'internal' elsewhere" error (not a warning). This means that if the code generator plugin(s) begin adding the access level modifiers by default based on how they're built, they could cause source-breakages for users unintentionally.
  • This isn't any different between language mode 5 or 6 - I tried changing the target's language mode and the behaviour is the same as described above in either case.

Given all this, defaulting UseAccessLevelOnImports to false always for now may be the easiest (and least surprising, from a users' perspective) thing to do, until InternalImportsByDefault are enabled by default in a future Swift >6.0 version (as the proposal states), where we can default to true again:

#if compiler(>=6.x) // where x is the version where internal imports by default is enabled
// default to true
#else
// default to false
#endif

The rationale behind this is that adding access levels to imports on your code is currently totally optional. If you choose to start adding them explicitly, then it's okay to also have to tell your tooling/generators that they should also add them explicitly. If you don't, they'll keep generating things the exact same way they've been doing it, which is what users of the generator would expect.

Modifications

  • Default UseAccessLevelOnImports to false always.
  • Regenerate protos
  • Remove InternalImportsByDefault from test and executable targets, since it doesn't make a lot of sense to have access level modifiers on imports here anyways as these targets cannot be imported.

It doesn't make a lot of sense to specify access level modifiers on imports if the targets cannot be imported
@gjcairo gjcairo added the v2 A change for v2 label Sep 6, 2024
@gjcairo gjcairo requested a review from glbrntt September 6, 2024 15:20
Copy link
Collaborator

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

Thanks Gus. Can I get you to change one tiny bit too, can you remove the explicit UseAccessLevelOnImports=false from Sources/GRPCCore/Documentation.docc/Tutorials/Route-Guide/Resources/route-guide-sec03-step04-gen-grpc.txt?

@gjcairo gjcairo enabled auto-merge (squash) September 9, 2024 11:16
@gjcairo gjcairo merged commit d3ef09d into grpc:main Sep 9, 2024
15 of 17 checks passed
@gjcairo gjcairo deleted the default-access-level-import-plugin branch September 9, 2024 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v2 A change for v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants