diff --git a/crates/djls-semantic/src/semantic/args.rs b/crates/djls-semantic/src/arguments.rs similarity index 70% rename from crates/djls-semantic/src/semantic/args.rs rename to crates/djls-semantic/src/arguments.rs index 668395af..1bc61449 100644 --- a/crates/djls-semantic/src/semantic/args.rs +++ b/crates/djls-semantic/src/arguments.rs @@ -1,104 +1,84 @@ use djls_source::Span; use djls_templates::tokens::TagDelimiter; use djls_templates::Node; -use rustc_hash::FxHashSet; use salsa::Accumulator; -use crate::semantic::forest::SegmentKind; -use crate::semantic::forest::SemanticNode; -use crate::templatetags::IntermediateTag; use crate::templatetags::TagArg; -use crate::templatetags::TagSpecs; +use crate::templatetags::TagArgSliceExt; use crate::Db; use crate::ValidationError; use crate::ValidationErrorAccumulator; -pub fn validate_block_tags(db: &dyn Db, roots: &[SemanticNode]) { - for node in roots { - validate_node(db, node); - } -} - -pub fn validate_non_block_tags( - db: &dyn Db, - nodelist: djls_templates::NodeList<'_>, - skip_spans: &[Span], -) { - let skip: FxHashSet<_> = skip_spans.iter().copied().collect(); - +/// Validate arguments for all tags in the template. +/// +/// Performs a single pass over the flat `NodeList`, validating each tag's arguments +/// against its `TagSpec` definition. This is independent of block structure - we validate +/// opening tags, closing tags, intermediate tags, and standalone tags all the same way. +/// +/// # Parameters +/// - `db`: The Salsa database containing tag specifications +/// - `nodelist`: The parsed template `NodeList` containing all tags +pub fn validate_all_tag_arguments(db: &dyn Db, nodelist: djls_templates::NodeList<'_>) { for node in nodelist.nodelist(db) { if let Node::Tag { name, bits, span } = node { let marker_span = span.expand(TagDelimiter::LENGTH_U32, TagDelimiter::LENGTH_U32); - if skip.contains(&marker_span) { - continue; - } validate_tag_arguments(db, name, bits, marker_span); } } } -fn validate_node(db: &dyn Db, node: &SemanticNode) { - match node { - SemanticNode::Tag { - name, - marker_span, - arguments, - segments, - } => { - validate_tag_arguments(db, name, arguments, *marker_span); - - for segment in segments { - match &segment.kind { - SegmentKind::Main => { - validate_children(db, &segment.children); - } - SegmentKind::Intermediate { tag } => { - validate_tag_arguments(db, tag, &segment.arguments, segment.marker_span); - validate_children(db, &segment.children); - } - } - } - } - SemanticNode::Leaf { .. } => {} - } -} - -fn validate_children(db: &dyn Db, children: &[SemanticNode]) { - for child in children { - validate_node(db, child); - } -} - -/// Validate a single tag invocation against its `TagSpec` definition. +/// Validate a single tag's arguments against its `TagSpec` definition. +/// +/// This is the main entry point for tag argument validation. It looks up the tag +/// in the `TagSpecs` (checking opening tags, closing tags, and intermediate tags) +/// and delegates to the appropriate validation logic. +/// +/// # Parameters +/// - `db`: The Salsa database containing tag specifications +/// - `tag_name`: The name of the tag (e.g., "if", "for", "endfor", "elif") +/// - `bits`: The tokenized arguments from the tag +/// - `span`: The span of the tag for error reporting pub fn validate_tag_arguments(db: &dyn Db, tag_name: &str, bits: &[String], span: Span) { let tag_specs = db.tag_specs(); + // Try to find spec for: opening tag, closing tag, or intermediate tag if let Some(spec) = tag_specs.get(tag_name) { - validate_args(db, tag_name, bits, span, spec.args.as_ref()); + validate_args_against_spec(db, tag_name, bits, span, spec.args.as_ref()); return; } if let Some(end_spec) = tag_specs.get_end_spec_for_closer(tag_name) { - validate_args(db, tag_name, bits, span, end_spec.args.as_ref()); + validate_args_against_spec(db, tag_name, bits, span, end_spec.args.as_ref()); return; } - if let Some(intermediate) = find_intermediate_spec(&tag_specs, tag_name) { - validate_args(db, tag_name, bits, span, intermediate.args.as_ref()); + if let Some(intermediate) = tag_specs.get_intermediate_spec(tag_name) { + validate_args_against_spec(db, tag_name, bits, span, intermediate.args.as_ref()); } -} -fn find_intermediate_spec<'a>(specs: &'a TagSpecs, tag_name: &str) -> Option<&'a IntermediateTag> { - specs.iter().find_map(|(_, spec)| { - spec.intermediate_tags - .iter() - .find(|it| it.name.as_ref() == tag_name) - }) + // Unknown tag - no validation (could be custom tag from unloaded library) } -fn validate_args(db: &dyn Db, tag_name: &str, bits: &[String], span: Span, args: &[TagArg]) { +/// Validate tag arguments against an argument specification. +/// +/// This function performs high-level validation (argument count) and delegates +/// to more detailed validation for argument order, choices, and literal values. +/// +/// # Parameters +/// - `db`: The Salsa database +/// - `tag_name`: The name of the tag being validated +/// - `bits`: The tokenized arguments +/// - `span`: The span for error reporting +/// - `args`: The argument specification from the `TagSpec` +fn validate_args_against_spec( + db: &dyn Db, + tag_name: &str, + bits: &[String], + span: Span, + args: &[TagArg], +) { + // Special case: tag expects no arguments if args.is_empty() { - // If the spec expects no arguments but bits exist, report once. if !bits.is_empty() { ValidationErrorAccumulator(ValidationError::TooManyArguments { tag: tag_name.to_string(), @@ -110,9 +90,8 @@ fn validate_args(db: &dyn Db, tag_name: &str, bits: &[String], span: Span, args: return; } - let has_varargs = args.iter().any(|arg| matches!(arg, TagArg::VarArgs { .. })); + // Check minimum required arguments let required_count = args.iter().filter(|arg| arg.is_required()).count(); - if bits.len() < required_count { ValidationErrorAccumulator(ValidationError::MissingRequiredArguments { tag: tag_name.to_string(), @@ -122,30 +101,33 @@ fn validate_args(db: &dyn Db, tag_name: &str, bits: &[String], span: Span, args: .accumulate(db); } - validate_literals(db, tag_name, bits, span, args); - + // For varargs tags, we can't validate order/excess (they consume everything) + // For all other tags, perform detailed positional validation + let has_varargs = args.iter().any(|arg| matches!(arg, TagArg::VarArgs { .. })); if !has_varargs { - validate_choices_and_order(db, tag_name, bits, span, args); - } -} - -fn validate_literals(db: &dyn Db, tag_name: &str, bits: &[String], span: Span, args: &[TagArg]) { - for arg in args { - if let TagArg::Literal { lit, required } = arg { - if *required && !bits.iter().any(|bit| bit == lit.as_ref()) { - ValidationErrorAccumulator(ValidationError::InvalidLiteralArgument { - tag: tag_name.to_string(), - expected: lit.to_string(), - span, - }) - .accumulate(db); - } - } + validate_argument_order(db, tag_name, bits, span, args); } } +/// Walk through arguments sequentially, consuming tokens and validating as we go. +/// +/// This is the core validation logic that performs detailed argument validation by: +/// - Walking through the argument specification in order +/// - Consuming tokens from the bits array as each argument is satisfied +/// - Validating that literals appear in the correct positions +/// - Validating that choice arguments have valid values +/// - Handling multi-token expressions that consume greedily until the next literal +/// - Detecting when there are too many arguments (leftover tokens) +/// - Detecting when required arguments are missing +/// +/// We can't use a simple `bits.len() > args.len()` check because Expression arguments +/// can consume multiple tokens (e.g., "x > 0" is 3 tokens, 1 arg) and this would cause +/// false positives for expressions with operators +/// +/// Instead, we walk through arguments and track how many tokens each consumes, +/// then check if there are leftovers. #[allow(clippy::too_many_lines)] -fn validate_choices_and_order( +fn validate_argument_order( db: &dyn Db, tag_name: &str, bits: &[String], @@ -153,15 +135,13 @@ fn validate_choices_and_order( args: &[TagArg], ) { let mut bit_index = 0usize; - let mut args_consumed = 0usize; + // Walk through argument spec, consuming tokens as we match each argument for (arg_index, arg) in args.iter().enumerate() { if bit_index >= bits.len() { - break; + break; // Ran out of tokens to consume } - args_consumed = arg_index + 1; - match arg { TagArg::Literal { lit, required } => { let matches_literal = bits[bit_index] == lit.as_ref(); @@ -175,12 +155,14 @@ fn validate_choices_and_order( span, }) .accumulate(db); - break; + break; // Can't continue if required literal is wrong } } else if matches_literal { - bit_index += 1; + bit_index += 1; // Optional literal matched, consume it } + // Optional literal didn't match - don't consume, continue } + TagArg::Choice { name, required, @@ -188,7 +170,7 @@ fn validate_choices_and_order( } => { let value = &bits[bit_index]; if choices.iter().any(|choice| choice.as_ref() == value) { - bit_index += 1; + bit_index += 1; // Valid choice, consume it } else if *required { ValidationErrorAccumulator(ValidationError::InvalidArgumentChoice { tag: tag_name.to_string(), @@ -201,97 +183,100 @@ fn validate_choices_and_order( span, }) .accumulate(db); - break; + break; // Can't continue if required choice is invalid } + // Optional choice didn't match - don't consume, continue } + TagArg::Var { .. } | TagArg::String { .. } => { - // Consume exactly 1 token + // Simple arguments consume exactly one token bit_index += 1; } - TagArg::Expr { required, .. } => { - // Expression arguments consume tokens until: - // - We hit the next literal keyword - // - We hit the end of bits - // - We've consumed at least one token + + TagArg::Expr { .. } => { + // Expression arguments consume tokens greedily until: + // - We hit the next literal keyword (if any) + // - We run out of tokens + // Must consume at least one token let start_index = bit_index; - let next_literal = find_next_literal(&args[arg_index + 1..]); + let next_literal = args[arg_index + 1..].find_next_literal(); // Consume tokens greedily until we hit a known literal while bit_index < bits.len() { - if let Some(lit) = next_literal { - if bits[bit_index] == lit { + if let Some(ref lit) = next_literal { + if bits[bit_index] == *lit { break; // Stop before the literal } } bit_index += 1; } - // Optional expressions shouldn't steal the next literal - if bit_index == start_index - && bit_index < bits.len() - && (*required || next_literal.is_none_or(|lit| bits[bit_index] != lit)) - { + // Ensure we consumed at least one token for the expression + if bit_index == start_index { bit_index += 1; } } + TagArg::Assignment { .. } => { - // Assignment can be: - // 1. Single token with = (e.g., "total=value") - // 2. Multiple tokens with "as" keyword (e.g., "url 'name' as varname") - // For now, consume until we find pattern or reach next literal + // Assignment arguments can appear as: + // 1. Single token: var=value + // 2. Multi-token: expr as varname + // Consume until we find = or "as", or hit next literal - let next_literal = find_next_literal(&args[arg_index + 1..]); + let next_literal = args[arg_index + 1..].find_next_literal(); while bit_index < bits.len() { - if let Some(lit) = next_literal { - if bits[bit_index] == lit { - break; - } - } - let token = &bits[bit_index]; bit_index += 1; - // If token contains =, we're done with this assignment + // If token contains =, we've found the assignment if token.contains('=') { break; } - // If we hit "as", consume one more token (the variable name) - if token == "as" { - if bit_index < bits.len() { - bit_index += 1; - } + // If we hit "as", consume the variable name after it + if token == "as" && bit_index < bits.len() { + bit_index += 1; // Consume the variable name break; } + + // Stop if we hit the next literal argument + if let Some(ref lit) = next_literal { + if token == lit { + break; + } + } } } + TagArg::VarArgs { .. } => { - // Consume all remaining tokens + // VarArgs consumes all remaining tokens bit_index = bits.len(); } } } - // Check for too many arguments: if we have unconsumed tokens after processing all args + // Check for unconsumed tokens (too many arguments) if bit_index < bits.len() { // We have unconsumed tokens - this is a too-many-arguments error // Note: VarArgs sets bit_index = bits.len(), so we never reach here for VarArgs tags ValidationErrorAccumulator(ValidationError::TooManyArguments { tag: tag_name.to_string(), - max: args.len(), + max: args.len(), // Approximate - imperfect for Expr args but close enough span, }) .accumulate(db); return; } - for arg in args.iter().skip(args_consumed) { + // Check for missing required arguments that weren't satisfied + // (Only matters if we consumed all tokens but didn't satisfy all required args) + for arg in args.iter().skip(bit_index) { if arg.is_required() { ValidationErrorAccumulator(ValidationError::MissingArgument { tag: tag_name.to_string(), - argument: argument_name(arg), + argument: arg.name().to_string(), span, }) .accumulate(db); @@ -299,29 +284,6 @@ fn validate_choices_and_order( } } -fn argument_name(arg: &TagArg) -> String { - match arg { - TagArg::Literal { lit, .. } => lit.to_string(), - TagArg::Choice { name, .. } - | TagArg::Var { name, .. } - | TagArg::String { name, .. } - | TagArg::Expr { name, .. } - | TagArg::Assignment { name, .. } - | TagArg::VarArgs { name, .. } => name.to_string(), - } -} - -/// Find the next literal keyword in the argument list. -/// This helps expression arguments know when to stop consuming tokens. -fn find_next_literal(remaining_args: &[TagArg]) -> Option<&str> { - for arg in remaining_args { - if let TagArg::Literal { lit, .. } = arg { - return Some(lit.as_ref()); - } - } - None -} - #[cfg(test)] mod tests { use std::sync::Arc; @@ -329,36 +291,26 @@ mod tests { use camino::Utf8Path; use camino::Utf8PathBuf; - use djls_source::Db as SourceDb; use djls_source::File; use djls_workspace::FileSystem; use djls_workspace::InMemoryFileSystem; - use rustc_hash::FxHashMap; use super::*; use crate::templatetags::django_builtin_specs; use crate::TagIndex; - use crate::TagSpec; - use crate::TagSpecs; #[salsa::db] #[derive(Clone)] struct TestDatabase { storage: salsa::Storage, fs: Arc>, - tag_specs: Arc, } impl TestDatabase { fn new() -> Self { - Self::with_specs(django_builtin_specs()) - } - - fn with_specs(specs: TagSpecs) -> Self { Self { storage: salsa::Storage::default(), fs: Arc::new(Mutex::new(InMemoryFileSystem::new())), - tag_specs: Arc::new(specs), } } } @@ -387,7 +339,7 @@ mod tests { #[salsa::db] impl crate::Db for TestDatabase { fn tag_specs(&self) -> crate::templatetags::TagSpecs { - (*self.tag_specs).clone() + django_builtin_specs() } fn tag_index(&self) -> TagIndex<'_> { @@ -409,6 +361,8 @@ mod tests { bits: &[String], _args: &[TagArg], ) -> Vec { + use djls_source::Db as SourceDb; + let db = TestDatabase::new(); // Build a minimal template content that parses to a tag @@ -619,54 +573,6 @@ mod tests { ); } - #[test] - fn test_optional_expr_skips_literal() { - let mut specs_map: FxHashMap = - django_builtin_specs().into_iter().collect(); - specs_map.insert( - "optional_expr".to_string(), - TagSpec { - module: "tests.optional".into(), - end_tag: None, - intermediate_tags: Vec::new().into(), - args: vec![ - TagArg::Expr { - name: "optional".into(), - required: false, - }, - TagArg::Literal { - lit: "reversed".into(), - required: true, - }, - ] - .into(), - }, - ); - let db = TestDatabase::with_specs(TagSpecs::new(specs_map)); - - let bits = ["reversed".to_string()]; - let bits_str = bits.join(" "); - let content = format!("{{% optional_expr {bits_str} %}}"); - - let path = Utf8Path::new("/optional.html"); - db.fs.lock().unwrap().add_file(path.to_owned(), content); - - let file = db.create_file(path); - let nodelist = djls_templates::parse_template(&db, file).expect("Failed to parse template"); - - crate::validate_nodelist(&db, nodelist); - - let errors: Vec<_> = - crate::validate_nodelist::accumulated::(&db, nodelist) - .into_iter() - .map(|acc| acc.0.clone()) - .collect(); - assert!( - errors.is_empty(), - "Optional expr should not consume following literal: {errors:?}" - ); - } - #[test] fn test_tag_with_no_args_rejects_extra() { // {% csrf_token extra_arg %} diff --git a/crates/djls-semantic/src/lib.rs b/crates/djls-semantic/src/lib.rs index 350516e1..90e582a5 100644 --- a/crates/djls-semantic/src/lib.rs +++ b/crates/djls-semantic/src/lib.rs @@ -1,3 +1,4 @@ +mod arguments; mod blocks; mod db; mod errors; @@ -7,6 +8,7 @@ mod semantic; mod templatetags; mod traits; +use arguments::validate_all_tag_arguments; pub use blocks::build_block_tree; pub use blocks::TagIndex; pub use db::Db; @@ -20,8 +22,6 @@ pub use resolution::resolve_template; pub use resolution::ResolveResult; pub use resolution::TemplateReference; pub use semantic::build_semantic_forest; -use semantic::validate_block_tags; -use semantic::validate_non_block_tags; pub use templatetags::django_builtin_specs; pub use templatetags::EndTag; pub use templatetags::TagArg; @@ -44,7 +44,6 @@ pub fn validate_nodelist(db: &dyn Db, nodelist: djls_templates::NodeList<'_>) { } let block_tree = build_block_tree(db, nodelist); - let forest = build_semantic_forest(db, block_tree, nodelist); - validate_block_tags(db, forest.roots(db)); - validate_non_block_tags(db, nodelist, forest.tag_spans(db)); + let _forest = build_semantic_forest(db, block_tree, nodelist); + validate_all_tag_arguments(db, nodelist); } diff --git a/crates/djls-semantic/src/semantic.rs b/crates/djls-semantic/src/semantic.rs index 99545ea7..880a054f 100644 --- a/crates/djls-semantic/src/semantic.rs +++ b/crates/djls-semantic/src/semantic.rs @@ -1,8 +1,5 @@ -mod args; mod forest; -pub(crate) use args::validate_block_tags; -pub(crate) use args::validate_non_block_tags; use forest::build_root_tag; pub(crate) use forest::SemanticForest; use rustc_hash::FxHashSet; diff --git a/crates/djls-semantic/src/templatetags.rs b/crates/djls-semantic/src/templatetags.rs index 4443bd0d..3d45e51b 100644 --- a/crates/djls-semantic/src/templatetags.rs +++ b/crates/djls-semantic/src/templatetags.rs @@ -3,7 +3,7 @@ mod specs; pub use builtins::django_builtin_specs; pub use specs::EndTag; -pub(crate) use specs::IntermediateTag; pub use specs::TagArg; +pub(crate) use specs::TagArgSliceExt; pub use specs::TagSpec; pub use specs::TagSpecs; diff --git a/crates/djls-semantic/src/templatetags/specs.rs b/crates/djls-semantic/src/templatetags/specs.rs index d1227b16..34fa2afd 100644 --- a/crates/djls-semantic/src/templatetags/specs.rs +++ b/crates/djls-semantic/src/templatetags/specs.rs @@ -69,6 +69,16 @@ impl TagSpecs { None } + /// Get the intermediate tag spec for a given intermediate tag + #[must_use] + pub fn get_intermediate_spec(&self, tag_name: &str) -> Option<&IntermediateTag> { + self.0.values().find_map(|spec| { + spec.intermediate_tags + .iter() + .find(|it| it.name.as_ref() == tag_name) + }) + } + #[must_use] pub fn is_opener(&self, name: &str) -> bool { self.0 @@ -337,6 +347,27 @@ impl TagArg { } } +/// Extension methods for slices of `TagArg`. +pub(crate) trait TagArgSliceExt { + /// Find the next literal keyword in the argument list. + /// + /// This helps expression and assignment arguments know when to stop consuming tokens. + /// For example, in `{% if expr reversed %}`, the expression should stop before "reversed". + fn find_next_literal(&self) -> Option; +} + +impl TagArgSliceExt for [TagArg] { + fn find_next_literal(&self) -> Option { + self.iter().find_map(|arg| { + if let TagArg::Literal { lit, .. } = arg { + Some(lit.to_string()) + } else { + None + } + }) + } +} + impl From for TagArg { fn from(value: djls_conf::TagArgDef) -> Self { match value.kind {