Skip to content

fix(ids): keep a type and a function differing only in case apart - #4908

Closed
bercedev wants to merge 1 commit into
Graphify-Labs:v8from
bercedev:fix/decl-id-case-only-names
Closed

bercedev wants to merge 1 commit into
Graphify-Labs:v8from
bercedev:fix/decl-id-case-only-names

Conversation

@bercedev

Copy link
Copy Markdown
Contributor

What does this PR do?

Closes #4904

class Foo and function foo in one file normalize to the same id, and DeclIdRegistry.resolve treated the second one as a duplicate of the first. One node got dropped and its edges went to the survivor, so foo() calling Foo showed up as a Foo -> Foo self loop.

Now an id is only reused when the declaration has the same kind or the exact same spelling. The other one gets a salted id, and the AST cache schema goes to 32 so old cache entries don't hide the fix.

This is a bit different from the diff in the issue. Kind alone also splits class Foo + namespace Foo in TS, which is one merged declaration, so same spelling across kinds still counts as a duplicate. There's a test for it and it fails with the kind-only check.

Limitation, also in the issue: two declarations of the same kind still collapse (class Foo / class FOO, function foo / function Foo). Should cover the FallListe / fallListe pair from #3726 too since that's a type and a function, I didn't run that file. #3738 is a pre-scan for that one in TS and JS only, this doesn't touch it.

Type of change

  • Bug fix
  • New feature
  • Documentation
  • Tests or CI
  • Refactor
  • Security fix

Verification & Invariants

Invariant: a type and a function that differ only in case keep their own nodes, a real redeclaration or merged declaration stays one node.

  • 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.

How was this tested?

tests/test_case_only_decl_ids_4904.py has the JS, TS, Kotlin, C++, PHP, Ruby and Swift cases from the issue:

this branch                 10 passed
v8 ids.py                   9 failed, 1 passed
kind-only check from issue  2 failed, 8 passed

Full suite, --network=none, no .git: 18 failed, 9166 passed, 34 skipped, 1 xfailed. The 18 are the test_security.py DNS and test_skillgen.py origin/v8 ones that #4894 fixes, they fail on a clean v8 in that setup too. ruff check is clean on the changed files.

Salted ids mean a node id can change for files that have this pair, that's what the schema bump is for.

Graphify-specific checklist

  • I updated generated skill artifacts (uv run python -m tools.skillgen --bless) when changing their source fragments.
  • I confirmed that AST/structural extraction remains deterministic (no ambient state dependencies like ENV variables).
  • I reviewed changes for security implications (no unsafe interpolation into shell/Python).
  • I confirmed no API keys or local-only graph data are included.
  • (If applicable) I disclosed AI authorship in my commit messages.

Note: I used Claude Code to write this and ran the tests above locally.

DeclIdRegistry reused the id of any declaration that normalized alike, so class Foo
and function foo collapsed into one node. Reuse now needs the same kind or the same
spelling, which keeps TS class + namespace merging. Bump the AST cache schema.
@bercedev
bercedev requested a review from safishamsi as a code owner October 31, 2026 12:21
@github-actions github-actions Bot added the contributor Pull request from a returning community contributor label Oct 31, 2026
@github-actions

Copy link
Copy Markdown

Thanks for the pull request, @bercedev. 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.

@graphify-labs graphify-labs Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Graphify reviewed this change.

No issues found by static checks — no coupling regressions or blocking issues in the code graph. That is not the same as safe to merge: see what was not checked.

Not checked: tests were not run; formal verification proved 0 of 2 changed function(s) (2 sampled, not proven).

Formal verification. PR-changed functions: 2/2 verified (0 proven, 2 may-equivalent, 0 distinguished) · 0 not verified.


Graphify review — findings

Fixes node-id collisions where a type and a function whose names differ only in letter case were merged into one node. They now get separate ids (#4904), while the same spelling across kinds still resolves to a single declaration and a call stays attached to the function rather than the class. To make that work, the id helpers gain a DeclIdRegistry that tracks declarations and hands out case-distinct ids. Bumping _AST_CACHE_SCHEMA to 32 forces cached extractions to re-run, so existing caches are rebuilt once on first use.

No blocking issues surfaced. 2 lower-confidence candidates did not survive cross-model review.

Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 5126 functions depend on the 111 functions this change touches.

Health — this change adds coupling hotspots:

  • worse: extract() — 1081 callers, 63 callees

Verification — 5126 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: 2256 function(s) in the blast radius were not formally verified this run

Test selection

Test selection

180 of 480 test file(s) selected (38%) via static blast radius.

  • tests/test_alias_folding.py — impact
  • tests/test_analyze.py — impact
  • tests/test_astro_extraction.py — impact
  • tests/test_astro_import_ids.py — impact
  • tests/test_atomic_writes.py — impact
  • tests/test_benchmark.py — impact
  • tests/test_benchmark_raw_graph.py — impact
  • tests/test_benchmark_real_query.py — impact
  • tests/test_binary_media_words.py — impact
  • tests/test_build.py — impact
  • tests/test_build_located_semantic_identity.py — impact
  • tests/test_build_merge_dedup_scope.py — impact
  • tests/test_build_merge_hyperedges_and_prune.py — impact
  • tests/test_build_merge_shrink_guard.py — impact
  • tests/test_c_include_resolution.py — impact
  • tests/test_cache.py — impact
  • tests/test_cache_long_paths.py — impact
  • tests/test_cache_stale_import_target.py — impact
  • tests/test_carried_hyperedge_remap.py — impact
  • tests/test_case_only_decl_ids_4904.py — impact, changed-test
  • tests/test_charmap_encoding.py — impact
  • tests/test_chunking.py — impact
  • tests/test_cjs_module_extension.py — impact
  • tests/test_cli_export.py — impact
  • tests/test_cluster.py — impact
  • tests/test_community_labels_skill.py — impact
  • tests/test_compact_graph_json.py — impact
  • tests/test_confidence.py — impact
  • tests/test_confidence_score_output.py — impact
  • tests/test_corrupt_graph_json.py — impact
  • tests/test_cpp_nested_and_cli.py — impact
  • tests/test_cpp_objc_cross_file_calls.py — impact
  • tests/test_cross_extension_reexport_self_cycle.py — impact
  • tests/test_csharp_method_owner_labels.py — impact
  • tests/test_csharp_object_creation.py — impact
  • tests/test_csharp_partial_classes.py — impact
  • tests/test_csharp_type_resolution.py — impact
  • tests/test_dart.py — impact
  • tests/test_dedup.py — impact
  • tests/test_dedup_remaps_hyperedges.py — impact
  • tests/test_dedup_shrink_refuses_force_write.py — impact
  • tests/test_dedup_survivor_richness.py — impact
  • tests/test_definition_file_portability.py — impact
  • tests/test_detect.py — impact
  • tests/test_dotnet.py — impact
  • tests/test_duplicate_annotation_edges.py — impact
  • tests/test_edge_also_seen.py — impact
  • tests/test_ejs_extractor.py — impact
  • tests/test_evidence_binding.py — impact
  • tests/test_export.py — impact
  • … and 130 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

No difference found (not proven): No behavior difference found in \_\_init\_\_ (not a proof).

The verifier ran both versions of \_\_init\_\_ 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.

No difference found (not proven): No behavior difference found in resolve (not a proof).

The verifier ran both versions of resolve 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.

· 1 more finding(s) on lines outside this diff (see the check run).

safishamsi added a commit that referenced this pull request Nov 1, 2026
…og for 0.9.100

#4908 (case-collision ids) carried the 31->32 bump; #4895 (PHP class-level
initializer const refs) also changes AST output and shares that global bump,
so extend the comment to credit it. Opens the unreleased 0.9.100 section.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
safishamsi added a commit that referenced this pull request Nov 1, 2026
…for video + extractors

The four @ayushcodes10 extractor fixes (#4902 R, #4900 elixir, #4899 dart,
#4897 fortran) change per-file AST output and are AST-cached; they share
the global 31->32 bump already landed via #4908/#4895. Extend the comment
to credit them, and add the 0.9.100 changelog entries for #4842 and the four.

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

Copy link
Copy Markdown
Member

Landed in v0.9.100, now on PyPI (pip install graphifyy==0.9.100). A type and a function differing only in case get distinct node ids instead of colliding. Thanks for the fix, @bercedev.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor Pull request from a returning community contributor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A type and a function named Foo / foo in one file share one node id (JS, TS, Kotlin, C++, PHP, Ruby, Swift)

2 participants