Add spi visibility #1308

FranzBusch · 2022-09-27T09:12:18Z

Motivation

The Swift compiler added @_spi() public visibility annotations in Swift 5.3 which allows you to create a system programming interface. This is often preferred to be used over @testable since @testable only works in debug modes or when passing specific compiler flags. Right now protoc-gen-swift only allows to generate internal or public visibility levels. However sometimes you want to use the generated proto types also in your test without public and without @testable

Modification

Added a new visibility modifier to the code gen plugin called _SPI(foo). I intentionally made it underscored and did not document it since it is not a stable annotation. Users that reach out to use it should know what they are doing.

Result

You can now generate types with @_spi(foo) public access level.

thomasvl · 2022-09-27T15:14:02Z

@tbkka @Lukasa thoughts?

Part of me wonders if this is sorta like #1157 and we should also add support for that if we're adding something like this.

cburrows · 2022-09-27T15:53:18Z

Sources/protoc-gen-swift/GeneratorOptions.swift

+        self = .internal
+      } else if rawValue == "Public" {
+        self = .public
+      } else if rawValue.hasPrefix("_SPI(") && rawValue.hasSuffix(")") {


I would recommend against using parens (shell special characters) here. It is unpleasant when you try to cut and past commands, or everyone needs to deal with escape characters, etc. I wonder if a separate parameter is in order, even though it's not orthogonal.

Valid point, since this should be rarely used I am unsure if making this more complicated is worth it.
I went with the parenthesis to keep it in line with how the actual attribute works, but we can also just switch it to something like SPI/Foo

cburrows · 2022-09-27T15:55:03Z

Sources/protoc-gen-swift/GeneratorOptions.swift

@@ -89,6 +114,8 @@ class GeneratorOptions {
      visibilitySourceSnippet = ""
    case .public:
      visibilitySourceSnippet = "public "
+    case .spi(let identifier):
+      visibilitySourceSnippet = "@_spi(\(identifier)) public "


Will this start spraying @_spi everywhere? Would we want it to appear only on type declarations and extension members?

It will spray everywhere and we also need it to. It is kinda like inlinability once you start adding it, everything needs to become spi.

Do we have an opinion about what happens when the string is not a swift identifier?

Users could supply bad inputs here like (); however, I don't think it is worth validating anything on our side. First, this should only be used by people who know what they want. Second, it will result in compiler errors when building the generated code.

You can use isValidSwiftIdentifier to do some very basic validation here if you want.

I think it is worth validating whether this is an identifier before we write it. Earlier errors are better.

cburrows · 2022-09-27T16:05:11Z

@thomasvl sure, why not! I suspect #1157 is more useful to a larger audience in fact

cburrows · 2022-09-27T16:49:29Z

I am thinking through scenarios here and one thing that I keep coming back to is the amount of control that is likely to be desired once you start producing API/SPI with generated pb types. A major example is availability annotation, which has the potential to be very complicated. In fact, I would probably advise most people to keep these types away from your API/SPI.

Combined with the limited audience for @_spi, I wonder if protoc-gen-swift should instead maybe produce insertion points where these annotations could go.

tbkka · 2022-09-27T17:03:53Z

Insertion points would be a good approach for this.

tbkka · 2022-09-27T17:57:50Z

@FranzBusch As a rule, all PRs should be based against main. We'll cherry-pick from there into the 1.x branch if appropriate.

thomasvl · 2022-09-27T18:01:46Z

I'd have to double check the stance on insertion points these days.

The one thing I can think of is they might interact poorly with the indexing of the sources as they are comments, so they would show up in IDEs, or prevent a real comment from getting associated with an api point, etc. 😞

tbkka · 2022-09-27T18:31:55Z

Based on protocolbuffers/protobuf#5721, it sounds like there is some interest in auditing what insertion points are provided by the existing implementations, but no concerns about the use of insertion points in principle. Since we keep getting asked about different ways to adjust the visibility modifiers on generated message types, an insertion point might be a natural answer.

FranzBusch · 2022-09-28T07:40:34Z

@thomasvl @tbkka While I understand that there are various annotations that users would want to add I think there is tremendous complexity behind this in where we want to allow such insertions points to be. Just the two examples we have here are quite different. @_implementationOnly wants to be inserted before imports where as @_spi must be inserted in front of every public declaration. Looking at a few other annotations that users might want to use: @inlinable is similar to @_spi that it must be inserted in a lot of places; however, it also requires @usableFromInline in other places that won't be public. Where only the code generator can know what these places are since they are implementation details.

Maybe it is a better to support these annotations on a one by one basis since they all have their own semantics where and how they should be used. Of course we can group them together where it makes sense like with visibility or maybe for protocolbuffers/protobuf#5721 we could introduce an import-annotations option. Let me know what you think!

# Motivation The Swift compiler added `@_spi() public` visibility annotations in Swift 5.3 which allows you to create a system programming interface. This is often preferred to be used over `@testable` since `@testable` only works in debug modes or when passing specific compiler flags. Right now `protoc-gen-swift` only allows to generate `internal` or `public` visibility levels. However sometimes you want to use the generated proto types also in your test without `public` and without `@testable` # Modification Added a new visibility modifier to the code gen plugin called `_SPI(foo)`. I intentionally made it underscored and did not document it since it is not a stable annotation. Users that reach out to use it should know what they are doing. # Result You can now generate types with `@_spi(foo) public` access level.

thomasvl · 2022-09-29T15:42:26Z

Looking around, I think the issues I was remember with insertion points is the concern of opening up too many things making maintenance of harder.

I can't say I've ever looked at writing a plugin or even using protoc's support for insertion points. So I have no idea how easy/hard that is. I'm not sure if we'd need to consider adding anything to our plugin lib to help someone write a plugin to work on insertion points in Swift.

One last though on insertion points - since we just got SwiftPM support, if this goes the insertion points path, does that imply we should support that in the SwiftPM space also? i.e. - do does build system support at all factor into any of this?

My reference to @_implementationOnly wasn't that there was direct overlap with this, just that moving into a space were we directly support some of the semi-private Swift features (both start with an underscore), maybe we should officially do them and have some defined idea when they should/shouldn't be supported since they could change in the future. As far as generally inserting inline, I don't think we should ever let developers decide that as it can have impact on the api we have to support, so that one I still see as a separate issue we might want to consider doing something about one day.

But back to @FranzBusch question - support @_spi here (and a little to @_implementationOnly) I'm likely deferring to Apple folks as you all own the this project and you all own Swift as far as adopting/encouraging the usage of these type of compiler features.

FranzBusch · 2022-09-30T08:35:40Z

One last though on insertion points - since we just got SwiftPM support, if this goes the insertion points path, does that imply we should support that in the SwiftPM space also? i.e. - do does build system support at all factor into any of this?

In my opinion, any option that we surface on the protoc-gen-swift plugin we should also surface in the SPM plugin through extending the JSON configuration. Both build systems that are building SPM plugins (Xcode and SPM) are taking the arguments that you pass into the invocation into consideration. So if you extend your config with more options it will change the generated build phase and it will re-run the generation. Is that what you meant with your question?

As far as generally inserting inline, I don't think we should ever let developers decide that as it can have impact on the api we have to support, so that one I still see as a separate issue we might want to consider doing something about one day.

Agreed, I think @inlinable and @usableFromInline needs some further thinking and I would prefer not to tackle this in same change as @_spi since the latter is more straight forward.

@tbkka @Lukasa What do you think about adding support for @_spi in general and if extending the visibility configuration is the right thing?

Lukasa · 2022-09-30T08:39:38Z

I'm more than happy to add support for @_spi.

cburrows · 2022-09-30T17:28:05Z

If we add direct support for _spi I think I would prefer a separate parameter, not to mix this up with the Visibility param with some kind of invented syntax to extract an identifier. We don't currently emit or configure for any of these _ attributes. What if say, SpiName=foo, which puts @_spi(foo) in the right places if Visibility= Public and otherwise gives an error.

ghost · 2023-07-02T05:00:47Z

Motivation

The Swift compiler added @_spi() public visibility annotations in Swift 5.3 which allows you to create a system programming interface. This is often preferred to be used over @testable since @testable only works in debug modes or when passing specific compiler flags. Right now protoc-gen-swift only allows to generate internal or public visibility levels. However sometimes you want to use the generated proto types also in your test without public and without @testable

Modification

Added a new visibility modifier to the code gen plugin called _SPI(foo). I intentionally made it underscored and did not document it since it is not a stable annotation. Users that reach out to use it should know what they are doing.

Result

You can now generate types with @_spi(foo) public access level.

thomasvl · 2023-09-11T16:39:49Z

Closing this since it's gone cold for a year, we can always reopen if this gets picked up again.

cburrows requested changes Sep 27, 2022

View reviewed changes

FranzBusch force-pushed the fb-spi-visiblity branch from 8123009 to 520408b Compare September 28, 2022 07:42

FranzBusch changed the base branch from 1_x_release_branch to main September 28, 2022 07:43

thomasvl closed this Sep 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add spi visibility #1308

Add spi visibility #1308

FranzBusch commented Sep 27, 2022

thomasvl commented Sep 27, 2022

cburrows Sep 27, 2022

FranzBusch Sep 27, 2022

cburrows Sep 27, 2022

FranzBusch Sep 27, 2022

cburrows Sep 27, 2022

FranzBusch Sep 27, 2022

tbkka Sep 27, 2022

cburrows Sep 30, 2022

cburrows commented Sep 27, 2022

cburrows commented Sep 27, 2022

tbkka commented Sep 27, 2022

tbkka commented Sep 27, 2022

thomasvl commented Sep 27, 2022

tbkka commented Sep 27, 2022

FranzBusch commented Sep 28, 2022 •

edited

thomasvl commented Sep 29, 2022

FranzBusch commented Sep 30, 2022

Lukasa commented Sep 30, 2022

cburrows commented Sep 30, 2022 •

edited

ghost commented Jul 2, 2023

Motivation

Modification

Result

thomasvl commented Sep 11, 2023

Add spi visibility #1308

Add spi visibility #1308

Conversation

FranzBusch commented Sep 27, 2022

Motivation

Modification

Result

thomasvl commented Sep 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cburrows commented Sep 27, 2022

cburrows commented Sep 27, 2022

tbkka commented Sep 27, 2022

tbkka commented Sep 27, 2022

thomasvl commented Sep 27, 2022

tbkka commented Sep 27, 2022

FranzBusch commented Sep 28, 2022 • edited

thomasvl commented Sep 29, 2022

FranzBusch commented Sep 30, 2022

Lukasa commented Sep 30, 2022

cburrows commented Sep 30, 2022 • edited

ghost commented Jul 2, 2023

Motivation

Modification

Result

thomasvl commented Sep 11, 2023

FranzBusch commented Sep 28, 2022 •

edited

cburrows commented Sep 30, 2022 •

edited