Merge pull request #12 from OxfordAbstracts/better-errors-and-caching

Better errors and caching

Author: roryc89

src/build/cache.rs

@@ -622,7 +622,7 @@ fn build_from_sources_impl(
         effective_timeout.map(|t| t.as_secs()).unwrap_or(0));
 
     let mut done = 0usize;
-    let mut rebuilt_set: HashSet<String> = HashSet::new();
+    let mut export_diffs: HashMap<String, cache::ExportDiff> = HashMap::new();
     let mut cached_count = 0usize;
 
     for level in &levels {
@@ -635,7 +635,7 @@ fn build_from_sources_impl(
                 {
                     let pm = &parsed[idx];
                     if let Some(ref mut cache) = cache {
-                        if !cache.needs_rebuild(&pm.module_name, pm.source_hash, &rebuilt_set) {
+                        if !cache.needs_rebuild_smart(&pm.module_name, pm.source_hash, &export_diffs) {
                             if let Some(exports) = cache.get_exports(&pm.module_name) {
                                 done += 1;
                                 cached_count += 1;
@@ -647,6 +647,7 @@ fn build_from_sources_impl(
                                 module_results.push(ModuleResult {
                                     path: pm.path.clone(),
                                     module_name: pm.module_name.clone(),
+
                                     type_errors: vec![],
                                     cached: true,
                                 });
@@ -705,24 +706,71 @@ fn build_from_sources_impl(
                             "  [{}/{}] ok: {} ({:.2?})",
                             done, total_modules, pm.module_name, elapsed
                         );
-                        let import_names: Vec<String> = pm.import_parts.iter()
-                            .map(|parts| interner::resolve_module_name(parts))
-                            .collect();
-                        let exports_changed = if let Some(ref mut c) = cache {
-                            c.update(pm.module_name.clone(), pm.source_hash, result.exports.clone(), import_names)
+                        let has_errors = !result.errors.is_empty();
+                        if has_errors {
+                            // Don't cache modules with type errors
+                            // Compute a full diff so downstream modules rebuild
+                            let old_exports = cache.as_mut().and_then(|c| c.get_exports(&pm.module_name).cloned());
+                            if let Some(ref mut c) = cache {
+                                c.remove(&pm.module_name);
+                            }
+                            // Treat error modules as having all exports changed
+                            let diff = if let Some(old) = old_exports {
+                                cache::ExportDiff::compute(&old, &result.exports)
+                            } else {
+                                // New module with errors — force downstream rebuild
+                                let mut d = cache::ExportDiff::default();
+                                d.instances_changed = true;
+                                d
+                            };
+                            if !diff.is_empty() {
+                                log::debug!(
+                                    "[build-plan] {} exports changed: values={:?}, types={:?}, classes={:?}, instances={}, operators={}",
+                                    pm.module_name, diff.changed_values, diff.changed_types, diff.changed_classes,
+                                    diff.instances_changed, diff.operators_changed
+                                );
+                                export_diffs.insert(pm.module_name.clone(), diff);
+                            }
                         } else {
-                            true
-                        };
-                        // Only add to rebuilt_set if exports actually changed
-                        if exports_changed {
-                            rebuilt_set.insert(pm.module_name.clone());
+                            let import_names: Vec<String> = pm.import_parts.iter()
+                                .map(|parts| interner::resolve_module_name(parts))
+                                .collect();
+                            let module_ref = pm.module.as_ref().unwrap();
+                            let import_items = cache::extract_import_items(&module_ref.imports);
+                            // Get old exports before updating for diff computation
+                            let old_exports = cache.as_mut().and_then(|c| c.get_exports(&pm.module_name).cloned());
+                            let exports_changed = if let Some(ref mut c) = cache {
+                                c.update(pm.module_name.clone(), pm.source_hash, result.exports.clone(), import_names, import_items)
+                            } else {
+                                true
+                            };
+                            // Compute per-symbol diff for smart rebuild
+                            if exports_changed {
+                                let diff = if let Some(old) = old_exports {
+                                    cache::ExportDiff::compute(&old, &result.exports)
+                                } else {
+                                    // New module — force downstream rebuild
+                                    let mut d = cache::ExportDiff::default();
+                                    d.instances_changed = true;
+                                    d
+                                };
+                                if !diff.is_empty() {
+                                    log::debug!(
+                                    "[build-plan] {} exports changed: values={:?}, types={:?}, classes={:?}, instances={}, operators={}",
+                                    pm.module_name, diff.changed_values, diff.changed_types, diff.changed_classes,
+                                    diff.instances_changed, diff.operators_changed
+                                );
+                                export_diffs.insert(pm.module_name.clone(), diff);
+                                }
+                            }
                         }
                         // Register exports immediately — result.exports is moved,
                         // then result (with its types HashMap) is dropped.
                         registry.register(&pm.module_parts, result.exports);
                         module_results.push(ModuleResult {
                             path: pm.path.clone(),
                             module_name: pm.module_name.clone(),
+
                             type_errors: result.errors,
                             cached: false,
                         });
@@ -746,7 +794,7 @@ fn build_from_sources_impl(
             for &idx in level.iter() {
                 let pm = &parsed[idx];
                 if let Some(ref mut cache) = cache {
-                    if !cache.needs_rebuild(&pm.module_name, pm.source_hash, &rebuilt_set) {
+                    if !cache.needs_rebuild_smart(&pm.module_name, pm.source_hash, &export_diffs) {
                         if let Some(exports) = cache.get_exports(&pm.module_name) {
                             done += 1;
                             cached_count += 1;
@@ -832,21 +880,63 @@ fn build_from_sources_impl(
                             "  [{}/{}] ok: {} ({:.2?})",
                             done, total_modules, pm.module_name, elapsed
                         );
-                        let import_names: Vec<String> = pm.import_parts.iter()
-                            .map(|parts| interner::resolve_module_name(parts))
-                            .collect();
-                        let exports_changed = if let Some(ref mut c) = cache {
-                            c.update(pm.module_name.clone(), pm.source_hash, result.exports.clone(), import_names)
+                        let has_errors = !result.errors.is_empty();
+                        if has_errors {
+                            // Don't cache modules with type errors
+                            let old_exports = cache.as_mut().and_then(|c| c.get_exports(&pm.module_name).cloned());
+                            if let Some(ref mut c) = cache {
+                                c.remove(&pm.module_name);
+                            }
+                            let diff = if let Some(old) = old_exports {
+                                cache::ExportDiff::compute(&old, &result.exports)
+                            } else {
+                                let mut d = cache::ExportDiff::default();
+                                d.instances_changed = true;
+                                d
+                            };
+                            if !diff.is_empty() {
+                                log::debug!(
+                                    "[build-plan] {} exports changed: values={:?}, types={:?}, classes={:?}, instances={}, operators={}",
+                                    pm.module_name, diff.changed_values, diff.changed_types, diff.changed_classes,
+                                    diff.instances_changed, diff.operators_changed
+                                );
+                                export_diffs.insert(pm.module_name.clone(), diff);
+                            }
                         } else {
-                            true
-                        };
-                        if exports_changed {
-                            rebuilt_set.insert(pm.module_name.clone());
+                            let import_names: Vec<String> = pm.import_parts.iter()
+                                .map(|parts| interner::resolve_module_name(parts))
+                                .collect();
+                            let module_ref = pm.module.as_ref().unwrap();
+                            let import_items = cache::extract_import_items(&module_ref.imports);
+                            let old_exports = cache.as_mut().and_then(|c| c.get_exports(&pm.module_name).cloned());
+                            let exports_changed = if let Some(ref mut c) = cache {
+                                c.update(pm.module_name.clone(), pm.source_hash, result.exports.clone(), import_names, import_items)
+                            } else {
+                                true
+                            };
+                            if exports_changed {
+                                let diff = if let Some(old) = old_exports {
+                                    cache::ExportDiff::compute(&old, &result.exports)
+                                } else {
+                                    let mut d = cache::ExportDiff::default();
+                                    d.instances_changed = true;
+                                    d
+                                };
+                                if !diff.is_empty() {
+                                    log::debug!(
+                                    "[build-plan] {} exports changed: values={:?}, types={:?}, classes={:?}, instances={}, operators={}",
+                                    pm.module_name, diff.changed_values, diff.changed_types, diff.changed_classes,
+                                    diff.instances_changed, diff.operators_changed
+                                );
+                                export_diffs.insert(pm.module_name.clone(), diff);
+                                }
+                            }
                         }
                         registry.register(&pm.module_parts, result.exports);
                         module_results.push(ModuleResult {
                             path: pm.path.clone(),
                             module_name: pm.module_name.clone(),
+
                             type_errors: result.errors,
                             cached: false,
                         });

src/build/mod.rs

@@ -3,7 +3,7 @@
 //! Uses a deduplicated string table so each symbol is stored once.
 //! Symbol references are u32 indices into the string table.
 
-use std::collections::{HashMap, HashSet};
+use std::collections::{BTreeMap, BTreeSet, HashMap};
 
 use serde::{Deserialize, Serialize};
 
@@ -64,7 +64,7 @@ impl StringTableReader {
 
 // ===== Portable QualifiedIdent =====
 
-#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq, Hash)]
+#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)]
 pub struct PQI {
     pub module: Option<u32>,
     pub name: u32,
@@ -227,34 +227,34 @@ fn rest_role(p: &PRole) -> Role {
 
 #[derive(Serialize, Deserialize, Clone, Debug)]
 pub struct PModuleExports {
-    pub values: HashMap<PQI, PScheme>,
-    pub class_methods: HashMap<PQI, (PQI, Vec<PQI>)>,
-    pub data_constructors: HashMap<PQI, Vec<PQI>>,
-    pub ctor_details: HashMap<PQI, (PQI, Vec<PQI>, Vec<PType>)>,
-    pub instances: HashMap<PQI, Vec<(Vec<PType>, Vec<(PQI, Vec<PType>)>)>>,
-    pub type_operators: HashMap<PQI, PQI>,
-    pub value_fixities: HashMap<PQI, (PAssociativity, u8)>,
-    pub type_fixities: HashMap<PQI, (PAssociativity, u8)>,
-    pub function_op_aliases: HashSet<PQI>,
-    pub value_operator_targets: HashMap<PQI, PQI>,
-    pub constrained_class_methods: HashSet<PQI>,
-    pub type_aliases: HashMap<PQI, (Vec<PQI>, PType)>,
-    pub class_param_counts: HashMap<PQI, usize>,
-    pub value_origins: HashMap<u32, u32>,
-    pub type_origins: HashMap<u32, u32>,
-    pub class_origins: HashMap<u32, u32>,
-    pub operator_class_targets: HashMap<u32, u32>,
-    pub class_fundeps: HashMap<u32, (Vec<u32>, Vec<(Vec<usize>, Vec<usize>)>)>,
-    pub type_con_arities: HashMap<PQI, usize>,
-    pub type_roles: HashMap<u32, Vec<PRole>>,
-    pub newtype_names: HashSet<u32>,
-    pub signature_constraints: HashMap<PQI, Vec<(PQI, Vec<PType>)>>,
-    pub type_kinds: HashMap<u32, PType>,
-    pub class_type_kinds: HashMap<u32, PType>,
-    pub partial_dischargers: HashSet<u32>,
-    pub self_referential_aliases: HashSet<u32>,
-    pub class_superclasses: HashMap<PQI, (Vec<u32>, Vec<(PQI, Vec<PType>)>)>,
-    pub method_own_constraints: HashMap<PQI, Vec<u32>>,
+    pub values: BTreeMap<PQI, PScheme>,
+    pub class_methods: BTreeMap<PQI, (PQI, Vec<PQI>)>,
+    pub data_constructors: BTreeMap<PQI, Vec<PQI>>,
+    pub ctor_details: BTreeMap<PQI, (PQI, Vec<PQI>, Vec<PType>)>,
+    pub instances: BTreeMap<PQI, Vec<(Vec<PType>, Vec<(PQI, Vec<PType>)>)>>,
+    pub type_operators: BTreeMap<PQI, PQI>,
+    pub value_fixities: BTreeMap<PQI, (PAssociativity, u8)>,
+    pub type_fixities: BTreeMap<PQI, (PAssociativity, u8)>,
+    pub function_op_aliases: BTreeSet<PQI>,
+    pub value_operator_targets: BTreeMap<PQI, PQI>,
+    pub constrained_class_methods: BTreeSet<PQI>,
+    pub type_aliases: BTreeMap<PQI, (Vec<PQI>, PType)>,
+    pub class_param_counts: BTreeMap<PQI, usize>,
+    pub value_origins: BTreeMap<u32, u32>,
+    pub type_origins: BTreeMap<u32, u32>,
+    pub class_origins: BTreeMap<u32, u32>,
+    pub operator_class_targets: BTreeMap<u32, u32>,
+    pub class_fundeps: BTreeMap<u32, (Vec<u32>, Vec<(Vec<usize>, Vec<usize>)>)>,
+    pub type_con_arities: BTreeMap<PQI, usize>,
+    pub type_roles: BTreeMap<u32, Vec<PRole>>,
+    pub newtype_names: BTreeSet<u32>,
+    pub signature_constraints: BTreeMap<PQI, Vec<(PQI, Vec<PType>)>>,
+    pub type_kinds: BTreeMap<u32, PType>,
+    pub class_type_kinds: BTreeMap<u32, PType>,
+    pub partial_dischargers: BTreeSet<u32>,
+    pub self_referential_aliases: BTreeSet<u32>,
+    pub class_superclasses: BTreeMap<PQI, (Vec<u32>, Vec<(PQI, Vec<PType>)>)>,
+    pub method_own_constraints: BTreeMap<PQI, Vec<u32>>,
 }
 
 impl PModuleExports {

src/build/portable.rs

@@ -4,7 +4,7 @@ use tower_lsp::lsp_types::*;
 
 use crate::cst::Module;
 use crate::interner;
-use crate::build::cache::ModuleCache;
+use crate::build::cache::{ModuleCache, extract_import_items};
 use crate::typechecker::registry::ModuleRegistry;
 
 use super::super::{Backend, FileState};
@@ -112,8 +112,9 @@ impl Backend {
         let import_names: Vec<String> = module.imports.iter()
             .map(|imp| interner::resolve_module_name(&imp.module.parts))
             .collect();
+        let import_items = extract_import_items(&module.imports);
         let mut cache = self.module_cache.write().await;
-        cache.update(module_name.clone(), source_hash, check_result.exports, import_names);
+        cache.update(module_name.clone(), source_hash, check_result.exports, import_names, import_items);
         drop(cache);
 
         // Publish diagnostics for the changed module
@@ -124,9 +125,27 @@ impl Backend {
 
         self.info(format!("[on_change] total: {:.2?}", on_change_start.elapsed())).await;
     }
+
+    /// Re-typecheck all files that are currently open in the editor.
+    /// Called after initialization completes to process files opened during loading.
+    pub(crate) async fn typecheck_open_files(&self) {
+        let open_files: Vec<(String, String)> = {
+            let files = self.files.read().await;
+            files.iter().map(|(uri, fs)| (uri.clone(), fs.source.clone())).collect()
+        };
+        if open_files.is_empty() {
+            return;
+        }
+        self.info(format!("[lsp] typechecking {} open file(s) after init", open_files.len())).await;
+        for (uri_str, source) in open_files {
+            if let Ok(uri) = Url::parse(&uri_str) {
+                self.on_change(uri, source).await;
+            }
+        }
+    }
 }
 
-fn type_errors_to_diagnostics(errors: &[crate::typechecker::error::TypeError], source: &str) -> Vec<Diagnostic> {
+pub(crate) fn type_errors_to_diagnostics(errors: &[crate::typechecker::error::TypeError], source: &str) -> Vec<Diagnostic> {
     errors
         .iter()
         .map(|err| {
@@ -149,14 +168,14 @@ fn type_errors_to_diagnostics(errors: &[crate::typechecker::error::TypeError], s
                 severity: Some(DiagnosticSeverity::ERROR),
                 code: Some(NumberOrString::String(format!("TypeError.{}", err.code()))),
                 source: Some("pfc".to_string()),
-                message: format!("{err}"),
+                message: format!("{}\n", err.format_pretty()),
                 ..Default::default()
             }
         })
         .collect()
 }
 
-fn error_to_range(err: &crate::diagnostics::CompilerError, source: &str) -> Range {
+pub(crate) fn error_to_range(err: &crate::diagnostics::CompilerError, source: &str) -> Range {
     match err.get_span() {
         Some(span) => match span.to_pos(source) {
             Some((start, end)) => Range {