Implement error handling in build_graph

Peter554 · Peter554 · commit 0c28293a6fd9 · 2025-11-10T15:31:27.000+01:00
diff --git a/rust/src/errors.rs b/rust/src/errors.rs
@@ -1,6 +1,6 @@
 use crate::exceptions;
 use pyo3::PyErr;
-use pyo3::exceptions::PyValueError;
+use pyo3::exceptions::{PyFileNotFoundError, PyIOError, PyValueError};
 use ruff_python_parser::ParseError as RuffParseError;
 use thiserror::Error;
 
@@ -36,6 +36,18 @@ pub enum GrimpError {
 
     #[error("Could not use corrupt cache file {0}.")]
     CorruptCache(String),
+
+    #[error("Failed to read file {path}: {error}")]
+    FileReadError { path: String, error: String },
+
+    #[error("Failed to get file metadata for {path}: {error}")]
+    FileMetadataError { path: String, error: String },
+
+    #[error("Failed to write cache file {path}: {error}")]
+    CacheWriteError { path: String, error: String },
+
+    #[error("Package directory does not exist: {0}")]
+    PackageDirectoryNotFound(String),
 }
 
 pub type GrimpResult<T> = Result<T, GrimpError>;
@@ -55,6 +67,12 @@ impl From<GrimpError> for PyErr {
                 line_number, text, ..
             } => PyErr::new::<exceptions::ParseError, _>((line_number, text)),
             GrimpError::CorruptCache(_) => exceptions::CorruptCache::new_err(value.to_string()),
+            GrimpError::FileReadError { .. } => PyIOError::new_err(value.to_string()),
+            GrimpError::FileMetadataError { .. } => PyIOError::new_err(value.to_string()),
+            GrimpError::CacheWriteError { .. } => PyIOError::new_err(value.to_string()),
+            GrimpError::PackageDirectoryNotFound(_) => {
+                PyFileNotFoundError::new_err(value.to_string())
+            }
         }
     }
 }
diff --git a/rust/src/graph/builder/cache.rs b/rust/src/graph/builder/cache.rs
@@ -5,6 +5,7 @@ use std::path::{Path, PathBuf};
 
 use bincode::{Decode, Encode};
 
+use crate::errors::{GrimpError, GrimpResult};
 use crate::import_parsing::ImportedObject;
 
 #[derive(Debug, Clone, Encode, Decode)]
@@ -48,20 +49,31 @@ pub fn load_cache(cache_dir: &Path, package_name: &str) -> ImportCache {
     HashMap::new()
 }
 
-pub fn save_cache(cache: &ImportCache, cache_dir: &Path, package_name: &str) {
-    if let Err(e) = fs::create_dir_all(cache_dir) {
-        eprintln!("Failed to create cache directory: {}", e);
-        return;
-    }
+pub fn save_cache(cache: &ImportCache, cache_dir: &Path, package_name: &str) -> GrimpResult<()> {
+    fs::create_dir_all(cache_dir).map_err(|e| GrimpError::CacheWriteError {
+        path: cache_dir.display().to_string(),
+        error: e.to_string(),
+    })?;
 
     let cache_file = cache_dir.join(format!("{}.imports.bincode", package_name));
 
-    match bincode::encode_to_vec(cache, bincode::config::standard()) {
-        Ok(encoded) => {
-            if let Ok(mut file) = fs::File::create(&cache_file) {
-                let _ = file.write_all(&encoded);
-            }
+    let encoded = bincode::encode_to_vec(cache, bincode::config::standard()).map_err(|e| {
+        GrimpError::CacheWriteError {
+            path: cache_file.display().to_string(),
+            error: e.to_string(),
         }
-        Err(e) => eprintln!("Failed to encode cache: {}", e),
-    }
+    })?;
+
+    let mut file = fs::File::create(&cache_file).map_err(|e| GrimpError::CacheWriteError {
+        path: cache_file.display().to_string(),
+        error: e.to_string(),
+    })?;
+
+    file.write_all(&encoded)
+        .map_err(|e| GrimpError::CacheWriteError {
+            path: cache_file.display().to_string(),
+            error: e.to_string(),
+        })?;
+
+    Ok(())
 }
diff --git a/rust/src/graph/builder/mod.rs b/rust/src/graph/builder/mod.rs
@@ -8,6 +8,7 @@ use crossbeam::channel;
 use derive_new::new;
 use ignore::WalkBuilder;
 
+use crate::errors::{GrimpError, GrimpResult};
 use crate::graph::Graph;
 use crate::import_parsing::{ImportedObject, parse_imports_from_code};
 
@@ -31,22 +32,31 @@ pub fn build_graph(
     include_external_packages: bool,
     exclude_type_checking_imports: bool,
     cache_dir: Option<&PathBuf>,
-) -> Graph {
+) -> GrimpResult<Graph> {
+    // Check if package directory exists
+    if !package.directory.exists() {
+        return Err(GrimpError::PackageDirectoryNotFound(
+            package.directory.display().to_string(),
+        ));
+    }
+
     // Load cache if available
     let mut cache = cache_dir
         .map(|dir| load_cache(dir, &package.name))
         .unwrap_or_default();
 
     // Create channels for communication
-    let (module_discovery_sender, module_discovery_receiver) = channel::bounded(10000);
-    let (import_parser_sender, import_parser_receiver) = channel::bounded(10000);
+    // This way we can start parsing moduels while we're still discovering them.
+    let (found_module_sender, found_module_receiver) = channel::bounded(10000);
+    let (parsed_module_sender, parser_module_receiver) = channel::bounded(10000);
+    let (error_sender, error_receiver) = channel::bounded(1);
 
     let mut thread_handles = Vec::new();
 
     // Thread 1: Discover modules
     let package_clone = package.clone();
     let handle = thread::spawn(move || {
-        discover_python_modules(&package_clone, module_discovery_sender);
+        discover_python_modules(&package_clone, found_module_sender);
     });
     thread_handles.push(handle);
 
@@ -55,32 +65,47 @@ pub fn build_graph(
         .map(|n| max(n.get() / 2, 1))
         .unwrap_or(4);
     for _ in 0..num_workers {
-        let receiver = module_discovery_receiver.clone();
-        let sender = import_parser_sender.clone();
+        let receiver = found_module_receiver.clone();
+        let sender = parsed_module_sender.clone();
+        let error_sender = error_sender.clone();
         let cache = cache.clone();
         let handle = thread::spawn(move || {
             while let Ok(module) = receiver.recv() {
-                if let Some(parsed) = parse_module_imports(&module, &cache) {
-                    sender.send(parsed).unwrap();
+                match parse_module_imports(&module, &cache) {
+                    Ok(parsed) => {
+                        sender.send(parsed).unwrap();
+                    }
+                    Err(e) => {
+                        // Channel has capacity of 1, since we only care to catch one error.
+                        // Drop further errors.
+                        let _ = error_sender.try_send(e);
+                    }
                 }
             }
         });
         thread_handles.push(handle);
     }
-    drop(module_discovery_receiver); // Close original receiver
-    drop(import_parser_sender); // Close original sender
-
-    // Collect parsed modules
-    let mut parsed_modules = Vec::new();
-    while let Ok(parsed) = import_parser_receiver.recv() {
-        parsed_modules.push(parsed);
-    }
 
+    // Close original found_module_receiver, so that the parser threads can exit.
+    drop(found_module_receiver);
     // Wait for all threads to complete
     for handle in thread_handles {
         handle.join().unwrap();
     }
 
+    // Check if any errors occurred
+    if let Ok(error) = error_receiver.try_recv() {
+        return Err(error);
+    }
+
+    // Collect parsed modules
+    let mut parsed_modules = Vec::new();
+    // Close original parsed_module_sender, so that parser_module_receiver iteration terminates.
+    drop(parsed_module_sender);
+    while let Ok(parsed) = parser_module_receiver.recv() {
+        parsed_modules.push(parsed);
+    }
+
     // Update and save cache if cache_dir is set
     if let Some(cache_dir) = cache_dir {
         for parsed in &parsed_modules {
@@ -89,17 +114,17 @@ pub fn build_graph(
                 CachedImports::new(parsed.module.mtime_secs, parsed.imported_objects.clone()),
             );
         }
-        save_cache(&cache, cache_dir, &package.name);
+        save_cache(&cache, cache_dir, &package.name)?;
     }
 
-    // Resolve imports and assemble graph (sequential)
+    // Resolve imports and assemble graph
     let imports_by_module = resolve_imports(
         &parsed_modules,
         include_external_packages,
         exclude_type_checking_imports,
     );
 
-    assemble_graph(&imports_by_module, &package.name)
+    Ok(assemble_graph(&imports_by_module, &package.name))
 }
 
 #[derive(Debug, Clone)]
@@ -154,6 +179,12 @@ fn discover_python_modules(package: &PackageSpec, sender: channel::Sender<FoundM
                 };
 
                 let path = entry.path();
+
+                // Only process files, not directories
+                if !path.is_file() {
+                    return WalkState::Continue;
+                }
+
                 if let Some(module_name) = path_to_module_name(path, &package) {
                     let is_package = is_package(path);
 
@@ -181,23 +212,27 @@ fn discover_python_modules(package: &PackageSpec, sender: channel::Sender<FoundM
         });
 }
 
-fn parse_module_imports(module: &FoundModule, cache: &ImportCache) -> Option<ParsedModule> {
+fn parse_module_imports(module: &FoundModule, cache: &ImportCache) -> GrimpResult<ParsedModule> {
     // Check if we have a cached version with matching mtime
     if let Some(cached) = cache.get(&module.path)
         && module.mtime_secs == cached.mtime_secs()
     {
         // Cache hit - use cached imports
-        return Some(ParsedModule {
+        return Ok(ParsedModule {
             module: module.clone(),
             imported_objects: cached.imported_objects().to_vec(),
         });
     }
 
     // Cache miss or file modified - parse the file
-    let code = fs::read_to_string(&module.path).ok()?;
-    let imported_objects =
-        parse_imports_from_code(&code, module.path.to_str().unwrap_or("")).ok()?;
-    Some(ParsedModule {
+    let code = fs::read_to_string(&module.path).map_err(|e| GrimpError::FileReadError {
+        path: module.path.display().to_string(),
+        error: e.to_string(),
+    })?;
+
+    let imported_objects = parse_imports_from_code(&code, module.path.to_str().unwrap_or(""))?;
+
+    Ok(ParsedModule {
         module: module.clone(),
         imported_objects,
     })
diff --git a/rust/src/graph_building.rs b/rust/src/graph_building.rs
@@ -27,13 +27,13 @@ pub fn build_graph_rust(
     include_external_packages: bool,
     exclude_type_checking_imports: bool,
     cache_dir: Option<String>,
-) -> GraphWrapper {
+) -> PyResult<GraphWrapper> {
     let cache_path = cache_dir.map(PathBuf::from);
     let graph = build_graph(
         &package.inner,
         include_external_packages,
         exclude_type_checking_imports,
         cache_path.as_ref(),
-    );
-    GraphWrapper::from_graph(graph)
+    )?;
+    Ok(GraphWrapper::from_graph(graph))
 }
