Repository navigation
fix(solidity): link Lib.f() library calls to the function they name - #5005
rohit-jsfreaky wants to merge 2 commits into
Conversation
…ey name `emit E(..)` and `revert Err(..)` are statements, not call_expressions, so the body walk skipped them and no function ever linked to the event or error it uses. Bind them as calls by (name, arity): in the caller's own contract, through the linearized bases (an interface's event, a base's error), or as `Lib.Err` through a type the file declares or imports. Fail closed when ambiguous. `require(c, Err(..))` is a plain call of the error and now resolves too. OpenZeppelin v5.4.0 vs solc 0.8.28: revert 0 -> 386 of 403, emit 0 -> 116 of 119; other call kinds unchanged. Co-Authored-By: Claude <noreply@anthropic.com>
A dotted call `Lib.f(..)` was parked as a bare member call, which the shared
cross-file pass drops, and Solidity has no member pass of its own, so no
library call got a `calls` edge, not even `Lib.f()` inside Lib itself.
Park `Recv.f(..)` (a plain identifier receiver) as a `qualified` call, the
kind `revert Errors.Err()` already uses, and bind it through the one library
or contract of that name the caller's file declares or imports directly
(named, whole-file or `{A as B}` import), then the member by arity. Fail
closed when the name or (name, arity) is ambiguous; a value receiver
(`token.transfer()`) names no type and gets no edge. Free functions resolve
qualified calls too, `require(c, Lib.Err(..))` binds the error, and
`!Lib.f(x)` (parsed by the grammar as `(!Lib).f`) reads its real receiver.
Node ids are unchanged; AST cache schema 38 -> 39.
OpenZeppelin v5.4.0 vs solc 0.8.28 (unique caller/target pairs): library
0 -> 199 of 232, contract-qualified 0 -> 6 of 6, revert 401 -> 403; other
kinds unchanged. Remaining library misses are same-arity overloads (out of
scope), a call inside a modifier argument, and an existing `_f`/`f` node-id
collision.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the pull request, @rohit-jsfreaky. A maintainer will review it soon. Want to talk it through while it is in review? Come join us on our Discord server. For longer-form discussion there is also GitHub Discussions. A couple of things that speed up review: make sure the test suite passes on Python 3.10 and 3.13, and that the change keeps extraction deterministic. |
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.
Not checked: tests were not run; formal verification proved 0 of 4 changed function(s) (1 sampled, not proven, 3 not verified).
Formal verification. PR-changed functions: 1/4 verified (0 proven, 1 may-equivalent, 0 distinguished) · 3 not verified (1 vacuous, 2 unsupported).
Not verified on this run: \_callee\_text (unsupported), \_inherited\_call (unsupported), resolve\_solidity\_inherited\_calls (vacuous: never exercised).
Graphify review — findings
Adds qualified-call resolution for Solidity, so Lib.f(..), Base.f(..) and require(c, Lib.Err(..)) bind to the library or contract member that the caller's file declares or imports (named, whole-file or {A as B}). Extends emit, revert and require handling so event and error references emit calls edges (same contract, a base contract or via a declared/imported type), and resolves inherited calls using C3 linearization. Bumps _AST_CACHE_SCHEMA to 39 so cached Solidity extractions re-run, while the routing of single-dot value-receiver calls in extract_solidity could stop producing the generic member raw call.
Worth a look
- Single-dot value-receiver calls no longer emit the generic member raw_call —
graphify/extractors/solidity.py:739· Escalate · medium- NOT verified (no proof, no reproducing execution) — needs human review
Review partial — this diff was larger than one review pass covers, so later files were not reviewed; some findings may be missing.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 3177 functions depend on the 143 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
extract()— 1149 callers, 68 callees
Verification — 3177 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: 1578 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
518 of 518 test file(s) selected (100%) via static blast radius.
Escalated to a full run for safety — the selection is not trustworthy on its own (see below). CI should run the whole suite.
tests/pi.test.ts— full-run-safetytests/test_4874_jvm_resolution.py— full-run-safetytests/test_4909_reference_binding_scale.py— full-run-safetytests/test_affected_cli.py— full-run-safetytests/test_affected_defines.py— full-run-safetytests/test_affected_member_seed.py— full-run-safetytests/test_affected_overrides.py— full-run-safetytests/test_affected_path_file_node.py— full-run-safetytests/test_affected_seed_reference_forms.py— full-run-safetytests/test_agents_platform.py— full-run-safetytests/test_alias_folding.py— full-run-safetytests/test_analyze.py— full-run-safetytests/test_anthropic_custom_endpoint.py— full-run-safetytests/test_antigravity_install.py— full-run-safetytests/test_apm_fallback_version.py— full-run-safetytests/test_architecture_doc.py— full-run-safetytests/test_astro_extraction.py— full-run-safetytests/test_astro_import_ids.py— full-run-safetytests/test_atomic_canvas_export.py— full-run-safetytests/test_atomic_version_stamp.py— full-run-safetytests/test_atomic_writes.py— full-run-safetytests/test_backend_env_isolation.py— full-run-safetytests/test_backend_extras.py— full-run-safetytests/test_backend_opt_in.py— full-run-safetytests/test_benchmark.py— full-run-safetytests/test_benchmark_raw_graph.py— full-run-safetytests/test_benchmark_real_query.py— full-run-safetytests/test_binary_media_words.py— impact, full-run-safetytests/test_blade_extractor.py— full-run-safetytests/test_build.py— full-run-safetytests/test_build_located_semantic_identity.py— full-run-safetytests/test_build_merge_dedup_scope.py— full-run-safetytests/test_build_merge_ghost_fold_4955.py— full-run-safetytests/test_build_merge_hyperedges_and_prune.py— full-run-safetytests/test_build_merge_shrink_guard.py— full-run-safetytests/test_builtin_global_type_refs.py— full-run-safetytests/test_c_include_resolution.py— full-run-safetytests/test_cache.py— impact, full-run-safetytests/test_cache_long_paths.py— impact, full-run-safetytests/test_cache_stale_import_target.py— impact, full-run-safetytests/test_cache_windows_case_4925.py— impact, full-run-safetytests/test_callflow_html.py— full-run-safetytests/test_cargo_introspect.py— full-run-safetytests/test_cargo_missing_manifest.py— full-run-safetytests/test_carried_hyperedge_remap.py— full-run-safetytests/test_case_only_decl_ids_4904.py— full-run-safetytests/test_case_sensitive_resolution.py— full-run-safetytests/test_charmap_encoding.py— impact, full-run-safetytests/test_chunking.py— impact, full-run-safetytests/test_cjs_module_extension.py— full-run-safety- … and 468 more
changed code file(s) with no mapped test (
graphify/extractors/solidity.py) — a coverage gap or a missing link — running the full suite rather than only the selected tests
Selection is safe under the controlled-regression assumption; always-run tests + a periodic full run are the backstops. Advisory — it never changes the check verdict.
Formal verification
Could not verify: Could not verify \_callee\_text.
The verifier did not have enough to check \_callee\_text, 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: no capturable inputs from the test suite; property tier: parameter `callee` is annotated `Node` — outside the synthesizable primitive/collection set
Could not verify: Could not verify \_inherited\_call.
The verifier did not have enough to check \_inherited\_call, 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: no capturable inputs from the test suite; property tier: parameter `node` is annotated `Node` — outside the synthesizable primitive/collection set
No difference found (not proven): No behavior difference found in extract\_solidity (not a proof).
The verifier ran both versions of extract\_solidity 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.
Could not verify: Could not verify resolve\_solidity\_inherited\_calls.
The verifier did not have enough to check resolve\_solidity\_inherited\_calls, 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: no capturable inputs from the test suite; property tier: non-vacuity: domain too small (only 1 distinct inputs exercised, need 3) — 'no divergence' would be near-vacuous
· 1 more finding(s) on lines outside this diff (see the check run).
|
I checked the bot finding:
|
Closes #5004
What does this PR do?
Recv.f(..)with a plain identifier receiver is now parked as aqualifiedcall, the same kindrevert Errors.Err()uses in #5001. It binds through the one library or contract of that name that the caller's file declares or imports directly (named import, whole-file import, or alias{A as B}), and then through the member by arity.Also:
require(c, Lib.Err(..))binds the library error (the 2revertmisses left in fix(solidity): link emit, revert and require to the event or error they name #5001).!Lib.f(x)works. The grammar parses it as a call of(!Lib).f, so_callee_textnow strips the unary operator, the same way it already handlesa || b.f().Fails closed: no edge when the name or the (name, arity) pair is ambiguous. A value receiver (
token.transfer()) names no type, so it gets no edge. Node ids do not change. AST cache schema 38 -> 39.Diff:
graphify/extractors/solidity.py+70 / -14, 8 new tests intests/test_solidity_extractor.py.Numbers
OpenZeppelin Contracts v5.4.0 (319 files), compared with the solc 0.8.28 AST (
referencedDeclaration):Lib.f()library callBase.f()contract-qualifiedrevert/require(c, Err())using X for Y/ typed receiver (out of scope)Most of the 27 library misses are overloads with the same number of arguments (
Arrays.unsafeAccess,SignatureChecker.isValidSignatureNow,Math.modExp, ...), which fail closed by design. A few come from_fandfalready sharing one node id (an existing id issue, not changed here). One is a call inside a modifier argument in a function header, which the body walk does not reach.Limitations
{A as B}plus a localAis ambiguous and gets no edge._f/fid collision above.Type of change
Verification & Invariants
Invariant: a
Recv.f()link is added only whenRecvnames exactly one library or contract the caller's file can see, and that type has exactly onefwith that arity. Nothing is guessed from a value receiver.How was this tested?
Graphify-specific checklist
uv run python -m tools.skillgen --bless) when changing their source fragments. (Not needed: no skill fragments changed.)🤖 Generated with Claude Code