diff --git a/objectivec/GPBUnknownField.m b/objectivec/GPBUnknownField.m index ba70dfe599..f574acf7e0 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 87e62b9a84..890be4286e 100644 --- a/objectivec/GPBUnknownFields.m +++ b/objectivec/GPBUnknownFields.m @@ -228,9 +228,8 @@ static BOOL MergeFromInputStream(GPBUnknownFields *self, GPBCodedInputStream *in } - (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 42c3f7dc87..c73e0649c1 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 {