Repository navigation
Conversation
|
Tick the box to add this pull request to the merge queue (same as
|
There was a problem hiding this comment.
🟡 Changes recommended
The new unused-fragment warning logic can produce false-positive warnings when fragment spreads occur inside spliced subqueries, which can break builds under fatal-warnings CI.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds compile-time warning support for unused GraphQL variable declarations and fragment definitions in the gql interpolator, replacing reliance on Grackle’s unused checks (which are unreliable around spliced subqueries). This fits the codebase by keeping core Grackle-free while still providing earlier feedback to users writing gql"...$subquery..." documents.
Changes:
- Introduce
GraphQLTextto do lightweight text scanning ofgqlliteral parts (variable defs, variable refs, fragment defs/spreads). - Update
GraphQLInterpolatorto useGraphQLTextand emit warnings for unused variables and fragments at macro expansion time. - Add unit tests for the text-scanning behavior and adjust generator fixtures accordingly.
File summaries
| File | Description |
|---|---|
| gen/rules/src/main/scala/clue/gen/QueryGen.scala | Documents that unused checks are now handled at gql macro compile time rather than via Grackle in the generator. |
| gen/input/src/main/scala/test/StarWarsUnusedVariable.scala | Removes an input fixture that depended on unused-variable diagnostics being disabled in the generator path. |
| core/src/main/scala/clue/GraphQLText.scala | New text-scanning utility for gql macro warnings (vars/fragments). |
| core/src/main/scala/clue/GraphQLInterpolator.scala | Switches macro logic to GraphQLText and adds warning emission for unused declarations. |
| core/src/test/scala/clue/GraphQLTextSuite.scala | New unit tests covering parsing and unused-vars/fragments detection logic. |
| core/src/test/scala/clue/GraphQLInterpolatorSuite.scala | Adds macro-level “positive case” tests to guard against false positives under fatal-warnings CI. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| GraphQLText.unusedFragments(bodyText).foreach { name => | ||
| report.warning(s"gql: fragment '$name' is defined but never spread", | ||
| Position.ofMacroExpansion | ||
| ) | ||
| } |
There was a problem hiding this comment.
🟡 Changes recommended
It introduces misleading/incorrect scaladoc claims in new/updated documentation that should be corrected to match actual behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
| * A subquery is self-contained: it is validated standalone against its root type, so it cannot | ||
| * spread fragments defined by the document that splices it (nor define fragments itself). The | ||
| * `gql` unused-fragment warning relies on this: it only looks at the splicing document's own text. |
| * Blank out GraphQL comments (`#` to end of line) and string literals (`"..."` with `\"` escapes, | ||
| * and `"""..."""` block strings), replacing every character of them with a space so positions and | ||
| * line structure are preserved. Applied before any scan, so text inside them can neither declare | ||
| * nor use anything. |
There was a problem hiding this comment.
There is a lot of string manipulation and things I don't really understand going on 😅. AI found some issues.
Side-note: would it be possible to have Grackle as a compile-time-only dependency to handle parsing? That might be easier and more robust than all these regular expressions and string splitting.
| def headerSpan(h: String): Option[(Int, Int)] = { | ||
| val open = h.indexOf('(') | ||
| val brace = h.indexOf('{') | ||
| if (open < 0 || (brace >= 0 && brace < open)) None |
There was a problem hiding this comment.
headerSpan bails when any { precedes the first (, so a document with fragment definitions before the operation (fragment f on C { id } query ($ep: Episode!) …) loses operationVars. Splicing a subquery into such a document now fails with a false compile error "operation does not declare variable $ep". The old operationVars had no brace guard and handled it; gen/input/.../StarWarsQuery2.scala is an existing fixture of this exact shape. Guard should key off the query/mutation/subscription keyword instead.
| * header is excluded from the body so its `$name` declarations don't count as usages. | ||
| */ | ||
| def parse(parts: List[String]): Parsed = { | ||
| val strippedParts = parts.map(p => stripCommentsAndStrings(p.replace("$$", "$"))) |
There was a problem hiding this comment.
String literals are stripped per part, so a GraphQL string containing a splice leaves the closing " at the head of the next part, where it's read as an opening quote and blanks the remainder — losing real $var usages and spreads, producing false unused warnings (noise, not the documented "err on the side of silence").
|
I had considered using grackle for parsing, but it introduces it as a dependency for clue-core, which we don't want. Anyway, I ran your comments through Claude and here's the answer. It recommends a small lexer, which is way better than the current regex parser. I'll send another PR migrating to the lexer and then rebase this PR on top of that one. Both of Hugo's bug reports are valid, and they point at the same root cause: the scan works on raw characters per part instead of on GraphQL tokens over the whole document. On the grackle question my answer is no, for a reason specific to our ecosystem. → Bug 1, header detection: confirmed. StarWarsQuery2 has its fragment before the query, so the first { precedes the header's ( and headerSpan gives up. The fixture passes only because it splices nothing; the same shape with a $Sub that requires a variable gets a false "does not declare variable" compile error. The pre-PR code didn't have that guard, but it had the opposite hole (a ( inside a fragment's field arguments was taken as the header). Keying off the query/mutation/subscription keyword is the correct fix. → Bug 2, a string literal spanning a splice: confirmed. Stripping per part leaves the closing quote at the head of the next part, which then blanks the rest of the document. Exotic input, but the fix is trivial: join the parts around placeholders first, strip once. → Grackle in the macro: I'd advise against it, and not for purity reasons. A Scala 3 macro's implementation classes load inside the compiler, so anything it depends on must be on the downstream project's compile classpath. There's no "compile-time only" scope for that: grackle would become a normal transitive dependency of clue-core, JVM and JS, for every consumer, including ones that never generate code. The concrete problem is lucuma-odb. It is a grackle server and it also depends on clue-core for the ITC client. clue-core pinning a grackle version would put odb in a diamond, forcing a lockstep upgrade every time either side bumps grackle. The Scala.js bundle would be fine (dead-code elimination), but that coupling isn't. → What I'd do instead: a small GraphQL lexer, not regexes. The GraphQL lexical grammar is tiny and fully specified: punctuators, names, numbers, strings with escapes and block strings, # comments. About 60 to 80 lines, unit-testable, no dependency. Then every check becomes a token walk: header variables are the $name : Type groups after the operation keyword, usages are $name tokens anywhere else, fragment Name on defines, ... Name spreads. Both of Hugo's bugs disappear structurally, strings and comments stop being special cases, and the code reads like the spec rather than like string surgery, which was his real complaint. → A useful side effect: tokenizing the joined document means a syntax error, like an unbalanced brace, becomes detectable at compile time for free later, without grackle. |
|
Closing in favor of https://github.com/gemini-hlsw/clue/compare/validate-unused-vars-2?expand=1 |
Since grackle can't drill down spliced subqueries, we do it ourselves. The machinery is already there.
One caveat: grackle checked for unused fragments. So we now do that ourselves too.