Skip to content

feat(extract): Variable nodes carry no initializer value — "same decision, different values" duplication is unqueryable #1950

Description

@noelsaw1

Feature request. Variable nodes carry no value/initializer property, so the graph cannot express "the same decision is encoded in several places with different values" — which in my experience is the highest-value duplication class in a real codebase. I'd like to check whether you'd take a patch for this, and get your read on the right place to capture it, before I write one.

Verified against HEAD ec08f76 (post-v0.10.8), not against the older binary I was originally running.

What I hit

I ran cbm as one arm of a blind three-way duplication audit on a ~550-file Python repo, against two human-driven arms using grep. The graph was much faster than both for structural duplication — one SIMILAR_TO query returned a pre-ranked cross-tree duplicate inventory in seconds, including a forked pair of 1,400-line shell scripts in unrelated directories that neither human arm found. That part was excellent and is why I'm filing rather than walking away.

It could not reach the audit's single most valuable finding, and the reason is structural rather than a query I failed to write:

  • Five rival definitions of "what counts as a paid order", with three different value sets. _REVENUE_VALID_STATUSES, _REAL_SALE_STATUSES, PAID_FINANCIAL_STATUSES, MONEY_COLLECTED_STATUSES, plus 11 inline sites. They share no name token and live in five packages with disjoint imports, so identifier search can't find them; and they're module-level constants, so SIMILAR_TO (function-body similarity) doesn't apply either.
  • One BigQuery byte ceiling written in 15 places with 7 different values, under four different constant names — including _MAX_BYTES_BILLED = 2_000_000_000 shipped under a comment reading # 2 GiB hard ceiling.

Both are value-comparison questions. The query I wanted is one line:

MATCH (a:Variable), (b:Variable)
WHERE a.name =~ '.*MAX_BYTES.*' AND a.file_path <> b.file_path AND a.value != b.value
RETURN a.file_path, a.start_line, a.value, b.file_path, b.start_line, b.value

What I found in the source

I first assumed this was cheap — that CBMStringConstantMap already had the data and it just wasn't plumbed through. That's wrong, and I want to say so up front so the ask isn't undersized. Two independent model reviews and my own re-check agree:

Verified at ec08f76
  1. CBMDefinition has no value field. internal/cbm/cbm.h:185-232 — ~30 fields, none carrying an initializer. The four label = "Variable" emission sites are internal/cbm/extract_defs.c:5358, :5497, :6708, :6736.

  2. The property JSON never emits one. build_def_props at src/pipeline/pass_definitions.c:250-319, written via cbm_gbuf_upsert_node at :328-330. get_graph_schema discovers keys dynamically with json_each(nodes.properties) (src/store/store.c:5902-6068), so no migration would be needed — the property simply doesn't exist.

  3. CBMStringConstantMap can't be reused as-is, for two reasons.

    • Ordering. internal/cbm/cbm.c:1318 runs cbm_extract_definitions and :1320 runs cbm_extract_unified — so Variable defs are minted before handle_string_constants (internal/cbm/extract_unified.c:2417) populates the map. At def time the map is empty; after cbm_extract_file_ex returns, the stack-local CBMExtractCtx is gone.
    • String-scalar only. is_string_node (internal/cbm/extract_unified.c:1062-1070) matches only string literal kinds. A numeric initializer (2_000_000_000, 10 * 1024**3) and any tuple/list/set/dict literal fail the gate and are dropped — so both of my motivating cases get zero benefit from exposing the existing map.
  4. The map silently truncates. internal/cbm/extract_unified.c:1052-1057: if (map->count < CBM_MAX_STRING_CONSTANTS) (256) with no else, no counter, no log. Same shape in record_url_builder at :1247-1252. Not the main ask, but relevant if the map is ever surfaced.

So the honest scope is new initializer capture in the definitions pass, not plumbing. The encouraging part: at push_var_def time in internal/cbm/extract_defs.c:5608-5640 the AST node and its value child are already in scope — the code just doesn't read them.

Questions

  1. Would you take this? If Variable.value isn't a direction you want, I'll stop here — no argument from me.
  2. Where would you want the capture? In the defs pass (extract_defs.c, value child already in scope), or as a fixup pass after cbm_extract_unified with the map extended to numeric/collection kinds?
  3. Raw text or parsed? I'd argue for the raw initializer text, truncated (256 chars?), rather than a parsed value: parsing is per-language and lossy, text is directly comparable, and 2 * 1024**3 vs 2_000_000_000 reading differently is useful — that discrepancy was itself one of my findings. But you know the storage budget.
  4. Scope limit? Module-level and class-level constants only, I'd think — locals would flood the index for no benefit. Worth a flag if it costs index time?

Happy to write the patch if the answers point somewhere specific. If you'd rather I split the 256-cap silent truncation into its own issue, say the word.

One note that isn't a request

v0.10.8's "list-valued fields extract correctly instead of collapsing to their first element" caught my eye as possibly adjacent to Q3, but as far as I can tell it's a different code path. Mentioning it in case it's more related than it looks.

Thanks for the tool — the SIMILAR_TO result genuinely found things two careful human passes missed.

Metadata

Metadata

Assignees

No one assigned

    Labels

    cypherCypher query language parser/executor bugs

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions