Gracefully fail in compiler if dependency cycle exists (#1772)

We currently assume/hope the front-end scenario will catch dependency
cycles (the JS, Python, or whatever scenario is feeding the dependencies
in) and panic otherwise.

Upon retrospection, this was naive, and we should handle dependency
cycles gracefully if they're passed through to the back-end. It
certainly doesn't hurt, and there are myriad opportunities for the
various calling paths from the front-end to invoke the compiler. We
shouldn't have to audit each one for dependency cycles. @idavis found a
panic which was not detected by the front-end, and therefore caused the
backend to panic, and the language service to crash.

This PR updates the compiler to report a `DependencyCycle` error, which
ultimately shows up in the _Problems_ tab, if one is detected in the
backend.


After the update, the error shows up like this (instead of panicking):
<img width="651" alt="image"
src="https://github.com/user-attachments/assets/534e5d0a-edc0-4622-8079-988a3d249c21">

compiler/qsc/src/compile.rs

@@ -14,23 +14,29 @@ use thiserror::Error;
 pub type Error = WithSource<ErrorKind>;
 
 #[derive(Clone, Debug, Diagnostic, Error)]
-#[diagnostic(transparent)]
 #[error(transparent)]
 /// `ErrorKind` represents the different kinds of errors that can occur in the compiler.
 /// Each variant of the enum corresponds to a different stage of the compilation process.
 pub enum ErrorKind {
     /// `Frontend` variant represents errors that occur during the frontend stage of the compiler.
     /// These errors are typically related to syntax and semantic checks.
+    #[diagnostic(transparent)]
     Frontend(#[from] qsc_frontend::compile::Error),
 
     /// `Pass` variant represents errors that occur during the `qsc_passes` stage of the compiler.
     /// These errors are typically related to optimization, transformation, code generation, passes,
     /// and static analysis passes.
+    #[diagnostic(transparent)]
     Pass(#[from] qsc_passes::Error),
 
     /// `Lint` variant represents lints generated during the linting stage. These diagnostics are
     /// typically emitted from the language server and happens after all other compilation passes.
+    #[diagnostic(transparent)]
     Lint(#[from] qsc_linter::Lint),
+
+    #[error("Cycle in dependency graph")]
+    /// `DependencyCycle` occurs when there is a cycle in the dependency graph.
+    DependencyCycle,
 }
 
 /// Compiles a package from its AST representation.

compiler/qsc/src/packages.rs

@@ -1,7 +1,11 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
 
-use crate::{compile, hir::PackageId, PackageStore, TargetCapabilityFlags};
+use crate::{
+    compile::{self, ErrorKind},
+    hir::PackageId,
+    PackageStore, TargetCapabilityFlags,
+};
 use qsc_data_structures::language_features::LanguageFeatures;
 use qsc_frontend::compile::SourceMap;
 use qsc_passes::PackageType;
@@ -42,11 +46,21 @@ pub fn prepare_package_store(
 
     let mut canonical_package_identifier_to_package_id_mapping = FxHashMap::default();
 
-    let (ordered_packages, user_code) = package_graph_sources
-        .compilation_order()
-        .expect("dependency cycle detected in package graph -- this should have been caught by the target scenario");
+    let (ordered_packages, user_code) = package_graph_sources.compilation_order();
 
     let mut dependency_errors = Vec::new();
+    let ordered_packages = if let Ok(o) = ordered_packages {
+        o
+    } else {
+        // If there was a cycle in the dependencies, we treat the compilation as if
+        // there were no dependencies, and report the error upwards
+        dependency_errors.push(qsc_frontend::error::WithSource::from_map(
+            &SourceMap::default(),
+            ErrorKind::DependencyCycle,
+        ));
+        vec![]
+    };
+
     for (package_name, package_to_compile) in ordered_packages {
         let sources: Vec<(Arc<str>, Arc<str>)> =
             package_to_compile.sources.into_iter().collect::<Vec<_>>();

compiler/qsc_project/src/lib.rs

@@ -22,6 +22,6 @@ pub use js::{JSFileEntry, JSProjectHost};
 pub use manifest::{Manifest, ManifestDescriptor, PackageRef, PackageType, MANIFEST_FILE_NAME};
 pub use project::FileSystemAsync;
 pub use project::{
-    key_for_package_ref, package_ref_from_key, DirEntry, EntryType, Error, FileSystem,
-    PackageCache, PackageGraphSources, PackageInfo, Project,
+    key_for_package_ref, package_ref_from_key, DependencyCycle, DirEntry, EntryType, Error,
+    FileSystem, PackageCache, PackageGraphSources, PackageInfo, Project,
 };

compiler/qsc_project/src/project.rs

@@ -618,14 +618,17 @@ pub type OrderedDependencies = Vec<(Arc<str>, PackageInfo)>;
 
 impl PackageGraphSources {
     /// Produces an ordered vector over the packages in the order they should be compiled
-    pub fn compilation_order(self) -> Result<(OrderedDependencies, PackageInfo), DependencyCycle> {
+    pub fn compilation_order(self) -> (Result<OrderedDependencies, DependencyCycle>, PackageInfo) {
         // The order is defined by which packages depend on which other packages
         // For example, if A depends on B which depends on C, then we compile C, then B, then A
         // If there are cycles, this is an error, and we will report it as such
         let mut in_degree: FxHashMap<&str, usize> = FxHashMap::default();
         let mut graph: FxHashMap<&str, Vec<&str>> = FxHashMap::default();
 
         // Initialize the graph and in-degrees
+        // This graph contains all direct and transient dependencies
+        // and tracks which packages depend on which other packages,
+        // as well as the in-degree (quantity of dependents) of each package
         for (key, package_info) in &self.packages {
             in_degree.entry(key).or_insert(0);
             for dep in package_info.dependencies.values() {
@@ -634,13 +637,23 @@ impl PackageGraphSources {
             }
         }
 
+        // this queue contains all packages with in-degree 0
+        // these packages are valid starting points for the build order,
+        // as they don't depend on any other packages.
+        // If there are no dependency cycles, then all packages will be reachable
+        // via this queue of build order entry points.
         let mut queue: Vec<&str> = in_degree
             .iter()
             .filter_map(|(key, &deg)| if deg == 0 { Some(*key) } else { None })
             .collect();
 
         let mut sorted_keys = Vec::new();
 
+        // from all build order entry points (the initial value of `queue`), we
+        // can build the build order by visiting each package and decrementing
+        // the in-degree of its dependencies. If the in-degree of a dependency
+        // reaches 0, then it can be added to the queue of build order entry points,
+        // as all of its dependents have been built.
         while let Some(node) = queue.pop() {
             sorted_keys.push(node.to_string());
             if let Some(neighbors) = graph.get(node) {
@@ -657,16 +670,29 @@ impl PackageGraphSources {
         }
 
         let mut sorted_packages = self.packages.into_iter().collect::<Vec<_>>();
+        let mut cycle_detected = false;
         sorted_packages.sort_by_key(|(a_key, _pkg)| {
             sorted_keys
                 .iter()
                 .position(|key| key.as_str() == &**a_key)
-                .unwrap_or_else(|| panic!("package {a_key} should be in sorted keys list"))
+                .unwrap_or_else(|| {
+                    // The only situation in which a package is not in the build order
+                    // is if there is a cycle in the dependency graph.
+                    // this is because the build order must start with a package that
+                    // has zero dependencies. If all packages have dependencies, then
+                    // a cycle must exist.
+                    cycle_detected = true;
+                    sorted_keys.len()
+                })
         });
 
+        if cycle_detected {
+            return (Err(DependencyCycle), self.root);
+        }
+
         log::debug!("build plan: {:#?}", sorted_keys);
 
-        Ok((sorted_packages, self.root))
+        (Ok(sorted_packages), self.root)
     }
 
     #[must_use]