Swiftgen: Strings: valid format `% d` is ignored by SwiftGen, but still formatted.

[franmowinckel]

100% daily becomes 1000aily because the Swift template function tr always returns String(format: format, locale: Locale.current, arguments: args) even if args is empty.

% (followed by space) is afaik the invisible plus sign so somehow String(format: interprets it like a valid format argument (not sure if it should).

But the problem is that the tr function should just return the result from NSLocalizedString if no args. So just adding guard !args.isEmpty else { return format } before the last line would fix it.

[djbe]

This is not a swiftgen bug, you need to escape the % with an extra %. See the Foundation formatting docs for this.

[AliSoftware]

The problem is that, per fprintf-style standard, % is interpreted as a placeholder. Like for %d or %@ etc. If you want a literal %, you need to escape it by doubling it, using %% in your Localizable.strings (see the man page for the fprintf-family of functions). You can see it as being similar to when you have \n or \r in a string and if you want a literal \ you need to double it using \\. This is very similar here.

This is not really related to SwiftGen itself, but to how String(format: ) works.

[AliSoftware]

PS: Returning the format if args.isEmpty wouldn't solve the general problem. Because if you have a string "%@ has %d% of the shares in the company %@", to which you'd want to pass person.name (String), shares (Int) and company.name (String) as arguments, then you'd still want to pass 3 arguments but need to double the % literal character to escape it, otherwise String(format: ) would consider that % after the %d as a placeholder consuming one of the arguments, and messing things up (and this is true even if you use String(format: ) without having SwiftGen involved, so it's not really SwiftGen-related). And in that case checking for args.isEmpty wouldn't help much, as you'll still have to pass 3 arguments anyway. So you'll need to fix your format instead.

So instead in that example you'll need to use "%@ has %d%% of the shares in the company %@" for you Localized string to use it as a format but still inserting a literal % in it.

[djbe]

And if you have to present a number to the user, always use NumberFormatter, in this case with numberStyle = percent.

[franmowinckel]

@djbe I think it's clear that I knew that you need to escape %% 🙂if you format a string.

This is a valid SwiftGen bug for a simple reason: I'd expect to get the same string using L10n.xxx as with NSLocalizedString(xxx) and this is false for this case as I've explained.

@AliSoftware If we're using arguments, that's a totally different case where I think is valid to expect that the user would escape characters as appropriate. So %@ has %d% of the shares in the company %@ I think is a wrong format string.

[franmowinckel]

Anyways, for anyone having a large amount of strings working just fine with NSLocalizedString and where it doesn't make sense to "fix" them all because the tr is returning a "different" result, just add guard !args.isEmpty else { return format } to your template.

[AliSoftware]

@franmowinckel I'm not sure if that would be enough, wouldn't then SwiftGen detect a % and thus generate a static func myKey(_ p0: Any) -> String anyway instead of generating a static let myKey: String, because you'd have a % in your string which would be seen as an argument placeholder? So the guard !args.isEmpty would be too late at that point?

[franmowinckel]

@AliSoftware That'd be a valid point (:+1:) if SwiftGen was generating the func, but it's not. Just generating a let (maybe http://pubs.opengroup.org/onlinepubs/000095399/functions/fprintf.html 🤣 ).

Look, I have a really big list of strings which were translating just fine, they're coming from PhraseApp so it's not feasible to check and "fix" the format of all of them.

Additionally, I really think that SwiftGen should return just the same string as NSLocalizedString would. So probably the guard still conceptually correct but not sufficient if you fix that % d is not generating a func, right?

[djbe]

No, see, SwiftGen correctly detects that % (percent with a space) is not a format placeholder, so it thinks that the string accepts no arguments --> don't generate a function, just a constant.

The thing still is that your string is an incorrect format string:

String(format: "100% daily", locale: Locale.current) // 1000aily
String(format: "100%% daily", locale: Locale.current) // 100% daily

We can't just add quick fixes to the bundled templates to fix standard Foundation behaviour due to weird input. I think in your case, since you'd rather not change your strings, you're best served with a custom template with the fix you mentioned.

[franmowinckel]

@djbe of course it's yours to decide for a quick fix or not! 😄This is a suggested quick fix for me and other people with the same issue.

However, there's something not right in your argument. The fact that String(format: "100% daily") is detected as a valid "format" by Foundation is an indication that SwiftGen should too.

Furthermore, in bash: printf "100% daily" outputs 100 0aily so (apart from the middle space), it shouldn't be ignored because this is not Swift/Foundation specific.

Lastly, why does it have to be a format string when there are no args? You could just add another tr function specifically for let's if you don't want to change that method, which just returns whatever NSLocalizedString says.

For non func translated strings, it's more consistent and efficient to return just what NSLocalizedString says. You avoid these weird issues and you skip a call to String(format:locale:arguments:).

[djbe]

I apologise, I only got around to reading the format docs now (on phone all day), and you're completely right, % d is a valid conversion specification, and SwiftGen incorrectly ignores it.

We've been focusing the whole time on the wrong issue (thinking we should somehow circumvent this), whereas the actual issue is that SwiftGen should properly parse this and generate a static func with an integer parameter.

[AliSoftware]

Ah, indeed % d is a valid placeholder so 100% daily" should indeed be interpreted as a String expecting oneInt` parameter.

Lastly, why does it have to be a format string when there are no args?

Because there's no way for SwiftGen to know, when your Localized string is "100% daily" if you meant for it to be "the string literal 100% daily" or "the format string composed by the literal 100 followed by the placeholder % d followed by the literal aily". So whenever we see in the localised string something that can be interpreted as a placeholder, we consider it as such because we have no other choice than to guess that this what was you intended (and if it's supposed to be a % literal then you have to escape it by doubling it — if we assumed the opposite, there would be no way for people actually wanting this to be a format containing the % d placeholder to force it to be interpreted like that, while doing what we currently do of assuming it's a format string as long as there's a placeholder, you still have a way to tell us it's the other way around by escaping it)

---

So in the end there's still an issue to be fixed as @djbe pointed out, which is that % d has to be interpreted as a placeholder as it's a valid placeholder per the sprintf spec.

[franmowinckel]

@djbe Ok, with this I can finally agree 😅 That string should have been mapped as a func, there are probably some more "weird" valid formats, but as you, I'm not awared of them and they're probably edge cases.

@AliSoftware Of course SwiftGen knows! You're mapping strings to either func or let, right? The func are expected to have arguments (so it's a "format"), the ones mapped to let not. You can just return the result of NSLocalizedString for the latter and skip the call to String(format:) (yeah, a tiny bit more efficient we could argue, that's up to you).

[AliSoftware]

Wooow I actually really thought that we were already skipping the String(format:) for let constants… but indeed I just looked back at the generated code and we call the same tr function, you're right. We should indeed have a different tr function with no additional CVaArg and that skips the call to String(format:) then, indeed… My apologies, I was so convinced we already had a separate function, and was so confused by the fact that your original issue looked a lot like what other people have reported when they just didn't know that they should use %% 😓

So in the end:

1. We should fix the fact that % d isn't recognised as a valid placeholder, while per the sprintf spec it should be seen as one. That means that 100% daily should lead to a static func foo(_ p1: Int) -> String and not a let foo: String like it does currently.
2. We should indeed skip the call to String(format: ) when using a let i.e when there's no placeholders detected and extra parameters to pass
3. Those both things would not change the fact that your "100% daily" example would still need to be escaped to "100%% daily" — because SwiftGen couldn't know that you meant a literal % otherwise, given that % d should be considered as a valid placeholder — once we fixed (1).

[franmowinckel]

@djbe @AliSoftware Yes! thank you so much for your understanding! great product by the way!

[djbe]

@franmowinckel I'd like to again apologise for quickly closing and dismissing the issue, we've had some previous issues where the user didn't understand the %% syntax, and we just assumed this was a duplicate.

I'm really glad you stuck with us though, and helped us understand the underlying issue. Hopefully when you report any future issues or requests, this will go better. 😅

[AliSoftware]

Indeed 👍 I second that and apologize too for misunderstanding your original issue and for us being quite stubborn with our beliefs of thinking it was similar to the other issues about %%. I'm glad you insisted and proved us wrong, revealing the real bug 😅

[simonseyer]

We should fix the fact that % d isn't recognised as a valid placeholder, while per the sprintf spec it should be seen as one.

I guess this change broke the current behaviour for us (Swiftgen 6.0.1):

templateName: flat-swift4

We are using Lokalise for our translations and lokalise explicitly doesn't detect it as a placeholder: https://docs.lokalise.co/developer-docs/universal-placeholders. Who is right?

In addition, the solution with the two functions doesn't seem to pan out. At least the Swift compiler (Apple Swift version 4.2 (swiftlang-1000.11.37.1 clang-1000.11.45.1)) doesn't seem to like it.

[simonseyer]

In addition, if I now manually escape the percent sign with %% it doesn't detect a parameter anymore (which is correct) and doesn't format the string at all. However, this has the effect that the two percent signs stay part of the string...

[djbe]

% c is a valid placeholder, it would left pad the character with spaces. Seems like that is a bug in Lokalise.

I can reproduce the compilation issue, but it's really weird. An integer parameter (% d) is fine for Swift, but a character parameter breaks things. From further testing, it's because Character does not conform to CVarArg.

[simonseyer]

Ok. Assuming Lokalise would fix this, then the problem in my second comment would kick in. Do you have an idea how to fix this one?

[djbe]

Yeah, we need to rollback #503.

[djbe]

Anyone know how to bridge Character to something sprintf %c would understand? ~Stackoverflow is down for me 😢~ back up!

Edit: it's insane how many google results for a topic are all SO posts...

[djbe]

Right, so the correct type for %c in Swift is CChar.

[franmowinckel]

There a simple trick to check if the string should be formatted or not, not sure if you can use it though 😅 :

let stringToCheck = "blabla and maybe %c or any other format"
let needsToFormat = String(format: stringToCheck) == stringToCheck

This could potentially fix the %% issue, about Lokalise its their call (IMHO).

[AliSoftware]

@franmowinckel How would that work? your example would crash the program (trying to actually format the %c, so trying to access the first CVarArg in your String(format:) call, not find any, getting out of bounds on the stack, and crash…

Besides, calling String(format: ) to know if we should call String(format: ) seems a little bit catch 22. And why bother doing that check, and then conditionally return the original stringToCheck vs the result of String(format: )… while in those situation, by definition of your very check, the result of String(format: ) would be equal to stringToCheck…
That's like writing return (a == b) ? b : a but if b is equal to a, then returning b or returning a would do the same, so why not just do return a instead…

In the end, adding more complexity to that logic and special cases (like we tried to do in 6.0.1 and #503 to "fix" it) would lead to more bugs and less understandable logic for end-users, rather than just respecting the standard in all cases.

[franmowinckel]

@AliSoftware just tested in a Playground and it doesn't crash, but I don't mean to do this in runtime. I mean, if this could be used when parsing the strings to decide whether you need a function or not.

You could use it in tests and/or assertions to warn users that they're either, using a format not supported or should escape the % character in a specific string.

In my case, that would trigger an error while generating the file, so I could quickly detect and correct all the errors in build time, which is acceptable. If you have to detect them in runtime (as I did with the one described here), that's not really acceptable for obvious reasons.

In short, it's just checking if String(format:) with no params returns the same output as you'd expect with the args that you have detected. Btw, if you check you'll find out that with no args, is like passing 0/nil to each of the format components (i.e. %@,%s -> (null), %ld, %i -> 0)

And there's the case with a single trailing %, which String(format:) just discards (haven't tested in SwiftGen).

[AliSoftware]

This would still be super unsafe and crash prone. The fprintf behavior clearly tells us that any placeholder will fetch into the stack of additional vararg params. So of course this doesn't crash at all time, especially with just %c which only overreach one byte so the guard fence can protect the first bytes before it overreach and then corrupt memory to the point of crashing