Fix availability logic refactor #1065

sofiaromorales · 2024-10-18T13:50:39Z

Bug/issue #, if applicable: 139722227

Summary

DocC current logic for determining API availability involves adding the symbol graphs operating system platform name to the final symbol availability.

This approach is problematic because the platform name may not always be in a human-readable format. While it's necessary for calculating availability, we don't want it included as an explicit availability item.

This is a major refactor of the availability logic to not include the symbolgraph operating system platform name as an availability item.

Dependencies

Describe any dependencies this PR might have, such as an associated branch in another repository.

Testing

Review unit tests added.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

Added tests
Ran the ./bin/test script and it succeeded
Updated documentation if necessary

Sources/SwiftDocCTestUtilities/FilesAndFolders.swift

Sources/SwiftDocC/Semantics/Symbol/Symbol.swift

Sources/SwiftDocC/Infrastructure/Symbol Graph/SymbolGraphLoader.swift

Sources/SwiftDocC/Semantics/Symbol/Symbol.swift

Sources/SwiftDocC/Infrastructure/Symbol Graph/SymbolGraphLoader.swift

d-ronnqvist

I think this is a great step towards making this complex logic both more correct and easier to read.

This refactor was initiated by the need of stop using the module operating system name as an availability item. This change required a big refactoring of the logic, and now the symbol graph availability loading logic is like the following: 1. Every symbol graph is loaded and the symbols get only the availability information that's marked in the SDK. 2. Once all the SGFs are oaded, whe iterate over the symbols and we add the fallbkac and default availability if applies.

Instead of making a `[(FallbackPlatform, InheritedPlaform)]. The new struture to hold this information is in the way of `[InheritedPlatfor:[FallbackPlatforms]]`.

d-ronnqvist · 2024-11-27T13:28:16Z

Tests/SwiftDocCTests/Infrastructure/SymbolGraph/SymbolGraphLoaderTests.swift

@@ -1652,6 +1644,122 @@ class SymbolGraphLoaderTests: XCTestCase {
        XCTAssertEqual(availability.first(where: { $0.domain?.rawValue == "macCatalyst" })?.introducedVersion, SymbolGraph.SemanticVersion(major: 1, minor: 0, patch: 0))
    }

+    func testNotAvailableSymbol() throws {
+        // Symbol from SG
+        let symbolGraphStringiOS = makeSymbolGraphString(


nit: This variable says iOS but the data says macOS

d-ronnqvist · 2024-11-27T13:28:31Z

Tests/SwiftDocCTests/Infrastructure/SymbolGraph/SymbolGraphLoaderTests.swift

+                 }
+            """
+        )
+        let symbolGraphStringMacOS = makeSymbolGraphString(


nit: and this does the opposite

d-ronnqvist · 2024-11-27T13:29:31Z

Tests/SwiftDocCTests/Infrastructure/SymbolGraph/SymbolGraphLoaderTests.swift

+        try symbolGraphStringMacOS.write(to: targetURL.appendingPathComponent("MyModule-macos.symbols.json"), atomically: true, encoding: .utf8)
+        try infoPlist.write(to: targetURL.appendingPathComponent("Info.plist"), atomically: true, encoding: .utf8)
+        // Load the bundle & reference resolve symbol graph docs
+        let (_, _, context) = try loadBundle(from: targetURL)


minor: Would it be possible to use the various File test helpers for this catalog instead of strings?

d-ronnqvist · 2024-11-27T13:32:21Z

Sources/SwiftDocC/Infrastructure/Symbol Graph/SymbolGraphLoader.swift

+                    guard symbolsByPlatformName[defaultAvailability.platformName.rawValue]?.contains(where: {
+                        symbolID == $0.precise
+                    }) != false else {


minor: AFAICT this is the only place that uses symbolsByPlatformName. If that's correct, maybe it would be worth using a set of symbol IDs so that this can use contains(_:) instead of contains(where:).

In other words, changing symbolsByPlatformName from [String: [SymbolGraph.Symbol.Identifier]] to [String: Set]

d-ronnqvist · 2024-11-27T13:33:29Z

Sources/SwiftDocC/Infrastructure/Workspace/DefaultAvailability.swift

-                        platformVersion: fallbackIntroducedVersion
-                    )
+            platformAvailabilities + DefaultAvailability.fallbackPlatforms.flatMap { (fallbackPlatform, platform) in
+                platform.compactMap { pla in


nit: This looks like an incomplete word.

Suggested change

platform.compactMap { pla in

platform.compactMap { platformName in

d-ronnqvist

These look like good improvements to me.

I left a few very minor suggestions but nothing blocking.

d-ronnqvist reviewed Oct 28, 2024

View reviewed changes