Skip to content

fix(csharp): keep member nodes out of the type-definition index - #3213

Open
durmazoguzhan wants to merge 1 commit into
Graphify-Labs:v8from
durmazoguzhan:fix-csharp-member-nodes-poison-type-index
Open

fix(csharp): keep member nodes out of the type-definition index#3213
durmazoguzhan wants to merge 1 commit into
Graphify-Labs:v8from
durmazoguzhan:fix-csharp-member-nodes-poison-type-index

Conversation

@durmazoguzhan

Copy link
Copy Markdown
Contributor

Closes #3212.

_build_csharp_type_def_index treats every sourced .cs code node with a plain identifier label as a type declaration. The property nodes from #3006 and the enum member nodes from #3063 are exactly that shape, so they entered the index and competed with real declarations for a (namespace, name) key.

The index does not require the key to be unique. It sorts colliding entries on (source_file, source_location, id) and takes the first, so file name order picked the winner, and a member captured every reference to the type it shares a name with, across the whole corpus. public Widget Widget { get; set; } is enough to trigger it.

Both of those PRs are mine, so this is my regression.

The change

A member node is stamped is_member at emission, and the index skips it. That is the same shape as the is_nested_type skip two lines above, and it keeps the decision at the one place that knows what the node is.

Nothing is removed: the member node, its defines / case_of edge, and its namespace metadata are untouched. add_node merges the stamp with what it fills in itself, and there is a test pinning that the namespace survives, because every C# resolution pass keys on it.

Measured

One .NET service, 1,497 .cs files:

before after
index entries 4,197 1,438
entries whose winner is a member 2,777 0
of those, shadowing a real declaration 18 0
references edges landing on a member 75 0
calls edges landing on a member 43 9

The 18 are the public ReviewInfo ReviewInfo { get; set; } idiom, where the property and the class it names share a namespace.

The 9 that remain

Nine new Foo() edges still reach a property node. They do not come through this index, which now holds no members at all, and I could not reduce them to a minimal case: the same three-file shape built by hand produces no call edge on either side of the change. Rather than widen this PR on a path I cannot demonstrate, I am reporting them as a remainder. They were 43 before this change.

Tests

Nine tests in tests/test_csharp_type_resolution.py, where the C# resolution tests and their helpers already live. Seven fail without the change; the other two are the regression guard and the control, which are meant to pass either way.

Two of them needed a second pass. Written the obvious way, they asserted that a resolved target does not carry the is_member stamp, which is this change's own marker, so on the unfixed code, where nothing carries it, they passed and proved nothing. They now use oracles that exist on both sides: the target is the node its class points at with defines, and a cross-namespace reference must land on a sourceless stub.

The cases: the index unit itself (both a collision and a member-only key), a property shadowing a type in another file, a property shadowing a type in the same file, a cross-namespace reference that must dangle instead of binding to a member, an enum member shadowing a type, an enum nested in a class whose members are not nested types and need their own stamp, that member nodes and their edges survive, that the stamp does not displace the namespace metadata, and a control where nothing shadows.

Full suite: 5,130 passed. ruff clean.

…hify-Labs#3212)

`_build_csharp_type_def_index` treated every sourced .cs code node with a plain
identifier label as a type declaration, so the property nodes from Graphify-Labs#3006 and the
enum member nodes from Graphify-Labs#3063 entered it and competed with real declarations for
a (namespace, name) key. The index does not require the key to be unique: it
sorts colliding entries on (source_file, source_location, id) and takes the
first, so file name order decided the winner and a member captured every
reference to the type it shares a name with, corpus-wide.

`public Widget Widget { get; set; }` is ordinary C# and is enough to trigger it.
Measured on a 1,497-file service: 2,777 of 4,197 index entries were won by a
member, 18 of them shadowing a real declaration, and 75 references plus 43 calls
landed on members. After this change the index holds 1,438 entries, none of them
a member, and no references reach a member.

Both member-node PRs are mine, so this is my regression. The stamp goes on the
node at emission and the index skips it, the same shape as the is_nested_type
skip two lines above.

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

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

Stops C# property and enum-member nodes from being treated as type declarations by stamping them is_member and skipping any such node in _build_csharp_type_def_index. Previously a member sharing a name with a real type could win the (namespace, name) key on the sort tiebreak, so references like public Widget Widget { get; set; } bound to the property instead of the class (and out-of-scope references bound to a member rather than dangling); now the real declaration always holds the key and unreachable references correctly dangle. Member nodes and their defines/case_of edges are otherwise unchanged.

No blocking issues surfaced. 1 lower-confidence candidate did not survive cross-model review.

Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 1752 functions depend on the 287 functions this change touches.

Health — this change adds coupling hotspots:

  • new: extract() — 525 callers, 43 callees
  • new: _rebuild_code() — 108 callers, 50 callees
  • new: _extract_generic() — 18 callers, 25 callees
  • new: extract_js() — 85 callers, 4 callees
  • new: extract_xaml() — 19 callers, 17 callees
  • new: dispatch_command() — 2 callers, 122 callees
  • new: extract_objc() — 27 callers, 9 callees
  • new: extract_julia() — 17 callers, 7 callees
  • …and 22 more — each is listed as a finding

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

Formal verification

No difference found (not proven): No behavior difference found in \_build\_csharp\_type\_def\_index (not a proof).

The verifier ran both versions of \_build\_csharp\_type\_def\_index on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.

Guarantee: Empirical: concolic exploration (CrossHair). 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 \_csharp\_extra\_walk.

The verifier did not have enough to check \_csharp\_extra\_walk, 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: not verifiable: all 200 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly AttributeError — names the real obstacle, not a sampling gap)

Could not verify: Could not verify \_extract\_generic.

The verifier did not have enough to check \_extract\_generic, 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 `Path` — outside the synthesizable primitive/collection set

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

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.

C#: property and enum member nodes enter the type-definition index, so a member can capture every reference to a same-named type

1 participant