Skip to content
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.

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

Allow naked LitExpr::LitPath in expression parsing context #6784

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Feb 12, 2025

PR #5994 introduced the $(...) wrapper for injecting LitExpr JSON expression literals into selection syntax. Along with -> method arguments, the $(...) wrapper is one of two ways to enter an expression parsing context, where the syntax to be parsed is closer to JSON than to GraphQL, and thus more convenient for crafting new/input data, as compared with the selection syntax, which is oriented towards selecting/transforming existing data.

Once parsing has entered the expression context, you shouldn't need to keep using $(...) to wrap JSON expressions. For many literal expressions, this is true: for example, you can write $({ a: 1, b: 2 }) rather than $({ a: $(1), b: $(2) }). However, if you wanted to apply a -> method or grab a .key from a JSON literal expression, the grammar previously required you to wrap that expression with $(...), as in $({ slice: $("asdf")->slice(0, 2) }). This PR allows that expression to be written without the inner $("asdf") wrapper, giving $({ slice: "asdf"->slice(0, 2) }), which is arguably easier to read.

This new syntax works for any JSON literal, not just strings, so you can have 1234->add(1111) or even { a: 1, b: 2 }.b or [1, 2, 3]->last as LitPath expressions. Two cases here are worth highlighting:

  1. Previously, a string literal expression with a path, like "asdf"->first, would have been interpreted/parsed as a quoted field selection, equivalent to $.asdf->first, meaning the "asdf" is not really a string literal, and "asdf"->first probably would not return the string "a". The quoted field syntax is useful for non-identifier field names, but in my opinion the string literal interpretation is much less surprising (in the LitExpr parsing mode) than the field interpretation. This PR changes the meaning of that syntax, but only within the expression parsing context. If you want to select a quoted field now (in an expression context), you can still write $."quoted field"->last, using the $. to disambiguate, just as you can do in a selection context. Three other JSON literal have a similar ambiguity: null->..., true->... and false->... refer to input fields called null, true, and false (respectively) in the outer selection syntax, but now refer to the corresponding JSON values in an expression context (again, much less surprising than before, IMO).

  2. JSON number literals are allowed to be negative, using the - unary operator like most programming languages. However, if you write -1->add(10) in an expression context, it's important to know the -> has a lower precedence than the -, so the -1 gets parsed first, and then ->add(10) operates on that negative value, giving 9 rather than -11. You might expect -11 if you're used to - having a lower precedence, negating the whole expression at the end, but that's not how it works here. The LitPath expression -1->add(10) is equivalent to $(-1)->add(10), in other words.

Thanks to @nicholascioli for noticing that "quoted field"->first was previously mis-parsing in an expression context, because our greedy parser was parsing "quoted field" as a string literal and then failing on the unexpected ->first suffix. Among the other benefits, this PR should fix that problem.

@benjamn benjamn self-assigned this Feb 12, 2025

This comment has been minimized.

@svc-apollo-docs
Copy link
Collaborator

svc-apollo-docs commented Feb 12, 2025

✅ Docs preview has no changes

The preview was not built because there were no changes.

Build ID: 941c9ecfff271e4dc4ff3692

@benjamn benjamn force-pushed the benjamn/allow-naked-LitPath-in-LitExpr-parsing-context branch from cda0e17 to f5dffa9 Compare February 12, 2025 18:05
Comment on lines -102 to +103
LitExpr ::= LitPrimitive | LitObject | LitArray | PathSelection
LitExpr ::= LitPath | LitPrimitive | LitObject | LitArray | PathSelection
LitPath ::= (LitPrimitive | LitObject | LitArray) PathStep+
Copy link
Member Author

Choose a reason for hiding this comment

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

It's important that LitPath is tried first in the list of LitExpr alternatives, because a LitPath always has a prefix that looks like one of the other JSON value types.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's also important that we never allow a PathSelection to appear at the beginning of the LitPath (only (LitPrimitive | LitObject | LitArray)), because then the boundary between the PathSelection and the PathStep+ suffix would be ambiguous.

@benjamn benjamn force-pushed the benjamn/allow-naked-LitPath-in-LitExpr-parsing-context branch from f5dffa9 to a76291c Compare February 12, 2025 18:11
@benjamn benjamn marked this pull request as ready for review February 12, 2025 18:19
@benjamn benjamn requested review from a team as code owners February 12, 2025 18:19
@benjamn benjamn force-pushed the benjamn/allow-naked-LitPath-in-LitExpr-parsing-context branch 3 times, most recently from 3969300 to 8704bbf Compare February 13, 2025 21:00
@benjamn benjamn force-pushed the benjamn/allow-naked-LitPath-in-LitExpr-parsing-context branch from 8704bbf to 1a21c1e Compare February 14, 2025 13:30
@benjamn benjamn force-pushed the benjamn/allow-naked-LitPath-in-LitExpr-parsing-context branch from bfef28c to 98f956a Compare February 14, 2025 17:28
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.

2 participants