[ObjC] Fix GPBUnknownField/GPBUnknownFields copy.
Since a group GPBUnknownField uses a GPBUnknownFields and that is mutable, it needs to be copied so two instances aren't linked.
PiperOrigin-RevId: 663323972
diff --git a/objectivec/GPBUnknownField.m b/objectivec/GPBUnknownField.m
index ba70dfe..f574acf 100644
--- a/objectivec/GPBUnknownField.m
+++ b/objectivec/GPBUnknownField.m
@@ -165,9 +165,17 @@
case GPBUnknownFieldTypeFixed32:
case GPBUnknownFieldTypeFixed64:
case GPBUnknownFieldTypeLengthDelimited:
- case GPBUnknownFieldTypeGroup:
// In these modes, the object isn't mutable, so just return self.
return [self retain];
+ case GPBUnknownFieldTypeGroup: {
+ // The `GPBUnknownFields` for the group is mutable, so a new instance of this object and
+ // the group is needed.
+ GPBUnknownFields *copyGroup = [storage_.group copyWithZone:zone];
+ GPBUnknownField *copy = [[GPBUnknownField allocWithZone:zone] initWithNumber:number_
+ group:copyGroup];
+ [copyGroup release];
+ return copy;
+ }
case GPBUnknownFieldTypeLegacy: {
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wdeprecated-declarations"
diff --git a/objectivec/GPBUnknownFields.m b/objectivec/GPBUnknownFields.m
index 87e62b9..890be42 100644
--- a/objectivec/GPBUnknownFields.m
+++ b/objectivec/GPBUnknownFields.m
@@ -228,9 +228,8 @@
}
- (id)copyWithZone:(NSZone *)zone {
- GPBUnknownFields *copy = [[GPBUnknownFields alloc] init];
- // Fields are r/o in this api, so just copy the array.
- copy->fields_ = [fields_ mutableCopyWithZone:zone];
+ GPBUnknownFields *copy = [[GPBUnknownFields allocWithZone:zone] init];
+ copy->fields_ = [[NSMutableArray allocWithZone:zone] initWithArray:fields_ copyItems:YES];
return copy;
}
diff --git a/objectivec/Tests/GPBUnknownFieldsTest.m b/objectivec/Tests/GPBUnknownFieldsTest.m
index 42c3f7d..c73e064 100644
--- a/objectivec/Tests/GPBUnknownFieldsTest.m
+++ b/objectivec/Tests/GPBUnknownFieldsTest.m
@@ -683,25 +683,30 @@
[ufs addFieldNumber:3 fixed64:3];
[ufs addFieldNumber:4 lengthDelimited:DataFromCStr("foo")];
GPBUnknownFields* group = [ufs addGroupWithFieldNumber:5];
+ [group addFieldNumber:10 varint:10];
+ GPBUnknownFields* subGroup = [group addGroupWithFieldNumber:100];
+ [subGroup addFieldNumber:20 varint:20];
GPBUnknownFields* ufs2 = [[ufs copy] autorelease];
XCTAssertTrue(ufs != ufs2); // Different objects
XCTAssertEqualObjects(ufs, ufs2); // Equal contents
- // All the actual field objects should be the same since they are immutable.
+ // All field objects but the group should be the same since they are immutable.
XCTAssertTrue([[ufs fields:1] firstObject] == [[ufs2 fields:1] firstObject]); // Same object
XCTAssertTrue([[ufs fields:2] firstObject] == [[ufs2 fields:2] firstObject]); // Same object
XCTAssertTrue([[ufs fields:3] firstObject] == [[ufs2 fields:3] firstObject]); // Same object
XCTAssertTrue([[ufs fields:4] firstObject] == [[ufs2 fields:4] firstObject]); // Same object
XCTAssertTrue([[ufs fields:4] firstObject].lengthDelimited ==
- [[ufs2 fields:4] firstObject].lengthDelimited); // Same object
- XCTAssertTrue([[ufs fields:5] firstObject] == [[ufs2 fields:5] firstObject]); // Same object
- XCTAssertTrue(group == [[ufs2 fields:5] firstObject].group); // Same object
-
- // Now force copies on the fields to confirm that is not making new objects either.
- for (GPBUnknownField* field in ufs) {
- GPBUnknownField* field2 = [[field copy] autorelease];
- XCTAssertTrue(field == field2); // Same object (since they aren't mutable).
- }
+ [[ufs2 fields:4] firstObject].lengthDelimited); // Same object
+ // Since the group holds another `GPBUnknownFields` object (which is mutable), it will be a
+ // different object.
+ XCTAssertTrue([[ufs fields:5] firstObject] != [[ufs2 fields:5] firstObject]);
+ XCTAssertTrue(group != [[ufs2 fields:5] firstObject].group);
+ XCTAssertEqualObjects(group, [[ufs2 fields:5] firstObject].group);
+ // And confirm that copy went deep so the nested group also is a different object.
+ GPBUnknownFields* groupCopied = [[ufs2 fields:5] firstObject].group;
+ XCTAssertTrue([[group fields:100] firstObject] != [[groupCopied fields:100] firstObject]);
+ XCTAssertTrue(subGroup != [[groupCopied fields:100] firstObject].group);
+ XCTAssertEqualObjects(subGroup, [[groupCopied fields:100] firstObject].group);
}
- (void)testInvalidFieldNumbers {
