Skip to content

fix: an expression too deep to walk is refused rather than crashing - #359

Merged
FBumann merged 4 commits into
mainfrom
claude/package-performance-zmbv5a-fixes
Sep 2, 2026
Merged

fix: an expression too deep to walk is refused rather than crashing#359
FBumann merged 4 commits into
mainfrom
claude/package-performance-zmbv5a-fixes

Conversation

@FBumann

@FBumann FBumann commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Prompt: Just fix right away with a pr?

Note

The following content was generated by AI.

What this changes

Two contradictions in the tree, found while investigating load performance (#357, #358) and independent of both — this branches from main.

to_spec raised RecursionError where it documents LanguageError. A 400-term sum is enough, and that is a model a generator writes rather than a pathological input. What a reader got was a traceback through expansion._descend naming no file, no declaration, and nothing to write instead.

There are two failure modes and one limit. A long chain parses fine and builds a tree the passes over it cannot recurse — expansion spends three Python frames per node. Parentheses nested that far exhaust the stack inside pyparsing, before a tree exists to measure. Both now raise a language error carrying one rewrite:

Constraint 'k': The expression nests 401 deep, past the 100 levels the language admits: 'x + x + x + x + …'
Reduce over a dimension with sum() rather than writing the terms out, or name an intermediate quantity under
expressions: and refer to it by name.

where: strings are bounded the same way, with their own rewrite.

expressions.md published a grammar that refused what the language accepts. NAME ::= [a-zA-Z][a-zA-Z0-9_]*, while expression_parser.NAME has always admitted a leading underscore — the same constant model.py:813 validates every declaration name against, whose own error message says "a letter or an underscore". _x + 1 parsed. The page is corrected rather than the code, because that constant is the single home and the schema's message already stated the intended rule.

Merged with main (#357 memoised both parsers and gave them one parse_text). The guard lives in that helper: each parser passes its child function and its rewrite, so the memo, the parse failure and the depth refusal are one code path for both grammars. expression_parser is _expression_parser since #342.

Why

Both are the tree disagreeing with itself, which is the one thing this repository sells. A crash is not a refusal, and a published grammar that is not the implemented one is worse than no grammar.

The cap is 100, and it is a deliberate break. An expression nesting 101–300 deep loaded before and is refused now.

How 100 was chosen
depth
deepest expression in this repository 18
median 3
whole pipeline (to_specto_programto_latex) still fine 300
pipeline raises RecursionError 350
the cap 100

The gap between 100 and 300 is the room a caller's own frames need. A guard that only holds when to_spec is called from a shallow stack has not done its job — the failure point otherwise depends on the consumer's call depth and on sys.getrecursionlimit(), neither of which this package controls.

And a chain long enough to reach it is precisely what sum() over a dimension exists to replace, so the refusal points at the language's own answer before the general one.

A claim in #358 that this corrects

#358's body and README say the RecursionError is the parser's, and that "a depth guard cannot be added inside pyparsing; a parser we own is what would make it fixable." That is wrong about the case that matters. parse_expression handles a 1000-term chain without trouble; the recursion is in our own tree walks, and the fix needed no new parser. Only the nested-parentheses variant fails inside pyparsing, and catching RecursionError at the front door covers it. I have commented the correction on #358.

What was verified
  • pytest — 1015 passed, 5 skipped, on 3.13 and on the 3.12 floor.
  • Every new guard test fails without the guard. Removing the depth check and the RecursionError arm fails all six depth cases (three in test_parser.py, three in test_validation.py); reverting the doc line fails the new drift test in test_docs.py. Both were run.
  • ruff check, ruff format --check, pyrefly check (0 errors, 9 suppressed — same as main), and the real prettier on the changed page.
  • Regenerating tools.schema and the typesetter goldens produces zero drift.
  • examples/pypsa.yaml and every other model still load unchanged.
  • After the merge with main: pixi run test (1031 passed), docs-build and compile-tex pass in a fresh worktree, and so does every lint job but pyrefly, which reports three types-PyYAML stub errors there — origin/main checked out in the same worktree reports the same three, so that is the fresh environment rather than this diff.
  • Removing the guard from the merged parse_text fails all six depth tests, re-run on the merged tree.
Scope, and two defaults departed from
  • Two commits, two types, one PR. fix for the crash, docs for the drift. They are separable and would normally stack, but both are one-line-of-investigation findings and each is small; say the word and I will split them.
  • MAX_DEPTH and the rewrite strings are inline, as caps and messages the code acts on. The depth numbers behind the cap live in this PR; the constant's comment carries the conclusion with the ref, and the docs page and the error message carry only the limit and the rewrite.
  • test_a_name_may_open_with_an_underscore is a pin, not a guard — it passes on the unfixed tree, because the code was already right. The actual guard for that finding is test_the_published_grammar_spells_a_name_the_way_the_code_reads_one, which does fail on the old page.
  • Deliberately not done: making the walks in expansion, resolution and typesetting iterative. That removes the ceiling rather than guarding it, and it is a much larger change than a contract fix warrants.

🤖 Generated with Claude Code

https://claude.ai/code/session_01HSgvWYbQLFB8GkLdiJSyuv


Generated by Claude Code

`to_spec` documents `Raises: LanguageError`, and a 400-term sum raised
`RecursionError` instead — a traceback through `expansion._descend`, with
nothing naming the file, the declaration or what to write. That is a model
a generator writes, not a pathological input.

Two failures, one limit: a long chain parses and builds a tree the passes
over it cannot recurse, while parentheses nested that far exhaust the stack
inside pyparsing before a tree exists to measure. Both now raise a language
error carrying the one rewrite — reduce over a dimension with sum(), or
name the quantity under expressions:. Where strings are bounded the same
way and get their own rewrite.

The cap is 100, measured against a deepest-in-repository of 18 and a
pipeline that survives 300 on a default stack; the gap is the room a
caller's own frames need, since a guard that only holds for a shallow
caller has not done its job. An expression nesting 101 to 300 deep loaded
before and is refused now.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HSgvWYbQLFB8GkLdiJSyuv
`expressions.md` published `NAME ::= [a-zA-Z][a-zA-Z0-9_]*` while
`expression_parser.NAME` has always admitted a leading underscore — the
same constant `model.py` validates every declaration name against, whose
own error says "a letter or an underscore". So the page refused what the
language accepts, and `_x + 1` parsed.

The page is corrected rather than the code: one constant is the single
home, and the schema's message already stated the intended rule. A test
compares the published production against that constant, because nothing
did and that is why it drifted.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HSgvWYbQLFB8GkLdiJSyuv
@read-the-docs-community

read-the-docs-community Bot commented Sep 1, 2026

Copy link
Copy Markdown

FBumann and others added 2 commits September 2, 2026 11:49
The depth guard moves into the shared parse_text helper main introduced in
#357, so both memoised parsers refuse an over-deep tree through one call.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SY1PzqSCRpFZsZ2y8iErwv
…guing for them

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SY1PzqSCRpFZsZ2y8iErwv
@FBumann
FBumann merged commit d6cee25 into main Sep 2, 2026
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants