refactor tag validation to not depend on semantic forest #354
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe PR refactors tag argument validation from a multi-pass, block-structure-dependent approach to a unified single-pass validation over a flat NodeList. This involves relocating validation logic to a new module structure, introducing helper methods on TagSpecs, and simplifying the overall validation pathway. Changes
Sequence DiagramsequenceDiagram
actor Caller
participant lib as lib.rs<br/>validate_nodelist
participant old as OLD: Multi-Pass
participant new as NEW: Single-Pass
participant specs as TagSpecs
Caller->>lib: validate_nodelist()
rect rgb(220, 240, 255)
Note over old: Previous Flow (Removed)
lib->>old: validate_block_tags()
lib->>old: validate_non_block_tags()
old->>old: Multi-pass hierarchical<br/>tree traversal
end
rect rgb(240, 255, 220)
Note over new: New Flow (Added)
lib->>new: validate_all_tag_arguments()
new->>new: Single-pass flat<br/>NodeList iteration
new->>specs: get_intermediate_spec()
new->>specs: validate_tag_arguments()
specs-->>new: Validation result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
CodSpeed Performance ReportMerging #354 will not alter performanceComparing Summary
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/djls-semantic/src/arguments.rs(7 hunks)crates/djls-semantic/src/lib.rs(3 hunks)crates/djls-semantic/src/semantic.rs(0 hunks)crates/djls-semantic/src/templatetags.rs(1 hunks)crates/djls-semantic/src/templatetags/specs.rs(2 hunks)
💤 Files with no reviewable changes (1)
- crates/djls-semantic/src/semantic.rs
🧰 Additional context used
🧬 Code graph analysis (2)
crates/djls-semantic/src/lib.rs (3)
crates/djls-semantic/src/arguments.rs (1)
validate_all_tag_arguments(21-28)crates/djls-semantic/src/semantic.rs (1)
build_semantic_forest(11-27)crates/djls-bench/benches/semantic.rs (1)
build_semantic_forest(26-41)
crates/djls-semantic/src/arguments.rs (2)
crates/djls-semantic/src/templatetags/specs.rs (2)
name(274-284)choice(299-305)crates/djls-semantic/src/templatetags/builtins.rs (1)
django_builtin_specs(716-718)
Comment on lines
+260
to
285
| // 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); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix missing required-argument detection for multi-token expressions.
When an Expr spec consumes multiple tokens (e.g. {% custom user.id > 0 %}) and the spec requires a keyword afterwards, bit_index may end up at 3 while we only declared two arguments (Expr, required literal). The final pass currently does args.iter().skip(bit_index), so it skips three entries and never flags the missing literal. Any TagSpec that expects a keyword after an expression can now silently pass validation even though the keyword is absent.
Please track the number of spec entries that were actually satisfied and use that when checking for missing required arguments:
- let mut bit_index = 0usize;
+ let mut bit_index = 0usize;
+ let mut consumed_args = 0usize;
@@
- for (arg_index, arg) in args.iter().enumerate() {
+ 'arg_loop: for (arg_index, arg) in args.iter().enumerate() {
if bit_index >= bits.len() {
break; // Ran out of tokens to consume
}
match arg {
TagArg::Literal { lit, required } => {
let matches_literal = bits[bit_index] == lit.as_ref();
if *required {
if matches_literal {
bit_index += 1;
+ consumed_args = arg_index + 1;
} else {
ValidationErrorAccumulator(ValidationError::InvalidLiteralArgument {
tag: tag_name.to_string(),
expected: lit.to_string(),
span,
})
.accumulate(db);
- break; // Can't continue if required literal is wrong
+ consumed_args = arg_index;
+ break 'arg_loop; // Can't continue if required literal is wrong
}
} else if matches_literal {
bit_index += 1; // Optional literal matched, consume it
+ consumed_args = arg_index + 1;
} else {
+ consumed_args = consumed_args.max(arg_index + 1);
}
}
…
}
+
+ consumed_args = consumed_args.max(arg_index + 1);
}
@@
- for arg in args.iter().skip(bit_index) {
+ for arg in args.iter().skip(consumed_args) {
if arg.is_required() {
ValidationErrorAccumulator(ValidationError::MissingArgument {
tag: tag_name.to_string(),
argument: arg.name().to_string(),
span,
})
.accumulate(db);
}
}Remember to bump consumed_args appropriately in the remaining match arms whenever they succeed so we only skip the portion of the spec we truly matched.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/djls-semantic/src/arguments.rs around lines 260 to 285, the final
missing-argument check uses args.iter().skip(bit_index) which is wrong when Expr
specs consume multiple input tokens; instead maintain and use a consumed_args
counter that represents how many spec entries were actually satisfied (increment
consumed_args in each successful match arm, including when Expr consumes
multiple tokens), then replace the args.iter().skip(bit_index) usage with
args.iter().skip(consumed_args) so required arguments after multi-token Exprs
are correctly detected and reported.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes