[PackageLoading] diagnose empty manifest separately from missing tools version specification (#5893)

This change makes it clearer to the user what problem SwiftPM thinks their manifest has, avoiding some confusions in [cases like this](https://forums.swift.org/t/missing-swift-tools-version-specification-build-fail/61051).

In the future, for errors not specific to the tools version specification, we should move their diagnostics to before calling `ToolsVersionParser.parse`. This probably requires some restructuring of manifest parsing in general.

Some additional changes:

- Removed an incorrect FIXME, and added a TODO for testing `ManifestParseError.runtimeManifestErrors`.

- Decapitalised the description for the other `ManifestParseError` cases, in keeping with [Swift’s diagnostics style](https://github.com/apple/swift/blob/3a939255c8fc1f2825560cca3a5fa6b167c414c3/docs/Diagnostics.md).

Author: WowbaggersLiquidLunch

Sources/PackageLoading/ManifestLoader.swift

@@ -17,9 +17,11 @@ import TSCBasic
 import enum TSCUtility.Diagnostics
 
 public enum ManifestParseError: Swift.Error, Equatable {
+    /// The manifest is empty, or at least from SwiftPM's perspective it is.
+    case emptyManifest(path: AbsolutePath)
     /// The manifest contains invalid format.
     case invalidManifestFormat(String, diagnosticFile: AbsolutePath?)
-
+    // TODO: Test this error.
     /// The manifest was successfully loaded by swift interpreter but there were runtime issues.
     case runtimeManifestErrors([String])
 }
@@ -28,10 +30,12 @@ public enum ManifestParseError: Swift.Error, Equatable {
 extension ManifestParseError: CustomStringConvertible {
     public var description: String {
         switch self {
+        case .emptyManifest(let manifestPath):
+            return "'\(manifestPath)' is empty"
         case .invalidManifestFormat(let error, _):
-            return "Invalid manifest\n\(error)"
+            return "invalid manifest\n\(error)"
         case .runtimeManifestErrors(let errors):
-            return "Invalid manifest (evaluation failed)\n\(errors.joined(separator: "\n"))"
+            return "invalid manifest (evaluation failed)\n\(errors.joined(separator: "\n"))"
         }
     }
 }

Sources/PackageLoading/ToolsVersionParser.swift

@@ -21,7 +21,17 @@ public struct ToolsVersionParser {
     private init() {}
 
     public static func parse(manifestPath: AbsolutePath, fileSystem: FileSystem) throws -> ToolsVersion {
-        // FIXME: We don't need the entire file, just the first line.
+        // FIXME: We should diagnose errors not specific to the tools version specification outside of this function.
+        // In order to that, maybe we can restructure the parsing to somthing like this:
+        //     parse(_ manifestContent: String) throws -> Manifest {
+        //         ...
+        //         guard !manifestContent.isEmpty else { throw appropariteError }
+        //         let (toolsVersion, remainingContent) = parseAndConsumeToolsVersionSpecification(manifestConent)
+        //         let packageDetails = parsePackageDetails(remainingContent)
+        //         ...
+        //         return Manifest(toolsVersion, ...)
+        //     }
+
         let manifestContents: ByteString
         do {
             manifestContents = try fileSystem.readFileContents(manifestPath)
@@ -37,6 +47,10 @@ public struct ToolsVersionParser {
             throw Error.nonUTF8EncodedManifest(path: manifestPath)
         }
 
+        guard !manifestContentsDecodedWithUTF8.isEmpty else {
+            throw ManifestParseError.emptyManifest(path: manifestPath)
+        }
+
         do {
           return try self.parse(utf8String: manifestContentsDecodedWithUTF8)
         } catch Error.malformedToolsVersionSpecification(.commentMarker(.isMissing)) {
@@ -45,6 +59,7 @@ public struct ToolsVersionParser {
     }
 
     public static func parse(utf8String: String) throws -> ToolsVersion {
+        assert(!utf8String.isEmpty, "empty manifest should've been diagnosed before parsing the tools version specification")
         /// The manifest represented in its constituent parts.
         let manifestComponents = Self.split(utf8String)
         /// The Swift tools version specification represented in its constituent parts.
@@ -427,7 +442,7 @@ extension ToolsVersionParser {
         public enum ToolsVersionSpecificationMalformationLocation {
             /// The nature of malformation at the location in Swift tools version specification.
             public enum MalformationDetails {
-                /// The Swift tools version specification component is missing.
+                /// The Swift tools version specification component is missing in the non-empty manifest.
                 case isMissing
                 /// The Swift tools version specification component is misspelt.
                 case isMisspelt(_ misspelling: String)

Tests/PackageLoadingTests/ToolsVersionParserTests.swift

@@ -24,8 +24,9 @@ class ToolsVersionParserTests: XCTestCase {
         body?(toolsVersion)
     }
 
+    /// Verifies correct parsing for valid version specifications, and that the parser isn't confused by contents following the version specification.
     func testValidVersions() throws {
-        let validVersions = [
+        let manifestsSnippetWithValidVersionSpecification = [
             // No spacing surrounding the label for Swift ≥ 5.4:
             "//swift-tools-version:5.4.0"              : (5, 4, 0, "5.4.0"),
             "//swift-tools-version:5.4-dev"            : (5, 4, 0, "5.4.0"),
@@ -105,8 +106,8 @@ class ToolsVersionParserTests: XCTestCase {
             "\u{D}\u{A}\t\u{85}\u{85}\u{A}\u{2028}\u{2029}//\u{2001}\u{2002}\u{202F}sWiFt-tOoLs-vErSiOn:\u{1680}\t\u{2000}\u{200A} \u{2002}\u{202F}5.5.2\nkkk\n"   : (5, 5, 2, "5.5.2"),
         ]
 
-        for (version, result) in validVersions {
-            try self.parse(version) { toolsVersion in
+        for (snippet, result) in manifestsSnippetWithValidVersionSpecification {
+            try self.parse(snippet) { toolsVersion in
                 XCTAssertEqual(toolsVersion.major, result.0)
                 XCTAssertEqual(toolsVersion.minor, result.1)
                 XCTAssertEqual(toolsVersion.patch, result.2)
@@ -135,11 +136,37 @@ class ToolsVersionParserTests: XCTestCase {
         }
     }
 
-    /// Verifies that the correct error is thrown for each manifest missing its Swift tools version specification.
+    /// Verifies that if a manifest appears empty to SwiftPM, a distinct error is thrown.
+    func testEmptyManifest() throws {
+        let fs = InMemoryFileSystem()
+
+		let packageRoot = AbsolutePath(path: "/lorem/ipsum/dolor")
+		try fs.createDirectory(packageRoot, recursive: true)
+
+		let manifestPath = packageRoot.appending(component: "Package.swift")
+        try fs.writeFileContents(manifestPath, bytes: "")
+
+        XCTAssertThrowsError(
+            try ToolsVersionParser.parse(manifestPath: manifestPath, fileSystem: fs),
+            "empty manifest '/lorem/ipsum/dolor/Package.swift'") { error in
+                guard let error = error as? ManifestParseError, case .emptyManifest(let errorPath) = error else {
+                    XCTFail("'ManifestParseError.emptyManifest' should've been thrown, but a different error is thrown")
+                    return
+                }
+
+                guard errorPath == manifestPath else {
+                    XCTFail("error is in '\(manifestPath)', but '\(errorPath)' is given for the error message")
+                    return
+                }
+
+                XCTAssertEqual(error.description, "'/lorem/ipsum/dolor/Package.swift' is empty")
+            }
+    }
+
+    /// Verifies that the correct error is thrown for each non-empty manifest missing its Swift tools version specification.
     func testMissingSpecifications() throws {
         /// Leading snippets of manifest files that don't have Swift tools version specifications.
         let manifestSnippetsWithoutSpecification = [
-            "",
             "\n",
             "\n\r\r\n",
             "ni",
@@ -185,7 +212,6 @@ class ToolsVersionParserTests: XCTestCase {
             "\n\n\nswift-tools-version:3.1\r",
             "\r\n\r\ncontrafibularity",
             "\n\r\t3.14",
-            "",
         ]
 
         for manifestSnippet in manifestSnippetsWithoutSpecificationCommentMarker {

Tests/WorkspaceTests/WorkspaceTests.swift

@@ -215,7 +215,7 @@ final class WorkspaceTests: XCTestCase {
             XCTAssert(rootManifests.count == 0, "\(rootManifests)")
 
             testDiagnostics(observability.diagnostics) { result in
-                let diagnostic = result.check(diagnostic: .prefix("Invalid manifest\n\(path.appending(components: "MyPkg", "Package.swift")):3:8: error: An error in MyPkg"), severity: .error)
+                let diagnostic = result.check(diagnostic: .prefix("invalid manifest\n\(path.appending(components: "MyPkg", "Package.swift")):3:8: error: An error in MyPkg"), severity: .error)
                 XCTAssertEqual(diagnostic?.metadata?.packageIdentity, .init(path: pkgDir))
                 XCTAssertEqual(diagnostic?.metadata?.packageKind, .root(pkgDir))
             }