CODEWITHSUNDEEP
objective-c
brian0
brian0

Reputation: 105

Relying on the (copy) attribute to copy NSMutableDictionary causes crash

The following program relies on the copy
attribute to copy a NSMutableDictionary.
The copy is apparently ok, but, if I try
to add a new element to the copy, the program crashes.
Is it some kind of bug?
PS. If it matters it's NON ARC

#import <Foundation/Foundation.h>

@interface Dog: NSObject
@property (copy) NSMutableDictionary *dict;
@end

@implementation Dog
@synthesize dict;

- (id) init
{
    if ( (self = [super init]) ) {
    dict = [[NSMutableDictionary alloc] init];
    }
    return self;
}

- (void) print {
    for (id key in dict) {
    printf("%s --> %s\n", [key UTF8String], [dict[key] UTF8String] );
    }
}

@end

//------------------------------------------------------

int main() {
    Dog *dog1 = [[Dog alloc] init];
    Dog *dog2 = [[Dog alloc] init];

    dog1.dict[@"color"] = @"black";
    dog2.dict = dog1.dict;
    [dog2 print];
    // the print shows that dog2.dict is indeed a copy of dog1.dict
    // lldb shows that it is a shallow copy, which I guess is ok
    // since values are immutable.
    // ... so far so good.
    dog1.dict[@"tail"] = @"long";   // This goes smoothly
    // 
    // But...
    dog2.dict[@"tail"] = @"long";
    // here program crashes with the following message
    //
    // -[__NSDictionaryI setObject:forKeyedSubscript:]: unrecognized selector sent to instance 0x100108ee0
    //*** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '-[__NSDictionaryI setObject:forKeyedSubscript:]: unrecognized selector sent to instance 0x100108ee0'
    return(0);
}

EDIT: If I replace the line

dog2.dict = dog1.dict;

with

[dog2.dict addEntriesFromDictionary: dog1.dict];

then it works. There is no crash.
So, OK, this is the correct way of doing it.
But my point is: don't I deserve at least a warning
from the compiler?

Upvotes: 0

Views: 198

Answers (2)

Rob Napier
Rob Napier

Reputation: 299575

You've broken some rules here, which is why it's going badly for you. As a rule, you should not return or accept NSMutableDictionary as a property. There are exceptions (and you have to code around that), but generally you should not. To code this correctly, you need to separate your interface from your implementation.

The correct interface is:

@interface Dog: NSObject
@property (copy) NSDictionary *dict;
@end

This object promises to accept and return NSDictionary, and it promises that the accepted and returned NSDictionaries will be independent copies, so the caller doesn't need to worry about mutability issues. We then need to implement those promises:

@implementation Dog {
    NSMutableDictionary *mutableDict;
}

- (id) init
{
    if ( (self = [super init]) ) {
        mutableDict = [[NSMutableDictionary alloc] init];
    }
    return self;
}

- (void) print {
    for (id key in mutableDict) {
        printf("%s --> %s\n", [key UTF8String], [mutableDict[key] UTF8String] );
    }
}

- (NSDictionary *)dict {
    return [mutableDict copy]; // Add an -autorelease if this is MRC
}

- (void)setDict: (NSDictionary *)newValue {
    // I'm going to pretend this is ARC; for MRC, code this in your style.
    mutableDict = [newValue mutableCopy];
}

@end

On the other hand, if you really do want to share state with an NSMutableDictionary, you should not copy it, you should retain it to make clear the semantics.

@interface Dog: NSObject
@property (retain) NSMutableDictionary *dict;
@end

And this makes it (hopefully) clear to the caller that this is shared state and should be treated as such, and the caller should make copies if they so desire. But you generally should avoid this. And if you do go this way, I would call the property mutableDict or something like that to make it clear that this is unusual. For an example, see -[NSAttributedString mutableString].

There's not really a "mutable, but independent copy" semantic in ObjC property annotations, and I wouldn't create one unless you have a very strong need (generally performance related). I would generally just return an immutable copy and let the caller make their own mutable copy.

One more side note: Sometimes Cocoa implements "returns an immutable copy" as "just return the mutable one, cast to an immutable type." This improves performance by avoiding the copy, at the cost of sometimes the data changing behind your back. As an example, look at the docs for -[NSView subviews]. In my opinion you should avoid this pattern unless it is critical for performance (and even then, I'd make a special method like -subviewsBackingStore or something silly like that to make it clear "this is weird."

Upvotes: 2

BoygeniusDexter
BoygeniusDexter

Reputation: 2262

copy attribute creates an NSDictionary instance, not NSMutableDictionary, that's why it's crashing. First, change the property to retain from copy:

@property (retain) NSMutableDictionary* duct;

Then you can do the following:

Dog *dog2 = [[Dog alloc] init];
dog2.dict = [dog1.dict mutableCopy];

Upvotes: 2

Related Questions