`optional_string_data_conversion`'s `Data` -> `String` rule should be optional

[jshier]

New Issue Checklist

• [x] I've Updated SwiftLint to the latest version.
• [x] I've searched for existing GitHub issues.

Feature or Enhancement Proposal

In 0.57.0, the non_optional_string_data_conversion rule was split: the Data -> String conversion check was actually inverted and made its own rule. Unfortunately, as a default rule it's very heavy handed. In the vast majority of Swift apps, using String(decoding:as:) is perfectly correct, as non-UTF-8 strings are increasingly rare, and there often aren't any arbitrary strings at all. Additionally, using this initializer is an explicit acknowledgment that we don't care if the string isn't fully UTF-8. That is, we don't care if the decoding placeholder is inserted. That's may be a bug, but a far more minor one than not handling a string at all.

This part of the rule made optional by default.

[jshier]

Additionally, since there's no way for Swift to convey the fact that a String is already known to be valid UTF-8, at least in a way that SwiftLint can see, this rule introduces a lot of unnecessary optionals. 100% of my use cases in various apps and other codebases are already known valid UTF-8. This includes both String literals and APIs like StaticString.withUTF8Buffer. These are good reasons why this rule shouldn't be on by default.

[SimplyDanny]

I understand that in your case, there are a lot of measures and implicit knowledge in place suggesting that some data is valid UTF-8. That's not necessarily the case in other code bases, though.

In your case, you'd go and disable the rule in your configuration because you know better. Others might like the rule pointing them to potential issues by default and leave it enabled.

Deciding about adding a rule as opt-in or enabled-by-default is always opinionated. Luckily, everything is configurable for your own best fit.

Changing defaults of a rule after it has been released causes some trouble as well. I don't think that's worth it since there are no hard objective arguments.

[jshier]

Except there are "hard objective arguments" about default rules, both in general and this one in particular. SwiftLint's README currently says this about opt-in rules:

Guidelines on when to mark a rule as opt-in:

• A rule that can have many false positives (e.g. empty_count)
• A rule that is too slow
• A rule that is not general consensus or is only useful in some cases (e.g. force_unwrapping)

optional_string_data_conversion fails the first and last guidelines.

First, in all my codebases it has a 100% false positive rate. This includes everything from high level apps to low level libraries. And given there's no way for the rule to know ahead of time whether the String being converted is already UTF-8 (aside from literals, maybe), there's no real way to improve this false positive rate either.

Second, I'm not sure what consensus means to this project, but the rule was enabled as a default through a single PR with little apparent discussion. But it certainly fails the second part of the guideline, as it's only really useful with dynamic strings (not round tripped literals or other content) produced by systems which don't default to UTF-8 (increasingly rare) where failure is desired over repair (not often the case in UI presenting code, perhaps the case in low level systems) and the developer is unaware of the behavior of String(decoding:as:).

So, by SwiftLint's own guidelines, this should be an opt-in rule.

Deciding about adding a rule as opt-in or enabled-by-default is always opinionated. Luckily, everything is configurable for your own best fit.

By that logic all rules should be opt-out, since everything's configurable. Obviously SwiftLint has to have some guidelines around what rules should be opt-in or out. It just doesn't seem like they were applied in this case.

Changing defaults of a rule after it has been released causes some trouble as well. I don't think that's worth it since there are no hard objective arguments.

Changing a rule to opt-in isn't breaking, and given the rule was added as a default, which is far more invasive, shipping a quick follow up release to turn it off seems better than the alternative. At worst people would've made some things optional that didn't need to be (though I expect the vast majority would just turn the rule off, given some thought).

[DarkDust]

In a project of mine with 1000+ Swift files, this rule is also 100% false-positive. "Just" 10 hits but still too noisy to disable on an individual basis so I've disabled it for the whole project. Like Jon, we explicitly used the non-failing initializers because there's no point in using the failing initializer when we know it can't fail.

I'm not sure whether this rule should be off by default or not, I can see the point of making you think about it that Danny makes. Just adding my comment here as a data point.