Implement NSProxy #2499

ms-jihua · 2017-04-12T00:38:38Z

aballway · 2017-04-12T17:34:51Z

Frameworks/Foundation/NSProxy.mm

 @Notes
 */
 - (void)forwardInvocation:(NSInvocation*)anInvocation {
-    UNIMPLEMENTED();
+    [NSException raise:NSInvalidArgumentException


nit: should these just be NSInvalidAbstractInvocationReturn() ? I know it's not a class cluster per se, but it's still "Ya gotta override this"

nope, documented NSInvalidArgumentException: https://developer.apple.com/reference/foundation/nsproxy/1416417-forwardinvocation?language=objc

aballway · 2017-04-12T17:37:15Z

Frameworks/Foundation/NSProxy.mm

+    intptr_t* refCount = ((intptr_t*)self) - 1;
+    // We allow refcounts to run into the negative, but should only
+    // deallocate once.
+    if (__sync_sub_and_fetch(refCount, 1) == -1) {


should this be <= ? Ideally we'd never go below -1 but in case we ever should due to async nonsense we wouldn't want to leak

this is just lifted from NSObject.mm

async nonsense wouldn't be able to cause that since __sync_sub_and_fetch is atomic

DHowett-MSFT · 2017-04-12T18:18:25Z

Frameworks/Foundation/NSObject.mm

@@ -708,7 +707,8 @@ void WinObjC_SetMissingSelectorFatal(BOOL fatal) {
 }
 @end

-static void _dealloc_zombify(id self, SEL _cmd); // forward declaration
+    static void


nit: I'd prefer not having the format changes in this PR

i'll fix this one and leave the rest, if that's ok

DHowett-MSFT · 2017-04-12T18:18:33Z

Frameworks/Foundation/NSObject.mm

@@ -737,7 +737,7 @@ + (BOOL)isSubclassOfClass:(Class)classRef {
 /**
 @Status Interoperable
 */
- (void)doesNotRecognizeSelector: (SEL)selector {
+- (void)doesNotRecognizeSelector:(SEL)selector {


then again, some of them are good ..

DHowett-MSFT · 2017-04-12T18:19:19Z

Frameworks/Foundation/NSProxy.mm

 @Notes
 */
 - (NSMethodSignature*)methodSignatureForSelector:(SEL)aSelector {
-    UNIMPLEMENTED();
-    return StubReturn();
+    [NSException raise:NSInvalidArgumentException


nit: macro these, and use %hs + __PRETTY_FUNCTION__ to get the method name in the macro

DHowett-MSFT · 2017-04-12T18:20:12Z

Frameworks/Foundation/NSProxy.mm

+ @Status Interoperable
+ @Notes
+*/
+- (Class)superclass {


Not sure on whether this should be calling [self class]; thoughts?

This is also the same as NSObject.mm... going to propose we leave both for the moment and let sleeping beasts lie.

I changed my mind and got curious how the reference platform behaved and it appears superclass is polymorphic so I will make this change in both places,

May want to do the fix for NSObject in a separate PR. Doubt it would cause any regressions but it's foundational enough that it might and should be thoroughly tested.

Mm, my thought is that if a change like that breaks something, we'll either be able to catch it in CI build, or it's so incredibly subtle despite being in such an exercised class that doing it in a separate change won't really gain us anything, so I'll elect to risk it. (I will probably regret saying this a week from now)

DHowett-MSFT · 2017-04-12T18:20:24Z

Frameworks/Foundation/NSProxy.mm

 @Notes
 */
 - (instancetype)self {
-    UNIMPLEMENTED();
-    return StubReturn();
+    return (id)self;


nit: cast necessary?

DHowett-MSFT · 2017-04-12T18:20:51Z

Frameworks/Foundation/NSProxy.mm

 @Notes
 */
 - (BOOL)isKindOfClass:(Class)aClass {
-    UNIMPLEMENTED();
-    return StubReturn();
+    Class forwardClass = static_cast<Class>([self performSelector:@selector(class)]);


aaa is it really guaranteed that it calls forwardInvocation: for class?

i'm not sure I like that this is different from calling class

it's the contract unfortunately :( [NSProxy class] does not return a forwarded class

DHowett-MSFT · 2017-04-12T18:21:49Z

Frameworks/Foundation/NSProxy.mm

 @Notes
 */
 - (id)performSelector:(SEL)aSelector withObject:(id)anObject {
-    UNIMPLEMENTED();
-    return StubReturn();
+    NSInvocation* invocation = [NSInvocation invocationWithMethodSignature:[self methodSignatureForSelector:aSelector]];


it's strange to me that these go through invocation instead of hitting the invocation-forwarding path

what do you mean?

It seems like performSelector: and friends could just call objc_msgSend; that would go through the invocation forwarding and capture codepath

It's less verbose, but perhaps there is an advantage to creating the invocation directly!

discussed offline, leaving as is

DHowett-MSFT · 2017-04-12T18:22:25Z

Frameworks/Foundation/NSProxy.mm

 }

-@end
+@end


you may have to implement doesNotRecognizeSelector:, and call it from the default forwardInvocation: implementation just like NSObject -- that's how people will get missing selector exceptions

This is already tested for in the unit test - I believe it forwards to the remote doesNotRecognizeSelector.

DHowett-MSFT · 2017-04-12T18:22:44Z

include/Foundation/NSProxy.h

 + (id)alloc;
 + (id)allocWithZone:(NSZone*)zone;
 - (void)dealloc;
 - (void)finalize;
 - (void)forwardInvocation:(NSInvocation*)anInvocation;
 - (NSMethodSignature*)methodSignatureForSelector:(SEL)aSelector;
 + (BOOL)respondsToSelector:(SEL)aSelector;
-+ (Class) class;
+ (Class)class;
+ (Class)superclass; // Not in doc, but is supported


Doesn't the NSObject protocol contain this?

it does not!

DHowett-MSFT · 2017-04-12T18:23:04Z

tests/unittests/Foundation/NSProxyTests.mm

+@implementation _NSTestMutableArrayProxy
+
+- (instancetype)initWithMutableArray:(NSMutableArray*)array {
+    self->_forwardArray = array;


nit: document not calling super init

can't/shouldn't call super init on NSProxy - it's NS_UNAVAILABLE (this matches the reference platform)

Indeed! It'd be good to have a comment in here so somebody doesn't think it's broken.
Our CC (or that one really didactic talk I gave) states that we should almost never not call init. Since this is one of the cases where we mustn't, it deserves callout!

…olymorphic with respect to [self class] - Changed both [NSObject self] and [NSProxy self] to no longer do a cast - Misc CR feedback

ms-jihua · 2017-04-13T19:13:15Z

Could not repro CI build failure locally, queuing again.

Implement NSProxy

977fcac

Fixes microsoft#2348

msftclas added the cla-not-required label Apr 12, 2017

aballway approved these changes Apr 12, 2017

View reviewed changes

DHowett-MSFT suggested changes Apr 12, 2017

View reviewed changes

- Changed both [NSObject superclass] and [NSProxy superclass] to be p…

c46b6aa

…olymorphic with respect to [self class] - Changed both [NSObject self] and [NSProxy self] to no longer do a cast - Misc CR feedback

DHowett-MSFT approved these changes Apr 13, 2017

View reviewed changes

ms-jihua merged commit 2da953b into microsoft:develop Apr 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement NSProxy #2499

Implement NSProxy #2499

ms-jihua commented Apr 12, 2017

aballway Apr 12, 2017

ms-jihua Apr 12, 2017

aballway Apr 12, 2017

ms-jihua Apr 12, 2017

DHowett-MSFT Apr 12, 2017

ms-jihua Apr 12, 2017

DHowett-MSFT Apr 12, 2017

DHowett-MSFT Apr 12, 2017

DHowett-MSFT Apr 12, 2017

ms-jihua Apr 12, 2017

ms-jihua Apr 12, 2017

aballway Apr 12, 2017

ms-jihua Apr 12, 2017

DHowett-MSFT Apr 12, 2017

ms-jihua Apr 12, 2017

DHowett-MSFT Apr 12, 2017

DHowett-MSFT Apr 12, 2017

ms-jihua Apr 12, 2017

DHowett-MSFT Apr 12, 2017

ms-jihua Apr 12, 2017

DHowett-MSFT Apr 12, 2017

ms-jihua Apr 12, 2017

DHowett-MSFT Apr 12, 2017

ms-jihua Apr 12, 2017

DHowett-MSFT Apr 12, 2017

ms-jihua Apr 12, 2017

DHowett-MSFT Apr 12, 2017

ms-jihua Apr 12, 2017

DHowett-MSFT Apr 12, 2017

ms-jihua commented Apr 13, 2017

               }
-              @end
+              @end

Implement NSProxy #2499

Implement NSProxy #2499

Conversation

ms-jihua commented Apr 12, 2017

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

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

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

ms-jihua commented Apr 13, 2017