Skip to content

fix(cypher): refuse a name that is not in scope instead of projecting a blank column - #1922

Open
CaptainMittens wants to merge 1 commit into
DeusData:mainfrom
CaptainMittens:fix/reject-out-of-scope-projection
Open

fix(cypher): refuse a name that is not in scope instead of projecting a blank column#1922
CaptainMittens wants to merge 1 commit into
DeusData:mainfrom
CaptainMittens:fix/reject-out-of-scope-projection

Conversation

@CaptainMittens

Copy link
Copy Markdown

Fixes #1919.

What was wrong

A RETURN could name a variable the WITH before it dropped. The query ran,
exited clean, printed a column for that name, and filled it with empty strings.
Nothing reported an error.

MATCH (f:Function)-[:CALLS]->(g) WITH f.name AS caller RETURN caller, g.name

g does not survive the WITH. Measured through query_graph on a real
indexed project before the fix:

rows: 5  (cols: caller g.name)
  appWith          -
  send             -
  appWithNoPartner -

The silence is the harm. A reader — a person or an agent — sees a column of
nothing and reads it as "the graph holds no such data". The true answer is
"your query named something that is out of scope". For a code graph, a false
"nothing calls X" is the worst answer it can give.

This is the same failure shape as #373, where an unknown function used to
project a blank column and now fails loudly. Same answer here.

The fix

One check on the parsed query, between parse and execute. Every name a RETURN
or WITH item uses must be a variable some pattern declares, or an alias the
previous WITH made.

The check reads the parsed query, not the run-time bindings, and that split is
the whole point.
It asks whether the query DECLARED the name — a different
question from whether a row happened to bind it. So the existing convention at
cypher.c:3345 and :3350 is untouched: an OPTIONAL MATCH target that
matched nothing is still declared, still legal, and still projects "". One of
the three tests below holds that line.

Two placeholders are skipped, because neither names anything the query
declared: count(*) stores "*" (cypher.c:1614) and a CASE expression
stores "CASE" (cypher.c:1643). Both were found by the existing suite —
skipping them turned 7 red tests green.

A query with more names than the guard can model skips the check rather than
guessing. A wrong refusal costs the caller a working query, which is worse than
the silence this removes.

The error names the offending variable and the clause, because a message that
does not say WHICH name is wrong sends the reader back to guessing:

variable 'g' is not in scope for RETURN — the WITH clause did not carry it through

Tests

Three added, red before the change:

Test What it holds
cypher_rejects_projection_of_dropped_with_var_issue1919 the fault — must error and name g
cypher_optional_match_target_still_allowed_issue1919 an unmatched OPTIONAL MATCH target must still project ""
cypher_with_alias_stays_in_scope_issue1919 a WITH alias, and a variable carried through whole, still work

Red-green, both runs quoted:

before:  cypher_rejects_projection_of_dropped_with_var_issue1919  FAIL  ASSERT(rc != 0)
         185 passed, 1 failed
after:   cypher_rejects_projection_of_dropped_with_var_issue1919  PASS
         186 passed

The two guard tests passed before the change as well as after, which is what
makes them useful — they were there to catch a fix that over-rejects, and they
did: a first attempt broke 7 existing tests before the placeholder skip.

Gates run locally

Both through the entry points CONTRIBUTING.md names, on macOS arm64:

Gate Result
scripts/lint.sh --ci exit 0 — "All linters passed"
scripts/test.sh --suites cypher exit 0 — 186 passed, 0 failed

The full suite was not run locally. CI covers it.

Scope

One issue, one fix. This does not touch #1918, which fixes a neighbouring fault
in RETURN * and can land in either order — the two change different
functions.

Names inside a CASE expression and inside a multi-argument function's
arguments are still unchecked. Widening the check there is a separate change
with its own over-rejection risk.

… a blank

A RETURN could name a variable the WITH before it dropped. The query ran,
exited clean, printed a column for that name, and filled it with empty
strings. Nothing said anything was wrong.

That silence is the harm. A reader sees a column of nothing and reads it as
"the graph holds no such data". The true answer is "your query named
something that is out of scope". For a code graph, a false "nothing calls X"
is the worst answer it can give.

The check runs on the parsed query rather than on the run-time bindings, and
that split is the whole point. It asks whether the query DECLARED the name,
which is a different question from whether a row happened to bind it. So the
existing convention is untouched: an OPTIONAL MATCH target that matched
nothing is still declared, still legal, and still projects "".

Two placeholders are skipped, because neither names anything the query
declared: count(*) stores "*", and a CASE expression stores "CASE".

A query with more names than the guard can model skips the check rather than
guessing. A wrong refusal costs the caller a working query, which is worse
than the silence this removes.

Same failure shape as DeusData#373, and the same answer: say so out loud.

Fixes DeusData#1919

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Joshua Richter <jrichter5781@gmail.com>
@github-actions

Copy link
Copy Markdown

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

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.

query_graph: a variable the WITH clause dropped is still accepted, and its column comes back empty with no error

1 participant