Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default values with UnsafeRawPointers do not compile #55

Closed
kkiermasz opened this issue Apr 18, 2024 · 4 comments 路 Fixed by #65
Closed

Default values with UnsafeRawPointers do not compile #55

kkiermasz opened this issue Apr 18, 2024 · 4 comments 路 Fixed by #65
Labels
bug Something isn't working

Comments

@kkiermasz
Copy link

kkiermasz commented Apr 18, 2024

Hi there,

First of all, I'm really glad you've created this, even though I'm scared to add any new dependencies to the project because Swift 6 is close, I'll give it a try 馃槄

I found an issue with %s and %p parameters.
I wanted to fix it by myself and open a PR, but it's the first time I've seen ### formatting, so I guess you know what to do better and faster :)

image

Btw you might want to create 0.1.3 anyway, as some handy fixes to the documentation were added since 0.1.2 馃枛馃徎

@liamnichols
Copy link
Owner

Hi @kkiermasz 馃憢 Good catch, thanks for raising this! I haven't actually used %s and %p format specifiers myself so that's why this was missed.

I have however been looking into some improvements around this part of how we generate the defaultValue (String.LocalizationValue type) because of a separate issue with parameter ordering and I think there might be some limitations with the new String.LocalizationValue type compared to using NSLocalizedString(..).

(I should mention that I only added parsing support for %s and %p as I was mirroring what other tools such as R.swift and SwiftGen supported)

The ### hashes are a quick workaround to escape double quotes that may exist inside the value that I construct. In the example from your screenshot ###"Test \###(arg1)"### is the same as "Test \(arg1)".

With that in mind, I think that we can work out the exact behaviour that we want by testing the String.LocalizationValue directly... I'm guessing that the following code doesn't work?

let arg: UnsafeRawPointer = ...
let value: String.LocalizationValue = "Test \(arg)"
dump(value)

I think that this is because the String.LocalizationValue type is much more restrictive compared to the NSLocalizedString(...) API. You can look at String.LocalizationValue.StringInterpolation to get an idea of which custom types it accepts for interpolation:

https://developer.apple.com/documentation/swift/string/localizationvalue/stringinterpolation

From what I can see, there are three overloads of interest to us that accept different values:

  1. NSObject
  2. String
  3. _FormatSpecifiable

It seems that UnsafeRawPointer doesn't fit into any of these overloads, which is why I think that we are seeing this compiler error 馃槙

Note

A note about the private _FormatSpecifiable type.. It's underscored which means that it isn't documented, but you can find more about it in the .swiftinterface file.

/Applications/Xcode-15.2.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS17.2.sdk/System/Library/Frameworks/Foundation.framework/Modules/Foundation.swiftmodule/arm64-apple-ios.swiftinterface

Here is the important stuff

@_alwaysEmitIntoClient @_semantics("constant_evaluable") private func formatSpecifier<T>(_ type: T.Type) -> Swift.String {
    switch type {
    case is Int.Type:
        fallthrough
    case is Int64.Type:
        return "%lld"
    case is Int8.Type:
        fallthrough
    case is Int16.Type:
        fallthrough
    case is Int32.Type:
        return "%d"
    case is UInt.Type:
        fallthrough
    case is UInt64.Type:
        return "%llu"
    case is UInt8.Type:
        fallthrough
    case is UInt16.Type:
        fallthrough
    case is UInt32.Type:
        return "%u"
    case is Float.Type:
        return "%f"
    case is CGFloat.Type:
        fallthrough
    case is Double.Type:
        return "%lf"
    default:
        return "%@"
    }
}
@available(macOS 12, iOS 15, tvOS 15, watchOS 8, *)
public protocol _FormatSpecifiable : Swift.Equatable {
  associatedtype _Arg : Swift.CVarArg
  var _arg: Self._Arg { get }
}
@available(macOS 12, iOS 15, tvOS 15, watchOS 8, *)
extension Swift.Int : Foundation._FormatSpecifiable {
  public var _arg: Swift.Int64 {
    get
  }
  public typealias _Arg = Swift.Int64
}
@available(macOS 12, iOS 15, tvOS 15, watchOS 8, *)
extension Swift.Int8 : Foundation._FormatSpecifiable {
  public var _arg: Swift.Int32 {
    get
  }
  public typealias _Arg = Swift.Int32
}
@available(macOS 12, iOS 15, tvOS 15, watchOS 8, *)
extension Swift.Int16 : Foundation._FormatSpecifiable {
  public var _arg: Swift.Int32 {
    get
  }
  public typealias _Arg = Swift.Int32
}
@available(macOS 12, iOS 15, tvOS 15, watchOS 8, *)
extension Swift.Int32 : Foundation._FormatSpecifiable {
  public var _arg: Swift.Int32 {
    get
  }
  public typealias _Arg = Swift.Int32
}
@available(macOS 12, iOS 15, tvOS 15, watchOS 8, *)
extension Swift.Int64 : Foundation._FormatSpecifiable {
  public var _arg: Swift.Int64 {
    get
  }
  public typealias _Arg = Swift.Int64
}
@available(macOS 12, iOS 15, tvOS 15, watchOS 8, *)
extension Swift.UInt : Foundation._FormatSpecifiable {
  public var _arg: Swift.UInt64 {
    get
  }
  public typealias _Arg = Swift.UInt64
}
@available(macOS 12, iOS 15, tvOS 15, watchOS 8, *)
extension Swift.UInt8 : Foundation._FormatSpecifiable {
  public var _arg: Swift.UInt32 {
    get
  }
  public typealias _Arg = Swift.UInt32
}
@available(macOS 12, iOS 15, tvOS 15, watchOS 8, *)
extension Swift.UInt16 : Foundation._FormatSpecifiable {
  public var _arg: Swift.UInt32 {
    get
  }
  public typealias _Arg = Swift.UInt32
}
@available(macOS 12, iOS 15, tvOS 15, watchOS 8, *)
extension Swift.UInt32 : Foundation._FormatSpecifiable {
  public var _arg: Swift.UInt32 {
    get
  }
  public typealias _Arg = Swift.UInt32
}
@available(macOS 12, iOS 15, tvOS 15, watchOS 8, *)
extension Swift.UInt64 : Foundation._FormatSpecifiable {
  public var _arg: Swift.UInt64 {
    get
  }
  public typealias _Arg = Swift.UInt64
}
@available(macOS 12, iOS 15, tvOS 15, watchOS 8, *)
extension Swift.Float : Foundation._FormatSpecifiable {
  public var _arg: Swift.Float {
    get
  }
  public typealias _Arg = Swift.Float
}
@available(macOS 12, iOS 15, tvOS 15, watchOS 8, *)
extension Swift.Double : Foundation._FormatSpecifiable {
  public var _arg: Swift.Double {
    get
  }
  public typealias _Arg = Swift.Double
}
@available(macOS 12, iOS 15, tvOS 15, watchOS 8, *)
extension CoreFoundation.CGFloat : Foundation._FormatSpecifiable {
  public var _arg: CoreFoundation.CGFloat {
    get
  }
  public typealias _Arg = CoreFoundation.CGFloat
}

So, with this in mind, I am not sure how to proceed 馃槃 I have a few ideas though:

  1. Try adding _FormatSpecifiable conformance to UnsafeRawPointer to the target where the code is generated (XCStringsTool can't do this).
  2. If you are working with NSObject types, maybe the codegen could accept the NSObject type rather than UnsafeRawPointer?
  3. Remove (the broken) support for %s and %p as it isn't supported by String.LocalizationValue (I think this would also help with any attempt that I make to solve Sendable conformance聽#40)
  4. In your use case, use %@ in your string and instead format the pointer value prior to passing it into the generated code
    let _arg: UnsafeRawPointer = ...
    let arg = String(format: "%p", _arg)
    let localized = String(localized: .localizable.p(arg))

I hope that this brain-dump helps 馃槃 As I don't really use this feature myself, I am not sure what the best option is so it would be great to hear your thoughts! Thanks 馃檹

@liamnichols liamnichols added the bug Something isn't working label Apr 19, 2024
@kkiermasz
Copy link
Author

kkiermasz commented Apr 19, 2024

Hey @liamnichols 馃憢馃徎 ,

wow, thanks for the response 馃槃

I'm not using them either. I was checking the library condition before adding it to the project 馃槢 The screenshot is from the test snapshot.

Thanks for the reference, now I see it's not support out-of-the-box :)

Since I'm not a String expert (which is a complex topic), I'm not voting for any of the 4 solutions you mentioned 馃珷 I just thought it might be worth mentioning this in the documentation in some "limitations" section, since someone will probably encounter this problem later.

Btw it's a pity that there's no way (or I'm the reason in this case :D) to use one shared catalog across multiple feature modules, as Apple invented (View -> Catalog -> .strings), I'd give it a try.

@kkiermasz
Copy link
Author

kkiermasz commented Apr 19, 2024

Btw did you notice any performance issues? One SwiftUI view (re)render causes ~2.4k LocalizedStringResource resolve attempts, each taking ~0.00016s. I wonder if that's because the tool (catalogs itself) was not designed to expose resources publicly :/
(you know, one Resources module imported by several feature modules).

When using the old .strings in the same shared module with localizedString(forKey: key, value: value, table: table) the resolve attempt time is pretty much the same, however, it's not called every time swiftui's view rerender.

It's unusable (LocalizedStringResource) with this performance tbh

I know it's not on your side, as I would assume SwiftUI is caching resources data or smth similar, but passing resources to the views was triggering the above issue. As I have some complex views it impacts performance too much. I made a fork and changed the file generation to something similar to SwiftGen. https://github.com/kkiermasz/xcstrings-tool/blob/main/Tests/XCStringsToolTests/__Snapshots__/GenerateTests/testGenerateWithPackageAccessLevel.Localizable.swift

Who's brain-dumping now? Haha

@liamnichols
Copy link
Owner

I'm not using them either. I was checking the library condition before adding it to the project 馃槢

Ahh right I see, that's a good strategy 馃檪

The screenshot is from the test snapshot.

Ooooh, good spot. I opened #56 to keep on top of that in the future.

Since I'm not a String expert (which is a complex topic), I'm not voting for any of the 4 solutions you mentioned 馃珷 I just thought it might be worth mentioning this in the documentation in some "limitations" section, since someone will probably encounter this problem later.

Well now that I've had a think about it, I think that I am going to peruse option 3 and remove direct support for %s and %p to help with the @Sendable work.

I believe that option 4 would then be the viable workaround for anybody that did need to use this kind of formatting... I'll keep this issue open to track that 馃憤

Btw it's a pity that there's no way (or I'm the reason in this case :D) to use one shared catalog across multiple feature modules, as Apple invented (View -> Catalog -> .strings), I'd give it a try.

I'm not quite sure if I understood what you are trying to achieve here, but does using the public or package access level do what you need here?


Btw did you notice any performance issues?

As for this - not yet. I haven't personally used XCStrings Tool in any complex SwiftUI projects where it has become a concern.

I'm not quite sure of the solution to this though because my understanding is that the purpose of LocalizedStringResource is to defer the resolution of the localized string so that SwiftUI can handle it instead. I think this was primarily to support out-of-process rendering for things like App Intents, but also it lets you pass a custom locale in via the SwiftUI environment and then have the correct language resolved dynamically.

Maybe it isn't common usage though, and in that case, caching the resolved strings might be a better option, but I don't think it's a good default.

I think that it probably needs some more investigation though, so thanks for flagging it.. Perhaps you can open a separate issue to track this? There might be some ways that we can optimise the generated code more, or maybe at least add some best practice documentation 馃檹

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants