preferDouble rule breaks system apis

[christiankm]

We have recently added the preferDouble rule to our project, which seems to just replace all instances of CGFloat with Double. In most cases that's perfectly fine and expected.

However it also replaces this for some system APIs fx. for UITableView, which means the API is no longer the same. So this rule should not replace instances in system APIs, as it will cause build errors. See attached screenshots.

override func tableView(_ tableView: UITableView, heightForRowAt indexPath: IndexPath) -> CGFloat {
        UITableView.automaticDimension
    }

One suggestion could be to not replace the CGFloat value in functions returning that type.

BEFORE:

AFTER:

[Sherlouk]

Plus one on this, caused some confusion when it broke various SwiftUI APIs for me. Disabled for now 🙂

Notably, simply disabling the rule when it's a returning type wouldn't work for SwiftUI situations where I'm passing in a CGFloat to an API which requires it. SwiftFormat would have no way of knowing that the API requires CGFloat.

[cameroncooke]

Have encountered the same issues. There are many edge cases and it's quite subtle where it can cause unexpected behaviour in cases where a type conforms to a non-source protocol that uses CGFloat as part of optional method declarations. Given the return type and argument types are part of the uniqueness of a function signature this is problematic.

[danieleformichelli]

Plus one on this. Another thing that breaks is id you define an extension on CGFloat and it get "moved" to Double, for example:

BEFORE:

extension CGFloat {
  var twice: Self {
    return 2 * self
  }
}

let someView = UIView()
someView.bounds.width.twice

AFTER:

extension Double {
  var twice: Self {
    return 2 * self
  }
}

let someView = UIView()
someView.bounds.width.twice ❌

[denizdogan]

Just chiming in with the same problem. This rule seems useless, perhaps even counter-productive, unless it takes into account each specific use of CGFloat, which is a very tall order. Looking at the source code, it literally just replaces all "CGFloat" tokens with "Double" tokens.

var CGFloat: Bool = true becomes var Double: Bool = true – contrived example, I know, but it still speaks volume about how dangerous this rule is in practice.

Unless someone is feeling really productive, and manages to figure out which instances are allowed to replace, I'd vote to remove this rule entirely, or mark it as experimental, or with a fat warning, just something. Running this on a large codebase can easily introduce bugs for something that was intended as an entirely cosmetic modification.

[cameroncooke]

I would agree with that. It’s far more dangerous than it is useful. In fact it introduced subtle bugs in our app. Luckily we found them in development.

[Sherlouk]

I do not disagree with the above, but I'd love to hear more about these bugs you've experienced?

I've only seen compiler errors thus far (which are of course much easier to spot!)

[cameroncooke]

As a concrete example, here is a change that was made that we had to reverse:

It's important to note that the return type is part of the uniqueness of the method signature, so after the conversion to Double the code to adjust the header height was no longer being called. This didn't cause any compiler issues as the method on the UITableViewDelegate protocol is optional.

[denizdogan]

@Sherlouk My issues have been pretty much what OP describes in this GitHub issue. We can't just blindly replace all instances of CGFloat with Double and expect things to work, unless we're working in some very specialized codebase. But if we're working in such a codebase, why would anyone use CGFloat in the first place?

The flag just doesn't make much sense to me – not as a part of CI, not as a pre-commit, barely even as a one-off.

To be honest, I'm curious about the opposite here: if anyone has actually found this flag useful in their day-to-day, I'd love to hear more about it. I can't imagine that people are just accidentally writing "CGFloat" when they really meant "Double"?

[Sherlouk]

Sorry @denizdogan - let me clarify. I am absolutely agree with the issue the OP has raised, and I too have experienced it (and have disabled this rule in my project, and agree it should just be deleted to avoid future issues).

I was just curious, just for my own learning, if somebody could share how changing CGFloat -> Double in their codebase caused a bug rather than a compiler error.

Cameron's example there is perfect. An optional delegate function where this rule changes the method signature and causes it to no longer be called! Not obvious in the slightest, and adds far more merit to removing this rule (in my opinion).

I can't imagine that people are just accidentally writing "CGFloat" when they really meant "Double"?

Aside from purely force of habit, I agree. Though as a one-off I can see this maybe being valuable to large pre-existing codebases? But in that case you'd get just as much value from Xcode's find and replace value.

[denizdogan]

@Sherlouk Ah, I see. Sorry for misunderstanding.

[nicklockwood]

Sorry for not chiming in earlier. I was aware of some of these issues (which is why the rule shipped disabled by default) but was hoping I'd be able mitigate some of them. I'm not sure there's much I can do though since there's no local indication when a method is part of a protocol conformance.

[Sherlouk]

I don't think anyone disagrees with you there Nick, it would be a mammoth task to make this rule work reliably given Swift's current implementation.

Apple will do a batch release next year, in all likelihood, and phase out/deprecate CGFloat.

Until then I think either remove the rule or add some extra warnings/notes/maybe a link to this issue explaining the risks of that rule.

I'm totally supportive of opt in rules, but I think when it has the possibility to cause hard to spot bugs... I personally draw a line there