Mark operation output and request/response body enums as @frozen

[PadraigK]

Since it's not expected that the generated code should support library evolution, we could add @Frozen to all of the generated enums. This would greatly improve the ergonomics of switching over enums in the generated code.

Borrowing an example from another issue:

switch result {
case let .ok(reponse):
    case let .json(payload):
        print(payload.body, payload.headers)
    @unknown default:
        break
    }
@unknown default:
    break
} 

would become

switch result {
case let .ok(reponse):
    case let .json(payload):
        print(payload.body, payload.headers)
    }
} 

[simonjbeaumont]

@PadraigK thanks for the suggestion.

We do generate @frozen enums in a bunch of places already¹²³ so potentially we might be able to do it for the response body enum too.

@czechboy0 might have some opinions here. Assuming there's no reason not to, is it something you'd want to work on?

Footnotes

1. https://github.com/apple/swift-openapi-generator/blob/ec7a1c5/Tests/OpenAPIGeneratorReferenceTests/Resources/ReferenceSources/Petstore/Types.swift#L94 ↩

2. https://github.com/apple/swift-openapi-generator/blob/ec7a1c5/Tests/OpenAPIGeneratorReferenceTests/Resources/ReferenceSources/Petstore/Types.swift#L219 ↩

3. https://github.com/apple/swift-openapi-generator/blob/ec7a1c5/Tests/OpenAPIGeneratorReferenceTests/Resources/ReferenceSources/Petstore/Types.swift#L430 ↩

[czechboy0]

Yeah we should add it to all our enums, I don't think there are any downsides.

[simonjbeaumont]

Great, should be a simple change in translateRequestBodyInTypes. I'll do it on Monday, unless someone else beats me to it.

[simonjbeaumont]

@PadraigK I've opened a PR for this. I retitled the issue to make it a little clearer for posterity, because there's a bunch of enums we generate which don't have any cases (just namespace types).

Thanks again for taking the time to try out the generator and for filing the issue!

[simonjbeaumont]

I discussed this some more with some folks and there's some feedback that we might not want to do this.

To summarise, they highlighted that adopters should only be forced to add the @unknown default case if their target is being compiled with library evolution and this isn't something we should be encouraging. They also suggest that, since @frozen is used for library evolution use cases, using it anywhere makes an implication that we're in the library-evolution game, which we are not.

As I highlighted earlier in this issue thread, we are currently already using @frozen in a couple of places. These are for enum (OpenAPI enum) and OneOf types.

I think we should take a consistent approach here. My PR moved us in the direction of consistent application of @frozen to all places where we generate a Swift enum with cases. But I think we should also consider whether this was completely wrong, and we should instead be removing it everywhere?

//cc @czechboy0 @FranzBusch @dnadoba

[dnadoba]

Since it's not expected that the generated code should support library evolution, we could add @Frozen to all of the generated enums.

@PadraigK @frozen only has an effect if library evolution mode is turned on:

When the compiler isn’t in library evolution mode, all structures and enumerations are implicitly frozen, and this attribute is ignored.
https://docs.swift.org/swift-book/documentation/the-swift-programming-language/attributes/#frozen

Adding @frozen is a no-op without library evolution mode turned on and @unknown default is not required. Are you sure you haven't turned on library evolution mode accidentally? How do you integrate/build the generated code and the OpenAPI runtime library? Note that library evolution mode is also known as "Build Libraries for Distribution" (BUILD_LIBRARY_FOR_DISTRIBUTION) in Xcode.

[czechboy0]

I'm for adding it everywhere, since it's a no-op to code not using library evolution, and solves a major usability issue for folks using library evolution. It's asymmetrical, so for pragmatic reasons here, I'd do the thing that unblocks a set of adopters, and doesn't hurt other adopters.

That said, we should probably also add a paragraph to our docs about how to use generated code, and mention that the generated code is not designed for having a stable ABI (like we already call out the lack of a stable API).

[FranzBusch]

I'm for adding it everywhere, since it's a no-op to code not using library evolution, and solves a major usability issue for folks using library evolution

I don't think I agree with this. The problem is if we start adding annotations for library evolution such as @frozen it conveys the feeling that we are indeed supporting ABI stability in both the generated code but also our runtime library. This is simply not true and would be a significant piece of work. While adding @frozen is silencing some of the errors and warnings for people it doesn't remove the danger of ABI mismatches at runtime.

However, adopters do have a way out of this. When they want to ship an ABI stable framework that uses generated code and the runtime they just need to make sure that all imports are marked with @_implementationOnly. This should make sure to hide the dependency for them and that they don't use any of the types in their public interface. We might have to do some work to allow adopters to add @_implementationOnly to some of the imports we generate but that is safer than adding @frozen to our types.

[czechboy0]

The problem is if we start adding annotations for library evolution such as @Frozen it conveys the feeling that we are indeed supporting ABI stability...

The argument makes sense, but it seems other Apple packages do exactly this – have @frozen annotations without promising stable ABI, only discussing API stability, and leaving it to the LE-enabled library author to do the right thing and ensure a stable ABI. Some examples (a non-exhaustive list):

• swift-async-algorithms: source stability, use of frozen
• swift-atomics: source stability, use of frozen
• swift-http-types: readme (no mention of API stability pre-1.0), use of frozen
• swift-crypto: use of frozen
• swift-system: source stability, use of frozen

Based on the examples above, it seems okay to me to both not explicitly support a stable ABI, but also help eventual ABI maintainers convey the right meaning about allowed changes to types.

...supporting ABI stability in both the generated code but also our runtime library.

For the generated code, we explicitly warn against assuming stable API in our docs, and I proposed adding an explicit section about the ABI as well, removing any ambiguity.

For the runtime library code, we also document the API stability guarantees here, and here too, we could add an explicit counter-promise of ABI stability.

While adding @Frozen is silencing some of the errors and warnings for people...

I don't think it's about the warnings and errors, it's about having to cover codepaths that will never be taken at runtime, artificially increasing the cyclomatic complexity of every adopter who pulls in a library with LE enabled. Adopters can just as well add fatalError() in all those @unknown default: cases and it will never crash, but the question is – why ask the adopters to do this, if at compile time, it's known that those codepaths won't be taken?

doesn't remove the danger of ABI mismatches at runtime

That's correct, but more than the responsibility of every library, that's the responsibility of the author of the LE-enabled library. The author is responsible for maintaining a stable ABI, even if they republish types from their dependencies.

For example, they could evolve their LE-enabled library in a way that they never update their dependencies within the same major version of their LE-enabled library, guaranteeing that only their hand-written code needs to be ABI-stable. We don't know, but as evidenced by the other Apple packages, I don't think the burden of enforcing this is on each library, but instead is on the LE-enabled library author.

Re @_implementationOnly, that would indeed help if they use the generated code as a dependency, but don't publish it in their public API. We recommend against it in our docs, as it's difficult to do correctly, but we also don't take steps to block people from doing it, because if someone is being careful, and their use case requires it, they can successfully do it. This frozen question falls into the same camp for me – we recommend that folks try to avoid it, as there are some gotchas, but for those who really understand the area, I don't see a reason to take steps to prevent them from having an LE-enable library with public generated types (maybe their OpenAPI doc changes once a year and they'll publish a new major version of the library).

[FranzBusch]

After talking with @czechboy0 about this offline. I am fine with us adopting the @frozen attributes if we properly document that neither the generated code nor the runtime library should be distributed in LE-enabled frameworks without using @_implementationOnly and hiding the types from the public API.

One suggestion though might be that we provide a configuration option which enables the generation of @frozen attributes so that by default users don't get that and we can even put that behind an unsafe or experimental flag. But I leave that up to @czechboy0 and @simonjbeaumont.

[simonjbeaumont]

One suggestion though might be that we provide a configuration option

We've not really added any configuration options so far, but this is not the first issue that could justify adding one. I'd be open to us starting to use it more.

[czechboy0]

I think we should think about how we could add configuration for cases where users genuinely need both options, in a backwards-compatible way. Here, I'm not sure it's needed yet, as always adding @frozen covers both sets of users (as without LE, it's ignored), the only difference is strictness.

As you can imagine, the cost of each configuration option in the generator is adding a whole axis to the test matrix, so it's not cheap, and each of them need to be well justified. I'm not saying this one isn't, just that we should start without it (make the behavior consistent, add @frozen to the missing places, since we already add it some places today), and later if we still feel not generating them is a valuable feature, add an opt-in configuration option to skip them.

[simonjbeaumont]

So where are we landing on this one? Should we merge #109 as-is (i.e. generating @frozen by default, with no configuration option)? //cc @FranzBusch @czechboy0