handle variable length args in tag validation #351
Conversation
joshuadavidthomas
force-pushed
the
fix-variable-args-in-tag
branch
from
d00a407 to
bfdd17d
Compare
CodSpeed Performance ReportMerging #351 will improve performances by 16.37%Comparing Summary
Benchmarks breakdown
|
✅ Actions performedReview triggered.
|
WalkthroughReplaces naive whitespace splitting with a quote-aware tag-argument tokenizer and refactors argument validation to consume tokens per TagArg variant, preventing false-positive "accepts at most N arguments" errors for operator-containing expressions and quoted strings with spaces. Changes
Sequence Diagram(s)sequenceDiagram
participant Parser
participant Tokenizer as parse_tag_args()
participant Semantic as validate_args()
participant Validator as validate_choices_and_order()
Parser->>Tokenizer: tag content string
activate Tokenizer
Tokenizer-->>Parser: (name, tokens[])
deactivate Tokenizer
Parser->>Semantic: pass tokens[]
activate Semantic
Semantic->>Semantic: extract literal specs
Semantic->>Validator: validate order & token consumption
activate Validator
alt Token is Expr
Validator->>Validator: consume tokens greedily until next literal or end
Note right of Validator: multi-token expressions (e.g., "message.input_tokens > 0")
else Token is String or Var
Validator->>Validator: consume exactly one token
else Token is Assignment
Validator->>Validator: consume up to next literal with "as"/"=" handling
else Token is VarArgs
Validator->>Validator: consume remaining tokens
end
Validator-->>Semantic: validation result / errors
deactivate Validator
Semantic-->>Parser: report errors or success
deactivate Semantic
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
joshuadavidthomas
force-pushed
the
fix-variable-args-in-tag
branch
from
7375b18 to
90d6ac8
Compare
joshuadavidthomas
force-pushed
the
fix-variable-args-in-tag
branch
from
90d6ac8 to
e0bc4ec
Compare
![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
🧹 Nitpick comments (1)
crates/djls-semantic/src/semantic/args.rs (1)
318-750: Excellent test coverage for the core scenarios.The test suite comprehensively covers:
- Multi-token expressions (the original issue in #339)
- Quoted strings as single tokens
- Optional and required literals
- VarArgs handling
- Assignment patterns
- Zero-argument tags rejecting extras
- Complex tag structures
The
check_validation_errorshelper (lines 390-431) properly constructs templates with appropriate closing tags for validation.Consider adding tests for the edge cases noted in previous comments:
- An
Exprfollowed by aLiteralwhen only the literal token is provided- An
Assignmentfollowed by aLiteralwhere the assignment consumes the literal (verifying the fix for the major issue flagged earlier)These would help prevent regressions and clarify the intended behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)crates/djls-semantic/src/semantic/args.rs(4 hunks)crates/djls-templates/src/parser.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🧰 Additional context used
🧬 Code graph analysis (2)
crates/djls-templates/src/parser.rs (1)
crates/djls-templates/src/nodelist.rs (1)
span(41-49)
crates/djls-semantic/src/semantic/args.rs (4)
crates/djls-semantic/src/templatetags/builtins.rs (1)
django_builtin_specs(716-718)crates/djls-semantic/src/templatetags/specs.rs (1)
name(264-274)crates/djls-semantic/src/db.rs (4)
tag_specs(12-12)tag_index(14-14)template_dirs(16-16)diagnostics_config(19-19)crates/djls-semantic/src/lib.rs (1)
validate_nodelist(41-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Python 3.14, Django 5.2 (ubuntu-latest)
- GitHub Check: Python 3.13, Django 6.0a1 (ubuntu-latest)
- GitHub Check: Python 3.14, Django 6.0a1 (ubuntu-latest)
- GitHub Check: Python 3.13, Django main (ubuntu-latest)
- GitHub Check: Python 3.11, Django 4.2 (ubuntu-latest)
- GitHub Check: Python 3.14, Django main (ubuntu-latest)
- GitHub Check: Python 3.13, Django 5.2 (ubuntu-latest)
- GitHub Check: Python 3.12, Django 4.2 (ubuntu-latest)
- GitHub Check: Python 3.14, Django 4.2 (ubuntu-latest)
- GitHub Check: Python 3.14, Django 5.1 (ubuntu-latest)
- GitHub Check: Python 3.12, Django 5.2 (ubuntu-latest)
- GitHub Check: Python 3.13, Django 5.1 (ubuntu-latest)
- GitHub Check: Python 3.11, Django 5.2 (ubuntu-latest)
- GitHub Check: Python 3.12, Django 6.0a1 (ubuntu-latest)
- GitHub Check: Python 3.13, Django 4.2 (ubuntu-latest)
- GitHub Check: Python 3.10, Django 5.2 (ubuntu-latest)
- GitHub Check: Python 3.10, Django 5.1 (ubuntu-latest)
- GitHub Check: Python 3.12, Django 5.1 (ubuntu-latest)
- GitHub Check: Python 3.11, Django 5.1 (ubuntu-latest)
- GitHub Check: benchmarks
🔇 Additional comments (8)
crates/djls-templates/src/parser.rs (2)
88-88: LGTM: Clean delegation to quote-aware parser.The refactor delegates tag argument parsing to the new
parse_tag_argshelper, which implements quote-aware tokenization to fix the variable-length argument issue.
94-130: Quote-aware tokenization looks solid.The implementation correctly:
- Preserves quoted strings as single tokens (lines 108-114)
- Handles escape sequences within quotes (lines 102-107)
- Splits on whitespace only outside quotes (lines 116-120)
- Handles edge cases like trailing tokens (lines 124-126) and empty tags (line 128)
One minor note: Unclosed quotes will cause the remainder of the content to be treated as a single token (the
in_quotestate remainsSome). This is reasonable for error recovery, though it means malformed quotes won't be explicitly flagged here.crates/djls-semantic/src/semantic/args.rs (6)
147-157: Appropriate setup for enhanced validation logic.The
clippy::too_many_linesallow is justified given the complexity of per-argument-type validation. Usingenumerateto trackarg_indexenables the subsequentfind_next_literallookups.
204-207: LGTM: Explicit single-token consumption for atomic arguments.
VarandStringarguments correctly consume exactly one token each, making the validation behavior explicit and clear.
208-231: Expression consumption logic handles the core issue correctly.The greedy consumption until the next literal (lines 218-225) correctly solves the problem where
{% if message.input_tokens > 0 %}was incorrectly flagged as too many arguments.Minor edge case: If an
Expris immediately followed by aLiteralin the spec, and the only remaining token is that literal, lines 228-230 will consume the literal as part of the expression. This creates an error state where both the expression and literal end up incorrect. However, this is an unlikely scenario in practice since well-formed templates would provide tokens for both required arguments.Consider adding a test for this edge case:
// {% mytag literal %} where spec expects [Expr(required), Literal("literal", required)] let bits = vec!["literal".to_string()]; let args = vec![ TagArg::Expr { name: "expr".into(), required: true }, TagArg::Literal { lit: "literal".into(), required: true }, ]; // What error should this produce?
263-267: LGTM: VarArgs correctly consumes all remaining tokens.Setting
bit_index = bits.len()ensures that variable-length argument tags (likeurlandload) never trigger the "too many arguments" check at line 271.
270-281: Correct fix: Check for unconsumed tokens after processing all arguments.This is the core fix for issue #339. After processing all argument specs, any remaining tokens indicate too many arguments were provided. The
max: args.len()represents the number of argument positions in the spec (not the number of tokens), which correctly reflects how Django counts "arguments."Note that
VarArgssetsbit_index = bits.len()(line 265), so this check is never reached for tags with variable-length arguments.
295-316: LGTM: Helper functions are straightforward and correct.
argument_name(lines 295-305): Extracts the name from anyTagArgvariant for error messages.find_next_literal(lines 307-316): Locates the nextLiteralkeyword in the remaining arguments, enablingExprandAssignmentarguments to know when to stop consuming tokens.Both helpers are implemented correctly and serve their purposes well.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/djls-semantic/src/semantic/args.rs (1)
232-263: Assignment "as" handling can still consume the next literal keyword.While the initial check (lines 241-245) correctly validates before consuming (addressing part of the previous review), the "as" handling (lines 255-261) still consumes the variable name without checking if it matches
next_literal. This can swallow the literal keyword, making it unavailable for subsequentLiteralarguments.Apply this diff to check before consuming the variable name after "as":
// If we hit "as", consume one more token (the variable name) if token == "as" { if bit_index < bits.len() { - bit_index += 1; + // Don't consume if it's the next literal keyword + if let Some(lit) = next_literal { + if bits[bit_index] != lit { + bit_index += 1; + } + } else { + bit_index += 1; + } } break; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/djls-semantic/src/semantic/args.rs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/djls-semantic/src/semantic/args.rs (4)
crates/djls-semantic/src/templatetags/builtins.rs (1)
django_builtin_specs(716-718)crates/djls-semantic/src/templatetags/specs.rs (1)
name(264-274)crates/djls-semantic/src/db.rs (4)
tag_specs(12-12)tag_index(14-14)template_dirs(16-16)diagnostics_config(19-19)crates/djls-semantic/src/lib.rs (1)
validate_nodelist(41-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Python 3.14, Django 5.1 (ubuntu-latest)
- GitHub Check: Python 3.14, Django 5.2 (ubuntu-latest)
- GitHub Check: Python 3.14, Django main (ubuntu-latest)
- GitHub Check: Python 3.14, Django 6.0a1 (ubuntu-latest)
- GitHub Check: Python 3.13, Django 6.0a1 (ubuntu-latest)
- GitHub Check: Python 3.12, Django 4.2 (ubuntu-latest)
- GitHub Check: Python 3.10, Django 5.1 (ubuntu-latest)
- GitHub Check: Python 3.13, Django 4.2 (ubuntu-latest)
- GitHub Check: Python 3.13, Django 5.1 (ubuntu-latest)
- GitHub Check: Python 3.13, Django 5.2 (ubuntu-latest)
- GitHub Check: Python 3.10, Django 5.2 (ubuntu-latest)
- GitHub Check: Python 3.12, Django 6.0a1 (ubuntu-latest)
- GitHub Check: Python 3.12, Django 5.2 (ubuntu-latest)
- GitHub Check: Python 3.12, Django main (ubuntu-latest)
- GitHub Check: Python 3.11, Django 5.2 (ubuntu-latest)
- GitHub Check: Python 3.10, Django 4.2 (ubuntu-latest)
- GitHub Check: Python 3.11, Django 4.2 (ubuntu-latest)
- GitHub Check: Python 3.12, Django 5.1 (ubuntu-latest)
- GitHub Check: Python 3.11, Django 5.1 (ubuntu-latest)
- GitHub Check: benchmarks
🔇 Additional comments (3)
crates/djls-semantic/src/semantic/args.rs (3)
271-282: Correct handling of excess arguments.The logic correctly identifies unconsumed tokens after processing all argument specs as a
TooManyArgumentserror. TheVarArgscase is properly handled by consuming all remaining tokens, preventing false positives.
308-317: Clean helper implementation.The
find_next_literalhelper is straightforward and correctly identifies the next literal keyword in the argument list, enabling expression and assignment arguments to know when to stop token consumption.
319-751: Excellent test coverage addressing the core issue.The test suite comprehensively validates the new token consumption logic:
- Core fix verification:
test_if_tag_with_comparison_operator(lines 435-454) directly tests issue #339, ensuring multi-token expressions with operators no longer trigger false-positive S105 errors.- Edge cases: Tests cover quoted strings, optional literals, complex expressions, assignments with filters, VarArgs, and literal boundary conditions.
- Regression prevention: Tests for no-arg tags (
csrf_token,debug) and fixed-arg tags (autoescape,now,regroup) ensure the refactored logic doesn't incorrectly accept extra arguments.The test infrastructure using
TestDatabaseandcheck_validation_errorsproperly exercises the validation path through real Django builtin specs.
closes #339
Summary by CodeRabbit
Bug Fixes
Tests
Documentation
Refactor