Skip to content

Add location to error messages #941

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hugo-vrijswijk merged 1 commit into from
Sep 11, 2026
Merged

Add location to error messages #941

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions docs/tutorial/in-memory-model.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,9 +209,10 @@ Grackle's query algebra consists of the following elements,
case class UntypedSelect(
name: String, alias: Option[String],
args: List[Binding], directives: List[Directive],
child: Query
child: Query,
location: Option[(Int, Int)] = None
)
case class Select(name: String, alias: Option[String], child: Query)
case class Select(name: String, alias: Option[String], child: Query, location: Option[(Int, Int)] = None)

@hugo-vrijswijk hugo-vrijswijk Sep 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also a source-breaking change for any users that pattern-match on Select(n, a, c), but I don't know if there is a way around

@milessabin milessabin Sep 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too concerned about this: SelectElatorator should protect most uses from this.

case class Group(queries: List[Query])
case class Unique(child: Query)
case class Filter(pred: Predicate, child: Query)
Expand Down
12 changes: 6 additions & 6 deletions modules/circe/src/test/scala/CirceEffectHandlerErrorSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ final class CirceEffectHandlerErrorSuite extends CatsEffectSuite {
val expected = json"""
{
"errors" : [
{ "message": "value: hi", "path": ["s"] },
{ "message": "value: 42", "path": ["n"] }
{ "message": "value: hi", "locations": [{ "line": 3, "column": 9 }], "path": ["s"] },
{ "message": "value: 42", "locations": [{ "line": 4, "column": 9 }], "path": ["n"] }
],
"data" : null
}
Expand All @@ -62,7 +62,7 @@ final class CirceEffectHandlerErrorSuite extends CatsEffectSuite {
val expected = json"""
{
"errors" : [
{ "message": "value: hi", "path": ["s"] }
{ "message": "value: hi", "locations": [{ "line": 3, "column": 9 }], "path": ["s"] }
],
"data" : null
}
Expand Down Expand Up @@ -111,7 +111,7 @@ final class CirceEffectHandlerErrorSuite extends CatsEffectSuite {
val expected = json"""
{
"errors" : [
{ "message": "boom", "path": ["viaEffect"] }
{ "message": "boom", "locations": [{ "line": 4, "column": 9 }], "path": ["viaEffect"] }
],
"data" : {
"ping" : "pong",
Expand Down Expand Up @@ -167,7 +167,7 @@ final class CirceEffectHandlerErrorSuite extends CatsEffectSuite {
val expected = json"""
{
"errors" : [
{ "message": "boom", "path": ["viaEffect", "name"] }
{ "message": "boom", "locations": [{ "line": 5, "column": 11 }], "path": ["viaEffect", "name"] }
],
"data" : null
}
Expand All @@ -190,7 +190,7 @@ final class CirceEffectHandlerErrorSuite extends CatsEffectSuite {
val expected = json"""
{
"errors" : [
{ "message": "boom", "path": ["child", "viaEffect"] }
{ "message": "boom", "locations": [{ "line": 5, "column": 11 }], "path": ["child", "viaEffect"] }
],
"data" : {
"ping" : "pong",
Expand Down
3 changes: 2 additions & 1 deletion modules/core/src/main/scala/ast.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ object Ast {
name: Name,
arguments: List[(Name, Value)],
directives: List[Directive],
selectionSet: List[Selection]
selectionSet: List[Selection],
location: Option[(Int, Int)] = None
) extends Selection

case class FragmentSpread(
Expand Down
37 changes: 19 additions & 18 deletions modules/core/src/main/scala/compiler.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,16 +156,16 @@ object QueryParser {
* GraphQL errors and warnings are accumulated in the result.
*/
def parseSelection(sel: Selection): Result[Query] = sel match {
case Field(alias, name, args, directives, sels) =>
case Field(alias, name, args, directives, sels, location) =>
for {
args0 <- parseArgs(args)
sels0 <- parseSelections(sels)
dirs <- parseDirectives(directives)
} yield {
val nme = name.value
val alias0 = alias.map(_.value).flatMap(n => if (n == nme) None else Some(n))
if (sels.isEmpty) UntypedSelect(nme, alias0, args0, dirs, Empty)
else UntypedSelect(nme, alias0, args0, dirs, sels0)
val child = if (sels.isEmpty) Empty else sels0
UntypedSelect(nme, alias0, args0, dirs, child, location)
}

case FragmentSpread(Name(name), directives) =>
Expand Down Expand Up @@ -357,7 +357,7 @@ object VariableUsage {

def loop(query: Query, tpe: Type): List[Problem] =
query match {
case UntypedSelect(nme, _, args, dirs, child) =>
case UntypedSelect(nme, _, args, dirs, child, _) =>
val dirProblems = checkDirectives(dirs)
val named = tpe.underlyingNamed
// An unknown field is reported by `SelectElaborator`.
Expand Down Expand Up @@ -881,14 +881,14 @@ object QueryCompiler {
dirs.foldMap(dir => argRefs(dir.args))

def loop(q: Query): (Set[String], Set[String]) = q match {
case UntypedSelect(_, _, args, dirs, child) =>
case UntypedSelect(_, _, args, dirs, child, _) =>
(argRefs(args) ++ dirRefs(dirs), Set.empty[String]) |+| loop(child)
case UntypedFragmentSpread(nme, dirs) =>
(dirRefs(dirs), Set(nme))
case UntypedInlineFragment(_, dirs, child) =>
(dirRefs(dirs), Set.empty[String]) |+| loop(child)
case Group(children) => children.foldMap(loop)
case Select(_, _, child) => loop(child)
case Select(_, _, child, _) => loop(child)
case Narrow(_, child) => loop(child)
case Unique(child) => loop(child)
case Filter(_, child) => loop(child)
Expand Down Expand Up @@ -1195,10 +1195,10 @@ object QueryCompiler {
*/
def transform(query: Query): Elab[Query] =
query match {
case s @ UntypedSelect(fieldName, alias, _, _, child) =>
case s @ UntypedSelect(fieldName, alias, _, _, child, _) =>
transformSelect(fieldName, alias, child).map(ec => s.copy(child = ec))

case s @ Select(fieldName, alias, child) =>
case s @ Select(fieldName, alias, child, _) =>
transformSelect(fieldName, alias, child).map(ec => s.copy(child = ec))

case n @ Narrow(subtpe, child) =>
Expand Down Expand Up @@ -1350,6 +1350,7 @@ object QueryCompiler {
_,
_,
_,
_,
_) =>
(fieldName, level) match {
case ("__typename", Disabled) =>
Expand Down Expand Up @@ -1387,7 +1388,7 @@ object QueryCompiler {
object VariablesSkipAndFragmentElaborator extends Phase {
override def transform(query: Query): Elab[Query] =
query match {
case sel @ UntypedSelect(fieldName, alias, args, dirs, child) =>
case sel @ UntypedSelect(fieldName, alias, args, dirs, child, _) =>
isSkipped(dirs).ifM(
Elab.pure(Empty),
for {
Expand Down Expand Up @@ -1534,7 +1535,7 @@ object QueryCompiler {
trait SelectElaborator extends Phase {
override def transform(query: Query): Elab[Query] =
query match {
case sel @ UntypedSelect(fieldName, resultName, args, dirs, child) =>
case sel @ UntypedSelect(fieldName, resultName, args, dirs, child, _) =>
for {
c <- Elab.context
s <- Elab.schema
Expand All @@ -1561,7 +1562,7 @@ object QueryCompiler {
_ <- Elab.pop
e2 <- elab(ec)
} yield {
val e1 = Select(sel.name, sel.alias, e2)
val e1 = Select(sel.name, sel.alias, e2, sel.location)
val e0 =
if (attrs.isEmpty) e1
else mergeQueries(e1 :: attrs.map { case (nme, child) => Select(nme, child) })
Expand Down Expand Up @@ -1700,7 +1701,7 @@ object QueryCompiler {
extends Phase {
override def transform(query: Query): Elab[Query] =
query match {
case s @ Select(fieldName, resultName, child) =>
case s @ Select(fieldName, resultName, child, _) =>
for {
c <- Elab.context
obj <- Elab.liftR(
Expand Down Expand Up @@ -1758,7 +1759,7 @@ object QueryCompiler {
extends Phase {
override def transform(query: Query): Elab[Query] =
query match {
case s @ Select(fieldName, resultName, child) =>
case s @ Select(fieldName, resultName, child, location) =>
for {
c <- Elab.context
childCtx = c.forFieldOrAttribute(fieldName, resultName)
Expand All @@ -1767,7 +1768,7 @@ object QueryCompiler {
_ <- Elab.pop
} yield effects(c, fieldName) match {
case Some(handler) =>
Select(fieldName, resultName, Effect(handler, s.copy(child = ec)))
Select(fieldName, resultName, Effect(handler, s.copy(child = ec)), location)
case None =>
s.copy(child = ec)
}
Expand Down Expand Up @@ -1827,11 +1828,11 @@ object QueryCompiler {
@tailrec
def loop(q: Query, depth: Int, width: Int): (Int, Int) =
q match {
case UntypedSelect(_, _, _, _, Empty) => (depth + 1, width + 1)
case Select(_, _, Empty) => (depth + 1, width + 1)
case UntypedSelect(_, _, _, _, Empty, _) => (depth + 1, width + 1)
case Select(_, _, Empty, _) => (depth + 1, width + 1)
case Count(_) => (depth + 1, width + 1)
case UntypedSelect(_, _, _, _, child) => loop(child, depth + 1, width)
case Select(_, _, child) => loop(child, depth + 1, width)
case UntypedSelect(_, _, _, _, child, _) => loop(child, depth + 1, width)
case Select(_, _, child, _) => loop(child, depth + 1, width)
case g: Group => handleGroup(g, depth, width)
case Component(_, _, child) => loop(child, depth, width)
case Effect(_, child) => loop(child, depth, width)
Expand Down
22 changes: 16 additions & 6 deletions modules/core/src/main/scala/parser.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ package grackle

import scala.util.matching.Regex

import cats.implicits._
import cats.implicits.*
import cats.parse.{Parser, Parser0}
import cats.parse.Numbers._
import cats.parse.Parser._
import cats.parse.Numbers.*
import cats.parse.Parser.*
import cats.parse.Rfc5234.{cr, crlf, digit, hexdig, lf}

trait GraphQLParser {
Expand Down Expand Up @@ -332,9 +332,19 @@ object GraphQLParser {
}

def Field(n: Int): Parser[Ast.Selection.Field] =
(Alias.backtrack.?.with1 ~ Name ~ Arguments.? ~ Directives ~ SelectionSetN(n).?).map {
case ((((alias, name), args), dirs), sel) =>
Ast.Selection.Field(alias, name, args.orEmpty, dirs, sel.orEmpty)
(caret.with1 ~
(Alias.backtrack.?.with1 ~ Name ~ Arguments.? ~ Directives ~ SelectionSetN(n).?)).map {
case (pos, ((((alias, name), args), dirs), sel)) =>
Ast
.Selection
.Field(
alias,
name,
args.orEmpty,
dirs,
sel.orEmpty,
// The caret counts lines and columns from zero, the specification from one.
Some((pos.line + 1, pos.col + 1)))
}

def InlineFragment(n: Int): Parser[Ast.Selection.InlineFragment] =
Expand Down
16 changes: 10 additions & 6 deletions modules/core/src/main/scala/problem.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,18 +36,22 @@ final case class Problem(
def atPath(path: List[Problem.PathSegment]): Problem =
if (this.path.isEmpty) copy(path = path) else this

/**
* Yields this problem with `locations` as its source locations, if it carries none.
*
* A problem which a deeper position raised already holds the location of that position, so an
* enclosing position leaves it in place.
*/
def atLocations(locations: List[(Int, Int)]): Problem =
if (this.locations.isEmpty) copy(locations = locations) else this

override def toString = {

lazy val pathText: String =
path.mkString("/")

lazy val locationsText: String =
locations
.map {
case (a, b) =>
if (a == b) a.toString else s"$a..$b"
}
.mkString(", ")
locations.map { case (line, column) => s"$line:$column" }.mkString(", ")

val s = (path.nonEmpty, locations.nonEmpty) match {
case (true, true) => s"$message (at $pathText: $locationsText)"
Expand Down
37 changes: 23 additions & 14 deletions modules/core/src/main/scala/query.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,16 @@ object Query {

/**
* Select field `name` possibly aliased, and continue with `child`
*
* `location` holds the line and the column of the selection in the text of the request, if
* the parser recorded them. Both count from one.
*/
case class Select(name: String, alias: Option[String], child: Query) extends Query {
case class Select(
name: String,
alias: Option[String],
child: Query,
location: Option[(Int, Int)] = None)
extends Query {
def resultName: String = alias.getOrElse(name)

def render = {
Expand Down Expand Up @@ -78,7 +86,8 @@ object Query {
alias: Option[String],
args: List[Binding],
directives: List[Directive],
child: Query)
child: Query,
location: Option[(Int, Int)] = None)
extends Query {
def resultName: String = alias.getOrElse(name)

Expand Down Expand Up @@ -311,8 +320,8 @@ object Query {
def rootName(q: Query): Option[(String, Option[String])] = {
def loop(q: Query): Option[(String, Option[String])] =
q match {
case UntypedSelect(name, alias, _, _, _) => Some((name, alias))
case Select(name, alias, _) => Some((name, alias))
case UntypedSelect(name, alias, _, _, _, _) => Some((name, alias))
case Select(name, alias, _, _) => Some((name, alias))
case Environment(_, child) => loop(child)
case TransformCursor(_, child) => loop(child)
case _ => None
Expand All @@ -327,8 +336,8 @@ object Query {
def resultName(q: Query): Option[String] = {
def loop(q: Query): Option[String] =
q match {
case UntypedSelect(name, alias, _, _, _) => Some(alias.getOrElse(name))
case Select(name, alias, _) => Some(alias.getOrElse(name))
case UntypedSelect(name, alias, _, _, _, _) => Some(alias.getOrElse(name))
case Select(name, alias, _, _) => Some(alias.getOrElse(name))
case Environment(_, child) => loop(child)
case TransformCursor(_, child) => loop(child)
case _ => None
Expand Down Expand Up @@ -379,8 +388,8 @@ object Query {
def children(q: Query): List[Query] = {
def loop(q: Query): List[Query] =
q match {
case UntypedSelect(_, _, _, _, child) => ungroup(child)
case Select(_, _, child) => ungroup(child)
case UntypedSelect(_, _, _, _, child, _) => ungroup(child)
case Select(_, _, child, _) => ungroup(child)
case Environment(_, child) => loop(child)
case TransformCursor(_, child) => loop(child)
case _ => Nil
Expand All @@ -395,8 +404,8 @@ object Query {
def extractChild(query: Query): Option[Query] = {
def loop(q: Query): Option[Query] =
q match {
case UntypedSelect(_, _, _, _, child) => Some(child)
case Select(_, _, child) => Some(child)
case UntypedSelect(_, _, _, _, child, _) => Some(child)
case Select(_, _, child, _) => Some(child)
case Environment(_, child) => loop(child)
case TransformCursor(_, child) => loop(child)
case _ => None
Expand Down Expand Up @@ -426,8 +435,8 @@ object Query {
def hasField(query: Query, fieldName: String): Boolean = {
def loop(q: Query): Boolean =
ungroup(q).exists {
case UntypedSelect(`fieldName`, _, _, _, _) => true
case Select(`fieldName`, _, _) => true
case UntypedSelect(`fieldName`, _, _, _, _, _) => true
case Select(`fieldName`, _, _, _) => true
case Environment(_, child) => loop(child)
case TransformCursor(_, child) => loop(child)
case _ => false
Expand All @@ -441,8 +450,8 @@ object Query {
def fieldAlias(query: Query, fieldName: String): Option[String] = {
def loop(q: Query): Option[String] =
ungroup(q).collectFirstSome {
case UntypedSelect(`fieldName`, alias, _, _, _) => alias
case Select(`fieldName`, alias, _) => alias
case UntypedSelect(`fieldName`, alias, _, _, _, _) => alias
case Select(`fieldName`, alias, _, _) => alias
case Environment(_, child) => loop(child)
case TransformCursor(_, child) => loop(child)
case _ => None
Expand Down
Loading
Loading