Repository navigation
fix(go): type a receiver's field when its struct is declared in a sibling file (#3368) - #4866
nothariharan wants to merge 1 commit into
Conversation
…ling file (Graphify-Labs#3368) 1695efa types `s.field.M()` through the caller file's own go_type_table. When the struct is declared in another file of the package, the usual Go layout, that file has no binding for `field` and the call is dropped, even for a same-package field type. Each Go file node now carries the field types of the structs it declares (`_go_fields`, keyed by struct id), and a self-rooted raw call names the struct that owns the field (`field_owner`). When the caller's table has no binding, the resolver reads the owner struct's fields, only when the owner is a single type declared by a single file. The fields ride on the file node, not the struct node: every method file emits the struct's id too, and on an incremental rebuild that fresh copy shadows the declaring file's. `_go_fields` is carried through the incremental resolution context, and the AST cache schema goes to 30.
|
Thanks for the pull request, @nothariharan. 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 (2 vacuous, 1 unsupported).
Not verified on this run: dispatch\_command (vacuous: never exercised), \_resolve\_go\_member\_calls (vacuous: never exercised), \_rebuild\_code (unsupported).
Graphify review — findings
Adds Go cross-file resolution of s.field.M() calls: the Go file node now records the field types of the structs it declares (_go_fields), and each such raw call carries a field_owner naming the struct that owns the field. A method in another file of the same package can then resolve the call through that field, and the _AST_CACHE_SCHEMA bump to 30 forces cached Go extractions to re-run. Owner-specific lookup in _resolve_go_member_calls is the weak spot: a same-name field in the flat field table can win over the owner's type, and a struct id that appears more than once in type_def_nids makes the field_owner lookup give up.
Worth a look
- Owner-specific Go field types are ignored when the flat field table has a same-name field —
graphify/extract.py:7435· Escalate · medium- NOT verified (no proof, no reproducing execution) — needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 4105 functions depend on the 670 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
_rebuild_code()— 207 callers, 65 callees - worse:
extract_go()— 19 callers, 12 callees
Verification — 4105 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: 4042 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
288 of 469 test file(s) selected (61%) via static blast radius.
tests/test_affected_cli.py— impacttests/test_affected_overrides.py— impacttests/test_agents_platform.py— impacttests/test_astro_extraction.py— impacttests/test_astro_import_ids.py— impacttests/test_binary_media_words.py— impacttests/test_blade_extractor.py— impacttests/test_build.py— impacttests/test_builtin_global_type_refs.py— impacttests/test_c_include_resolution.py— impacttests/test_cache.py— impacttests/test_cache_long_paths.py— impacttests/test_cache_stale_import_target.py— impacttests/test_case_sensitive_resolution.py— impacttests/test_charmap_encoding.py— impacttests/test_chunking.py— impacttests/test_cjs_module_extension.py— impacttests/test_cli_export.py— impacttests/test_cloud_cta.py— impacttests/test_cobol_extractor.py— impacttests/test_codebuddy.py— impacttests/test_compact_graph_json.py— impacttests/test_cpp_method_declarations.py— impacttests/test_cpp_nested_and_cli.py— impacttests/test_cpp_objc_cross_file_calls.py— impacttests/test_cpp_template_type_params.py— impacttests/test_cpp_type_alias.py— impacttests/test_cross_extension_reexport_self_cycle.py— impacttests/test_cross_language_call_resolution.py— impacttests/test_cross_repo_external_call_guards.py— impacttests/test_cross_repo_member_calls.py— impacttests/test_csharp_call_site_generic_args.py— impacttests/test_csharp_delegate_declarations.py— impacttests/test_csharp_enum_members.py— impacttests/test_csharp_field_generic_args.py— impacttests/test_csharp_foreach_loop_variable.py— impacttests/test_csharp_generic_callsites.py— impacttests/test_csharp_interface_dispatch.py— impacttests/test_csharp_member_calls.py— impacttests/test_csharp_member_chain_calls.py— impacttests/test_csharp_member_nodes.py— impacttests/test_csharp_object_creation.py— impacttests/test_csharp_overloads.py— impacttests/test_csharp_partial_classes.py— impacttests/test_csharp_tuple_type_refs.py— impacttests/test_csharp_type_resolution.py— impacttests/test_csharp_underscore_collisions.py— impacttests/test_csharp_var_call_return_type.py— impacttests/test_csharp_verbatim_interpolation.py— impacttests/test_dedup_shrink_refuses_force_write.py— impact- … and 238 more
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 dispatch\_command.
The verifier did not have enough to check dispatch\_command, 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: not verifiable: all 40 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly SystemExit — names the real obstacle, not a sampling gap)
Could not verify: Could not verify \_resolve\_go\_member\_calls.
The verifier did not have enough to check \_resolve\_go\_member\_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
No difference found (not proven): No behavior difference found in extract\_go (not a proof).
The verifier ran both versions of extract\_go 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 \_rebuild\_code.
The verifier did not have enough to check \_rebuild\_code, 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: nondeterministic output (both versions disagreed with themselves on the witness) — not a behaviour change; a function whose output is not a function of its inputs (random / time / uuid / os.urandom, or state carried across calls) cannot be soundly distinguished by execution, so this tier abstains for the whole function
· 1 grounded finding(s) anchored inline below; 1 more finding(s) on lines outside this diff (see the check run).
| return fields_by_struct | ||
|
|
||
|
|
||
| def extract_go(path: Path) -> dict: |
There was a problem hiding this comment.
extract_go()
fans out to 12 callees (efferent coupling); 19 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
Go (#4867/#4868/#4866) and Python (#4773) extractor changes alter per-file AST output and are AST-cached, so one combined _AST_CACHE_SCHEMA bump 30->31 forces stale caches to re-extract. Install (#4770/#4771) and the skill reword (#4721) need no bump. Folded into the unreleased 0.9.99. Each PR omitted its own bump / carried a stale number; the single combined bump lands here. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Landed in v0.9.99, now on PyPI (pip install graphifyy==0.9.99). A receiver's field whose struct is declared in a sibling file is typed so member calls resolve. Thanks for the fix, @nothariharan. |
What does this PR do?
Refs #3368. On v8, a Go method that calls through a field of its own receiver (
s.field.M()) gets nocallsedge when the struct is declared in a different file of the same package, even when the field's type is a same-package type. That is the usual Go layout: the struct and its constructor in one file, the methods spread over others.Why #3368 is not resolved yet on v8
1695efa (
feat(go): resolve member calls through the receiver's declared type, 0.9.89) types a receiver throughgo_type_table, which is built per file by_go_receiver_type_table. Fors.field.M()the resolver looksfieldup in the table of the caller's own file. When the struct is declared in another file, the caller's file has nofieldbinding, the lookup returns nothing, and the call is dropped. This is independent of the receiver's type being in the same package or not.Minimal repro on v8 (
0a8cd4e1):Move
type Appintorun.goand the edge appears, which is the existingtest_a_struct_field_types_the_receiver_through_the_methods_own_receiver.On Grafana (
pkg/router, 45 non-test files),GrafanaRouteris declared inrouter.goand its methods live indiscovery_status.go,reconcile.goand others. v8 has 488callsedges there. With this change it has 492, and all 4 new edges match the source:GrafanaRouter.groupStatusr.discoveryCache.lookup(...)(discovery_status.go:210)discoveryCache.lookupGrafanaRouter.publishr.discoveryCache.retain(snap)(reconcile.go:375)discoveryCache.retainGrafanaRouter.Runr.loader.Notify(ctx)(reconcile.go:68)RoutesLoader.NotifyGrafanaRouter.reconciler.loader.Load(ctx)(reconcile.go:185)RoutesLoader.LoadNo edge was removed.
The change
extractors/go.py:_go_struct_field_typesrecords, for each struct the file declares,field -> TypeNameunder the same rule the flat table uses (_go_single_type_name, so onlyT/*T). The map is stored on the file node as_go_fields, keyed by struct id. A raw calls.field.M()rooted at the method's own receiver also carriesfield_owner(the receiver's type).extract.py_resolve_go_member_calls: when the caller file's table has no binding for the field, look it up in the fields offield_owner, only when that owner resolves to a single type node that a single file declares fields for. The rest of the path is unchanged: the single-definition and single-method guards, INFERRED / 0.85, and parking for an undeclared type.watch.py/cli.py:_go_fieldsis added to the persisted resolver markers, so an incremental rebuild that re-extracts only the method's file still sees the unchanged struct file's fields.cache.py:_AST_CACHE_SCHEMA29 → 30, so cached Go extractions pick up the new field and marker.Why the file node and not the struct node: every file that holds a method of
Appemits theAppnode id too, so on an incremental rebuild ofrun.gothe fresh copy ofAppfromrun.goshadows the declaring file's copy in the resolution context. The file node's id is unique to the declaring file.This is independent of #4853 (receivers typed from another package,
pkg.T, through the caller'sgo_imports). That PR changes how a qualified type enters the tables. This one changes which file's fields a self-rooted chain can read. Each applies on v8 without the other. Grafana'shs.pluginContextProvider.GetWithDataSourceinpkg/api/datasources.goneeds both, because the field type is*plugincontext.ProviderandHTTPServeris declared inhttp_server.go. Both PRs bump_AST_CACHE_SCHEMAto 30, so whichever lands second needs a one-line rebase to 31.Type of change
Verification & Invariants
Invariant: a Go member call binds only to a method of the one type its receiver is declared as. For
s.field.M(), the field's type comes from the declaration ofs's own struct, wherever in the package that struct is declared, and never from a same-named field of another struct.test_a_sibling_files_field_of_another_struct_does_not_type_the_receiver:greeteris a field ofOther, not ofApp, so no edge is created.The existing negative cases still pass unchanged: a chain not rooted at the method's receiver, a package-qualified type, two packages that declare the same type.
Persisted state: the new
_go_fieldsattribute is written tograph.jsonon Go file nodes. The cache schema bump retires extractions cached without it.Read the CONTRIBUTING.md guide.
Reproduced the issue and identified the invariant.
Made the smallest fix necessary.
Added a regression test (if bug fix) or isolated boundary test.
Kept the PR description synchronized with the final implementation.
Documented any limitations / unsupported cases explicitly.
Not covered (unchanged):
*pkg.T). See fix(go): resolve member calls on receivers typed from another package (#3368) #4853.p.field.M()withpa parameter), excluded by design in 1695efa.app.gowithtype App): on v8 the file node and the type node get the same id, so an incremental rebuild of a method file loses the declaring file's node, and this field data with it. A full build resolves. This is a separate id collision and is not changed here.How was this tested?
Run on Windows 11, Python 3.12, in the environment
uv run --all-extrassyncs (.venv; the later runs call.venv/Scripts/python -m pytestdirectly becauseuv runrewritesuv.lockon Windows).Graphify-specific checklist
uv run python -m tools.skillgen --bless) when changing their source fragments.