bc-3rjan: B7e-def gives a function the wrong extent when its body holds a regex… - #679
Open
mordam wants to merge 5 commits into
Open
bc-3rjan: B7e-def gives a function the wrong extent when its body holds a regex…#679mordam wants to merge 5 commits into
mordam wants to merge 5 commits into
Conversation
The brace walk that decides where a definition ends is hand-rolled, and a
regex literal, a `{2,3}` quantifier or an escaped backtick in a nested
template each hand it a character that does not mean what it looks like —
`normalizeEntry` in lib/beadfiles.js was reported as 129-525 for a function
that ends at 136, because the `"` inside `/^["']|["']$/` opened a string
that closed at some later unrelated quote.
The walk now runs over `blankForBraceWalk(text)`: the same file, same
length, same line numbers, with every comment, string, template and regex
literal spaced out off an `acorn` parse, so it never meets a quote, a
backtick or a `/` at all. `${ … }` expressions stay, since their braces are
balanced. acorn is imported optionally — it is a devDependency and this
command is on the default tool list — and without it, or on a file that
will not parse, the walk reads the raw text exactly as before.
82 of 3,118 top-level function declarations disagreed with acorn's own
`loc`; none do now, pinned in test/b7edef.mjs over the whole tree.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
A beadcause worker opened this and does not merge its own work. It is on the merge queue as bc-khxlq. |
Owner
Author
|
The beadcause merge queue tried to merge this and could not: it has been tried 3 times and stopped at the same place each time. the branch still conflicts with |
…nt-3rjan # Conflicts: # bin/b7e-def
Contributor
|
beadcause-resolver: stood down — worktree-b7e-def-extent-3rjan is locked by another live resolver (pid 91632, reason "resolver pid 91632 #679"); left the tree untouched. |
…nt-3rjan # Conflicts: # bin/b7e-def
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.
bin/b7e-defnow measures where a definition ends off a realacornparse instead ofoff a hand-rolled scan, so a regex literal or a nested template in the body can no longer
run the reported span past the end of the function. The walk itself is unchanged — what
changed is what it walks:
blankForBraceWalk(text)hands it the same file, same length,same line numbers, with every comment, string, template and regex literal spaced out, so
it never meets a quote, a backtick or a
/at all.${ … }expressions are left as code,because their braces are balanced.
The reproduction on the bead:
node bin/b7e-def normalizeEntryreportedlib/beadfiles.js:129-525for a function that ends at 136, because the"inside/^["']|["']$/on line 130 was read byskipStringas a string opener — the string then"closed" at some later unrelated quote and the brace count desynced from there. A
{2,3}quantifier unbalances the same counter, and an escaped backtick in a nested template
(real,
lib/amendment.js) endsskipStringin the wrong place again. Over the fivedirectories this command searches, 82 of 3,118 top-level function declarations had an end
line
acorndisagreed with; none do now, andtest/b7edef.mjspins exactly that checkover the whole tree — it is the check to run against any future port of this walk.
Two decisions worth arguing with.
acornis imported optionally (await import, intoa
letthat staysnullon failure): it is a devDependency andb7e-defis onDEFAULT_TOOL_LIST, so an agent may well run it from a fresh worktree with nonode_modules— without it the blanking pass hands back the raw text and the walk isexactly as good, and as wrong, as it was before. Same fallback for a file
acorncannotparse. Both paths have a check. The fix is ported into
bin/b7e-defrather than sharedwith
lib/callers.js, which has the same code from #622 — that PR has not merged, soimporting it would be a dependency on an unmerged branch, and this bead is filed against
this file. Two copies of the walk is the state #622 already left; a third source of truth
would be worse than either.
Two performance changes came with it, because parsing per call is not free.
definitionsFornow returns early when the file does not contain the name as a substringat all (weaker than the
\b…\bevery head applies, so it cannot drop a match) — that iswhat keeps
b7e-def loadBeadat 0.5s rather than 1.8s against 0.27s before. AndblankForBraceWalkkeeps a one-slot memo, because callers ask about many names in onefile:
lib/server.jsalone holds a few hundred top-level functions, and re-parsing it pername cost the new whole-tree check 52 seconds where the memo costs it 20.
It conflicts with #678 (bc-dgx7.36), on one line, and I checked the resolution. That PR
fixes a different bug in the same walk — a call passing an object literal read as a phantom
method definition — and both branches rewrite the same
findBodycall site.git merge-treegives exactly one conflict hunk; resolving it tofindBody(scan, afterHeadChar, { strict: Boolean(head.verifyBody) })(both sides' changes,neither dropped) merges
test/b7edef.mjscleanly and the combined suite is green — thephantom
lib/server.js:3320foropenReviewAnswerSessionis gone and all 3,118 extentsstill agree with
acorn. Whichever of the two lands second wants that one line.What I am not sure of: the region-slice lift in
test/b7edef.mjs.b7e-defis anextensionless bin with no exports, so the whole-tree check runs
definitionsForout of anode:vmrather than spawning the CLI three thousand times, and the slice is anchored ontwo section-marker comments. It fails loudly if they move — the slice stops parsing or the
function comes back undefined, and the suite says so — but it is still a static read of the
source's shape, and a cleaner answer would be to lift the scanner into a
lib/module once#622 has landed and both copies can become one.
Tests: node bin/b7e-gate: 419/420. The one red, test/teardown.mjs, was a ten-hour-old orphaned headless Chrome (PPID 1, from another session's gate run, scratch dir BkqaKe vs mine pEB3g9) that its 'no Chrome outlives a killed check' assertion counts process-wide; killed it and the suite is 7/7. Four edits landed after the gate started (README prose, the substring short-circuit, the one-slot memo, two comment corrections), so I re-ran every suite that reads README.md plus b7edef/filter/teardown — 34 suites, all green — against the exact committed tree. Also trial-merged #678 with git merge-tree: one conflict line, resolved to keep both sides, combined suite green.
Worth knowing: It conflicts with #678 (bc-dgx7.36) on one line — both branches rewrite the same findBody call site in bin/b7e-def. Resolve to findBody(scan, afterHeadChar, { strict: Boolean(head.verifyBody) }), which keeps both sides; I verified that resolution merges test/b7edef.mjs cleanly and leaves both bugs fixed and the suite green. Second risk: bin/b7e-def now imports acorn, a devDependency, so a worktree with no node_modules takes the raw-text fallback — that path has a check, but it means the command is silently less accurate there rather than loudly broken.
Left undone: Did not unify the two copies of this walk. lib/callers.js has the same code from #622, which is still open, so importing it would be a dependency on an unmerged branch — worth doing as its own bead once #622 lands. Did not touch bc-dgx7.36's phantom-definition bug; that is #678's, already delivered.
Files changed — 3 files · +296 −9 · against `main`
Opened by a beadcause worker session on bc-3rjan — b7e-def gives a function the wrong extent when its body holds a regex or a nested template. It merges itself once the checks report; merging is what closes the bead. If this is still open, something stopped that, and the reason is on bc-3rjan and in Adam's inbox.
bead: bc-3rjan