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

Full compatibility with the "ExistentialAny" upcoming feature #99

Merged
merged 7 commits into from
Jul 12, 2023

Conversation

MahdiBM
Copy link
Contributor

@MahdiBM MahdiBM commented Jul 4, 2023

Motivation

The generated code doesn't use existential any which disables the user targets of this package to enable the .enableUpcomingFeature("ExistentialAny") swift settings.

The code of the generator itself is also not compatible with the mentioned flag, which is suboptimal as this is going to be required for Swift 6.

Modifications

Always use existential any in the generated code. An exhaustive list of the protocols I found which now use existential any:

  • any Decoder
  • any Encoder
  • any ClientTransport
  • any ServerTransport
  • any ClientMiddleware
  • any ServerMiddleware

And make the code of the package itself compatible with the ExistentialAny upcoming feature.

Result

Both compatibility with users who wish to enable the ExistentialAny upcoming-feature flag on their targets, as well as being more ready for Swift 6.

Test Plan

Modified the tests to reflect this change.

Considerations

  • Moving to existential any should not break any user codes.

@simonjbeaumont
Copy link
Collaborator

@swift-server-bot add to allowlist

Copy link
Collaborator

@simonjbeaumont simonjbeaumont left a comment

Choose a reason for hiding this comment

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

@MahdiBM thanks for the contribution!

@czechboy0
Copy link
Contributor

Hi @MahdiBM, thanks for helping us prepare for Swift 6!

I'm generally in favor of this change, and I'd actually like it done even more holistically:

  • first, can you confirm that this feature is based on an accepted Swift Evolution proposal, to make sure it's not something that might still get backed out (as that'd cause a breaking change in this project)?
  • can you confirm what the lowest supported version of Swift allows explicitly specifying any for existentials?
  • if this explicit opt-in to existential any can be set in Package.swift, could you actually enable it in the integration test package as well, to verify that the generated code actually compiles with this option?
  • regarding the implementation, since the existential any could appear more than in just function parameters, but also properties, I think the any part should be thought of as part of the type name (as wrapping it happens as Wrap<any Foo> rather than any Wrap<Foo>, right?). That way, we don't need the extra property in the structured Swift representation, we just change e.g. "Encoder" to "any Encoder" when providing the type to generate.

Thanks!

Copy link
Contributor

@czechboy0 czechboy0 left a comment

Choose a reason for hiding this comment

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

See comment above – glad to see this, just a few things needed.

@MahdiBM
Copy link
Contributor Author

MahdiBM commented Jul 11, 2023

first, can you confirm that this feature is based on an accepted Swift Evolution proposal, to make sure it's not something that might still get backed out (as that'd cause a breaking change in this project)?

Generally, as i understand, the "upcoming feature" flags are all accepted and confirmed for Swift 6.
There are another type of flags called "experimental" which are not confirmed.

The proposal: https://github.com/apple/swift-evolution/blob/main/proposals/0335-existential-any.md

can you confirm what the lowest supported version of Swift allows explicitly specifying any for existentials?

The proposal mentions Swift 5.6 as the lowest version.

if this explicit opt-in to existential any can be set in Package.swift, could you actually enable it in the integration test package as well, to verify that the generated code actually compiles with this option?

We could use it in the whole generator as well, so the code of the generator itself is forced to use existential anys. That's not 100% related to the generated code though so i will still need to look into enabling the flag for integration tests as well.

regarding the implementation, since the existential any could appear more than in just function parameters, but also properties, I think the any part should be thought of as part of the type name (as wrapping it happens as Wrap rather than any Wrap, right?). That way, we don't need the extra property in the structured Swift representation, we just change e.g. "Encoder" to "any Encoder" when providing the type to generate.

I agree. Does that mean i should create a new "TypeDescription" struct instead and use that instead of just a simple string for type names in ParameterDescription?
In that case should i move everything to this new TypeDescription, or should i just use it where it's needed for now?

@czechboy0
Copy link
Contributor

czechboy0 commented Jul 11, 2023

Generally, as i understand, the "upcoming feature" flags are all accepted and confirmed for Swift 6.
There are another type of flags called "experimental" which are not confirmed.

The proposal: https://github.com/apple/swift-evolution/blob/main/proposals/0335-existential-any.md

👍

The proposal mentions Swift 5.6 as the lowest version.

Great, so we can adopt it right away, as we require 5.8.

We could use it in the whole generator as well, so the code of the generator itself is forced to use existential anys. That's not 100% related to the generated code though so i will still need to look into enabling the flag for integration tests as well.

I don't want to feature-creep your PR too much, but if you try to enable it across the board in the project, even for the generator code itself, and it doesn't require too many changes, let's do it all here. If it requires any non-trivial changes, we'll break it out into a separate issue, and just focus this PR on adding any to generated code.

I agree. Does that mean i should create a new "TypeDescription" struct instead and use that instead of just a simple string for type names in ParameterDescription?
In that case should i move everything to this new TypeDescription, or should i just use it where it's needed for now?

Let's just use a simple string for now. After re-reading SE-335, the any keyword is very much part of the type name, so I don't think we need to break it out just yet. If we need to, we can always do it in the future, but let's do the simplest thing that gets the job done for now.

@MahdiBM
Copy link
Contributor Author

MahdiBM commented Jul 11, 2023

it doesn't require too many changes, let's do it all here

Yeah it's not a problem

Let's just use a simple string for now.

👍

@MahdiBM
Copy link
Contributor Author

MahdiBM commented Jul 11, 2023

@czechboy0 I can add the upcoming flag to the swift build command (in the CI script), but i think you should just add it to the swift settings of targets of the test package in Package.swift (like in this PR)... Do you want to do that or should i just add it to the swift build command?

@czechboy0
Copy link
Contributor

I think Package.swift is the right place. Would you also be open to adding this to swift-openapi-runtime, and possibly swift-openapi-urlsession and swift-openapi-async-http-client? Feel free to say no, and I'll file it as a separate issue, I just like the change and would like it to have across the board 🙂

@MahdiBM
Copy link
Contributor Author

MahdiBM commented Jul 11, 2023

Sure that's no problem 🙂

@MahdiBM
Copy link
Contributor Author

MahdiBM commented Jul 12, 2023

@czechboy0
After noticing the docker compose files i see i can add the "existential any" flag to those. The other way, as i mentioned right above, is to add the flag to the that test-project the CI seems to be pulling which i don't think is public? (please correct me if i'm wrong)
I think adding the flag to CI might be better but i'm not sure at all so feel free to disagree.

One question, is this PR waiting on me adding existential-any to the other projects as well? Because i don't think this PR should need any of those for it to be merged (it doesn't make any of those fail to build/compile or anything. It's completely "backward compatible" as we're already using it with no-existential-any openapi-runtime and async-http-client-generator)

@MahdiBM MahdiBM changed the title Use existential any in generated code Full compatibility with the "ExistentialAny" upcoming feature Jul 12, 2023
@czechboy0 czechboy0 merged commit 8c5258b into apple:main Jul 12, 2023
@czechboy0
Copy link
Contributor

Thanks, @MahdiBM! 👏

simonjbeaumont pushed a commit to swift-server/swift-openapi-async-http-client that referenced this pull request Jul 13, 2023
simonjbeaumont pushed a commit to apple/swift-openapi-urlsession that referenced this pull request Jul 13, 2023
czechboy0 added a commit to apple/swift-openapi-runtime that referenced this pull request Jul 17, 2023
Adopt explicit existential any

### Motivation

As a follow-on to apple/swift-openapi-generator#99, let's also enable explicit existential any in the runtime library.

### Modifications

Enabled the flag in Package.swift (for Swift 5.9 and newer) and updated all the usage sites.

### Result

The codebase is closer to being ready for Swift 6.

### Test Plan

Updated unit tests as well, all is passing locally.


Reviewed by: simonjbeaumont

Builds:
     ✔︎ pull request validation (5.8) - Build finished. 
     ✔︎ pull request validation (5.9) - Build finished. 
     ✔︎ pull request validation (docc test) - Build finished. 
     ✔︎ pull request validation (integration test) - Build finished. 
     ✔︎ pull request validation (nightly) - Build finished. 
     ✔︎ pull request validation (soundness) - Build finished. 
     ✖︎ pull request validation (api breakage) - Build finished. 

#26
@czechboy0 czechboy0 added the semver/none No version bump required. label Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/none No version bump required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants