Skip to content

fix(scip): read typed_range, not just the deprecated range - #198

Merged
joyful-ii-V-I merged 3 commits into
redhat-et:mainfrom
dpunosevac:fix/scip-typed-range
Sep 12, 2026
Merged

joyful-ii-V-I merged 3 commits into
redhat-et:mainfrom
dpunosevac:fix/scip-typed-range

Conversation

@dpunosevac

@dpunosevac dpunosevac commented Sep 12, 2026 •

Copy link
Copy Markdown
Contributor

--scip silently no-ops against any index scip-java produces.

Found this while testing --scip against a Java index. The patch is a byproduct of
confirming 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:

docker run --rm -v "$PWD":/sources --env JVM_VERSION=21 \
  ghcr.io/scip-code/scip-java:latest scip-java index --build-tool=maven
ripwire . --callers=getPet --scip=index.scip --legend=compact

ripwire --scip: index 'index.scip' matched no parsed (file,line) — proceeding name-based

stdout is byte-identical to the run without --scip.

Why

src/scip.h reads Occurrence.range (field 1) only. SCIP deprecated that in favour of the
typed_range oneof, fields 8 and 9. Across all 6,677 occurrences in that index the only keys
present are TypedRange, TypedEnclosingRange, symbol and symbol_roles. Field 1 never
appears, so every startLine stays -1 and nothing joins the (file,line) map.

From scip.proto: consumers "SHOULD prefer the typed form when available and fall back to the
repeated int32 form 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 ambiguous dropping 6 to 4.

test/scipfix/make_index.py writes packed_int32(1, range_ints), the same field the reader
expects, so scipcheck.sh exercises the reader against its own generator and cannot catch
this. Happy to add a typed_range arm 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 so typed_range
outranks 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:

                       stock +scip     patched +scip
overlay match          none            79% of occurrences (521/656)
--callers=getPet       7 callers       14 callers
  declined_calls       9               0
--callers=findAll      0 callers       7 callers
  declined_calls       5               0
graph_ambiguous        6               0

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-clang or scip-python still emit the deprecated form. If they do, this only
affects scip-java consumers today, and the patch leaves their path untouched.

@coderabbitai

coderabbitai Bot commented Sep 12, 2026 •

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: d87aa2fa-e82a-4355-b720-2b89d3b6644a

📥 Commits

Reviewing files that changed from the base of the PR and between 44d6e56 and 6ddd9cf.

📒 Files selected for processing (2)
  • src/scip.h
  • test/scipcheck.sh
🚧 Files skipped from review as they are similar to previous changes (1)
  • test/scipcheck.sh

Included review availability: Your plan provides up to 4 included reviews per hour; 0 remain after this review.


📝 Summary

Summary by CodeRabbit

  • New Features
    • Added support for decoding single-line and multi-line typed ranges.
    • Typed range data takes precedence over the deprecated range format when both are present.
    • Improved source location accuracy for references and definitions using typed range information.
  • Documentation
    • Updated range field documentation to describe typed ranges and precedence behavior.
  • Tests
    • Added coverage for mixed, typed, and legacy range data, including graceful handling when typed information is unavailable.

Walkthrough

The SCIP wire decoder now reads single_line_range and multi_line_range fields. Typed ranges take precedence over deprecated ranges. Test fixtures and integration checks cover mixed field order, precise edges, deterministic output, and typed-field removal.

Changes

Typed range support

Layer / File(s) Summary
Typed occurrence decoding
src/scip.h
The decoder extracts start lines from typed range sub-messages and prevents packed or unpacked deprecated ranges from overriding them.
Typed fixture generation
test/scipfix/make_index.py
The fixture generator encodes typed ranges, mixed range forms, field ordering, and blind indexes through new helpers and CLI flags.
Integration validation
test/scipcheck.sh
The checks validate typed definitions and references, edge precision, matching, XML validity, determinism, and graceful degradation when typed fields are removed.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Suggested reviewers: joyful-ii-v-i

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
Loading

Merge Risk: ⚪ Minimal · up to 6ddd9

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)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 9.09% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: support typed SCIP ranges instead of relying only on the deprecated range field.
Description check ✅ Passed The description directly explains the typed-range parsing issue, the implementation, test coverage, and observed results.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

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.
@dpunosevac
dpunosevac marked this pull request as ready for review September 12, 2026 14:07
@joyful-ii-V-I

Copy link
Copy Markdown
Collaborator

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 scipcheck.sh can't see it, because make_index.py writes the same deprecated field the reader expects.

Two things before it lands:

  1. A typed_range arm, yes please. This project writes the gate before the code it measures. The fixture should emit single_line_range and multi_line_range occurrences, plus one occurrence that carries both forms, to pin that the typed form wins whichever field arrives first. That arm should fail on stock 0.6.0 and pass with your change. If you'd rather not write it, say so and we'll add it as a maintainer commit on your branch. The fix stays yours either way.
  2. CI. Workflow runs from a first-time contributor wait for a maintainer to approve them. Yours is approved now, so the checks will report here.

A small change to how --scip opens the index file is also in flight in src/scip.h (#197). It touches different lines, and your branch merges cleanly with it.

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.
@dpunosevac

dpunosevac commented Sep 12, 2026 •

Copy link
Copy Markdown
Contributor Author

Arm added in 44d6e56.

make_index.py --typed re-encodes the fixture in the typed form, spread over four occurrences so each encoding is independently observable:

occurrence range form wire order
handler def, alpha.cpp:11 multi_line_range alone
helperAlpha def, alpha.cpp:6 single_line_range alone
helperAlpha ref, alpha.cpp:13 both deprecated first
handler ref, caller.cpp:9 both typed first

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 multi_line_range spans 10 to 13 so start_line and end_line differ: a reader taking field 3 for the start binds that def to line 14, where no handler exists, and it surfaces as a def unmatched.

helperAlpha is new to the fixture. The third encoding needed a second def/ref pair, and alpha.cpp already had the call.

Arm 10 asserts 100% (2/2), 0 defs unmatched, precise=2, prov="scip" on both edges, no amb= on run, no staleness hint, determinism on stdout and stderr, well-formed XML.

Arm 10b is the vacuity check. --typed --blind re-emits the same index with fields 8 and 9 stripped, exactly the view a typed_range-blind reader has of it, and asserts it pins nothing.

Results:

  • patched: scipcheck: ALL PASS, 50 PASS lines, up from 34.
  • stock: 5 FAIL, all in arm 10. Refs unmatched, precise= absent from the header, run back to a two-way split with amb="1", handler to helperAlpha unpinned. stderr reads SCIP matched 0% of occurrences (0/0) against 100% (2/2) patched. Arm 10b passes on both, as it must.

One qualification on "stock": I built f82177af~1, the PR base, not the v0.6.0 tag. git diff v0.6.0 f82177af~1 -- src/scip.h is comment-only, three lines in the loadScipOverlay doc block about the missing-file refusal, so the decode path is identical and the base stands in for the release.

Also green: scipjoincheck, namedfileinputcheck, commentcoherencecheck, bashsourcecheck, gatecountcheck, shellgateindexcheck, pargatescheck, formatgatecheck. The regenerated index.scip is byte-identical to the checked-in one, so the occurrence() refactor is wire-neutral.

.github/pargates-shard-weights.json lists scipcheck.sh at 11.6 and the arm adds six binary runs. pargatescheck passes so nothing pins it, but say the word if you want it renumbered.

…t#197)

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

# Conflicts:
#	test/scipcheck.sh
@joyful-ii-V-I

Copy link
Copy Markdown
Collaborator

Thank you, @dpunosevac. The arm is exactly what we hoped for:

  • four occurrences, so each encoding can be observed on its own;
  • the line-99 decoy in the deprecated half, so only a typed-range win survives the S5 gate;
  • a multi_line_range whose start and end differ;
  • the --blind vacuity arm.

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 test/scipcheck.sh, so the PR showed a conflict and CI could not start. I merged main into your branch as a maintainer commit and kept both comment blocks: #197's lines about the refusals, then your TYPED RANGES bullet. No code changed. scipcheck, scipjoincheck and namedfileinputcheck pass on the merged tree, and CI is running now.

No need to renumber the shard weights.

@joyful-ii-V-I
joyful-ii-V-I merged commit 486a01b into redhat-et:main Sep 12, 2026
31 checks passed
@joyful-ii-V-I

Copy link
Copy Markdown
Collaborator

Merged. Thank you, @dpunosevac.

--scip had silently ignored every index scip-java writes, because ripwire read only SCIP's deprecated range field. You tracked the problem to the encoding before touching code, and fixed the reader so the typed form takes precedence, as scip.proto asks. Then you gated it: arm 10 goes red on the old reader, and arm 10b proves the fixture cannot pass without fields 8 and 9. On spring-petclinic that took the overlay from no matches to 79% of occurrences, and graph_ambiguous from 6 to 0.

We merged main into your branch once, to resolve a comment conflict with #197 in test/scipcheck.sh. Your code is unchanged.

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.

@dpunosevac

Copy link
Copy Markdown
Contributor Author

@joyful-ii-V-I thank you!
Glad I could help.

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