Skip to content

Parity: NSCoding: NSIndexSet #2216

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 6, 2019
Merged

Parity: NSCoding: NSIndexSet #2216

merged 2 commits into from
May 6, 2019

Conversation

millenomi
Copy link
Contributor

  • Bridge NSData fully to CF. Work around the fact that NSData has a mutable subclass.

  • Implement NS{Mutable,}IndexSet encoding and decoding.

 - Bridge NSData fully to CF. Work around the fact that NSData has a mutable subclass.

 - Implement NS{Mutable,}IndexSet encoding and decoding.
@millenomi
Copy link
Contributor Author

@millenomi
Copy link
Contributor Author

@swift-ci please test

1 similar comment
@millenomi
Copy link
Contributor Author

@swift-ci please test

@millenomi
Copy link
Contributor Author

millenomi commented May 6, 2019

This needs to be written down somewhere, and here is a good spot to do it at a minimum.

On Darwin, NSData and many other classes are implemented as class clusters. This means that the classes you interact with normally, like NSData, NSString, etc., are actually abstract; when you invoke their +alloc/-init… methods, a suitable subclass is selected. (More info on that is available in the documentation.) Darwin provides a tuned set of private subclasses tuned to the specific needs and performance characteristics of the platform. However, if you subclass the public, abstract class, you get none of these implementations; it acts like a true abstract class in all respects.

You may notice that your closest NSData.h only declares @property… bytes and @property… length in the @interface NSData proper; most other NSData methods appear in a category. That's because the declared properties are abstract funnels; if the class is subclassed, these need to be overridden and reimplemented without calling super. NSMutableData adds the .length setter and the @property… mutableBytes. Methods in the categories are concrete; if they aren't overridden by a subclass, they have default implementations in terms of the abstract funnels. The Darwin subclasses often override these methods to provide more efficient implementations that are aware of the details of their specific backends.

In swift-corelibs-foundation, we have a single concrete implementation of NS{Mutable,}Data: the one in CFData…. So far, we haven't thought much about this subclassing situation, but with the advent of full NSData-to-CFData… bridging — which is required by this patch to allow __NSSwiftData to encode correctly — we need to resolve the fact, because, where on Darwin we would hit the default implementation, here the methods were immediately calling the CFData implementation, which now detects the subclassing and tries to invoke the potentially overridden Swift method again, which invokes the CFData implementation in an infinite loop.

This patch matches the Darwin behavior. We cannot have all-abstract methods in the base class and then the concrete methods in an extension like on Darwin, because the latter methods need to be open; we also cannot make a class cluster in Swift, since there is no equivalent to the placeholder/trampoline initializer interception method ("factory initializers") that NSData uses in its ObjC implementation; NSData needs to be both a concrete class and an abstract class. Thankfully, it is easy to check at runtime which is which, since it needs to act as an abstract class only when subclassed (and not a NSMutableData).

This patch adds branched default implementations to all the open methods that would be in a category on Darwin, and makes them use the abstract funnels for subclasses. This makes NS{Mutable,}Data behave exactly like its ObjC counterpart wrt. subclassing, at the cost of increasing the amount of memory required per-object for subclass instances when comparing to Darwin (since, unlike Darwin's, NSData here cannot for now avoid the need to have all the properties needed for its concrete, non-subclassed implementation). Clients that subclassed this class will need to ensure all funnel methods are overridden without invoking super going forward, in a way that matches (and thus should be source-compatible with) what one needs to do on Darwin. If a subclasser needs a concrete, default implementation, they will need to delegate to a CFData or NSData object which hasn't been subclassed.

@millenomi
Copy link
Contributor Author

millenomi commented May 6, 2019

tl;dr please do not subclass Foundation model classes unless you have a legitimately well-thought and considered use case for a pressing need.

@millenomi
Copy link
Contributor Author

@swift-ci please test

1 similar comment
@millenomi
Copy link
Contributor Author

@swift-ci please test

@millenomi
Copy link
Contributor Author

The funnels need tests added, but this is blocking at least another patch, and I'd rather do it concurrently.

@millenomi millenomi merged commit c6e4e2c into swiftlang:master May 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant