Enable String, URL and XML-related types for WASI #3051

MaxDesiatov · 2021-08-17T14:38:22Z

APIs related to file access (i.e. those depending on NSCoding) and multi-threading (those that require NSRunLoop) had to be disabled for WASI/Wasm to make this work.

MaxDesiatov · 2021-08-17T14:38:30Z

@swift-ci please test

millenomi · 2021-08-20T18:50:40Z

Quick q: NSCoding only has a handful of places where it does file access; what's up with removing all that?

MaxDesiatov · 2021-08-22T11:18:23Z

AFAIR NSCoding was tightly coupled with FS access, so it was easier to disable it. We also weren't expecting people to use APIs from Objective-C era on Wasm anyway. I can try and partially re-enable NSCoding w/o FS access if you think that makes more sense.

MaxDesiatov · 2021-08-25T11:06:34Z

@swift-ci please test

MaxDesiatov · 2021-08-25T11:06:57Z

@millenomi I've excluded changes related to NSCoding from this PR, ready for review

millenomi · 2021-08-25T17:46:31Z

Sources/Foundation/NSURL.swift

@@ -528,13 +537,16 @@ open class NSURL : NSObject, NSSecureCoding, NSCopying {
            components.path = _pathByRemovingDots(pathComponents!)
        }

+#if !os(WASI)
        if let filePath = components.path, isFileURL {
            return URL(fileURLWithPath: filePath, isDirectory: hasDirectoryPath, relativeTo: baseURL)


I don't see why we need to disable (fileURL… entirely rather than disable the paths in (fileURL… that do actual I/O?

In general the whole point of the library is to have a solid amount of compatible API surface; while obviously WASI cannot I/O I don't see why code cannot create file:-scheme URLs at all.

millenomi · 2021-08-25T17:47:45Z

Sources/Foundation/NSURL.swift

@@ -573,6 +586,7 @@ open class NSURL : NSObject, NSSecureCoding, NSCopying {
        return CFURLGetTypeID()
    }

+#if !os(WASI)
    open func removeAllCachedResourceValues() {


In general, let's remove the implementation and not the API surface wherever possible.

millenomi · 2021-08-25T17:48:43Z

Sources/Foundation/NSString.swift

@@ -18,6 +18,7 @@ extension unichar {
    }
 }

+#if !os(WASI)


This is logically part of Bundle's API surface. If Bundle is available, this should do fallback, not be removed. Is Bundle available in your code?

I only have a very limited subset of Bundle available in the fork. It's basically a shim that adds very basic Bundle.main and Bundle.path(forResource:ofType:) implementations, everything else from Bundle is disabled. I excluded that from this PR as something that goes beyond a simple #if os(WASI) check.

Let me know if that should be included, or if I should re-enable APIs relying on Bundle here and tackle the whole Bundle thing in a separate PR.

In case that would help, the shim looks like this:

open class Bundle { public let bundlePath: String public static let main = Bundle(path: "")! public init?(path: String) { bundlePath = path } // ----------------------------------------------------------------------------------- // MARK: - Path Resource Lookup - Instance open func path(forResource name: String?, ofType ext: String?) -> String? { path(forResource: name, ofType: ext, inDirectory: nil) } open func path(forResource name: String?, ofType ext: String?, inDirectory subpath: String?) -> String? { guard let name = name else { return nil } let subpathOrEmpty: String if let subpath = subpath { subpathOrEmpty = "\(subpath)/" } else { subpathOrEmpty = "" } if let ext = ext { return "\(bundlePath)/\(subpathOrEmpty)\(name).\(ext)" } else { return "\(bundlePath)/\(subpathOrEmpty)\(name)" } } }

Wait; if there's no I/O access, how is path(forResource… useful at all?

It allows us to utilize Swift package resources https://github.com/apple/swift-evolution/blob/main/proposals/0271-package-manager-resources.md

SwiftPM emits relative paths to package resources that WASI apps in the browser can load, like images, fonts, CSS etc.

millenomi · 2021-08-25T17:49:24Z

Sources/Foundation/NSString.swift

@@ -1365,6 +1369,7 @@ extension NSString {
        }
    }

+#if !os(WASI)


re API surface removal avoidance: this should probably throw at a lower level rather than be removed.

millenomi · 2021-08-25T17:52:29Z

Sources/Foundation/NSTextCheckingResult.swift

@@ -116,6 +116,7 @@ internal class NSRegularExpressionCheckingResult: NSTextCheckingResult {
        super.init()
    }

+#if !os(WASI)


I've audited NSKeyedArchiver, and it looks like we can salvage basically all the implementation except for the few parts that dealing from reading bytes from a stream; it is not at all bound tightly to file I/O. If NSMutableData exists, I see little reason to cut it right this second.

And its unarchiving counterpart.

millenomi · 2021-08-25T17:53:26Z

Sources/Foundation/URL.swift

@@ -534,6 +535,11 @@ public struct URL : ReferenceConvertible, Equatable {
    public init(fileURLWithPath path: String) {
        _url = URL._converted(from: NSURL(fileURLWithPath: path.isEmpty ? "." : path))
    }
+#else
+    public init(fileURLWithPath path: String) {
+        _url = NSURL(string: "file://\(path)")!


If there's a space or other non-URL-encodable character, this will create an invalid URL. Nope, sorry.

Also, this should at least check that path begins with a slash. Otherwise passing in "foo" results in the URL file://foo (i.e. with "foo" in the hostname position and no path)

millenomi · 2021-08-25T17:54:46Z

Sources/FoundationXML/XMLDocument.swift

@@ -85,6 +85,7 @@ open class XMLDocument : XMLNode {
        try self.init(data: data, options: mask)
    }

+#if !os(WASI)


re API surface removal avoidance: this should probably throw at a lower level rather than be removed.

millenomi · 2021-08-25T17:55:06Z

Sources/FoundationXML/XMLParser.swift

@@ -48,13 +48,15 @@ private func UTF8STRING(_ bytes: UnsafePointer<UInt8>?) -> String? {
    return nil
 }

+#if !os(WASI)
 internal func _NSXMLParserCurrentParser() -> _CFXMLInterface? {


I assume CFXML is not available at all on WASI?

If so — if XML parsing does not exist at all on WASI — I'd rather not compile FoundationXML at all for WASI rather than burden it with os(WASI) checks.

If a type exists, I want it to offer as much of its API surface as it possibly can, and use runtime signaling for inabilities of its host. This allows e.g. packages to be ported without having to work through source compatibility errors. But for secondary-use API like XML, if it isn't supported at all, it's okay to just drop the entire surface.

XML parsing does exist on WASI, we were able to compile libxml without issues. Thanks for the feedback, I'll make the API surface available.

millenomi · 2021-08-25T17:58:18Z

Sources/FoundationXML/XMLParser.swift

@@ -63,11 +65,13 @@ internal func _NSXMLParserExternalEntityWithURL(_ interface: _CFXMLInterface, ur
    if let allowedEntityURLs = parser.allowedExternalEntityURLs {
        if let url = URL(string: String(describing: urlStr)) {
            a = url
+            #if !os(WASI)


If we restore {NS,}URL(fileURL…, all these fallout consequences should be removed. (This one is in XML, which is dependent on whether the module is compiled for WASI, but this applies in general.)

millenomi · 2021-08-25T17:58:53Z

Sources/FoundationXML/XMLParser.swift

@@ -993,8 +1031,10 @@ extension NSObject {
 func setupXMLParsing() {
    _CFSetupXMLInterface()
    _CFSetupXMLBridgeIfNeededUsingBlock {
+#if !os(WASI)
        __CFSwiftXMLParserBridge.CFBridge = CF.originalBridge


How does CFXML work at all without access to CF functions?

I've re-enabled this in the updated commit

karwa · 2021-08-30T01:15:19Z

Sources/FoundationXML/XMLParser.swift

+            let chunk = data.withUnsafeMutableBytes { (rawBuffer: UnsafeMutableRawBufferPointer) -> Data in
+                let ptr = rawBuffer.baseAddress!.advanced(by: range.location)
+                return Data(bytesNoCopy: ptr, count: range.length, deallocator: .none)


This escapes a scoped inner-pointer. Since Data is self-slicing, can't you just use a regular slice?

karwa · 2021-08-30T01:17:58Z

Sources/FoundationXML/XMLParser.swift

+    internal func parse(from data: Data) -> Bool {
+        var data = data
+        var result = true
+        let buffer = malloc(_chunkSize)!.bindMemory(to: UInt8.self, capacity: _chunkSize)


Where is this used?

It's used in open func parse() below.

I mean the buffer allocated on this line. It doesn’t seem to be used AFAICT.

MaxDesiatov · 2021-09-04T18:48:07Z

@swift-ci please test

MaxDesiatov · 2021-09-04T18:50:37Z

@millenomi I've cleaned it up, re-enabled as much as possible based on your feedback. There's still API surface relying on InputStream, I kept that unavailable. Do you think that's fine, or is keeping InputStream available on WASI a requirement to get this merged?

MaxDesiatov · 2021-09-06T13:40:05Z

@swift-ci please test

millenomi · 2021-09-06T22:09:36Z

I feel the usage on Stream subclasses is low enough that I'm less worried.

xwu

Just some nits :)

xwu · 2021-09-07T13:53:27Z

Sources/Foundation/NSTextCheckingResult.swift

@@ -53,7 +53,7 @@ open class NSTextCheckingResult: NSObject, NSCopying, NSSecureCoding {
    open class var supportsSecureCoding: Bool {
        NSRequiresConcreteImplementation()
    }
-    
+


Is it possible to revert the unrelated whitespace changes?

xwu · 2021-09-07T13:54:00Z

Sources/Foundation/NSURL.swift

@@ -841,6 +850,9 @@ extension NSURL {

    open func appendingPathComponent(_ pathComponent: String) -> URL? {
        var result : URL? = appendingPathComponent(pathComponent, isDirectory: false)
+
+        // File URLs can't be handled on WASI without file system access
+        #if !os(WASI)


Suggested change

#if !os(WASI)

#if !os(WASI)

xwu · 2021-09-07T13:54:20Z

Sources/Foundation/NSURL.swift

@@ -852,6 +864,7 @@ extension NSURL {
            }

        }
+        #endif


Suggested change

#endif

#endif

xwu · 2021-09-07T13:55:23Z

Sources/Foundation/PropertyListSerialization.swift

@@ -74,6 +74,7 @@ open class PropertyListSerialization : NSObject {
        }
    }

+    #if !os(WASI)


Similarly outdent here for consistency?

xwu · 2021-09-07T13:57:01Z

Sources/FoundationXML/XMLParser.swift

    // called to start the event-driven parse. Returns YES in the event of a successful parse, and NO in case of error.
    open func parse() -> Bool {
+        #if os(WASI)


Same comment here.

fix

MaxDesiatov · 2021-09-07T14:29:19Z

@swift-ci please test

millenomi · 2021-09-10T18:04:47Z

@MaxDesiatov I'm looking today.

MaxDesiatov · 2021-09-10T18:05:48Z

Thanks! I think I've addressed all of the current feedback, happy to update it again or answer any questions if anything else comes up.

millenomi · 2021-09-10T18:06:24Z

LGTM.

MaxDesiatov requested a review from millenomi August 17, 2021 14:38

MaxDesiatov mentioned this pull request Aug 17, 2021

Upstream all the changes 🙌 swiftwasm/swift#187

Closed

78 tasks

MaxDesiatov force-pushed the maxd/wasi-foundation2 branch 2 times, most recently from e9b0b82 to 553f373 Compare August 25, 2021 11:06

millenomi reviewed Aug 25, 2021

View reviewed changes

millenomi suggested changes Aug 25, 2021

View reviewed changes

millenomi reviewed Aug 25, 2021

View reviewed changes

karwa reviewed Aug 30, 2021

View reviewed changes

MaxDesiatov force-pushed the maxd/wasi-foundation2 branch 8 times, most recently from 47d8ff4 to f95d19a Compare September 4, 2021 18:47

MaxDesiatov force-pushed the maxd/wasi-foundation2 branch from 54fa78b to cb3e8c9 Compare September 6, 2021 13:39

MaxDesiatov requested a review from millenomi September 6, 2021 22:10

xwu reviewed Sep 7, 2021

View reviewed changes

MaxDesiatov force-pushed the maxd/wasi-foundation2 branch 5 times, most recently from 21cee4a to 5809266 Compare September 7, 2021 14:27

Enable String, URL and XML-related types for WASI

7688233

fix

MaxDesiatov force-pushed the maxd/wasi-foundation2 branch from 5809266 to 7688233 Compare September 7, 2021 14:28

millenomi merged commit 8834372 into main Sep 10, 2021

kateinoigakukun mentioned this pull request Jan 5, 2023

Ensure last chunk of XML data is parsed swiftwasm/swift-corelibs-foundation#547

Closed

@@ @@ -852,6 +864,7 @@ extension NSURL { @@
                           }
                       }
+                      #endif

Enable String, URL and XML-related types for WASI #3051

Enable String, URL and XML-related types for WASI #3051

Conversation

MaxDesiatov commented Aug 17, 2021

MaxDesiatov commented Aug 17, 2021

millenomi commented Aug 20, 2021 • edited Loading

MaxDesiatov commented Aug 22, 2021

MaxDesiatov commented Aug 25, 2021

MaxDesiatov commented Aug 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

millenomi Aug 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxDesiatov commented Sep 4, 2021

MaxDesiatov commented Sep 4, 2021

MaxDesiatov commented Sep 6, 2021

millenomi commented Sep 6, 2021

xwu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxDesiatov commented Sep 7, 2021

millenomi commented Sep 10, 2021

MaxDesiatov commented Sep 10, 2021 • edited Loading

millenomi commented Sep 10, 2021

millenomi commented Aug 20, 2021 •

edited

Loading

millenomi Aug 25, 2021 •

edited

Loading

MaxDesiatov commented Sep 10, 2021 •

edited

Loading