Repository navigation
fix(scip): read typed_range, not just the deprecated range - #198
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 0 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe SCIP wire decoder now reads ChangesTyped range support
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant FixtureGenerator
participant SCIPDecoder
participant SCIPIndexChecks
FixtureGenerator->>FixtureGenerator: build typed or blind SCIP index
FixtureGenerator->>SCIPDecoder: provide occurrence ranges
SCIPDecoder->>SCIPDecoder: prefer typed range over deprecated range
SCIPDecoder->>SCIPIndexChecks: produce resolved index data
SCIPIndexChecks->>SCIPIndexChecks: validate edges, matching, XML, and degradation
Merge Risk: ⚪ Minimal · up to Typed-range decoding and precedence are covered by the updated fixtures and integration checks, with no unresolved merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
SCIP deprecated Occurrence.range (field 1) in favour of the typed_range oneof, fields 8 and 9. scip-java 0.13.1 emits only the typed form, so the reader left every startLine at -1, nothing joined the (file,line) map, and --scip degraded to name-based against any Java index. Both submessages carry the start line in field 1 (SingleLineRange.line, MultiLineRange.start_line), so this needs a sub-message variant of the existing decode, not new logic. A flag keeps typed_range outranking the deprecated field whatever order they arrive in, as the spec requires.
5b95641 to
f82177a
Compare
|
Thank you, @dpunosevac. This is an excellent report and a clean fix. You isolated the cause before touching any code: an index hand-built with the same documents and lines, using the deprecated field, attaches on stock 0.6.0. That pins the problem to the encoding, not the join. You're also right that Two things before it lands:
A small change to how |
scipcheck.sh only ever fed the reader indexes make_index.py wrote with Occurrence.range, the deprecated field the reader already knew, so the gate exercised the reader against its own generator: a reader blind to fields 8 and 9 passed all 34 arms. make_index.py --typed re-encodes the same fixture in the typed form, over four occurrences: the handler def as a lone multi_line_range, a helperAlpha def as a lone single_line_range, and both refs carrying a typed range plus a deprecated one pointing past EOF, one with each field first on the wire. The new arm asserts both refs pin, the ratio reads 100% (2/2) and no def binds to a wrong line, so the typed form must win in either order. --typed --blind re-emits that index with fields 8 and 9 stripped, the view a typed_range-blind reader has of it, and the arm asserts it pins nothing. That is what keeps the arm from going vacuous.
|
Arm added in 44d6e56.
Each both-forms occurrence puts line 99, past EOF in both fixture files, in its deprecated half. The S5 gate drops anything resolving there, so the edge survives only if the typed half won. One occurrence each way covers both wire orders rather than only the one my patch happened to hit first. The
Arm 10 asserts 100% (2/2), 0 defs unmatched, Arm 10b is the vacuity check. Results:
One qualification on "stock": I built Also green:
|
…t#197) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> # Conflicts: # test/scipcheck.sh
|
Thank you, @dpunosevac. The arm is exactly what we hoped for:
Five FAILs on the old reader and 50 PASS with the fix is the evidence this project asks for. One thing had moved under the branch. #197 merged a few hours ago, and it also edited the header comment of No need to renumber the shard weights. |
|
Merged. Thank you, @dpunosevac.
We merged main into your branch once, to resolve a comment conflict with #197 in It is merged, not squashed, so your commits keep your name in the history, and you are credited in the 0.6.1 release notes. |
|
@joyful-ii-V-I thank you! |
--scipsilently no-ops against any indexscip-javaproduces.Found this while testing
--scipagainst a Java index. The patch is a byproduct ofconfirming the diagnosis, so no attachment to it: if it is the wrong shape, or you would
rather fix it differently, just close it.
What happens
ripwire 0.6.0, scip-java 0.13.1, spring-petclinic @ 818c413:
stdout is byte-identical to the run without
--scip.Why
src/scip.hreadsOccurrence.range(field 1) only. SCIP deprecated that in favour of thetyped_rangeoneof, fields 8 and 9. Across all 6,677 occurrences in that index the only keyspresent are
TypedRange,TypedEnclosingRange,symbolandsymbol_roles. Field 1 neverappears, so every
startLinestays -1 and nothing joins the (file,line) map.From
scip.proto: consumers "SHOULD prefer the typed form when available and fall back to therepeated int32form otherwise".I isolated it to the encoding before touching any code: hand-built an index with the same
documents, symbols and line numbers using field 1, and that one attaches cleanly on stock
0.6.0, with
ambiguousdropping 6 to 4.test/scipfix/make_index.pywritespacked_int32(1, range_ints), the same field the readerexpects, so
scipcheck.shexercises the reader against its own generator and cannot catchthis. Happy to add a
typed_rangearm to the fixture if you want one.The change
One helper reading field 1 of either range submessage (
SingleLineRange.line,MultiLineRange.start_line), two branches for fields 8 and 9, and a flag sotyped_rangeoutranks the deprecated field whatever order they arrive in. No new decode logic.
Results
test/scipcheck.sh: 34/34 PASS, including the fuzz and degrade arms.Same petclinic index, before and after:
Scored against a scip-java oracle on the same repo, overload-resolution recall goes from 23%
to 100% with precision staying at 100%. Answers that were already correct do not change.
Not checked
Whether
scip-clangorscip-pythonstill emit the deprecated form. If they do, this onlyaffects
scip-javaconsumers today, and the patch leaves their path untouched.