Skip to content

Restore lexer depth on the unclosed-bracket error path#791

Merged
jhpratt merged 1 commit into
time-rs:mainfrom
greymoth-jp:fix-nested-fd-panic
Jun 29, 2026
Merged

Restore lexer depth on the unclosed-bracket error path#791
jhpratt merged 1 commit into
time-rs:mainfrom
greymoth-jp:fix-nested-fd-panic

Conversation

@greymoth-jp

@greymoth-jp greymoth-jp commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Parsing a format description with an unclosed nested bracket ([a[, [optional [, [hour[) panics in debug builds via debug_assert!(self.context().is_component()), reached through consume_whitespace.

The opening bracket increments self.depth. On the branch where no closing bracket is found, the lexer returns UnclosedOpeningBracket without restoring self.depth. consume_component discards that error and re-derives the unclosed-bracket error itself, so the lexer keeps going with depth one too high; context() then reports the wrong context and the assert fails.

The fix decrements self.depth before returning, matching the entry state like the other early returns in this function. Release builds skip the assert, so the observable symptom is a debug-build panic; the underlying issue is the leftover depth on this error path.

@codecov

codecov Bot commented Jun 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.4%. Comparing base (8faa889) to head (4712e1e).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #791   +/-   ##
=====================================
  Coverage   87.4%   87.4%           
=====================================
  Files        104     104           
  Lines      13982   13983    +1     
=====================================
+ Hits       12223   12224    +1     
  Misses      1759    1759           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jhpratt

jhpratt commented Jun 26, 2026

Copy link
Copy Markdown
Member

Please rewrite the description. LLM-generated writing is not acceptable, and frankly I have no idea what it even means. There is no such thing as a "native" parser. Let me know when the description is rewritten and I will take a look.

@jhpratt jhpratt added the C-needs-details Category: more details are needed to assess the situation label Jun 26, 2026
@greymoth-jp greymoth-jp changed the title Fix native parser panic on unclosed nested bracket Restore lexer depth on the unclosed-bracket error path Jun 26, 2026
@greymoth-jp

Copy link
Copy Markdown
Contributor Author

Rewrote the description and title. You're right that "native parser" was meaningless; it's a debug_assert in the format-description lexer. The opening bracket increments depth and the unclosed-bracket error path didn't restore it, so a later context() assert trips on inputs like [a[. Happy to add a regression test covering those inputs if you'd want it in the same PR.

@jhpratt

jhpratt commented Jun 28, 2026

Copy link
Copy Markdown
Member

Yes, please add a regression test for the inputs. They can go in the parse_format_description.rs test file. Also, if you could ensure that an LLM is not listed as co-author in the commit, that would be preferred.

A component header immediately followed by an unclosed nested bracket
(`[a[`, `[hour[`) could panic in debug builds. The opening bracket
increments `self.depth`, but the unclosed-bracket error path returned
without restoring it. `consume_component` swallows that error and
re-derives the unclosed-bracket error itself, continuing with `depth`
one too high, so a later `debug_assert!(self.context().is_component())`
in `consume_whitespace` trips. Release builds skip the assert and report
the unclosed bracket as before.

Decrement `self.depth` on that error path so depth and context match the
entry state, like the other early returns in this function. Add
regression cases to `nested_v2_error_unclosed`.
@greymoth-jp greymoth-jp force-pushed the fix-nested-fd-panic branch from 07659d6 to 4712e1e Compare June 28, 2026 05:54
@greymoth-jp

Copy link
Copy Markdown
Contributor Author

Added regression cases for [a[ and [hour[ to nested_v2_error_unclosed. Without the depth restore they trip the debug_assert!(self.context().is_component()) in consume_whitespace; with it they report UnclosedOpeningBracket at index 0 like the existing cases. [optional [ was already covered there and, owing to its trailing space, skips that consume_whitespace call, so it never reached the assert. The commit no longer lists a co-author.

@jhpratt

jhpratt commented Jun 29, 2026

Copy link
Copy Markdown
Member

Looks good; thanks!

@jhpratt jhpratt merged commit 6b0d468 into time-rs:main Jun 29, 2026
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-needs-details Category: more details are needed to assess the situation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants