Refactor error handling to use accumulators directly

crates/djls-ide/src/diagnostics.rs

@@ -103,46 +103,51 @@ fn error_to_diagnostic(
     }
 }
 
-/// Collect all diagnostics (syntax and semantic) for a template file.
+/// Collect all diagnostics for a template file.
 ///
-/// This function:
-/// 1. Parses the template file
-/// 2. If parsing succeeds, runs semantic validation
-/// 3. Collects syntax diagnostics from the parser
-/// 4. Collects semantic diagnostics from the validator
-/// 5. Converts errors to LSP diagnostic types
+/// This function collects and converts errors that were accumulated during
+/// parsing and validation. The caller must provide the parsed `NodeList` (or `None`
+/// if parsing failed), making it explicit that parsing should have already occurred.
+///
+/// # Parameters
+/// - `db`: The Salsa database
+/// - `file`: The source file (needed to retrieve accumulated template errors)
+/// - `nodelist`: The parsed AST, or None if parsing failed
+///
+/// # Returns
+/// A vector of LSP diagnostics combining both template syntax errors and
+/// semantic validation errors.
+///
+/// # Design
+/// This API design makes it clear that:
+/// - Parsing must happen before collecting diagnostics
+/// - This function only collects and converts existing errors
+/// - The `NodeList` provides both line offsets and access to validation errors
 #[must_use]
 pub fn collect_diagnostics(
     db: &dyn djls_semantic::Db,
     file: SourceFile,
+    nodelist: Option<djls_templates::NodeList<'_>>,
 ) -> Vec<lsp_types::Diagnostic> {
     let mut diagnostics = Vec::new();
 
-    // Parse and get template errors
-    let nodelist = djls_templates::parse_template(db, file);
     let template_errors =
         djls_templates::parse_template::accumulated::<TemplateErrorAccumulator>(db, file);
 
-    // Get line offsets for conversion
-    let line_offsets = if let Some(ref nl) = nodelist {
-        nl.line_offsets(db).clone()
-    } else {
-        LineOffsets::default()
-    };
+    let line_offsets = nodelist
+        .as_ref()
+        .map(|nl| nl.line_offsets(db).clone())
+        .unwrap_or_default();
 
-    // Convert template errors to diagnostics
     for error_acc in template_errors {
         diagnostics.push(error_to_diagnostic(&error_acc.0, &line_offsets));
     }
 
-    // If parsing succeeded, run validation
     if let Some(nodelist) = nodelist {
-        djls_semantic::validate_nodelist(db, nodelist);
         let validation_errors = djls_semantic::validate_nodelist::accumulated::<
             djls_semantic::ValidationErrorAccumulator,
         >(db, nodelist);
 
-        // Convert validation errors to diagnostics
         for error_acc in validation_errors {
             diagnostics.push(error_to_diagnostic(&error_acc.0, &line_offsets));
         }

crates/djls-semantic/src/errors.rs

@@ -1,4 +1,3 @@
-// Import Span from templates - we'll need to re-export it or create our own
 use djls_templates::Span;
 use serde::Serialize;
 use thiserror::Error;

crates/djls-semantic/src/lib.rs

@@ -8,7 +8,6 @@ pub use builtins::django_builtin_specs;
 pub use db::Db;
 pub use db::ValidationErrorAccumulator;
 pub use errors::ValidationError;
-use salsa::Accumulator;
 pub use specs::ArgType;
 pub use specs::EndTag;
 pub use specs::IntermediateTag;
@@ -32,9 +31,5 @@ pub fn validate_nodelist(db: &dyn Db, nodelist: djls_templates::NodeList<'_>) {
         return;
     }
 
-    let validation_errors = TagValidator::new(db, nodelist).validate();
-
-    for error in validation_errors {
-        ValidationErrorAccumulator(error).accumulate(db);
-    }
+    TagValidator::new(db, nodelist).validate();
 }

crates/djls-semantic/src/validation.rs

@@ -22,8 +22,10 @@ use djls_templates::nodelist::Span;
 use djls_templates::nodelist::TagBit;
 use djls_templates::nodelist::TagName;
 use djls_templates::NodeList;
+use salsa::Accumulator;
 
 use crate::db::Db as SemanticDb;
+use crate::db::ValidationErrorAccumulator;
 use crate::errors::ValidationError;
 use crate::specs::ArgType;
 use crate::specs::SimpleArgType;
@@ -35,7 +37,6 @@ pub struct TagValidator<'db> {
     ast: NodeList<'db>,
     current: usize,
     stack: Vec<Node<'db>>,
-    errors: Vec<ValidationError>,
 }
 
 impl<'db> TagValidator<'db> {
@@ -46,12 +47,10 @@ impl<'db> TagValidator<'db> {
             ast,
             current: 0,
             stack: Vec::new(),
-            errors: Vec::new(),
         }
     }
 
-    #[must_use]
-    pub fn validate(mut self) -> Vec<ValidationError> {
+    pub fn validate(mut self) {
         while !self.is_at_end() {
             if let Some(node) = self.current_node() {
                 if let Node::Tag { name, bits, .. } = &node {
@@ -92,14 +91,12 @@ impl<'db> TagValidator<'db> {
         // Any remaining stack items are unclosed
         while let Some(node) = self.stack.pop() {
             if let Node::Tag { name, .. } = &node {
-                self.errors.push(ValidationError::UnclosedTag {
+                self.report_error(ValidationError::UnclosedTag {
                     tag: name.text(self.db),
                     span: node.full_span(),
                 });
             }
         }
-
-        self.errors
     }
 
     fn check_arguments(
@@ -117,7 +114,7 @@ impl<'db> TagValidator<'db> {
         let required_count = args.iter().filter(|arg| arg.required).count();
 
         if bits.len() < required_count {
-            self.errors.push(ValidationError::MissingRequiredArguments {
+            self.report_error(ValidationError::MissingRequiredArguments {
                 tag: name.to_string(),
                 min: required_count,
                 span,
@@ -130,7 +127,7 @@ impl<'db> TagValidator<'db> {
             .any(|arg| matches!(arg.arg_type, ArgType::Simple(SimpleArgType::VarArgs)));
 
         if !has_varargs && bits.len() > args.len() {
-            self.errors.push(ValidationError::TooManyArguments {
+            self.report_error(ValidationError::TooManyArguments {
                 tag: name.to_string(),
                 max: args.len(),
                 span,
@@ -162,7 +159,7 @@ impl<'db> TagValidator<'db> {
             };
             let context = format!("must appear within '{parents}' block");
 
-            self.errors.push(ValidationError::OrphanedTag {
+            self.report_error(ValidationError::OrphanedTag {
                 tag: name.to_string(),
                 context,
                 span,
@@ -175,7 +172,7 @@ impl<'db> TagValidator<'db> {
 
         if self.stack.is_empty() {
             // Stack is empty - unexpected closer
-            self.errors.push(ValidationError::UnbalancedStructure {
+            self.report_error(ValidationError::UnbalancedStructure {
                 opening_tag: name_str.to_string(),
                 expected_closing: String::new(),
                 opening_span: span,
@@ -188,7 +185,7 @@ impl<'db> TagValidator<'db> {
         let expected_opener = self.db.tag_specs().find_opener_for_closer(&name_str);
         let Some(opener_name) = expected_opener else {
             // Unknown closer
-            self.errors.push(ValidationError::UnbalancedStructure {
+            self.report_error(ValidationError::UnbalancedStructure {
                 opening_tag: name_str.to_string(),
                 expected_closing: String::new(),
                 opening_span: span,
@@ -248,7 +245,7 @@ impl<'db> TagValidator<'db> {
         } else if !bits.is_empty() {
             // Named closer with no matching named block
             // Report the mismatch
-            self.errors.push(ValidationError::UnmatchedBlockName {
+            self.report_error(ValidationError::UnmatchedBlockName {
                 name: bits[0].text(self.db),
                 span,
             });
@@ -267,7 +264,7 @@ impl<'db> TagValidator<'db> {
                         name: block_name, ..
                     } = nearest_block
                     {
-                        self.errors.push(ValidationError::UnclosedTag {
+                        self.report_error(ValidationError::UnclosedTag {
                             tag: block_name.text(self.db),
                             span: nearest_block.full_span(),
                         });
@@ -282,7 +279,7 @@ impl<'db> TagValidator<'db> {
             }
         } else {
             // No opener found at all
-            self.errors.push(ValidationError::UnbalancedStructure {
+            self.report_error(ValidationError::UnbalancedStructure {
                 opening_tag: opener_name,
                 expected_closing: name_str.to_string(),
                 opening_span: span,
@@ -295,7 +292,7 @@ impl<'db> TagValidator<'db> {
         while self.stack.len() > index + 1 {
             if let Some(unclosed) = self.stack.pop() {
                 if let Node::Tag { name, .. } = &unclosed {
-                    self.errors.push(ValidationError::UnclosedTag {
+                    self.report_error(ValidationError::UnclosedTag {
                         tag: name.text(self.db),
                         span: unclosed.full_span(),
                     });
@@ -315,6 +312,10 @@ impl<'db> TagValidator<'db> {
     fn is_at_end(&self) -> bool {
         self.current >= self.ast.nodelist(self.db).len()
     }
+
+    fn report_error(&self, error: ValidationError) {
+        ValidationErrorAccumulator(error).accumulate(self.db);
+    }
 }
 
 // TODO: Add validation tests after HIR migration is complete

crates/djls-server/src/server.rs

@@ -92,7 +92,10 @@ impl DjangoLanguageServer {
         let diagnostics: Vec<lsp_types::Diagnostic> = self
             .with_session_mut(|session| {
                 let file = session.get_or_create_file(&path);
-                session.with_db(|db| djls_ide::collect_diagnostics(db, file))
+                session.with_db(|db| {
+                    let nodelist = djls_templates::parse_template(db, file);
+                    djls_ide::collect_diagnostics(db, file, nodelist)
+                })
             })
             .await;
 
@@ -412,7 +415,8 @@ impl LanguageServer for DjangoLanguageServer {
                         return vec![];
                     };
 
-                    djls_ide::collect_diagnostics(db, file)
+                    let nodelist = djls_templates::parse_template(db, file);
+                    djls_ide::collect_diagnostics(db, file, nodelist)
                 })
             })
             .await;

crates/djls-server/src/session.rs

@@ -12,6 +12,7 @@ use djls_project::Db as ProjectDb;
 use djls_project::Interpreter;
 use djls_workspace::db::SourceFile;
 use djls_workspace::paths;
+use djls_workspace::FileKind;
 use djls_workspace::PositionEncoding;
 use djls_workspace::TextDocument;
 use djls_workspace::Workspace;
@@ -155,64 +156,85 @@ impl Session {
     ///
     /// Updates both the workspace buffers and database. Creates the file in
     /// the database or invalidates it if it already exists.
+    /// For template files, immediately triggers parsing and validation.
     pub fn open_document(&mut self, url: &Url, document: TextDocument) {
         // Add to workspace buffers
         self.workspace.open_document(url, document);
 
-        // Update database if it's a file URL
         if let Some(path) = paths::url_to_path(url) {
-            // Check if file already exists (was previously read from disk)
             let already_exists = self.db.has_file(&path);
-            let _file = self.db.get_or_create_file(&path);
+            let file = self.db.get_or_create_file(&path);
 
             if already_exists {
                 // File was already read - touch to invalidate cache
                 self.db.touch_file(&path);
             }
+
+            if FileKind::from_path(&path) == FileKind::Template {
+                let nodelist = djls_templates::parse_template(&self.db, file);
+                if let Some(nodelist) = nodelist {
+                    djls_semantic::validate_nodelist(&self.db, nodelist);
+                }
+            }
         }
     }
 
     /// Update a document with incremental changes.
     ///
     /// Applies changes to the document and triggers database invalidation.
+    /// For template files, immediately triggers parsing and validation.
     pub fn update_document(
         &mut self,
         url: &Url,
         changes: Vec<lsp_types::TextDocumentContentChangeEvent>,
         version: i32,
     ) {
-        // Update in workspace
         self.workspace
             .update_document(url, changes, version, self.position_encoding);
 
-        // Touch file in database to trigger invalidation
         if let Some(path) = paths::url_to_path(url) {
             if self.db.has_file(&path) {
+                // Touch file in database to trigger invalidation
                 self.db.touch_file(&path);
+
+                if FileKind::from_path(&path) == FileKind::Template {
+                    let file = self.db.get_or_create_file(&path);
+                    let nodelist = djls_templates::parse_template(&self.db, file);
+                    if let Some(nodelist) = nodelist {
+                        djls_semantic::validate_nodelist(&self.db, nodelist);
+                    }
+                }
             }
         }
     }
 
     pub fn save_document(&mut self, url: &Url) {
-        // Touch file in database to trigger re-analysis
         if let Some(path) = paths::url_to_path(url) {
-            self.with_db_mut(|db| {
-                if db.has_file(&path) {
-                    db.touch_file(&path);
+            if self.db.has_file(&path) {
+                // Touch file in database to trigger invalidation
+                self.db.touch_file(&path);
+
+                if FileKind::from_path(&path) == FileKind::Template {
+                    let file = self.db.get_or_create_file(&path);
+                    let nodelist = djls_templates::parse_template(&self.db, file);
+                    if let Some(nodelist) = nodelist {
+                        djls_semantic::validate_nodelist(&self.db, nodelist);
+                    }
                 }
-            });
+            }
         }
     }
 
     /// Close a document.
     ///
     /// Removes from workspace buffers and triggers database invalidation to fall back to disk.
+    /// For template files, immediately re-parses from disk.
     pub fn close_document(&mut self, url: &Url) -> Option<TextDocument> {
         let document = self.workspace.close_document(url);
 
-        // Touch file in database to trigger re-read from disk
         if let Some(path) = paths::url_to_path(url) {
             if self.db.has_file(&path) {
+                // Touch file in database to trigger re-read from disk
                 self.db.touch_file(&path);
             }
         }

crates/djls-templates/src/error.rs

@@ -1,7 +1,7 @@
 use serde::Serialize;
 use thiserror::Error;
 
-use crate::parser::ParserError;
+use crate::parser::ParseError;
 
 #[derive(Clone, Debug, Error, PartialEq, Eq, Serialize)]
 pub enum TemplateError {
@@ -15,8 +15,8 @@ pub enum TemplateError {
     Config(String),
 }
 
-impl From<ParserError> for TemplateError {
-    fn from(err: ParserError) -> Self {
+impl From<ParseError> for TemplateError {
+    fn from(err: ParseError) -> Self {
         Self::Parser(err.to_string())
     }
 }