From 6e323bbe91c99936a85b15fc94ecd15564882a6e Mon Sep 17 00:00:00 2001 From: Alex Hansen Date: Tue, 23 Jul 2024 11:21:02 -0700 Subject: [PATCH] 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): image --- compiler/qsc/src/compile.rs | 8 +++++++- compiler/qsc/src/packages.rs | 22 ++++++++++++++++---- compiler/qsc_project/src/lib.rs | 4 ++-- compiler/qsc_project/src/project.rs | 32 ++++++++++++++++++++++++++--- 4 files changed, 56 insertions(+), 10 deletions(-) diff --git a/compiler/qsc/src/compile.rs b/compiler/qsc/src/compile.rs index c2b9c8c0b7..1234a0102b 100644 --- a/compiler/qsc/src/compile.rs +++ b/compiler/qsc/src/compile.rs @@ -14,23 +14,29 @@ use thiserror::Error; pub type Error = WithSource; #[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. diff --git a/compiler/qsc/src/packages.rs b/compiler/qsc/src/packages.rs index 8703c467b0..cbb0d8be94 100644 --- a/compiler/qsc/src/packages.rs +++ b/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, Arc)> = package_to_compile.sources.into_iter().collect::>(); diff --git a/compiler/qsc_project/src/lib.rs b/compiler/qsc_project/src/lib.rs index af299b634d..29d25aa4d7 100644 --- a/compiler/qsc_project/src/lib.rs +++ b/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, }; diff --git a/compiler/qsc_project/src/project.rs b/compiler/qsc_project/src/project.rs index a5ab16893a..d3383c7968 100644 --- a/compiler/qsc_project/src/project.rs +++ b/compiler/qsc_project/src/project.rs @@ -618,7 +618,7 @@ pub type OrderedDependencies = Vec<(Arc, 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, 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 @@ -626,6 +626,9 @@ impl PackageGraphSources { 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,6 +637,11 @@ 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, °)| if deg == 0 { Some(*key) } else { None }) @@ -641,6 +649,11 @@ impl PackageGraphSources { 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::>(); + 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]