Enable InternalImportsByDefault in remaining modules
#2042
Conversation
|
|
||
| for proto in "${protos[@]}"; do | ||
| generate_message "$proto" "$here/tests/interoperability" "$output" "Visibility=Public" "FileNaming=DropPath" | ||
| generate_message "$proto" "$here/tests/interoperability" "$output" "Visibility=Public" "FileNaming=DropPath" "UseAccessLevelOnImports=true" |
There was a problem hiding this comment.
Isn't this the default for protobuf?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Gotcha. I think we should be consistent with generate_grpc, so either explicitly set both or set neither.
There was a problem hiding this comment.
Oh, protobuf uses Swift 6 language mode:
We default to true but ignore it in v1:
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?
There was a problem hiding this comment.
Ah, good catch! Fixed.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| for proto in "${protos[@]}"; do | ||
| generate_message "$proto" "$here/tests/interoperability" "$output" "Visibility=Public" "FileNaming=DropPath" | ||
| generate_message "$proto" "$here/tests/interoperability" "$output" "Visibility=Public" "FileNaming=DropPath" "UseAccessLevelOnImports=true" |
There was a problem hiding this comment.
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?
gjcairo
force-pushed
the
explicit-access-level-imports
branch
from
90fce86 to
02f4c93
Compare
gjcairo
force-pushed
the
explicit-access-level-imports
branch
from
02f4c93 to
4a51a89
Compare
## 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.
## Motivation After the discussion on grpc/grpc-swift#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.
## Motivation After the discussion on grpc/grpc-swift#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.
2 participants
This PR enables
InternalImportsByDefaultonGRPCHealthandInteroperabilityTests.It also regenerates the protos with the latest version of swift-protobuf (v1.28.1), and increases its min version to 1.28.1 to make sure that we don't get build failures when using earlier generators.