Repository navigation
Conversation
`rgctl slice` needs a function to slice within. Without `--function` it guessed
one from the file stem (`path.file_stem()`, and the literal `"process"` for
Python). A file name is not a defensible default: files routinely hold several
functions and the stem may match none of them. Two failure modes followed.
Stem matches nothing — the function is invented from the file name:
// src/probe.ts holds `calc`
slice src/probe.ts --line 3 --variable y
before: Error: Resource not found: function 'probe' not found
after: { "lines": [2, 3] }
Stem matches one function but the line is in another — the error blames the
wrong thing:
// useAgentPositions.ts holds useAgentPositions (1-4) and fetchPositions (6-10)
slice src/useAgentPositions.ts --line 8 --variable total
before: no PDG node for total at line 8 # blames the variable and line
after: { "lines": [7, 8] }
The second is the worse one. It names the variable and the line — the two things
the caller supplied and which are both correct — while the real cause is the
function they never supplied and did not know was being guessed. It reads as a
broken PDG, so the natural response is to try other lines and variables rather
than to pass `--function`, which the error never mentions.
Resolve the function that contains the criterion line, using the existing
`index_function_locations` index. `--function` still wins when given. Nested
functions mean several spans can contain a line, so take the innermost (smallest
span). If no span contains the line, keep the historical stem fallback, so
behaviour changes only where the old default was plainly wrong.
Also name the function that was searched when a criterion lookup fails. The
slicer reports `no PDG node for <var> at line <N>` with no mention of the
function, so the failure is unattributable; it now says which function was
searched, whether it came from `--function` or from the line, and which functions
the file actually defines. Unrelated errors pass through unchanged.
Tests: line-start mapping including 1-based and out-of-range, resolution across
two functions, innermost-span preference for nested functions, no-match, and the
error annotation (including the explicit-`--function` wording and the
pass-through case). Verified end to end as well — the four commands above return
the lines shown.
Signed-off-by: Sami Jan <sami.jan@gmail.com>
…monstrate
Two rows were out of date after the rebase onto main:
- `dashboard/dist` regen was marked **blocked**, "source fixes are not in the
embedded bundle until dist is rebuilt". That landed in 8f09655 as
sshaaf#110. The row now says done, and says why it mattered: `dist` is
compile-time embedded, so until it is rebuilt the source fixes are invisible in
a built binary while every test passes.
- Change 7 was blocked on two `rgctl slice` defects. Both are now fixed and under
review (issues sshaaf#113/sshaaf#114, PRs sshaaf#115/sshaaf#116), so the row records what actually
gates it.
Also adds open question 4, because the more interesting problem is a gap rather than
a stale value: the maintainer's parking comment on sshaaf#61 names the code browser as the
missing piece, and no change in this sequence is the code browser. Change 1 is the
shell, changes 2-6 are rooms, and none renders source. The prototype does not cover
for it either — the Code lens shows a count of source lines, not source, and Spine
renders an empty state. The one requirement singled out is the one this design
cannot demonstrate.
Recorded as an open question, not invented as a change number: folding source
rendering into change 1 versus giving it its own change ahead of the rooms is a
scoping decision, not a correction.
Signed-off-by: Sami Jan <sami.jan@gmail.com>
sshaaf
left a comment
There was a problem hiding this comment.
Thanks for this, Sami — really clear write-up of the failure mode, and the fix matches how people actually use slice (point at a line, don’t invent a function from the filename).
I ran the new cli::slice::tests locally: 7/7 passed. The error annotation that names the searched function (and lists candidates) is a genuine UX win.
One small correctness niggle inline below on the span check — happy to land this once that’s a half-open range. Appreciate the careful tests and the “leave unrelated errors alone” coverage.
| ) -> Option<String> { | ||
| locations | ||
| .iter() | ||
| .filter(|(_, loc)| loc.start_byte <= offset && offset <= loc.end_byte) |
There was a problem hiding this comment.
Tiny span boundary note: FunctionLocation::end_byte is documented as exclusive (see cfg_builder.rs). Using offset <= loc.end_byte makes the range closed on the right.
At a boundary where function A’s exclusive end equals function B’s start, A can also match, and min_by_key might pick the wrong one.
Suggested:
.filter(|(_, loc)| loc.start_byte <= offset && offset < loc.end_byte)A tiny adjacent-functions test would lock that in if you fancy — not blocking if you prefer to keep this PR lean.
| SliceDirection::Backward => BackwardSlicer::new(&pdg, &cfg).slice(criterion.clone()), | ||
| SliceDirection::Forward => ForwardSlicer::new(&pdg, &cfg).slice(criterion.clone()), | ||
| } | ||
| .map_err(|e| name_the_function_searched(e.into(), &fn_name, &locations, explicit_function))?; |
There was a problem hiding this comment.
Nice touch wiring name_the_function_searched only around the slice criterion lookup. Matching on no PDG node for (and leaving other errors untouched) is exactly the right scope — thanks for testing the pass-through case too.
Raised in review by @sshaaf, and correct: `FunctionLocation::end_byte` is documented "Exclusive end byte offset", so `offset <= loc.end_byte` closed a range that is not closed. The consequence is exactly the case this function exists for. Where function A's exclusive end equals function B's start, both spans contain that offset, so both match the filter and the tie goes to whichever `min_by_key` sees first — A, if it wins, is the wrong answer for a line that belongs to B. Adjacent functions in one file are the situation the function was written to resolve. The test lays the bytes out explicitly rather than parsing adjacent functions, because the thing under test is the comparison, not the parser: first = [0, 10) second = [10, 20) offset 9 -> first offset 10 -> second (A's exclusive end, B's inclusive start) offset 20 -> neither (B's exclusive end) Verified by mutation: restoring `<=` makes the new test fail on offset 20, so the test pins the boundary rather than merely passing alongside it. Signed-off-by: Sami Jan <sami.jan@gmail.com>
cfdba34 to
c55313f
Compare
…monstrate
Two rows were out of date after the rebase onto main:
- `dashboard/dist` regen was marked **blocked**, "source fixes are not in the
embedded bundle until dist is rebuilt". That landed in 8f09655 as
sshaaf#110. The row now says done, and says why it mattered: `dist` is
compile-time embedded, so until it is rebuilt the source fixes are invisible in
a built binary while every test passes.
- Change 7 was blocked on two `rgctl slice` defects. Both are now fixed and under
review (issues sshaaf#113/sshaaf#114, PRs sshaaf#115/sshaaf#116), so the row records what actually
gates it.
Also adds open question 4, because the more interesting problem is a gap rather than
a stale value: the maintainer's parking comment on sshaaf#61 names the code browser as the
missing piece, and no change in this sequence is the code browser. Change 1 is the
shell, changes 2-6 are rooms, and none renders source. The prototype does not cover
for it either — the Code lens shows a count of source lines, not source, and Spine
renders an empty state. The one requirement singled out is the one this design
cannot demonstrate.
Recorded as an open question, not invented as a change number: folding source
rendering into change 1 versus giving it its own change ahead of the rooms is a
scoping decision, not a correction.
Signed-off-by: Sami Jan <sami.jan@gmail.com>
|
Span check is half-open now — Added the adjacent-functions test you mentioned. It lays the bytes out explicitly rather than parsing, and it's verified by mutation: restoring Nothing else outstanding from your review. |
Fixes the default function resolution in
rgctl slice. Based onmain(8f09655).Without
--function, the function to slice within was guessed from the file stem (and the literal"process"for Python). Two failure modes followed, and the second misreported the cause:The second is the costly one: it names the variable and the line — both supplied by the caller and both correct — while the real cause is the function they never supplied and did not know was being guessed. It reads as a broken PDG, so the response is to try other lines and variables rather than to pass
--function, which the error never mentions.Fix: resolve the function that contains the criterion line, using the existing
index_function_locations.--functionstill wins. Nested functions mean several spans can contain a line, so take the innermost. If no span contains the line, the historical stem fallback is kept, so behaviour changes only where the old default was plainly wrong.Also name the function searched when a criterion lookup fails:
Unrelated errors pass through unchanged.
Verified: 7 new tests (1-based line mapping, out-of-range, resolution across two functions, innermost-span preference, no-match, error annotation, and pass-through). Full lib suite adds no new failures — the two failures present (
languages::registry::tests::test_registry_creation,cli::agent_pack::tests::embedded_pack_lists_cursor_skills) reproduce on a cleanmainwith no changes. fmt and clippy add no new findings.Fixes #114. (Opening on
sshaaf/rgctl, where#114resolves to the reported issue, so the keywordlinks and auto-closes it. This branch is also staged in the fork as samiujan#3, where a closing
keyword would point at a fork-local issue that does not exist.)