Skip to content

Fixed BMPString encoding#111

Merged
Lukasa merged 1 commit intoapple:mainfrom
Craz1k0ek:fixed/bmp-string-from-literal
Dec 1, 2025
Merged

Fixed BMPString encoding#111
Lukasa merged 1 commit intoapple:mainfrom
Craz1k0ek:fixed/bmp-string-from-literal

Conversation

@Craz1k0ek
Copy link
Copy Markdown
Contributor

public init(stringLiteral value: StringLiteralType) {
self.bytes = ArraySlice(value.utf8)
self.bytes = ArraySlice(value.utf16.flatMap { codeUnit in
[UInt8(codeUnit >> 8), UInt8(codeUnit & 0xFF)]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[UInt8(codeUnit >> 8), UInt8(codeUnit & 0xFF)]
[UInt8(truncatingIfNeeded: codeUnit >> 8), UInt8(truncatingIfNeeded: codeUnit)]

Separately here, I think we need to handle the case of UTF-16 surrogate pairs. We should probably detect these, and fatalError out. Seeing either a high or low surrogate is an error.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the code. Not sure if the error message is clear enough and whether it's better to have a separate error for high and low surrogates

@Craz1k0ek Craz1k0ek force-pushed the fixed/bmp-string-from-literal branch 2 times, most recently from 88617a1 to f759527 Compare November 26, 2025 18:29
@rnro rnro added the 🔨 semver/patch No public API change. label Nov 27, 2025
Copy link
Copy Markdown
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks!

@Lukasa
Copy link
Copy Markdown
Contributor

Lukasa commented Nov 28, 2025

Seems like the format checker needs to be appeased.

@Craz1k0ek
Copy link
Copy Markdown
Contributor Author

Seems like the format checker needs to be appeased.

I checked out what he was complaining about yesterday, but I couldn't figure it out. Is it the triple property tuple for testing? If you let me know, I'll update the code.

@Lukasa
Copy link
Copy Markdown
Contributor

Lukasa commented Nov 28, 2025

It wants this diff applied:

diff --git a/Sources/SwiftASN1/Basic ASN1 Types/ASN1Strings.swift b/Sources/SwiftASN1/Basic ASN1 Types/ASN1Strings.swift
index bd976a6..d20b058 100644
--- a/Sources/SwiftASN1/Basic ASN1 Types/ASN1Strings.swift	
+++ b/Sources/SwiftASN1/Basic ASN1 Types/ASN1Strings.swift	
@@ -284,15 +284,19 @@ public struct ASN1BMPString: DERImplicitlyTaggable, BERImplicitlyTaggable, Hasha
 
     @inlinable
     public init(stringLiteral value: StringLiteralType) {
-        guard value.utf16.allSatisfy ({ codeUnit in
-            !(0xD800...0xDFFF).contains(codeUnit)
-        }) else {
+        guard
+            value.utf16.allSatisfy({ codeUnit in
+                !(0xD800...0xDFFF).contains(codeUnit)
+            })
+        else {
             fatalError("BMPString cannot contain characters outside the Basic Multilingual Plane: '\(value)'")
         }
 
-        self.bytes = ArraySlice(value.utf16.flatMap { codeUnit in
-            [UInt8(truncatingIfNeeded: codeUnit >> 8), UInt8(truncatingIfNeeded: codeUnit)]
-        })
+        self.bytes = ArraySlice(
+            value.utf16.flatMap { codeUnit in
+                [UInt8(truncatingIfNeeded: codeUnit >> 8), UInt8(truncatingIfNeeded: codeUnit)]
+            }
+        )
     }
 
     @inlinable
diff --git a/Tests/SwiftASN1Tests/ASN1StringTests.swift b/Tests/SwiftASN1Tests/ASN1StringTests.swift
index d338806..a43bd40 100644
--- a/Tests/SwiftASN1Tests/ASN1StringTests.swift
+++ b/Tests/SwiftASN1Tests/ASN1StringTests.swift
@@ -121,7 +121,7 @@ final class ASN1StringTests: XCTestCase {
                 "中文",
                 [78, 45, 101, 135],
                 [30, 4, 78, 45, 101, 135]
-            )
+            ),
         ]
         

Basically, two indentation tweaks, and a trailing comma.

@Craz1k0ek Craz1k0ek force-pushed the fixed/bmp-string-from-literal branch from f759527 to 7d8ae1f Compare November 28, 2025 20:13
@Lukasa
Copy link
Copy Markdown
Contributor

Lukasa commented Dec 1, 2025

Looks like this bit got missed:

diff --git a/Sources/SwiftASN1/Basic ASN1 Types/ASN1Strings.swift b/Sources/SwiftASN1/Basic ASN1 Types/ASN1Strings.swift
index b5073a6..d20b058 100644
--- a/Sources/SwiftASN1/Basic ASN1 Types/ASN1Strings.swift	
+++ b/Sources/SwiftASN1/Basic ASN1 Types/ASN1Strings.swift	
@@ -285,7 +285,7 @@ public struct ASN1BMPString: DERImplicitlyTaggable, BERImplicitlyTaggable, Hasha
     @inlinable
     public init(stringLiteral value: StringLiteralType) {
         guard
-            value.utf16.allSatisfy ({ codeUnit in
+            value.utf16.allSatisfy({ codeUnit in
                 !(0xD800...0xDFFF).contains(codeUnit)
             })
         else {

* Fixed apple#110 by using UTF-16 to encode the string literal
* Added a unit test to test the added functionality
@Craz1k0ek Craz1k0ek force-pushed the fixed/bmp-string-from-literal branch from 7d8ae1f to fcaeb73 Compare December 1, 2025 15:36
@Lukasa Lukasa merged commit 810496c into apple:main Dec 1, 2025
55 of 56 checks passed
@dnadoba
Copy link
Copy Markdown
Member

dnadoba commented Dec 1, 2025

FWIW you can now test these fatalErrors with exit tests: https://github.com/swiftlang/swift-evolution/blob/main/proposals/testing/0008-exit-tests.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🔨 semver/patch No public API change.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect ASN1BMPString encoding ExpressibleByStringLiteral

4 participants