Skip to content

fix: preserve case-colliding TypeScript symbols - #3738

Open
hopstreax wants to merge 1 commit into
Graphify-Labs:v8from
hopstreax:fix/3726-case-sensitive-node-ids
Open

hopstreax wants to merge 1 commit into
Graphify-Labs:v8from
hopstreax:fix/3726-case-sensitive-node-ids

Conversation

@hopstreax

Copy link
Copy Markdown
Contributor

Summary

Fixes #3726 by preserving distinct TypeScript/JavaScript symbols whose names differ only by case.

Previously, symbols such as FallListe and fallListe could collapse onto the same normalized node ID, causing lost declarations, incorrect import/re-export targets, and ambiguous graphify explain results.

What changed

  • Added deterministic salting for genuine case-only symbol collisions during extraction.
  • Preserved the exact authored symbol name on JS/TS import and re-export edges.
  • Updated symbol resolution to reconcile existing import edges against the exact extracted symbol.
  • Ensured cross-file imports and re-exports resolve to the correct salted node.
  • Added exact-case precedence to graphify explain.
  • Added ambiguity detection for case-insensitive queries involving same-file case-colliding symbols.
  • Added regression coverage for declarations, imports, aliases, re-exports, and explain lookup.

Validation

  • 185 passed, 1 skipped — collision, explain, and serve tests
  • 90 passed — JS/TS import and forwarding resolution
  • 3 passed — TypeScript/TSX extraction tests
  • 79 passed — import-cycle, relation-precedence, and exported-binding tests
  • git diff --check — clean

No changes were made to the existing global ID normalization behavior; salted IDs are applied only where an actual case collision exists.

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

Worth a look — the grounded gate found no coupling regressions or blocking issues, but 5 advisory finding(s) below merit a look before merge.


Graphify review — findings

Adds case-only collision handling for TypeScript/JavaScript symbol extraction: _ts_pre_scan_case_collisions walks top-level declarations before extraction and, when two distinct authored names casefold to the same node ID (e.g. FallListe vs fallListe), assigns each a deterministic sha1 salt so neither declaration is silently dropped by add_node; overload/merge cases with identical names are left unsalted. Threads the resulting override map through _js_extra_walk, _ts_extra_walk, and _extract_generic so functions, consts, destructured exports, and namespaces use the salted IDs. Stamps JS import edges with imported_symbol, and improves the CLI ambiguity message to report whether matches are in the same file (naming it) or different files and to print each rival's label.

Worth a look

  • Ambiguous re-exports now drop import edges — graphify/extractors/resolution.py:1432 · Escalate · medium
    • agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
  • matched_edge popped from one index remains stale in the other index — graphify/extractors/resolution.py:1464 · Escalate · medium
    • agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
  • Retargeting matched import edges bypasses duplicate-edge guard — graphify/extractors/resolution.py:1446 · Escalate · medium
    • agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
  • func override branch applied without checking namespace_stack/parent context — graphify/extractors/engine.py:5120 · Escalate · medium
    • agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
  • ensure_named_node override ignores namespace_stack so nested references get wrong salted id — graphify/extractors/engine.py:3768 · Escalate · medium
    • agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 3053 functions depend on the 892 functions this change touches.

Health — this change adds coupling hotspots:

  • new: extract() — 661 callers, 45 callees
  • new: _rebuild_code() — 142 callers, 54 callees
  • new: _extract_generic() — 18 callers, 30 callees
  • new: extract_js() — 94 callers, 4 callees
  • new: extract_xaml() — 19 callers, 17 callees
  • new: _resolve_js_module_path() — 34 callers, 9 callees
  • new: _query_graph_text() — 27 callers, 10 callees
  • new: dispatch_command() — 2 callers, 125 callees
  • …and 67 more — each is listed as a finding

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

Test selection

Test selection

157 of 291 test file(s) selected (54%) via static blast radius.

  • tests/test_affected_cli.py — impact
  • tests/test_agents_platform.py — impact
  • tests/test_astro_extraction.py — impact
  • tests/test_astro_import_ids.py — impact
  • tests/test_benchmark.py — impact
  • tests/test_benchmark_raw_graph.py — impact
  • tests/test_build.py — impact
  • tests/test_builtin_global_type_refs.py — impact
  • tests/test_case_sensitive_resolution.py — impact
  • tests/test_cjs_module_extension.py — impact
  • tests/test_codebuddy.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_cross_language_call_resolution.py — impact
  • tests/test_cross_repo_external_call_guards.py — impact
  • tests/test_cross_repo_member_calls.py — impact
  • tests/test_csharp_call_site_generic_args.py — impact
  • tests/test_csharp_enum_members.py — impact
  • tests/test_csharp_field_generic_args.py — impact
  • tests/test_csharp_generic_callsites.py — impact
  • tests/test_csharp_interface_dispatch.py — impact
  • tests/test_csharp_member_calls.py — impact
  • tests/test_csharp_member_nodes.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_definition_file_portability.py — impact
  • tests/test_detect.py — impact
  • tests/test_devin.py — impact
  • tests/test_dotnet.py — impact
  • tests/test_duplicate_annotation_edges.py — impact
  • tests/test_elixir_import_resolution.py — impact
  • tests/test_explain_cli.py — impact, changed-test
  • tests/test_extract.py — impact
  • tests/test_extract_cache_location.py — impact
  • tests/test_extract_cli.py — impact
  • tests/test_file_label_disambiguation.py — impact
  • tests/test_file_node_id_spec.py — impact
  • tests/test_forwarding_review_findings.py — impact
  • tests/test_global_add_tag_inference.py — impact
  • tests/test_go_builtin_call_targets.py — impact
  • tests/test_go_interface_methods.py — impact
  • tests/test_go_qualified_resolution.py — impact
  • tests/test_god_nodes_cli.py — impact
  • tests/test_hollow_chunks_arm_shrink_guard.py — impact
  • tests/test_hook_guard_token_match.py — impact
  • tests/test_hook_out_of_project_paths.py — impact
  • tests/test_hook_strict.py — impact
  • tests/test_import_extension_resolution.py — impact
  • … and 107 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.

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

This branch has not been deployed

No deployments
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.

Node ids are lower-cased per file: interface Foo and function foo collapse, one is silently dropped

1 participant