OneOf Inhabitability

[jbellenger]

This PR implements the validation described at graphql/graphql-spec#1211

More discussion is at the link above, but the core issue is around uninhabited OneOf types -- the property that a type can be defined in a way that a value cannot be generated for it.

The simplest example of an uninhabited type is this recursive OneOf:

input A @oneOf { a:A }

While this type definition is currently valid by the rules of OneOf types and circular input types, it violates the spirit of the Circular References because it does not allow construction of a finite value.

This PR updates the existing cycle validation to consider OneOf types. I've tried to follow the existing conventions, though I'm also new here and might have missed something. Any feedback on style or substance is welcome.

[vercel]

@jbellenger is attempting to deploy a commit to the The GraphQL Foundation Team on Vercel.

A member of the Team first needs to authorize it.

[benjie]

Great tests! (It's 10:30pm so I've not tried to review the code... 😅)

[yaacovCR]

@jbellenger

I finally think I understand everything here and this is awesome.

I pushed some changes/reorg added another memoization layer.

[yaacovCR]

@jbellenger

to add some detail

• meaning adding another layer of memoization in b9acf03 grabbed back some of the perf drop
• using that same map when reporting in 9ed3ad6 / HEAD doesn't add much perf wise, although it perhaps cleans the code a bit (?)

it's all somewhat slower than baseline 17.x.x but of course it's doing more (and correct) validation.

[yaacovCR]

ok, we now have parity:

we avoid a full reachability calculation, but just iterate through all input objects once saving metadata, and then backwards propagate

[yaacovCR]

@jbellenger

Do we want (A) an error per input object cycles, or do we want (B) one error per input object listing all the cycles it has? Let's assume that both options filter so that we never report the same cycle more than once, but see the change in the last commit, we do (B), which would apply to non-OneOf cycles as well? In v16 (and prior to that commit ) we did (A), and have for a while with regard to non-OneOf cycles, but I think whatever we do should be consistent for both.

I feel like @benjie weighed in on this at the graphql-wg discussion about it?

[jbellenger]

@yaacovCR Thanks for the polish you've put on this!

Re reporting, I think the wg discussion was here: https://youtu.be/0JP4x-Uf2qw?si=qhrpHaOaKFxYKsoO&t=997

My takeaway was that we'd want one error per cause, which I think means option B. This would also have the nice property of providing complete diagnostic information to an agent.

[yaacovCR]

Shoot, I edited my comment. I think A and B were technically the same but I think it's right now.

[yaacovCR]

Hmmm I guess the preference I read was toward A ie keep doing what we were doing, we should get a different error per cycle as the cycle was the cause not the input object per se…. I’m going to roll back that last commit…..

[yaacovCR]

Basically, we can table that debate for this PR as it also changes how errors have been reported with regard to circular Input Objects for quite some time. If we want to change how multiple cycles starting at the same Input Object works, we should do so in lockstep across non-OneOf and OneOf cycles, and do so in a separate PR. I am pretty much agnostic, leaning toward grouping together; at the bottom line, we have all of the information in both variations both with regards to the messages and erroring location, the question is only the grouping.

[jbellenger]

@yaacovCR

If we want to change how multiple cycles starting at the same Input Object works, we should do so in lockstep across non-OneOf and OneOf cycles, and do so in a separate PR.

Big plus one.