fix(objc): remap the field->type table keys with the ids they hold (#3150) - #3170
fix(objc): remap the field->type table keys with the ids they hold (#3150)#3170abhay-codes07 wants to merge 1 commit into
Conversation
…raphify-Labs#3150) _resolve_objc_member_calls' field-type table (Graphify-Labs#2591) is the one extractor bucket keyed BY class node id. The Graphify-Labs#1529 passes rewrote node ids, edge endpoints, raw_calls[].caller_nid and swift_extensions[].nid - but never those keys - and the prefix strip fires whenever the inputs share an absolute prefix, i.e. always via `graphify update <dir>`. So the keys went stale, field_types_by_class.get(cls) missed for every class, and [self.<field> ...] receiver typing emitted zero edges through the CLI while working in tests, which hand extract() already-relative paths. The cached shard had the same split: _rewrite_strings deliberately never touches dict keys (its docstring even asserted no bucket is keyed by a node id - this one is), so a replayed shard kept absolute-derived table keys while the node ids were re-anchored. Both in-process remap passes now rewrite the table keys through the same mapping (_remap_objc_field_tables), and the cache portability round-trip rewrites them beside every _rewrite_strings call (_rewrite_id_keyed_table_keys), store and load.
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Formal verification. 1 change(s) tested, no difference found (not proven).
Graphify review — findings
Fixes ObjC receiver typing (#2591) that was silently inert through graphify update <dir>: the objc_field_types["tables"] bucket is keyed by class node id, but the #1529 id-remap and the #2257 cache-portability rewrite both left those keys stale whenever a common absolute prefix was stripped, so every field_types_by_class.get(cls) missed and [self.<field> …] sends resolved to nothing. Adds _remap_objc_field_tables to rewrite the table keys through the same id/prefix mappings in extract, and _rewrite_id_keyed_table_keys to re-anchor them alongside _rewrite_strings on cache store and load, keeping the keys in lockstep with the node ids they reference. Covers the CLI absolute-path shape, warm-cache replay, and the store/load round-trip with tests, so the previously test-only-green path is now exercised end to end.
No blocking issues surfaced. 1 lower-confidence candidate did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 2005 functions depend on the 312 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract()— 504 callers, 43 callees - new:
_rebuild_code()— 98 callers, 50 callees - new:
detect()— 108 callers, 15 callees - new:
save_semantic_cache()— 58 callers, 9 callees - new:
load_cached()— 48 callers, 7 callees - new:
extract_xaml()— 19 callers, 17 callees - new:
file_hash()— 50 callers, 6 callees - new:
extract_corpus_parallel()— 26 callers, 11 callees - …and 35 more — each is listed as a finding
Verification — 2005 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 1857 function(s) in the blast radius were not formally verified this run
Formal verification
Could not verify: Could not verify \_absolutize\_ids\_in.
The verifier did not have enough to check \_absolutize\_ids\_in, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: parameter `path` is annotated `'str | Path'` — outside the synthesizable primitive/collection set
Could not verify: Could not verify \_relativize\_ids\_in.
The verifier did not have enough to check \_relativize\_ids\_in, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: parameter `path` is annotated `'str | Path'` — outside the synthesizable primitive/collection set
No difference found (not proven): No behavior difference found in \_rewrite\_strings (not a proof).
The verifier ran both versions of \_rewrite\_strings on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
· 43 more finding(s) on lines outside this diff (see the check run).
|
Shipped in v0.9.52 via authorship-preserving cherry-pick so you keep contributor-graph credit. Thanks @abhay-codes07! Release: https://github.com/Graphify-Labs/graphify/releases/tag/v0.9.52 |
Closes #3150.
The problem
_resolve_objc_member_calls' field→type table (#2591) is the one extractor bucket keyed by class node id. The #1529 passes rewrite node ids, edge endpoints,raw_calls[].caller_nidandswift_extensions[].nid— but never those keys. The prefix strip fires whenever the input paths share an absolute prefix, i.e. always viagraphify update <dir>— so the keys went stale,field_types_by_class.get(cls)missed for every class, and[self.<field> …]receiver typing emitted zero edges through the CLI while working in tests, which handextract()already-relative paths.The cached shard has the same split, from the same assumption:
_rewrite_strings' docstring says "no extractor bucket is keyed by a node id" — this one is — so the portability round-trip re-anchored every id except the table keys.The change
_remap_objc_field_tables), right beside theswift_extensions[].nidrewrites they mirror._rewrite_stringscall (_rewrite_id_keyed_table_keys), on store and on load, and_rewrite_strings' docstring now names the exception instead of denying it.A note on verification
The staleness only manifests where the prefix strip actually fires — POSIX path resolution, i.e. the Linux CI and every macOS/Linux user of
graphify update— not on this Windows dev machine, where thesource_filere-resolution misses and ids keep their absolute form (matching keys, by accident). So: the three end-to-end tests (absolute inputs, relative inputs, warm-cache replay) exercise the fix on the Linux CI, and two platform-independent tests carry the teeth locally — the in-process remap unit test and a cache round-trip built with the real_make_idslug, which fails before the fix (key and node id diverge on store) and passes after.Tests
tests/test_objc_field_table_remap.py— 5 tests as above, using the issue's own four-file corpus.test_objc*,test_swift*,test_cacheandtest_stat_index_portabilityare unchanged (132 passed); the full suite matches the freshv8(0.9.51) baseline.Composes with the #3151 fix (separate PR): its inherits-chain walk reads the same table this PR keeps keyed correctly.