Remove slightly dangerous optimization (#4317)

If there happens to be a file at the location passed into the `ManifestLoader, we were using that to compile the manifest, instead of the actual contents. This is dangerous, because the public API of `ManifestLoader` accepts a custom filesystem parameter that is not taken into account at all here, so the file being used might not be the right one, it just has the same path on the local filesystem.

We could have done some more work to preserve the optimization in the case where the used filesystem is the local one and the correct file is being used, but that doesn't seem worth it since the vast majority of loads are being done for remote dependencies (this we may load multiple times as we are resolving) which are never using the local filesystem.

Author: neonichu

Sources/Basics/CMakeLists.txt

@@ -32,6 +32,7 @@ add_library(Basics
   SwiftVersion.swift
   SQLiteBackedCache.swift
   Version+Extensions.swift
+  VFSOverlay.swift
   WritableByteStream+Extensions.swift)
 target_link_libraries(Basics PUBLIC
   SwiftCollections::OrderedCollections

Sources/Basics/VFSOverlay.swift

@@ -0,0 +1,53 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the Swift open source project
+//
+// Copyright (c) 2022 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See http://swift.org/LICENSE.txt for license information
+// See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+
+import class Foundation.JSONEncoder
+import struct TSCBasic.AbsolutePath
+import protocol TSCBasic.FileSystem
+
+public struct VFSOverlay: Encodable {
+    public struct File: Encodable {
+        enum CodingKeys: String, CodingKey {
+            case externalContents = "external-contents"
+            case name
+            case type
+        }
+
+        private let externalContents: String
+        private let name: String
+        private let type = "file"
+
+        public init(name: String, externalContents: String) {
+            self.name = name
+            self.externalContents = externalContents
+        }
+    }
+
+    enum CodingKeys: String, CodingKey {
+        case roots
+        case useExternalNames = "use-external-names"
+        case version
+    }
+
+    private let roots: [File]
+    private let useExternalNames = false
+    private let version = 0
+
+    public init(roots: [File]) {
+        self.roots = roots
+    }
+
+    public func write(to path: AbsolutePath, fileSystem: FileSystem) throws {
+        // VFS overlay files are YAML, but ours is simple enough that it works when being written using `JSONEncoder`.
+        try JSONEncoder.makeWithDefaults(prettified: false).encode(path: path, fileSystem: fileSystem, self)
+    }
+}

Sources/PackageLoading/ManifestLoader.swift

@@ -442,29 +442,26 @@ public final class ManifestLoader: ManifestLoaderProtocol {
         completion: @escaping (Result<EvaluationResult, Error>) -> Void
     ) throws {
         do {
-            if localFileSystem.isFile(manifestPath) {
+            try withTemporaryDirectory { tempDir, cleanupTempDir in
+                let manifestTempFilePath = tempDir.appending(component: "manifest.swift")
+                try localFileSystem.writeFileContents(manifestTempFilePath, bytes: ByteString(manifestContents))
+
+                let vfsOverlayTempFilePath = tempDir.appending(component: "vfs.yaml")
+                try VFSOverlay(roots: [
+                    VFSOverlay.File(name: manifestPath.pathString, externalContents: manifestTempFilePath.pathString)
+                ]).write(to: vfsOverlayTempFilePath, fileSystem: localFileSystem)
+
                 try self.evaluateManifest(
                     at: manifestPath,
+                    vfsOverlayPath: vfsOverlayTempFilePath,
                     packageIdentity: packageIdentity,
                     toolsVersion: toolsVersion,
-                    delegateQueue:  delegateQueue,
-                    callbackQueue: callbackQueue,
-                    completion: completion
-                )
-            } else {
-                try withTemporaryFile(suffix: ".swift") { tempFile, cleanupTempFile in
-                    try localFileSystem.writeFileContents(tempFile.path, bytes: ByteString(manifestContents))
-                    try self.evaluateManifest(
-                        at: tempFile.path,
-                        packageIdentity: packageIdentity,
-                        toolsVersion: toolsVersion,
-                        delegateQueue: delegateQueue,
-                        callbackQueue: callbackQueue
-                    ) { result in
-                        dispatchPrecondition(condition: .onQueue(callbackQueue))
-                        cleanupTempFile(tempFile)
-                        completion(result)
-                    }
+                    delegateQueue: delegateQueue,
+                    callbackQueue: callbackQueue
+                ) { result in
+                    dispatchPrecondition(condition: .onQueue(callbackQueue))
+                    cleanupTempDir(tempDir)
+                    completion(result)
                 }
             }
         } catch {
@@ -477,6 +474,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
     /// Helper method for evaluating the manifest.
     func evaluateManifest(
         at manifestPath: AbsolutePath,
+        vfsOverlayPath: AbsolutePath? = nil,
         packageIdentity: PackageIdentity,
         toolsVersion: ToolsVersion,
         delegateQueue: DispatchQueue,
@@ -512,6 +510,10 @@ public final class ManifestLoader: ManifestLoaderProtocol {
         var cmd: [String] = []
         cmd += [self.toolchain.swiftCompilerPathForManifests.pathString]
 
+        if let vfsOverlayPath = vfsOverlayPath {
+            cmd += ["-vfsoverlay", vfsOverlayPath.pathString]
+        }
+
         // if runtimePath is set to "PackageFrameworks" that means we could be developing SwiftPM in Xcode
         // which produces a framework for dynamic package products.
         if runtimePath.extension == "framework" {

Tests/PackageLoadingTests/PD_4_2_LoadingTests.swift

@@ -609,7 +609,7 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {
 
                 XCTAssertNoDiagnostics(observability.diagnostics)
                 XCTAssertEqual(try delegate.loaded(timeout: .now() + 1), [manifestPath])
-                XCTAssertEqual(try delegate.parsed(timeout: .now() + 1), expectCached ? [] : [manifestPath])
+                XCTAssertEqual(try delegate.parsed(timeout: .now() + 1).count, expectCached ? 0 : 1)
                 XCTAssertEqual(manifest.displayName, "Trivial")
                 XCTAssertEqual(manifest.targets[0].name, "foo")
             }
@@ -669,7 +669,7 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {
 
                 XCTAssertNoDiagnostics(observability.diagnostics)
                 XCTAssertEqual(try delegate.loaded(timeout: .now() + 1), [manifestPath])
-                XCTAssertEqual(try delegate.parsed(timeout: .now() + 1), expectCached ? [] : [manifestPath])
+                XCTAssertEqual(try delegate.parsed(timeout: .now() + 1).count, expectCached ? 0 : 1)
                 XCTAssertEqual(manifest.displayName, "Trivial")
                 XCTAssertEqual(manifest.targets[0].name, "foo")
             }

Tests/PackageLoadingTests/PD_5_6_LoadingTests.swift

@@ -221,19 +221,19 @@ class PackageDescription5_6LoadingTests: PackageDescriptionLoadingTests {
             
             let fileManager = FileManager.default
             let contents = (try? fileManager.contentsOfDirectory(atPath: Context.packageDirectory)) ?? []
-            let swiftFiles = contents.filter { $0.hasPrefix("TemporaryFile") && $0.hasSuffix(".swift") }
             
-            let package = Package(name: swiftFiles.joined(separator: ","))
+            let package = Package(name: contents.joined(separator: ","))
             """
 
         let observability = ObservabilitySystem.makeForTesting()
         let (manifest, validationDiagnostics) = try loadAndValidateManifest(content, observabilityScope: observability.topScope)
         XCTAssertNoDiagnostics(observability.diagnostics)
         XCTAssertNoDiagnostics(validationDiagnostics)
 
-        let name = parsedManifest.components?.last ?? ""
-        let swiftFiles = manifest.displayName.split(separator: ",").map(String.init)
-        XCTAssertNotNil(swiftFiles.firstIndex(of: name))
+        let files = manifest.displayName.split(separator: ",").map(String.init)
+        // Since we're loading `/Package.swift` in these tests, the context's package directory is supposed to be /.
+        let expectedFiles = try FileManager.default.contentsOfDirectory(atPath: "/")
+        XCTAssertEqual(files, expectedFiles)
     }
 
     func testCommandPluginTarget() throws {