Skip to content

Conversation

@Iskander508
Copy link
Contributor

@Iskander508 Iskander508 commented Mar 22, 2025

Identifying enums that conform to the CaseIterable protocol is relatively simple, so we probably can autogenerate the swift protocol conformance code without any risks.

Potentially we could add a new config toggle specifically for this protocol.

@Iskander508 Iskander508 requested a review from a team as a code owner March 22, 2025 17:07
@Iskander508 Iskander508 requested review from mhammond and removed request for a team March 22, 2025 17:07
@mhammond
Copy link
Member

so we probably can autogenerate the swift protocol conformance code without any risks.

I'm interested to know why this wouldn't need a config option whereas, say, Codable does? This isn't to say I think it should, but rather I'm trying to work out where we would draw the line?

@Iskander508
Copy link
Contributor Author

Iskander508 commented Mar 24, 2025

I'm interested to know why this wouldn't need a config option whereas, say, Codable does? This isn't to say I think it should, but rather I'm trying to work out where we would draw the line?

Yes, why not. I have added a config flag similar to the one about LocalizedError (omit_case_iterable_conformance). Also I have added autogeneration for simple errors, since there it is a similar case.

In general the default conformance autogenerated by swift should not produce anything out-of-ordinary. Similar to the automatic generation of Hashable and Equatable. But for peace of mind it might be better to have a config toggle to remove these if not desired by somebody.

@Iskander508 Iskander508 changed the title Generate swift CaseIterable for simple enums Generate swift CaseIterable for simple enums and errors Mar 24, 2025
Copy link
Member

@mhammond mhammond left a comment

Choose a reason for hiding this comment

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

Thanks, this is great - I like that it's enabled by default rather than opt-in as it seems useful and something we should maybe do in #2490?

I'm mildly confused by the contains_variant_fields rather than object_references checks though?


{% if !config.omit_case_iterable_conformance() && !e.is_flat() && !e.contains_variant_fields() %}
extension {{ type_name }}: CaseIterable {}
{% endif %}
Copy link
Member

Choose a reason for hiding this comment

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

nano-nit, please fix this eol

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be OK now

@Iskander508
Copy link
Contributor Author

Iskander508 commented Mar 25, 2025

I'm mildly confused by the contains_variant_fields rather than object_references checks though?

The CaseIterable protocol can only be autogenerated for enums that don't contain additional fields (not only objects, but even primitive fields). Apple docs
Few examples:

public enum ConnectionEventEnum {
    case dummy
    case connected(deviceAddress: String)
    case disconnected(deviceAddress: String)
}
extension ConnectionEventEnum: CaseIterable {} // ERROR, this will not compile, because some variants contain fields

public enum AnimalEnum {
    case dog
    case cat
}
extension AnimalEnum: CaseIterable {} // OK here, all variants are simple (no additional fields)

Copy link
Member

@mhammond mhammond left a comment

Choose a reason for hiding this comment

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

Given the discussion in #2490, we want to default this to off, right? cc @crazytonyli . And any chance of a changelog note for both PRs? :)

Pavel Zarecky added 3 commits April 2, 2025 18:13
…se_iterable

# Conflicts:
#	docs/manual/src/swift/configuration.md
#	uniffi_bindgen/src/bindings/swift/gen_swift/mod.rs
@Iskander508
Copy link
Contributor Author

I have changed the config setting to off-by-default and renamed to generate_case_iterable_conformance (with default false).
And also added points to changelog. Please adjust as needed.

@Iskander508 Iskander508 requested a review from mhammond April 7, 2025 07:42
Copy link
Member

@mhammond mhammond left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, thank you for your patience!

@mhammond mhammond merged commit 8720ef1 into mozilla:main Apr 11, 2025
5 checks passed
bendk pushed a commit that referenced this pull request May 5, 2025
Co-authored-by: Pavel Zarecky <zarecky@procivis.ch>
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.

2 participants