protobuf icon indicating copy to clipboard operation
protobuf copied to clipboard

[Objective-C] NSObject property with the same name will skip message forwarding and cause Getter to fail

Open yzkmh opened this issue 3 years ago • 5 comments

We encountered some problems with the iOS file generated by the PB script. When executing description, the value of valueType is 1, but when calling the getter method, it gets 4. screenshot-20220809-210727 During the investigation, it was found that there is a valueType property with the same name in NSObject. Since NSObject is the base class of the PB class, the call of NSObject is directly triggered when the Getter method of valueType is executed, rather than resolveInstanceMethod. Is there any way to avoid this problem?

What version of protobuf and what language are you using? Language: Objective-C

yzkmh avatar Aug 09 '22 13:08 yzkmh

Taking a quick look at this, I don't see an issue:

diff --git a/objectivec/Tests/GPBMessageTests.m b/objectivec/Tests/GPBMessageTests.m
index b2c75ba0c..bc828553a 100644
--- a/objectivec/Tests/GPBMessageTests.m
+++ b/objectivec/Tests/GPBMessageTests.m
@@ -93,6 +93,16 @@
 
 @implementation MessageTests
 
+
+- (void)testIssueWithValueTypeField {
+  TestingIssue *msg = [TestingIssue message];
+  XCTAssertEqual(msg.valueType, 0);
+  msg.valueType = 1000;
+  XCTAssertEqual(msg.valueType, 1000);
+  NSString *desc = [msg description];
+  XCTAssertTrue([desc containsString:@"value_type: 1000"]);
+}
+
 // TODO(thomasvl): this should get split into a few files of logic junks, it is
 // a jumble of things at the moment (and the testutils have a bunch of the real
 // assertions).
diff --git a/objectivec/Tests/unittest_objc.proto b/objectivec/Tests/unittest_objc.proto
index 91c213921..3e1ab1ec9 100644
--- a/objectivec/Tests/unittest_objc.proto
+++ b/objectivec/Tests/unittest_objc.proto
@@ -34,6 +34,10 @@ import "google/protobuf/unittest.proto";
 
 package protobuf_unittest;
 
+message TestingIssue {
+  optional int32 value_type = 1;
+}
+
 // Used to check that Headerdocs and appledoc work correctly. If these comments
 // are not handled correctly, Xcode will fail to build the tests.
 message TestGeneratedComments {

This little test passes with that included example message.

My one guess is are you testing this from Swift? If so, is the po command you are doing accidentally evaluating to https://developer.apple.com/documentation/swift/anykeypath/valuetype/ instead? So if you call valueType from actual Swift/ObjC code, does it work? i.e. things may be working, but maybe only in the debugger context or maybe in the context of Swift you are getting a different, Swift specific override and might need to do some explicit type declarations to get the ObjC property from the message invoked instead.

thomasvl avatar Aug 09 '22 16:08 thomasvl

Extending the diff into the .swift test files also seems to show things working:

diff --git a/objectivec/Tests/GPBSwiftTests.swift b/objectivec/Tests/GPBSwiftTests.swift
index 03d751068..0117d82db 100644
--- a/objectivec/Tests/GPBSwiftTests.swift
+++ b/objectivec/Tests/GPBSwiftTests.swift
@@ -35,6 +35,15 @@ import XCTest
 
 class GPBBridgeTests: XCTestCase {
 
+  func testValueType() {
+    let msg = TestingIssue()
+    XCTAssertEqual(msg.valueType, 0)
+    msg.valueType = 1000
+    XCTAssertEqual(msg.valueType, 1000)
+    let desc = msg.description
+    XCTAssert(desc.contains("value_type: 1000"))
+  }
+

thomasvl avatar Aug 09 '22 16:08 thomasvl

Expanded the test to nest a message also, and I'm still not seeing any issues. This works in the iOS and macOS Xcode projects in the objectivec directory:

diff --git a/objectivec/Tests/GPBMessageTests.m b/objectivec/Tests/GPBMessageTests.m
index b2c75ba0c..23b411e8c 100644
--- a/objectivec/Tests/GPBMessageTests.m
+++ b/objectivec/Tests/GPBMessageTests.m
@@ -93,6 +93,22 @@
 
 @implementation MessageTests
 
+
+- (void)testIssueWithValueTypeField {
+  TestingIssue *msg = [TestingIssue message];
+  XCTAssertEqual(msg.valueType, 0);
+  msg.valueType = 1000;
+  XCTAssertEqual(msg.valueType, 1000);
+  NSString *desc = [msg description];
+  XCTAssertTrue([desc containsString:@"value_type: 1000"]);
+
+  XCTAssertEqual(msg.subMsg.valueType, 0);
+  msg.subMsg.valueType = 2000;
+  XCTAssertEqual(msg.subMsg.valueType, 2000);
+  NSString *desc2 = [msg description];
+  XCTAssertTrue([desc2 containsString:@"value_type: 2000"]);
+}
+
 // TODO(thomasvl): this should get split into a few files of logic junks, it is
 // a jumble of things at the moment (and the testutils have a bunch of the real
 // assertions).
diff --git a/objectivec/Tests/GPBSwiftTests.swift b/objectivec/Tests/GPBSwiftTests.swift
index 03d751068..36cc68e5c 100644
--- a/objectivec/Tests/GPBSwiftTests.swift
+++ b/objectivec/Tests/GPBSwiftTests.swift
@@ -35,6 +35,21 @@ import XCTest
 
 class GPBBridgeTests: XCTestCase {
 
+  func testValueType() {
+    let msg = TestingIssue()
+    XCTAssertEqual(msg.valueType, 0)
+    msg.valueType = 1000
+    XCTAssertEqual(msg.valueType, 1000)
+    let desc = msg.description
+    XCTAssert(desc.contains("value_type: 1000"))
+
+    XCTAssertEqual(msg.subMsg.valueType, 0)
+    msg.subMsg.valueType = 2000
+    XCTAssertEqual(msg.subMsg.valueType, 2000)
+    let desc2 = msg.description
+    XCTAssert(desc2.contains("value_type: 1000"))
+  }
+
   func testProto2Basics() {
     let msg = Message2()
     let msg2 = Message2()
diff --git a/objectivec/Tests/UnitTests-Bridging-Header.h b/objectivec/Tests/UnitTests-Bridging-Header.h
index 46292fce7..9d670b1ac 100644
--- a/objectivec/Tests/UnitTests-Bridging-Header.h
+++ b/objectivec/Tests/UnitTests-Bridging-Header.h
@@ -4,3 +4,4 @@
 
 #import "google/protobuf/UnittestRuntimeProto2.pbobjc.h"
 #import "google/protobuf/UnittestRuntimeProto3.pbobjc.h"
+#import "google/protobuf/UnittestObjc.pbobjc.h"
diff --git a/objectivec/Tests/unittest_objc.proto b/objectivec/Tests/unittest_objc.proto
index 91c213921..0ac7204d9 100644
--- a/objectivec/Tests/unittest_objc.proto
+++ b/objectivec/Tests/unittest_objc.proto
@@ -34,6 +34,11 @@ import "google/protobuf/unittest.proto";
 
 package protobuf_unittest;
 
+message TestingIssue {
+  optional int32 value_type = 1;
+  optional TestingIssue sub_msg = 2;
+}
+
 // Used to check that Headerdocs and appledoc work correctly. If these comments
 // are not handled correctly, Xcode will fail to build the tests.
 message TestGeneratedComments {

thomasvl avatar Aug 09 '22 16:08 thomasvl

Thanks for your reply. ValueType is not a native property of NSObject. It's an AssociatedObject added from Category. Like this. ` @interface NSObject (Test)

@property (nonatomic, assign) NSUInteger typeValue;

@end ` Our project has many libraries that add AssociatedObject to NSObject, not all naming is canonical.

yzkmh avatar Aug 10 '22 04:08 yzkmh

Ugh, yea, there's not anything the library can do for that, if you are using other libraries that are clamming selectors on NSObject it's going to have issues like this.

For our own projects, we tend to only allow categories that put prefixes on the selectors they add for just this sorta reason, they can end up conflicting with newer Apple SDK apis, as well as getting into issues like this.

thomasvl avatar Aug 10 '22 14:08 thomasvl

All right.It seems that it can only be solved from the category specification,thank you again.

yzkmh avatar Aug 15 '22 08:08 yzkmh