Bug fix: setting properties through kf extension causes compiler error

[GarthSnyder]

In the current master branch (and I believe, Kingfisher 5 generally), attempting to set Kingfisher attributes through the kf variable of KingfisherCompatible causes a compiler error:

class TestView: UIImageView {
    func setUp() {
        kf.indicatorType = .activity  // Error: cannot assign to property: 'self' is immutable
    }
}

The reason for this is subtle. The KingfisherCompatible protocol is not class-specific, so it could be conformed to by a struct. Setters are implicitly mutating. And structs are allowed to reassign self inside of any mutating method.

However, class instances do not allow the assignment of self. So when KingfisherCompatible is adopted by a class entity, the compiler does not allow access to the setter.

I know this sounds wacky; more details here and here, verified by a couple of Swift devs.

There are a couple of possible solutions. KingfisherCompatible could derive from AnyObject, which is the current version of the old class protocol construct. Or you can explicitly mark the setter as nonmutating, which seems like the better option here since it actually does nothing.

[onevcat]

@GarthSnyder Making the nonmutating seems crashing the tests with EXC_BAD_ACCESS on Xcode 10.1 when setting the kf properties. So I don't think we should merge it before we can fully understand why. (It's going well on Xcode 10.2, so it might be a bug in Swift 4 compiler?)

Now you can always assign kf to a var to use it. For your example, try:

class TestView: UIImageView {
    func setUp() {
        var kf = self.kf
        kf.indicatorType = .activity
    }
}

[GarthSnyder]

Indeed not! How interesting...

Unfortunately, KingfisherCompatible is applied to both value and reference types, so it can't simply be a class protocol (see the OP above for an explanation of why this is an alternate solution). However, it looks like there are no extensions other than kf, so it appears pretty straightforward to split out a separate KingfisherCompatibleValue protocol, as in the updated PR.

I see in the git history that this protocol was for a while split into KingfisherClassCompatible and KingfisherValueCompatible between 4 and 5, so I may be going down a path that someone has already trodden. That previous split seems to have involved different types for KingfisherWrapper as well, however, so I'm not sure if the motivation was related.

[onevcat]

Hi, @GarthSnyder

Thanks for updating it. I am so sorry that I didn't explain it clearly.

Yes, you are right on the git history. The change to make it only struct compatible was made in Kingfisher 5. The main purpose was for performance.

This compatible helper type (kf) was once split into two kinds, one for class and another for struct. For class base members (like UIImage), it was using the class version; while for struct (CGSize, etc.), the struct version.

However, the getter of KingfisherCompatible.kf is trying to create a new instance of KingfisherWrapper every time when it is called. When it is a class, heap allocating will happen and it would create quite a few "temporary" objects during a simple image setting call. Making KingfisherWrapper a struct prevents it, because creating a struct only happens in the stack, which makes it faster. So it is an intended change in Kingfisher 5, with some minor inconvenience as price when setting in a nonmutating context.

As you pointed out, nonmutating would be the best solution for it, since it actually not mutates anything. But since we still want to support Xcode 10.1 for a while, I guess we'd better just leave it as is now.

[GarthSnyder]

Thanks for the additional comments.

Did you take a look at the detailed diffs? The KingfisherWrapper can continue to be a struct for both reference and value types.

When the outer KingfisherCompatible entity is a class, the compiler knows it won't try to assign self. When it's a value type, assigning self would be fine, so the compiler doesn't complain there either. Only the outermost protocol needs to split.

[onevcat]

Oh, that looks great!

[onevcat]

Thank you for it!

[mrsnow-git]

This code now crash our app.
Latest Kingfisher version (5.4, any since this commit), swift 5, xcode 10.2.1 (10E1001).
on any code like this:
imageView.kf.setImage(with: url)
.
I think on any code used .kf
But we don't now understand why and how to fix it.

[onevcat]

@mrsnow-git

It is the first time we receive a report on it. These changes also work well on quite a few apps and tests without a problem.

Is there a crash log or can you reproduce it in a new project? Maybe try a full clean and rebuild for your app? Or do you have any extension on Kingfisher's compatible struct?

(Also worth to say if you are depending on Kingfisher in different targets, you may need to rebuild them all and keep the commit the same since the changes in this commit also affects calling convention and Swift now does not have module compatibility.)

[mrsnow-git]

sorry for long delay, had a lot of work on two projects.

for sure, I tried full clean (and folders cleaning) and full rebuild - nothing helped at this moment. And I don't have any code intersection - checked for it.

BUT
right now I'm tried to use it again (version 5.4) - and all working well. now I can't reproduce this issue, tried different environments - all work as needed.

Only changes what can cause effects on it - it's simulators restarts and working computer restart. Not any other things come into my head when I thinking what else was changed.

Will look at this more during my work. For now I'm used Kingfisher 5.5 version.