[swift] Provide a default value for sub fields and common types as per proto spec #2650

dnkoutso · 2023-09-23T16:21:02Z

This implements the proto spec:

If the default value is not specified for an optional element, a type-specific default value is used instead: for strings, the default value is the empty string. For bytes, the default value is the empty byte string. For bools, the default value is false. For numeric types, the default value is zero. For enums, the default value is the first value listed in the enum’s type definition. This means care must be taken when adding a value to the beginning of an enum value list. See the Updating A Message Type section for guidelines on how to safely change definitions.

wire-swift-generator/src/main/java/com/squareup/wire/swift/SwiftGenerator.kt

wire-runtime-swift/src/test/swift/sample/Dinosaur.swift

lickel

This is effectively doubling the storage requirements for everything...
It's now making me a bit nervous...

Maybe we can do something more like BetterCodable and use statics for String, Number, etc...

The empty structs could also be solved by conforming to a protocol like EmptyInitializable

@propertyWrapper
public struct DefaultedZero<Value: Numeric> {
    public var wrappedValue: Value?
     
    public var projectedValue: Value {
        wrappedValue ?? .zero
    }
}

@propertyWrapper
public struct DefaultedEmpty<Value: EmptyInitializable> {
    public var wrappedValue: Value?

    public var projectedValue: Value {
        wrappedValue ?? Value.init()
    }
}

lickel · 2023-10-11T23:35:33Z

wire-runtime-swift/src/test/swift/sample/Dinosaur.swift

@@ -8,13 +8,17 @@ public struct Dinosaur {
    /**
     * Common name of this dinosaur, like "Stegosaurus".
     */
+    @Defaulted(defaultValue: "")


Should probably update the README to include info about @defaulted and this updated model

yeap, thanks for reminding

lickel · 2023-10-11T23:36:31Z

wire-swift-generator/src/main/java/com/squareup/wire/swift/SwiftGenerator.kt

+          }
+          if (isEnum) {
+            val enumType = schema.getType(type!!) as EnumType
+            return if (enumType.constants.getOrNull(0) != null) CodeBlock.of("%T.%L", typeName.makeNonOptional(), enumType.constants[0].name) else null


Is enumType.constants.getOrNull(0) correct? That just looks like the first (mapped) constant, not the 0-th value.

Based on the proto3 code, I think we want enum.constants.filter { it.tag == 0 }.firstOrNull()

Yes this is probably incorrect. There is also enum.constant(0) but I think this didnt work, will be updating and looking again.

lickel · 2023-10-11T23:37:54Z

wire-swift-generator/src/main/java/com/squareup/wire/swift/SwiftGenerator.kt

+            DOUBLE -> return CodeBlock.of("%L", 0)
+            STRING -> return CodeBlock.of("%S", "")
+            DATA -> return CodeBlock.of(
+              "%T(base64Encoded: %S)!",


Can you not just do Data() ?

yeah it was an initial copy paste from below will be updating

lickel · 2023-10-11T23:53:57Z

Discussed offline: we need Defaulted to have conditional conformance to Redactable

dnkoutso · 2023-10-12T18:25:11Z

wire-runtime-swift/src/main/swift/Redactable.swift

@@ -44,16 +44,20 @@ extension Redactable {
            guard let label = label else {
                return "\(value)"
            }
-            if RedactedKeys(rawValue: label) != nil {
+            var strippedLabel = label


I believe this is an existing bug today. If a custom default value was specified due to applying @Defaulted the internal property is renamed with a _ in front. This fails this check.

Test caught this now because we emit @Defaulted identity types for many properties which now as an empty string exacerbated this issue which is now fixed and the test passes.

wire-tests-proto3-swift/src/main/swift/ContainsDuration.swift

…r proto spec

jamieQ · 2023-10-13T17:35:43Z

Discussed offline: we need Defaulted to have conditional conformance to Redactable

before this falls out of my memory: the conditional conformance did not appear necessary to address the test failures occurring with the nested, partially redacted types. the reason being that the Redactable conformance is present on the types the Defaulted property wrapper applies to, and that appears to be what the Mirror actually sees when constructing the recursive description (vs seeing the property wrapper type). e.g. the Mirror iteration logic sees _defaultedProp: SomeRedactableType vs defaultedProp: Defaulted<SomeRedactableType>, or at least that was my impression of what was happening.

oldergod reviewed Sep 25, 2023

View reviewed changes

wire-swift-generator/src/main/java/com/squareup/wire/swift/SwiftGenerator.kt Outdated Show resolved Hide resolved

dnkoutso force-pushed the default_values_test branch from b27b725 to eac52e6 Compare September 25, 2023 21:16

dnkoutso changed the title ~~[swift] WIP Generate a default value for sub fields with only optional fields~~ [swift] Generate a default value for sub fields with only optional fields Sep 25, 2023

dnkoutso requested review from efirestone and lickel September 25, 2023 21:17

dnkoutso force-pushed the default_values_test branch 2 times, most recently from 35a6331 to b1b41ec Compare October 2, 2023 17:22

dnkoutso force-pushed the default_values_test branch 2 times, most recently from 4f6ba0c to 8e7fb83 Compare October 11, 2023 14:53

dnkoutso commented Oct 11, 2023

View reviewed changes

wire-swift-generator/src/main/java/com/squareup/wire/swift/SwiftGenerator.kt Outdated Show resolved Hide resolved

dnkoutso changed the title ~~[swift] Generate a default value for sub fields with only optional fields~~ [swift] Provide a default value for sub fields and common types as per proto spec Oct 11, 2023

dnkoutso force-pushed the default_values_test branch 5 times, most recently from 6db6816 to a4af441 Compare October 11, 2023 20:23

dnkoutso commented Oct 11, 2023

View reviewed changes

wire-runtime-swift/src/test/swift/sample/Dinosaur.swift Outdated Show resolved Hide resolved

dnkoutso force-pushed the default_values_test branch 3 times, most recently from f009d6e to a7bdfd8 Compare October 11, 2023 23:06

lickel reviewed Oct 11, 2023

View reviewed changes

dnkoutso force-pushed the default_values_test branch 5 times, most recently from f2772d1 to e6a251e Compare October 12, 2023 16:43

dnkoutso commented Oct 12, 2023

View reviewed changes

wire-tests-proto3-swift/src/main/swift/ContainsDuration.swift Outdated Show resolved Hide resolved

dnkoutso force-pushed the default_values_test branch from e6a251e to e1c4eaa Compare October 12, 2023 18:47

dnkoutso force-pushed the default_values_test branch 2 times, most recently from 7a8f922 to dd5aafe Compare October 13, 2023 00:17

[swift] Provide a default value for sub fields and common types as pe…

db6b654

…r proto spec

dnkoutso force-pushed the default_values_test branch from dd5aafe to db6b654 Compare October 13, 2023 00:22

lickel approved these changes Oct 13, 2023

View reviewed changes

dnkoutso merged commit b0c6e13 into master Oct 13, 2023
11 of 13 checks passed

dnkoutso deleted the default_values_test branch October 13, 2023 01:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[swift] Provide a default value for sub fields and common types as per proto spec #2650

[swift] Provide a default value for sub fields and common types as per proto spec #2650

dnkoutso commented Sep 23, 2023 •

edited

Loading

lickel left a comment •

edited

Loading

lickel Oct 11, 2023 •

edited by dnkoutso

Loading

dnkoutso Oct 12, 2023

dnkoutso Oct 12, 2023

lickel Oct 11, 2023

lickel Oct 11, 2023

dnkoutso Oct 12, 2023

dnkoutso Oct 12, 2023

lickel Oct 11, 2023

dnkoutso Oct 12, 2023

dnkoutso Oct 12, 2023

lickel commented Oct 11, 2023

dnkoutso Oct 12, 2023

dnkoutso Oct 12, 2023

jamieQ commented Oct 13, 2023

[swift] Provide a default value for sub fields and common types as per proto spec #2650

[swift] Provide a default value for sub fields and common types as per proto spec #2650

Conversation

dnkoutso commented Sep 23, 2023 • edited Loading

lickel left a comment • edited Loading

Choose a reason for hiding this comment

lickel Oct 11, 2023 • edited by dnkoutso Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lickel commented Oct 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamieQ commented Oct 13, 2023

dnkoutso commented Sep 23, 2023 •

edited

Loading

lickel left a comment •

edited

Loading

lickel Oct 11, 2023 •

edited by dnkoutso

Loading