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

Explicitly annotate existential usage of Message with the existential any keyword. #1509

Closed
wants to merge 1 commit into from

Conversation

mrabiciu
Copy link

@mrabiciu mrabiciu commented Nov 22, 2023

This PR separates out the addition of the existential any from #1504 into its own distinct PR for easier consumption.

Context: #1504 adds Self constraints to Message with the addition of static var _fields: Field<Self>. With this change the swift compiler will require uses of the existential Message type to be explicitly annotated with the any keyword. This keyword is currently optional for existential types without Self constraints however it will be required in swift 6.

@thomasvl
Copy link
Collaborator

Can you expand the description for why this is needed (for history, etc.).

And looking at the other PR, public struct Field<M: Message> { ... } doesn't both with any there, which seems to say not all uses of Message need to change to any Message, so what were the concrete requirements and are all these changes actually needed?

@mrabiciu mrabiciu changed the title Prefix uses of Message with any Explicitly annotate existential usage of Message with the existential any keyword. Nov 22, 2023
@mrabiciu
Copy link
Author

Updated title and description

@FranzBusch
Copy link
Member

This keyword is currently optional for existential types without Self constraints however it will be required in swift 6.

This statement is no longer true. The language workgroup has recently put out a post where they stated that ExistentialAny is no longer going to become a default feature in Swift 6. So for now we should put this on hold.

Can you explain why your other PR needed this to be changed everywhere?

@mrabiciu
Copy link
Author

mrabiciu commented Nov 27, 2023

This statement is no longer true. The language workgroup has recently put out a post where they stated that ExistentialAny is no longer going to become a default feature in Swift 6

Ah TIL good to know!

Can you explain why your other PR needed this to be changed everywhere?

I'm adding a static var _fields: [Field<Self>] { get } property to the Message protocol, which means this protocol now has Self constraints. With this change swift requires existential uses of Message to be explicitly annotated with the existential any.

Without this the compiler complains Use of protocol 'Message' as a type must be written 'any Message'

Copy link
Contributor

@tbkka tbkka left a comment

Choose a reason for hiding this comment

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

I see a lot of any Message.Type and similar in here. I believe that is wrong -- Message.Type is not being used as an existential here, so the any is not appropriate.

Sources/Conformance/main.swift Show resolved Hide resolved
Sources/SwiftProtobuf/BinaryDecoder.swift Show resolved Hide resolved
Sources/SwiftProtobuf/Decoder.swift Show resolved Hide resolved
@mrabiciu
Copy link
Author

mrabiciu commented Nov 27, 2023

I see a lot of any Message.Type and similar in here. I believe that is wrong -- Message.Type is not being used as an existential here, so the any is not appropriate.

@tbkka All of the places that I've annotated with any were labeled by the compiler as required when combining with my changes from #1509. Including uses of Message.Type.

Screenshot 2023-11-27 at 3 34 50 PM

Just like how trying to use Equatable.Type would also require an any. I think for this to not be an existential use of Message you'd need the above to be expressed as

public mutating func decodeExtensionFieldsAsMessageSet<M: Message>(
    values: inout ExtensionFieldValueSet,
    messageType: M.Type
)

@tbkka
Copy link
Contributor

tbkka commented Nov 27, 2023

Yep. My mistake.

@FranzBusch
Copy link
Member

In general, I am happy with adopting ExistentialAny in this package since the language steering group has just deferred it to the next language revision. However, for consistency reason I would like to enable the upcomingFeatureFlag in our Package.swift then and just stick it everywhere. This way the compiler is going to enforce it for us.
@thomasvl @tbkka WDYT?

@thomasvl
Copy link
Collaborator

In general, I am happy with adopting ExistentialAny in this package since the language steering group has just deferred it to the next language revision. However, for consistency reason I would like to enable the upcomingFeatureFlag in our Package.swift then and just stick it everywhere. This way the compiler is going to enforce it for us. @thomasvl @tbkka WDYT?

Having the validation sounds good to me.

@tbkka
Copy link
Contributor

tbkka commented Nov 28, 2023

@tbkka WDYT?

Sounds like a good plan to me. Let's do that.

@mrabiciu
Copy link
Author

mrabiciu commented Dec 1, 2023

I found a workaround for this in #1504 so this is no longer necessary for me. LMK if I should close this. I'm happy to merge it too if you'd like.

@thomasvl
Copy link
Collaborator

thomasvl commented Dec 1, 2023

I think we're good landing this, just need to do @FranzBusch 's suggestion to enable the workings in the Package.swift file.

thomasvl added a commit to thomasvl/swift-protobuf that referenced this pull request Dec 5, 2023
Enable the feature on all targets, and then go through doing the fixits within
Xcode 15 that are triggered as a result.

This basically replaces apple#1509
thomasvl added a commit to thomasvl/swift-protobuf that referenced this pull request Dec 5, 2023
Enable the feature on all targets, and then go through doing the fixits within
Xcode 15 that are triggered as a result.

This basically replaces apple#1509
@mrabiciu mrabiciu closed this Dec 5, 2023
thomasvl added a commit that referenced this pull request Dec 6, 2023
Enable the feature on all targets, and then go through doing the fixits within
Xcode 15 that are triggered as a result.

This basically replaces #1509
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants