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

Swift: Fix for @ProtoDefaulted #2706

Merged
merged 5 commits into from
Nov 14, 2023
Merged

Swift: Fix for @ProtoDefaulted #2706

merged 5 commits into from
Nov 14, 2023

Conversation

lickel
Copy link
Collaborator

@lickel lickel commented Nov 13, 2023

Across module boundaries, Message.fields is often (always?) empty. When that happens, we can't correctly know that we want @ProtoDefaulted or not. Given it's unknowable in the current context, it chooses not to emit them.

@lickel lickel changed the title Fix for @ProtoDefaulted Swift: Fix for @ProtoDefaulted Nov 13, 2023
import XCTest
@testable import Wire

final class ProtoEnumTests: XCTestCase {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are new tests for other work that happened in v4.9.2.

@lickel lickel marked this pull request as ready for review November 13, 2023 22:41
@@ -186,7 +186,8 @@ class SwiftGenerator private constructor(
if (type == ProtoType.ANY) return false
if (isMessage) {
val messageType = schema.getType(type!!) as MessageType
return messageType.supportsEmptyInitialization

return messageType.supportsEmptyInitialization && messageType.fields.isNotEmpty()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the actual fix/change. I believe it is the correct fix overall if for any reason we cannot load the fields of a message we do not know whether we can provide a default value for it.

@lickel lickel merged commit eca69ec into master Nov 14, 2023
13 of 14 checks passed
@lickel lickel deleted the ergonomics-fix branch November 14, 2023 14:03
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.

3 participants