Increasing safety in loadFromNib #40

Skoti · 2017-02-12T23:10:31Z

In NibOwnerLoadable extension there is this dangerous line where it is assumed that a class which inherits from UIView also implements init(frame: CGRect) initializer:

static func loadFromNib(owner: Self = Self()) -> Self

and if it does not it will crash in runtime.

I'm surprised that this even compiles, because according to the Swift documentation:

subclasses do not inherit their superclass initializers by default

Write the required modifier before the definition of a class initializer to indicate that every subclass of the class must implement that initializer:

and since: required init?(coder aDecoder: NSCoder) is the only required initializer in UIView class then it should be the only initializer to be possibly called on Self which is a subclass of UIView in this extension.

It is getting even more fun when you realize that this initializer is not called directly because in loadFromNib declaration, no-argument initializer init() from NSObject is used (which underneath seems to fallback to init(frame:) with CGRect.zero passed as an argument).
This is even more surprising in terms of compilation since the code below (which mimics NSObject and UIView initializers) does not compile:

class Base : NSObject {
}

class Subclass : Base {
    init(frame: CGRect){}
    required init?(coder aDecoder: NSCoder) {}
}

class Custom : Subclass {
    required init?(coder aDecoder: NSCoder) {
        super.init(coder: aDecoder)
    }
}

extension NibOwnerLoadable where Self : Subclass {
    static func testing(test: Self = Self()) -> Self {
        return test
    }
}

and produces compile-time error saying:

Cannot invoke initializer for type 'Self' with no arguments

or if we change where Self : Subclass to where Self : Base

Constructing an object of class type 'Self' with a metatype value must use a 'required' initializer

which is the best description of what is happening.

But for some reasons it compiles when using UIView ...

Having all this in mind, I have created a pull request for increased safety when using init(frame: CGRect) to explicitly say that this is possible by implementing FrameInitializable protocol.

So loadFromNib() with no arguments version is added to an extension of NibOwnerLoadable for UIView implementing FrameInitializable protocol and uses the argument version from NibOwnerLoadable extension for UIView. Consequently, the non-argument version does not need @discardableResult because one for sure needs to use the returned object, otherwise it is pointless calling this function.

This is a simple and small change and it expresses more explicitly what is actually needed for UIView to safely use loadFromNib() without owner parameter.

…conforming to FrameInitializable protocol

AliSoftware · 2017-02-12T23:22:15Z

Thanks for the PR! 👍

Indeed this is surprising that = Self() is accepted given that the only constraint we have on Self here is that it inherits UIView, and UIView shouldn't inherit init() from NSObject according to Swift rules. But I think that's a special case / discrepency that is due to the fact that UIView comes from ObjC and that this initializer is inherited… in the ObjC side, and then brought to Swift by bridging, making it escaping the Swift-specific rules of initializer inheritance?

Anyway, that should definitely be addressed indeed. But I'm wondering why this FrameIntializable protocol is needed, given than the extension where you use it already (rightfully) assume that Self: UIView and that we know that every UIView has an init(frame:) initializer?

I mean, maybe not all UIView subclasses would be guaranteed to have an init(frame:) initializer per Swift initializer rules, as the only required init is init(coder:) as you point out, but in practice given ObjC bridging I'm wonder if those apply here. Could be worth a test, creating a subclass of UIView which only provide an init(coder: ) implementation and see if the init(frame: ) is still available and works for this subclass (even if that doesn't follow Swift rules, but because it's an @objc class) or not. If it is, then FrameInitializable shouldn't be necessary and only making Self: UIView should suffice, and then we can keep the original code before your PR but just replace owner: Self = Self() with owner: Self = Self(frame: .zero) instead?

Whatever the solution will be, it's an interesting problem, and might reveal some surprising behaviour of ObjC bridging vs Swift init rules…

Skoti · 2017-02-12T23:39:48Z

I think this is a compiler bug, because in runtime init(frame:CGRect) is not added to a custom subclass of UIView (even though this is an NSObject) and any call to that initializer will crash in runtime. This can be easily checked by removing init(frame:CGRect) from MyCustomWidget:

fatal error: use of unimplemented initializer 'init(frame:)' for class 'ReusableDemo.MyCustomWidget'

The same happens even if replacing no-argument initializer with loadFromNib(owner: Self = Self(frame: CGRect.zero)).

So, this FrameInitializable protocol is there to guarantee that a particular UIView subclass does have init(frame:CGRect) initializer to make everything safe.

AliSoftware · 2017-02-12T23:44:56Z

I see. Worth filling a radar then.

Wouldn't that change mean that we'd have to make every UIView we want to use here conform to FrameInitializable explicitly? 😢

Skoti · 2017-02-12T23:53:50Z

You need to conform to FrameInitializable only if you would like to load a custom view directly from code (so an owner must be created too). If one tends to use such a view only in other XIBs (in UIViewController or other UIViews xibs) then overriding init(coder:) is enough, and the owner is already created, so you pass self and use loadFromNib with arguments.

Like in MyCustomWidget:

  required init?(coder aDecoder: NSCoder) {
    super.init(coder: aDecoder)
    MyCustomWidget.loadFromNib(owner: self)
  }

AliSoftware · 2017-02-12T23:55:26Z

Oh you're right, indeed! Fair enough.

AliSoftware · 2017-02-13T19:49:50Z

Just a thought, but after re-reading https://oleb.net/blog/2016/12/protocols-have-semantics/ — and also given that this is merely a bug, I wonder if Introspection to see if init(frame:) exists wouldn't be better than a protocol conveying API info rather than semantics?

Skoti · 2017-02-13T22:37:45Z

Great post! Thanks! 👍
I liked the idea.

Probably it would be better to check if init(frame:) exists by calling .responds(to: #selector(UIView.init(frame:))) on Self. However, then the loadFromNib function would need to return optional, and from the calling side it is not clear why it might return nil (although it could be explained in documentation comments). Still this gives us only a runtime safety.

So I propose two solutions:

Give more semantics by renaming FrameInitializable protocol to UIViewFrameInitializable -> this gives a little bit more of knowledge or to UIViewDefaultOwnerZeroFrameInitializable 😃 which is kind of ridiculous and it could take part in a UIKit competition for the longest name (and still be at the tail end of it 😄)
Use the solution mentioned at the end of the article. I really liked the idea of passing in an initializer as a parameter. This would be clear enough. On the calling side you exactly see what is needed for this version of loadFromNib to create a view of a particular class.
So loadFromNib would look like this:
loadFromNib(createOwnerWithFrame: @noescape ((CGRect) -> Self)) -> Self

I would recommend the second solution, seems to be very elegant. Do you agree?

AliSoftware · 2017-02-13T22:52:30Z

Good point on the optionality of the init…

Actually, even though the second solution is elegant, for our specific case it seems to complexify the solution to give such a closure just to let the responsibility to loadFromNib to be able to call this with CGRect.zero… and why .zero, anyway?
(I mean, the answer is obviously because "what other frame could we use anyway when we can't know which other to use" and that's my point, looking back, if loadFromNib isn't the one knowing the initializer, it shouldn't be the one knowing which frame to create the owner with either, right?)

All that to say, if we continue that reasoning, we should even pass a closure @noescape (() -> Self) and not @noescape ((CGRect) -> Self), and let the closure use whatever rect it wants (so we can loadFromNib() { MyView(frame: .zero) } or use another arbitrary frame. And continuing down that path… isn't that just the same as passing the instance of Self directly?

So, in practice, wouldn't it be much simpler (in terms of "having a more simple and understandable API" but also in terms of architecture and readability) to… just not have a default value for that owner parameter after all, and forcing people to pass their UIView subclass instance in the first place? After all, if you look at the difference between:

MyView.loadFromNib(createOwnerWithFrame: MyView.init(frame:) ) // your solution, explicit param
MyView.loadFromNib() { MyView(frame: $0) } // same solution, with trailing closure
MyView.loadFromNib(MyView(frame: .zero)) // the current solution when not using default param value

Then I think the two first solutions are not bringing that much simplicity compared to the already existing third one, right?

So overall, I think the simple solution to this bug is to simply get rid of the default parameter value altogether. What do you think?

Also, to balance the removal of the possibility to call MyView.loadFromNib(), we could provide as a counterpart a method that loads the content of an already-instantiated view from the Nib. Meaning instead of having to write MyView.loadFromNib(owner: existingMyView), it would be nicer to be able to call existingMyView.loadNibContent() or something.
So just a convenience func loadNibContent() { type(of: self).loadFromNib(owner: self) } would help mourn the loss of the default parameter value of static func loadNibContent(owner: …) 😉

AliSoftware · 2017-02-13T22:58:52Z

Thinking about it more, even if for NibLoadable it can make sense to have a class func loadFromNib(), for NibOwnerLoadable where the instance of the class conforming to that protocol is supposed to be the Nib Owner and thus already instantiated anyway, it would make sense to not have a class func loadFromNib(owner: ) class method at all, but directly func loadFromNib() instance method instead, right?

Skoti · 2017-02-13T23:34:38Z

That's true we do not know which other frame to use. And if one needs to create a view directly in code then one probably knows which frame would be the best to use, so it makes sense to get rid of that frame from this closure. However, as you pointed out it is almost the same as passing in an instance to loadFromNib(owner:). So we could just leave loadFromNib(owner:) without the default parameter value, and if that's the case then we can change it to an instance method as well.

All things considered, it totally makes sense for NibOwnerLoadable to have an instance method instead of a static one!
The simpler the better. I absolutely agree.

AliSoftware · 2017-02-13T23:43:58Z

Cool 😎
Care to push a new commit to your branch to implement that new solution?

Also don't forget to add an entry to the CHANGELOG.md file to credit yourself!

(Note that this change would mean a major new version as it would be a breaking change in the API. But for the better)

Skoti · 2017-02-13T23:55:48Z

Sure! I will push a commit. Thanks.
Would you like me to bump a version in changelog too, or use #Unreleased?

We could even change the instance method name to something more appropriate, since it is not just loading a view from nib (like in NibLoadable case) but rather filling out or populating the owner with the nib content.
Following an exapmle of UIView and addSubview and all set of add methods for UIView, we could use addNibContent. What do you think?

AliSoftware · 2017-02-14T00:00:19Z

Just use # Unreleased or # master, we'll then determine the right version matching semantic versioning only at the time we do a new release.

I agree a more appropriate name for the instance methods could be nice. I think we should keep the term "load" in the method name though, as it mirrors the "Loadable" part in the protocol name.

What about loadNibContent() or loadContentFromNib()?

Skoti · 2017-02-14T00:08:21Z

You're right, being consistent with the protocol name is better.
I would opt for loadNibContent() it kind of says for itself and loadFromNib() slightly suggests a parameter like loadWithNib would totally do.

AliSoftware · 2017-02-14T00:36:48Z

Quick note: don't forget to also update the example project to use that new instance methods, and to update the README file accordingly too 😉

…t() instance method in NibOwnerLoadable protocol, removing the old static loadFromNib(owner:) method from NibOwnerLoadable protocol

…tionHeaderView and MyXIBIndexSquaceCell

Skoti · 2017-02-15T22:07:04Z

@AliSoftware
New commits have been pushed. I hope you don't mind two super minor fixes: one for documentation comments and the second one for displaying cells above UITabBar in a TablewViewController scene.

AliSoftware · 2017-02-15T22:10:04Z

Sources/View/NibOwnerLoadable.swift

                      (and to which you want to add the XIB's views as subviews).
   - returns: A `NibOwnerLoadable`, `UIView` instance
   */
  @discardableResult


This method doesn't return any result anymore so @discardableResult can be removed here

@discardableResult and documentation comments too 😄

Ah, good catch!

AliSoftware · 2017-02-15T22:10:55Z

README.md

 Overriding `init?(coder:)` allows your `MyCustomWidget` custom view to load its content from the associated XIB `MyCustomWidget.xib` and add it as subviews of itself.

-_💡 Note: overriding `init(frame:)`, even just to call `super.init(frame: frame)` might seems pointless, but seems necessary in some cases due to a strange issue with Swift and dynamic dispatch not being able to detect and call the superclass implementation all by itself: I've sometimes seen crashes when not implementing it explicitly, so better safe than sorry._
+_💡 Note: possible to override `init(frame:)`, in order to be able to create an instance of that view programatically and call `loadNibContent()` to fill with views if needed.


"Note: it is also possible to …"

AliSoftware · 2017-02-15T22:14:20Z

Sources/View/NibOwnerLoadable.swift

+   - returns: A `NibOwnerLoadable`, `UIView` instance
   */
  @discardableResult
-  static func loadFromNib(owner: Self = Self()) -> Self {


Now that the new instance method doesn't return any value, @discardableResult can be removed too (as it doesn't make sense on a function returning Void)

huh at first I thought my comment on this line had disappeared, so I reposted it as you saw above… but now I see it's present twice… on the same line… but presented as 2 separated comments if it was 2 different files?! GitHub bug?!

I was wondering about that too. I don't know if that's github bug, however, it made me noticed that documentation comments needs to be changed 😄

AliSoftware · 2017-02-15T22:29:41Z

CHANGELOG.md

+
+* Fixing table view controller scene to display cells above UITabBar
+  [@Skoti](https://github.com/Skoti)
+


Please rework your entry so that it follows the same format as the other entries.
In particular, use a period + 2 spaces at the end of the lines describing the changes so that its properly formatted when the markdown is rendered (as a "new line in the same paragraph")

May I suggest something along those lines instead?

## Unreleased ### Breaking changes * `static func loadFromNib(owner:)` of `NibOwnerLoadable` has been replaced by instance method `func loadNibContent()`. This is more consistent and also avoids possible crashes when used with `UIView` subclasses not implementing non-required initializers `init()`/`init(frame:)`. [@Skoti](https://github.com/Skoti) [#40](https://github.com/AliSoftware/Reusable/pull/40) ### Enhencements * Fixing documentation typos for `CollectionHeaderView` and `MyXIBIndexSquaceCell`. [@Skoti](https://github.com/Skoti) * Fixing table view controller scene in Example project, to display cells above `UITabBar`. [@Skoti](https://github.com/Skoti)

sure, although I'm gonna use "Enhancements"

Whoops 😄 👍

AliSoftware · 2017-02-15T22:33:45Z

Thanks for the changes! We're close to the solution I think; just some very little nitpickings before we can merge then we should be good to go 👍

AliSoftware · 2017-02-15T22:41:52Z

Example/ReusableDemo/TableViewController.swift

  override func tableView(_ tableView: UITableView, viewForHeaderInSection section: Int) -> UIView? {
-    let view = MyHeaderTableView.loadFromNib()
+    let view = MyHeaderTableView(frame: CGRect(x: 0, y: 0, width: tableView.bounds.size.width, height: self.tableView(tableView, heightForHeaderInSection: section)))
+    view.loadNibContent()


Maybe to better demonstrate the possibility of loading NibOwnerLoadable views from code, it would be better in this case to override the init(frame: ) of MyHeaderTableView so it calls self.loadNibContent() in that subclass' initializer? (instead of having to think about calling loadNibContent() on every new MyHeaderTableView instance we create like here?)

good point, I'm gonna use it

Could be the occasion to:

Break that line 30 in 2 lines (let frame = CGRect(…) + let view = MyHeaderTableView(frame: frame)

Add a comment just above this call to init to remind the reader of the example project that "this calls the overriden init of MyHeaderTableView which loads its content from its Nib automatically via loadNibContent()" or something?

Like that?
"This calls the overriden init of MyHeaderTableView which loads its content from the default nib (see NibOwnerLoadable extension) automatically via loadNibContent() instance method."

Yeah, maybe just shorten a bit like this?

// See the overridden `MyHeaderTableView.init(frame:)` initializer, which // automatically loads the view content from its nib using loadNibContent()

…cumentation comments

…e view content from its nib using loadNibContent()

…e a proper markdown format

AliSoftware · 2017-02-15T23:53:05Z

I'll leave the CI finish its work tonight, I'm going to bed. Will merge it tomorrow (and probably do a release alongside) 😉

Thanks again for spotting this bug and for your work on that PR, that was awesome 👍

Skoti · 2017-02-16T06:30:49Z

Thanks for your big help and great ideas too 😄
BTW: This Travis seems to fail everytime 😅

AliSoftware · 2017-02-16T09:15:59Z

Yeah, idk what's wrong with Travis, I'll have to migrate to Circle-CI some time!

Increasing safety in loadFromNib by the default owner argument value …

f6a914e

…conforming to FrameInitializable protocol

Skoti added 4 commits February 15, 2017 22:47

removing FrameInitializable protocol in favour of a new loadNibConten…

beda40f

…t() instance method in NibOwnerLoadable protocol, removing the old static loadFromNib(owner:) method from NibOwnerLoadable protocol

fixing incorrect protocol naming in documentation comments for Collec…

76b7478

…tionHeaderView and MyXIBIndexSquaceCell

fixing the table view controller scence to show cells above UITabBar

9a0c647

updating README and CHANGELOG files

cac93c8

AliSoftware requested changes Feb 15, 2017

View reviewed changes

AliSoftware reviewed Feb 15, 2017

View reviewed changes

Skoti added 4 commits February 16, 2017 00:15

removing @discardableResult from loadNibContent() and changing its do…

5801ad9

…cumentation comments

overriding init(frame:) of MyHeaderTableView to automatically load th…

659b3fa

…e view content from its nib using loadNibContent()

improving NibOwnerLoadable init(frame:) remark text in README

688376e

improving changelog entry description, updating changelog entry to us…

3fc0c11

…e a proper markdown format

AliSoftware approved these changes Feb 15, 2017

View reviewed changes

AliSoftware merged commit 3fc0c11 into AliSoftware:master Feb 16, 2017

AliSoftware mentioned this pull request Mar 14, 2017

[SR-4241] Generic function with NSObject type allows unimplemented initializer to be called. swiftlang/swift#46824

Open


		* Fixing table view controller scene to display cells above UITabBar
		[@Skoti](https://github.com/Skoti)

Increasing safety in loadFromNib #40

Increasing safety in loadFromNib #40

Uh oh!

Conversation

Skoti commented Feb 12, 2017

Uh oh!

AliSoftware commented Feb 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Skoti commented Feb 12, 2017

Uh oh!

AliSoftware commented Feb 12, 2017

Uh oh!

Skoti commented Feb 12, 2017

Uh oh!

AliSoftware commented Feb 12, 2017

Uh oh!

AliSoftware commented Feb 13, 2017

Uh oh!

Skoti commented Feb 13, 2017

Uh oh!

AliSoftware commented Feb 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AliSoftware commented Feb 13, 2017

Uh oh!

Skoti commented Feb 13, 2017

Uh oh!

AliSoftware commented Feb 13, 2017

Uh oh!

Skoti commented Feb 13, 2017

Uh oh!

AliSoftware commented Feb 14, 2017

Uh oh!

Skoti commented Feb 14, 2017

Uh oh!

AliSoftware commented Feb 14, 2017

Uh oh!

Skoti commented Feb 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AliSoftware Feb 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Skoti Feb 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AliSoftware commented Feb 15, 2017

Uh oh!

AliSoftware Feb 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AliSoftware commented Feb 15, 2017

Uh oh!

AliSoftware commented Feb 12, 2017 •

edited

Loading

AliSoftware commented Feb 13, 2017 •

edited

Loading

AliSoftware Feb 15, 2017 •

edited

Loading

Skoti Feb 15, 2017 •

edited

Loading

AliSoftware Feb 15, 2017 •

edited

Loading