Add support for Protobuf Editions #284
Conversation
rebello95
force-pushed
the
rebello-editions
branch
from
3156a2f
to
f231ccd
Compare
| override var outputFileExtension: String { | ||
| ".connect.swift" | ||
| } | ||
|
|
There was a problem hiding this comment.
This is the default extension in the super-class. Maybe omit this?
There was a problem hiding this comment.
I noticed this as well. Is the Generator ever initialized and used on its own? It seems like if it's just a superclass that holds some core/shared functionality, perhaps we could even consider putting an assertion or something in that superclass outputFileExtension implementation and specify that it must be overridden in subclasses?
There was a problem hiding this comment.
Is the Generator ever initialized and used on its own?
It's not, I added a comment to clarify this as well
perhaps we could even consider putting an assertion or something in that superclass outputFileExtension implementation and specify that it must be overridden in subclasses?
This sounds good to me -- I'd rather do that than omit it in subclasses so that it's explicit in each generator. Updated
| } | ||
|
|
||
| public var supportedEditionRange: ClosedRange<SwiftProtobuf.Google_Protobuf_Edition> { | ||
| return SwiftProtobufPluginLibrary.DescriptorSet.bundledEditionsSupport |
There was a problem hiding this comment.
This seems potentially dangerous: a future edition could introduce service or method-specific changes that need to be handled by the plugin. In such a case, if this plugin code (without the necessary updates) were paired with a newer version of the runtime (that was updated to support that edition) then it would report that the edition is supported even though it may generate incorrect code.
While it's a bit of a maintenance nuisance to have to manually bump the max supported edition, I think that's the safest way to implement this.
There was a problem hiding this comment.
Fair point, updated
| DescriptorSet.bundledEditionsSupport.lowerBound, Google_Protobuf_Edition.legacy | ||
| ) | ||
| let maxEdition = min( | ||
| DescriptorSet.bundledEditionsSupport.upperBound, Google_Protobuf_Edition.edition2024 |
There was a problem hiding this comment.
It is sadly confusing in descriptor.proto, which shows 2024 as the latest edition. But 2024 isn't actually fully-defined yet. It's an experimental edition that is still in progress.
If you try to compile a file with it (using buf or the latest v27.2 of protoc) you'll get the following:
$ echo 'edition = "2024";' > test.proto
$ protoc test.proto -o /dev/null
test.proto:1:1: Edition 2024 is later than the maximum supported edition 2023
$ buf build
test.proto:1:11:edition value "2024" not recognized; should be one of ["2023"]
(You can try out 2024 in protoc by passing the --experimental_editions flag.)
Since it's incomplete (and 2023 changed quite a bit between early experimental releases like v24 and the final version in v27), we probably should hold off and use 2023 here.
There was a problem hiding this comment.
Ah, got it. This was also failing without the min here for the same reason, but that makes sense to not support 2024 until it's done. Updated
rebello95
force-pushed
the
rebello-editions
branch
from
46393da
to
acedf12
Compare
updates Update Generator.swift Signed-off-by: Michael Rebello <[email protected]>
Signed-off-by: Michael Rebello <[email protected]>
Signed-off-by: Michael Rebello <[email protected]>
Signed-off-by: Michael Rebello <[email protected]>
Signed-off-by: Michael Rebello <[email protected]>
rebello95
force-pushed
the
rebello-editions
branch
from
acedf12
to
26aff6d
Compare
|
(Had to rebase for DCO) |
This PR adds support for Protobuf Editions to Connect-Swift plugins. There is no change to generated outputs in this PR, but it adds
supportsEditionsandsupportedEditionRangespecifications in theGeneratorclass to ensure the Connect plugins can run with Protobuf files that use Editions.SwiftProtobuf also deprecated some usages of
Google_Protobuf_Compiler_CodeGeneratorResponse:As part of these changes, I migrated over to using
CodeGeneratorin order to be able to specify the new Editions-related variables mentioned above.