fix(cypher): refuse a name that is not in scope instead of projecting a blank column - #1922
Conversation
… 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>
|
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. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
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. |
|
Merged as The framing in your description is the reason this went in quickly, and it is worth repeating back because it is the right standard for this project: "The silence is the harm." A blank column reads as "the graph holds no such data" when the truth is "your query named something that is out of scope" — and for a code graph a false nothing calls X is the worst answer it can give. That is a graph-quality argument, not a validation nicety, and it is why this is a fix rather than a feature. Two things made it reviewable without a round trip: You drew the line the fix could have blurred, and then tested it. Checking the parsed query rather than the run-time bindings is the whole distinction — "did the query declare this name" is a different question from "did a row happen to bind it". That is what keeps an Two negative controls, not just the reproduction. The Citing #373 as the same failure shape was also the right move — it made the precedent do the arguing instead of the description. Thank you. 35 checks green, no round trips needed. |
Fixes #1919.
What was wrong
A
RETURNcould name a variable theWITHbefore it dropped. The query ran,exited clean, printed a column for that name, and filled it with empty strings.
Nothing reported an error.
gdoes not survive theWITH. Measured throughquery_graphon a realindexed project before the fix:
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
RETURNor
WITHitem uses must be a variable some pattern declares, or an alias theprevious
WITHmade.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:3345and:3350is untouched: anOPTIONAL MATCHtarget thatmatched nothing is still declared, still legal, and still projects
"". One ofthe three tests below holds that line.
Two placeholders are skipped, because neither names anything the query
declared:
count(*)stores"*"(cypher.c:1614) and aCASEexpressionstores
"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:
Tests
Three added, red before the change:
cypher_rejects_projection_of_dropped_with_var_issue1919gcypher_optional_match_target_still_allowed_issue1919OPTIONAL MATCHtarget must still project""cypher_with_alias_stays_in_scope_issue1919WITHalias, and a variable carried through whole, still workRed-green, both runs quoted:
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.mdnames, on macOS arm64:scripts/lint.sh --ciscripts/test.sh --suites cypherThe 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 differentfunctions.
Names inside a
CASEexpression and inside a multi-argument function'sarguments are still unchecked. Widening the check there is a separate change
with its own over-rejection risk.