Improve the diagnostics that are emitted when a package dependency specifies a non-existent branch or commit

This also sets the code up to make it easier to do other similar heuristics as needed, and as a bonus, adds one that is related to the `master` -> `main` renaming (suggesting `main` if `master` is requested and doesn't exist, but `main` does).  There isn't yet any support for fix-its in SwiftPM, but this would be a case for such a facility once it is supported.

Note that tags go through different processing, since they are the inputs to the semantic versioning and missing tags are reported differently.  There is no way in SwiftPM to define an explicit dependency on a non-semver tag, which is why this code only needs to handle branches and commits.

The error message does not list all the branches, since there can be many, but that could be another future improvement (or at least to list all the branches whose names are similar to what was requested).

rdar://34099187

Author: abertelrud

Sources/PackageGraph/RepositoryPackageContainerProvider.swift

@@ -1,7 +1,7 @@
 /*
  This source file is part of the Swift.org open source project
 
- Copyright (c) 2014 - 2017 Apple Inc. and the Swift project authors
+ Copyright (c) 2014 - 2020 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
@@ -251,16 +251,28 @@ public class RepositoryPackageContainer: BasePackageContainer, CustomStringConve
 
     // A wrapper for getDependencies() errors. This adds additional information
     // about the container to identify it for diagnostics.
-    public struct GetDependenciesErrorWrapper: Swift.Error {
+    public struct GetDependenciesError: Error, CustomStringConvertible {
 
-        /// The container which had this error.
+        /// The container (repository) that encountered the error.
         public let containerIdentifier: String
 
-        /// The source control reference i.e. version, branch, revsion etc.
+        /// The source control reference (version, branch, revision, etc) that was involved.
         public let reference: String
 
         /// The actual error that occurred.
-        public let underlyingError: Swift.Error
+        public let underlyingError: Error
+        
+        /// Optional suggestion for how to resolve the error.
+        public let suggestion: String?
+        
+        /// Description shown for errors of this kind.
+        public var description: String {
+            var desc = "\(underlyingError) in \(containerIdentifier)"
+            if let suggestion = suggestion {
+                desc += " (\(suggestion))"
+            }
+            return desc
+        }
     }
 
     /// This is used to remember if tools version of a particular version is
@@ -370,8 +382,8 @@ public class RepositoryPackageContainer: BasePackageContainer, CustomStringConve
                 return try getDependencies(at: revision, version: version, productFilter: productFilter)
             }.1
         } catch {
-            throw GetDependenciesErrorWrapper(
-                containerIdentifier: identifier.repository.url, reference: version.description, underlyingError: error)
+            throw GetDependenciesError(
+                containerIdentifier: identifier.repository.url, reference: version.description, underlyingError: error, suggestion: nil)
         }
     }
 
@@ -383,8 +395,28 @@ public class RepositoryPackageContainer: BasePackageContainer, CustomStringConve
                 return try getDependencies(at: revision, productFilter: productFilter)
             }.1
         } catch {
-            throw GetDependenciesErrorWrapper(
-                containerIdentifier: identifier.repository.url, reference: revision, underlyingError: error)
+            // Examine the error to see if we can come up with a more informative and actionable error message.  We know that the revision is expected to be a branch name or a hash (tags are handled through a different code path).
+            if let gitInvocationError = error as? ProcessResult.Error, gitInvocationError.description.contains("Needed a single revision") {
+                // It was a Git process invocation error.  Take a look at the repository to see if we can come up with a reasonable diagnostic.
+                if let rev = try? repository.resolveRevision(identifier: revision), repository.exists(revision: rev) {
+                    // Revision does exist, so something else must be wrong.
+                    throw GetDependenciesError(
+                        containerIdentifier: identifier.repository.url, reference: revision, underlyingError: error, suggestion: nil)
+                }
+                else {
+                    // Revision does not exist, so we customize the error.
+                    let sha1RegEx = try! RegEx(pattern: #"\A[:xdigit:]{40}\Z"#)
+                    let isBranchRev = sha1RegEx.matchGroups(in: revision).compactMap{ $0 }.isEmpty
+                    let errorMessage = "could not find " + (isBranchRev ? "a branch named ‘\(revision)’" : "the commit \(revision)")
+                    let mainBranchExists = (try? repository.resolveRevision(identifier: "main")) != nil
+                    let suggestion = (revision == "master" && mainBranchExists) ? "did you mean ‘main’?" : nil
+                    throw GetDependenciesError(
+                        containerIdentifier: identifier.repository.url, reference: revision,
+                        underlyingError: StringError(errorMessage), suggestion: suggestion)
+                }
+            }
+            // If we get this far without having thrown an error, we wrap and throw the underlying error.
+            throw GetDependenciesError(containerIdentifier: identifier.repository.url, reference: revision, underlyingError: error, suggestion: nil)
         }
     }
 

Sources/Workspace/Workspace.swift

@@ -2064,16 +2064,7 @@ extension Workspace {
             return bindings
 
         case .error(let error):
-            switch error {
-            // Emit proper error if we were not able to parse some manifest during dependency resolution.
-            case let error as RepositoryPackageContainer.GetDependenciesErrorWrapper:
-                let location = PackageLocation.Remote(url: error.containerIdentifier, reference: error.reference)
-                diagnostics.emit(error.underlyingError, location: location)
-
-            default:
-                diagnostics.emit(error)
-            }
-
+            diagnostics.emit(error)
             return []
         }
     }

Tests/PackageGraphTests/RepositoryPackageContainerProviderTests.swift

@@ -1,7 +1,7 @@
 /*
  This source file is part of the Swift.org open source project
 
- Copyright (c) 2014 - 2017 Apple Inc. and the Swift project authors
+ Copyright (c) 2014 - 2020 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
@@ -11,6 +11,7 @@
 import XCTest
 
 import TSCBasic
+import TSCUtility
 import PackageLoading
 import PackageModel
 import PackageGraph
@@ -274,7 +275,7 @@ class RepositoryPackageContainerProviderTests: XCTestCase {
             let revision = try container.getRevision(forTag: "1.0.0")
             do {
                 _ = try container.getDependencies(at: revision.identifier, productFilter: .specific([]))
-            } catch let error as RepositoryPackageContainer.GetDependenciesErrorWrapper {
+            } catch let error as RepositoryPackageContainer.GetDependenciesError {
                 let error = error.underlyingError as! UnsupportedToolsVersion
                 XCTAssertMatch(error.description, .and(.prefix("package at '/' @"), .suffix("is using Swift tools version 3.1.0 which is no longer supported; consider using '// swift-tools-version:4.0' to specify the current tools version")))
             }
@@ -495,4 +496,62 @@ class RepositoryPackageContainerProviderTests: XCTestCase {
             )
         }
     }
+    
+    
+    func testMissingBranchDiagnostics() {
+        mktmpdir { tmpDir in
+            // Create a repository.
+            let packageDir = tmpDir.appending(component: "SomePackage")
+            try localFileSystem.createDirectory(packageDir)
+            initGitRepo(packageDir)
+            let packageRepo = GitRepository(path: packageDir)
+            
+            // Create a package manifest in it (it only needs the `swift-tools-version` part, because we'll supply the manifest later).
+            let manifestFile = packageDir.appending(component: "Package.swift")
+            try localFileSystem.writeFileContents(manifestFile, bytes: ByteString("// swift-tools-version:4.2"))
+            
+            // Commit it and tag it.
+            try packageRepo.stage(file: "Package.swift")
+            try packageRepo.commit(message: "Initial")
+            try packageRepo.tag(name: "1.0.0")
+            
+            // Rename the `master` branch to `main`.
+            try systemQuietly([Git.tool, "-C", packageDir.pathString, "branch", "-m", "main"])
+
+            // Create a repository manager for it.
+            let repoProvider = GitRepositoryProvider()
+            let repositoryManager = RepositoryManager(path: packageDir, provider: repoProvider, delegate: nil, fileSystem: localFileSystem)
+            
+            // Create a container provider, configured with a mock manifest loader that will return the package manifest.
+            let manifest = Manifest.createV4Manifest(
+                name: packageDir.basename,
+                path: packageDir.pathString,
+                url: packageDir.pathString,
+                packageKind: .root,
+                targets: [
+                    TargetDescription(name: packageDir.basename, path: packageDir.pathString)
+                ]
+            )
+            let containerProvider = RepositoryPackageContainerProvider(repositoryManager: repositoryManager, manifestLoader: MockManifestLoader(manifests: [.init(url: packageDir.pathString, version: nil) : manifest]))
+            
+            // Get a hold of the container for the test package.
+            let packageRef = PackageReference(identity: "somepackage", path: packageDir.pathString)
+            let container = try await { containerProvider.getContainer(for: packageRef, completion: $0) } as! RepositoryPackageContainer
+            
+            // Simulate accessing a fictitious dependency on the `master` branch, and check that we get back the expected error.
+            do { _ = try container.getDependencies(at: "master", productFilter: .everything) }
+            catch let error as RepositoryPackageContainer.GetDependenciesError {
+                // We expect to get an error message that mentions main.
+                XCTAssertMatch(error.description, .and(.prefix("could not find a branch named ‘master’"), .suffix("(did you mean ‘main’?)")))
+            }
+            
+            // Simulate accessing a fictitious dependency on some random commit that doesn't exist, and check that we get back the expected error.
+            do { _ = try container.getDependencies(at: "535f4cb5b4a0872fa691473e82d7b27b9894df00", productFilter: .everything) }
+            catch let error as RepositoryPackageContainer.GetDependenciesError {
+                // We expect to get an error message that mentions main.
+                XCTAssertMatch(error.description, .prefix("could not find the commit 535f4cb5b4a0872fa691473e82d7b27b9894df00"))
+            }
+        }
+    }
+
 }