Keep the expression pass off fork_pointer - #65
Merged
Conversation
A fork_pointer holds a node id, and a node id is a number, so the pass that
replaces every evaluable scalar with its value "evaluated" it too and wrote
the result back through format_double -- the shortest text that round-trips
as a double. For most ids that is the digits themselves, but an id of 110
comes back as `1.1e+02`.
Every reader of the pointer parses it with std::stoull, which stops at the
`.` and yields 1, so one such id broke three things at once and only the
last of them said anything:
- run_element_bookkeeper seeded reference/floor propagation on node 1, so
the destination branch inherited nothing from the Fork.
- link_fork_from found node 1 was not a map and moved on, so the
destination silently lost its ForkFromP entry.
- remap_fork_pointers reported the target as outside the expanded lattice.
The fix is to leave the pointer alone: non_expr_keys() already skips the
Fork's other bookkeeping keys (to_line, destination_element, new_branch,
propagate_reference), and fork_pointer was missed only because expansion
creates it rather than the user writing it. A node id is not a physics
parameter and has no business going through the evaluator, which is a better
place to fix this than making three call sites parse defensively.
The regression test needs an id in the range that formats as an exponent --
a multiple of ten from 100 up -- so it carries three unused padding elements
that shift the work-tree ids until two of its five destinations land there.
Without them every pointer comes out in plain digits and the bug hides.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
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.
A
fork_pointerholds a node id, and a node id is a number, sosubstitute_values— the pass that replaces every evaluable scalar with its value — "evaluated" it too and wrote the result back throughformat_double, the shortest text that round-trips as a double:which is why only some pointers looked wrong: an id has to be a multiple of ten from 100 up to reach exponent form. The value is still numerically right — but every reader parses it with
std::stoull, which stops at the.and returns 1.Three readers, three failures, one message
stoull("1.1e+02") == 1run_element_bookkeeperfork_seed[1] = def— the destination branch inherits no reference or floor from the Forklink_fork_fromcontinue— the destination silently loses itsForkFromPentryremap_fork_pointersfork_pointer target is outside the expanded latticeThe fix
Add
fork_pointertonon_expr_keys(). Its siblingsto_line,destination_element,new_branchandpropagate_referencewere already there; this one was missed because expansion creates it rather than the user writing it, so it never looked like a value the expression pass would see. A node id is not a physics parameter and has no business going through the evaluator — a better place to fix this than making three call sites parse defensively.The regression test
It reproduces a chained-and-cross-linked fork lattice and asserts no problems, five all-digit pointers, and five
ForkFromPgroups.The first version of it passed with the fix reverted: every id happened to land outside the exponent-form range. It now carries three unused
padDrifts that shift the work-tree ids until two of the five destinations land in it. Verified both ways — fails with the fix reverted (lat.problems.count == 0reports 1), passes with it in. The assertions hold whatever the ids turn out to be; they just stop exercising this particular bug if an unrelated change shifts them again, which the comment says.153/153.
🤖 Generated with Claude Code