diff --git a/crates/djls-ide/src/diagnostics.rs b/crates/djls-ide/src/diagnostics.rs index 30d3f24a..d8c3d51b 100644 --- a/crates/djls-ide/src/diagnostics.rs +++ b/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>, ) -> Vec { 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::(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)); } diff --git a/crates/djls-semantic/src/errors.rs b/crates/djls-semantic/src/errors.rs index 434e7008..670a031a 100644 --- a/crates/djls-semantic/src/errors.rs +++ b/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; diff --git a/crates/djls-semantic/src/lib.rs b/crates/djls-semantic/src/lib.rs index f0576027..89688153 100644 --- a/crates/djls-semantic/src/lib.rs +++ b/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(); } diff --git a/crates/djls-semantic/src/validation.rs b/crates/djls-semantic/src/validation.rs index de5ff280..b06f6e6c 100644 --- a/crates/djls-semantic/src/validation.rs +++ b/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>, - errors: Vec, } 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 { + 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 diff --git a/crates/djls-server/src/server.rs b/crates/djls-server/src/server.rs index 14235391..1a638069 100644 --- a/crates/djls-server/src/server.rs +++ b/crates/djls-server/src/server.rs @@ -92,7 +92,10 @@ impl DjangoLanguageServer { let diagnostics: Vec = 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; diff --git a/crates/djls-server/src/session.rs b/crates/djls-server/src/session.rs index 0fee0e1f..5302208e 100644 --- a/crates/djls-server/src/session.rs +++ b/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, 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 { 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); } } diff --git a/crates/djls-templates/src/error.rs b/crates/djls-templates/src/error.rs index 601dc48f..6fb5e78c 100644 --- a/crates/djls-templates/src/error.rs +++ b/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 for TemplateError { - fn from(err: ParserError) -> Self { +impl From for TemplateError { + fn from(err: ParseError) -> Self { Self::Parser(err.to_string()) } } diff --git a/crates/djls-templates/src/lib.rs b/crates/djls-templates/src/lib.rs index e579170a..e5275035 100644 --- a/crates/djls-templates/src/lib.rs +++ b/crates/djls-templates/src/lib.rs @@ -61,8 +61,8 @@ pub use lexer::Lexer; pub use nodelist::LineOffsets; pub use nodelist::NodeList; pub use nodelist::Span; +pub use parser::ParseError; pub use parser::Parser; -pub use parser::ParserError; use salsa::Accumulator; use tokens::TokenStream; @@ -102,14 +102,9 @@ pub fn parse_template(db: &dyn Db, file: SourceFile) -> Option> { } let nodelist = match Parser::new(db, token_stream).parse() { - Ok((nodelist, errors)) => { - for error in errors { - let template_error = TemplateError::Parser(error.to_string()); - TemplateErrorAccumulator(template_error).accumulate(db); - } - nodelist - } + Ok(nodelist) => nodelist, Err(err) => { + // Fatal error - accumulate and return empty let template_error = TemplateError::Parser(err.to_string()); TemplateErrorAccumulator(template_error).accumulate(db); diff --git a/crates/djls-templates/src/parser.rs b/crates/djls-templates/src/parser.rs index 80479577..504b891d 100644 --- a/crates/djls-templates/src/parser.rs +++ b/crates/djls-templates/src/parser.rs @@ -1,7 +1,10 @@ +use salsa::Accumulator; use serde::Serialize; use thiserror::Error; use crate::db::Db as TemplateDb; +use crate::db::TemplateErrorAccumulator; +use crate::error::TemplateError; use crate::nodelist::FilterName; use crate::nodelist::LineOffsets; use crate::nodelist::Node; @@ -18,7 +21,6 @@ pub struct Parser<'db> { tokens: Vec>, line_offsets: LineOffsets, current: usize, - errors: Vec, } impl<'db> Parser<'db> { @@ -29,11 +31,10 @@ impl<'db> Parser<'db> { tokens: tokens.stream(db).clone(), line_offsets: tokens.line_offsets(db).clone(), current: 0, - errors: Vec::new(), } } - pub fn parse(&mut self) -> Result<(NodeList<'db>, Vec), ParserError> { + pub fn parse(&mut self) -> Result, ParseError> { let mut nodelist = Vec::new(); while !self.is_at_end() { @@ -42,8 +43,8 @@ impl<'db> Parser<'db> { nodelist.push(node); } Err(err) => { + self.report_error(&err); if !self.is_at_end() { - self.errors.push(err); self.synchronize()?; } } @@ -52,15 +53,15 @@ impl<'db> Parser<'db> { let nodelist = NodeList::new(self.db, nodelist, self.line_offsets.clone()); - Ok((nodelist, std::mem::take(&mut self.errors))) + Ok(nodelist) } - fn next_node(&mut self) -> Result, ParserError> { + fn next_node(&mut self) -> Result, ParseError> { let token = self.consume()?; match token { Token::Comment { .. } => self.parse_comment(), - Token::Eof { .. } => Err(ParserError::stream_error(StreamError::AtEnd)), + Token::Eof { .. } => Err(ParseError::stream_error(StreamError::AtEnd)), Token::Block { .. } => self.parse_block(), Token::Variable { .. } => self.parse_variable(), Token::Error { .. } => self.parse_error(), @@ -70,7 +71,7 @@ impl<'db> Parser<'db> { } } - fn parse_comment(&mut self) -> Result, ParserError> { + fn parse_comment(&mut self) -> Result, ParseError> { let token = self.peek_previous()?; Ok(Node::Comment { @@ -79,7 +80,7 @@ impl<'db> Parser<'db> { }) } - fn parse_error(&mut self) -> Result, ParserError> { + fn parse_error(&mut self) -> Result, ParseError> { let token = self.peek_previous()?; match token { @@ -87,18 +88,18 @@ impl<'db> Parser<'db> { content, offset, .. } => { let error_text = content.text(self.db).clone(); - Err(ParserError::MalformedConstruct { + Err(ParseError::MalformedConstruct { position: *offset, content: error_text, }) } - _ => Err(ParserError::InvalidSyntax { + _ => Err(ParseError::InvalidSyntax { context: "Expected Error token".to_string(), }), } } - pub fn parse_block(&mut self) -> Result, ParserError> { + pub fn parse_block(&mut self) -> Result, ParseError> { let token = self.peek_previous()?; let Token::Block { @@ -106,14 +107,14 @@ impl<'db> Parser<'db> { .. } = token else { - return Err(ParserError::InvalidSyntax { + return Err(ParseError::InvalidSyntax { context: "Expected Block token".to_string(), }); }; let mut parts = content_ref.text(self.db).split_whitespace(); - let name_str = parts.next().ok_or(ParserError::EmptyTag)?; + let name_str = parts.next().ok_or(ParseError::EmptyTag)?; let name = TagName::new(self.db, name_str.to_string()); let bits = parts.map(|s| TagBit::new(self.db, s.to_string())).collect(); @@ -122,7 +123,7 @@ impl<'db> Parser<'db> { Ok(Node::Tag { name, bits, span }) } - fn parse_variable(&mut self) -> Result, ParserError> { + fn parse_variable(&mut self) -> Result, ParseError> { let token = self.peek_previous()?; let Token::Variable { @@ -130,14 +131,14 @@ impl<'db> Parser<'db> { .. } = token else { - return Err(ParserError::InvalidSyntax { + return Err(ParseError::InvalidSyntax { context: "Expected Variable token".to_string(), }); }; let mut parts = content_ref.text(self.db).split('|'); - let var_str = parts.next().ok_or(ParserError::EmptyTag)?.trim(); + let var_str = parts.next().ok_or(ParseError::EmptyTag)?.trim(); let var = VariableName::new(self.db, var_str.to_string()); let filters: Vec> = parts @@ -151,7 +152,7 @@ impl<'db> Parser<'db> { Ok(Node::Variable { var, filters, span }) } - fn parse_text(&mut self) -> Result, ParserError> { + fn parse_text(&mut self) -> Result, ParseError> { let first_token = self.peek_previous()?; // Skip standalone newlines @@ -186,24 +187,24 @@ impl<'db> Parser<'db> { } #[inline] - fn peek(&self) -> Result<&Token<'db>, ParserError> { + fn peek(&self) -> Result<&Token<'db>, ParseError> { self.tokens.get(self.current).ok_or_else(|| { if self.tokens.is_empty() { - ParserError::stream_error(StreamError::Empty) + ParseError::stream_error(StreamError::Empty) } else { - ParserError::stream_error(StreamError::AtEnd) + ParseError::stream_error(StreamError::AtEnd) } }) } #[inline] - fn peek_previous(&self) -> Result<&Token<'db>, ParserError> { + fn peek_previous(&self) -> Result<&Token<'db>, ParseError> { if self.current == 0 { - return Err(ParserError::stream_error(StreamError::BeforeStart)); + return Err(ParseError::stream_error(StreamError::BeforeStart)); } self.tokens .get(self.current - 1) - .ok_or_else(|| ParserError::stream_error(StreamError::InvalidAccess)) + .ok_or_else(|| ParseError::stream_error(StreamError::InvalidAccess)) } #[inline] @@ -212,15 +213,15 @@ impl<'db> Parser<'db> { } #[inline] - fn consume(&mut self) -> Result<&Token<'db>, ParserError> { + fn consume(&mut self) -> Result<&Token<'db>, ParseError> { if self.is_at_end() { - return Err(ParserError::stream_error(StreamError::AtEnd)); + return Err(ParseError::stream_error(StreamError::AtEnd)); } self.current += 1; self.peek_previous() } - fn synchronize(&mut self) -> Result<(), ParserError> { + fn synchronize(&mut self) -> Result<(), ParseError> { while !self.is_at_end() { let current = self.peek()?; match current { @@ -236,6 +237,10 @@ impl<'db> Parser<'db> { } Ok(()) } + + fn report_error(&self, error: &ParseError) { + TemplateErrorAccumulator(TemplateError::Parser(error.to_string())).accumulate(self.db); + } } #[derive(Clone, Debug, PartialEq, Eq, Serialize)] @@ -287,9 +292,6 @@ pub enum ParseError { StreamError { kind: StreamError }, } -// Keep ParserError as alias for compatibility -pub type ParserError = ParseError; - impl ParseError { pub fn stream_error(kind: impl Into) -> Self { Self::StreamError { kind: kind.into() } @@ -353,7 +355,7 @@ mod tests { let (tokens, line_offsets) = Lexer::new(db, source).tokenize(); let token_stream = TokenStream::new(db, tokens, line_offsets); let mut parser = Parser::new(db, token_stream); - let (nodelist, _) = parser.parse().unwrap(); + let nodelist = parser.parse().unwrap(); nodelist } diff --git a/crates/djls-templates/src/templatetags.rs b/crates/djls-templates/src/templatetags.rs deleted file mode 100644 index 0d0f1baa..00000000 --- a/crates/djls-templates/src/templatetags.rs +++ /dev/null @@ -1,38 +0,0 @@ -mod builtins; -mod snippets; -mod specs; - -pub use builtins::django_builtin_specs; -pub use snippets::generate_partial_snippet; -pub use snippets::generate_snippet_for_tag; -pub use snippets::generate_snippet_for_tag_with_end; -pub use snippets::generate_snippet_from_args; -pub use specs::ArgType; -pub use specs::EndTag; -pub use specs::IntermediateTag; -pub use specs::SimpleArgType; -pub use specs::TagArg; -pub use specs::TagSpec; -pub use specs::TagSpecs; - -pub enum TagType { - Opener, - Intermediate, - Closer, - Standalone, -} - -impl TagType { - #[must_use] - pub fn for_name(name: &str, tag_specs: &TagSpecs) -> TagType { - if tag_specs.is_opener(name) { - TagType::Opener - } else if tag_specs.is_closer(name) { - TagType::Closer - } else if tag_specs.is_intermediate(name) { - TagType::Intermediate - } else { - TagType::Standalone - } - } -}