finish migrating to tagspec v0.6.0 #356
Open
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
WalkthroughThis PR refactors the TagArg enum to enhance semantic clarity and validation: renames Var/Expr to Variable/Any, introduces TokenCount and LiteralKind enums, augments variants with metadata, and adds new constructor functions. Updates span completions, snippets, semantic validation, and built-in tag definitions. Adds a nox session for Django tag analysis. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Key areas requiring extra attention during review:
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
CodSpeed Performance ReportMerging #356 will not alter performanceComparing Summary
|
joshuadavidthomas
force-pushed
the
tagspec-v0.6.0-v2
branch
from
d37019f to
e265ed1
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/djls-semantic/src/arguments.rs (1)
225-263: Stop assignment args from eating required literals and add Exact bounds checks.The assignment branch has the same Exact-token gap as above, and its greedy loop consumes the “next literal” sentinel before checking it. For inputs like
{% with total=items|length only %}the current code swallowsonly, so the required literal never validates and we falsely succeed. Please (1) add the availability check forTokenCount::Exact, and (2) inspect the next literal before incrementingbit_indexso we bail out without consuming it.- TagArg::Assignment { count, .. } => { - match count { - crate::templatetags::TokenCount::Exact(n) => { - // Consume exactly N tokens - bit_index += n; - } - crate::templatetags::TokenCount::Greedy => { + TagArg::Assignment { + name, + required, + count, + } => { + match count { + crate::templatetags::TokenCount::Exact(n) => { + let available = bits.len().saturating_sub(bit_index); + if available < n { + if *required { + ValidationErrorAccumulator(ValidationError::MissingArgument { + tag: tag_name.to_string(), + argument: name.to_string(), + span, + }) + .accumulate(db); + return; + } + continue; + } + + // Consume exactly N tokens + bit_index += n; + } + crate::templatetags::TokenCount::Greedy => { // 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 = args[arg_index + 1..].find_next_literal(); - while bit_index < bits.len() { - let token = &bits[bit_index]; - bit_index += 1; - - // If token contains =, we've found the assignment - if token.contains('=') { - break; - } - - // 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; - } - } - } + while bit_index < bits.len() { + if let Some(ref lit) = next_literal { + if bits[bit_index] == *lit { + break; + } + } + + let token = &bits[bit_index]; + + // If token contains =, we've found the assignment + if token.contains('=') { + bit_index += 1; + break; + } + + // If we hit "as", consume the variable name after it + if token == "as" { + bit_index += 1; + if bit_index < bits.len() { + bit_index += 1; // Consume the variable name + } + break; + } + + bit_index += 1; + } } } }
🧹 Nitpick comments (1)
crates/djls-ide/src/completions.rs (1)
610-613: Refresh the comment to match the renamed variantThe comment still references the old
Exprvariant; updating it toAnywill keep the guidance aligned with the current TagArg naming.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
crates/djls-ide/src/completions.rs(1 hunks)crates/djls-ide/src/snippets.rs(5 hunks)crates/djls-semantic/src/arguments.rs(8 hunks)crates/djls-semantic/src/lib.rs(1 hunks)crates/djls-semantic/src/templatetags.rs(1 hunks)crates/djls-semantic/src/templatetags/builtins.rs(20 hunks)crates/djls-semantic/src/templatetags/specs.rs(10 hunks)noxfile.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
crates/djls-ide/src/snippets.rs (1)
crates/djls-semantic/src/templatetags/specs.rs (4)
var(382-388)syntax(357-363)modifier(366-372)expr(339-345)
crates/djls-semantic/src/arguments.rs (1)
crates/djls-semantic/src/templatetags/specs.rs (6)
expr(339-345)var(382-388)syntax(357-363)modifier(366-372)assignment(407-413)varargs(399-404)
Comment on lines
+197
to
223
| TagArg::Variable { count, .. } | TagArg::Any { count, .. } => { | ||
| match count { | ||
| crate::templatetags::TokenCount::Exact(n) => { | ||
| // Consume exactly N tokens | ||
| bit_index += n; | ||
| } | ||
| crate::templatetags::TokenCount::Greedy => { | ||
| // Greedy: consume tokens until next literal or end | ||
| let start_index = bit_index; | ||
| let next_literal = args[arg_index + 1..].find_next_literal(); | ||
|
|
||
| while bit_index < bits.len() { | ||
| if let Some(ref lit) = next_literal { | ||
| if bits[bit_index] == *lit { | ||
| break; // Stop before the literal | ||
| } | ||
| } | ||
| bit_index += 1; | ||
| } | ||
|
|
||
| // Consume tokens greedily until we hit a known literal | ||
| while bit_index < bits.len() { | ||
| if let Some(ref lit) = next_literal { | ||
| if bits[bit_index] == *lit { | ||
| break; // Stop before the literal | ||
| // Ensure we consumed at least one token | ||
| if bit_index == start_index { | ||
| bit_index += 1; | ||
| } | ||
| } | ||
| bit_index += 1; | ||
| } | ||
|
|
||
| // Ensure we consumed at least one token for the expression | ||
| if bit_index == start_index { | ||
| bit_index += 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
Prevent silently accepting too few tokens for Exact arguments.
TokenCount::Exact(n) blindly bumps bit_index even when there are < n tokens left. In that case we step past bits.len() and the later “missing argument” scan never fires, so a required argument that only received a partial token sequence is treated as valid (e.g. {% with value as %} slips through). Please gate consumption on the available token count and surface a MissingArgument for required args; optional args should simply leave the remaining tokens untouched.
- TagArg::Variable { count, .. } | TagArg::Any { count, .. } => {
- match count {
- crate::templatetags::TokenCount::Exact(n) => {
- // Consume exactly N tokens
- bit_index += n;
- }
+ TagArg::Variable {
+ name,
+ required,
+ count,
+ }
+ | TagArg::Any {
+ name,
+ required,
+ count,
+ } => {
+ match count {
+ crate::templatetags::TokenCount::Exact(n) => {
+ let available = bits.len().saturating_sub(bit_index);
+ if available < n {
+ if *required {
+ ValidationErrorAccumulator(ValidationError::MissingArgument {
+ tag: tag_name.to_string(),
+ argument: name.to_string(),
+ span,
+ })
+ .accumulate(db);
+ return;
+ }
+ continue;
+ }
+
+ // Consume exactly N tokens
+ bit_index += n;
+ }🤖 Prompt for AI Agents
In crates/djls-semantic/src/arguments.rs around lines 197–223, the
TokenCount::Exact(n) branch unconditionally advances bit_index by n and can run
past bits.len(), letting required args silently accept too few tokens; change
this to first compute available = bits.len() - bit_index, and if available < n
then: for required TagArg variants return/surface a MissingArgument error (do
not advance bit_index), and for optional variants leave bit_index unchanged (do
not consume any tokens). Otherwise (available >= n) advance bit_index by n but
never beyond bits.len(). Ensure no panics from out-of-bounds indexing and that
the greedy branch behavior is unchanged.
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
Improvements
Tools