Don't check variant exhaustiveness if a fallback variant is provided

[jakubadamw]

Why are we making this change?

Sometimes it is useful to capture the name of a variant of a GraphQL enum into cynic's fallback variant without enumerating all the existing variants in the cynic-mapped Rust enum. In such case one may want to opt out of exhaustiveness.

What effects does this change have?

Enums that contain a fallback variant are excluded from the exhaustiveness check.

I am not sure about this change – perhaps it should be controlled by another cynic(non_exhaustive) attribute.

[netlify]

✅ Deploy Preview for cynic-querygen-web canceled.

Name	Link
🔨 Latest commit	1d5d6ab
🔍 Latest deploy log	https://app.netlify.com/sites/cynic-querygen-web/deploys/659d2e959b8aa400084ffb09

[obmarg]

I am not sure about this change – perhaps it should be controlled by another cynic(non_exhaustive) attribute.

I think the change in this PR does make sense. It seems a reasonable default - quite consistent with how exhaustiveness is handled in the InlineFragments derive, and I'm happy with that.

Though I wonder if we should be adding a #[cynic(exhaustive)] attribute to the Enum derive, similar to the one that exists on InlineFragments. That way cynic can continue serving the use case where a user adds a fallback for forwards compatibility, but uses compiler errors to make sure their newest builds don't miss schema changes.

What do you think?

[jakubadamw]

@obmarg sounds good! Would you like me to add the attribute in this PR? I can do that, just wondering if it should be a separate change.

[jakubadamw]

However, now that I think about it, having exhaustiveness checking enabled is a sane default for enums. It's what one wants most of the time, I think. No?

[obmarg]

However, now that I think about it, having exhaustiveness checking enabled is a sane default for enums. It's what one wants most of the time, I think. No?

It's definitely the best default when you don't have a fallback. When you do have a fallback.... I am not sure. I can see an argument for both approaches there.

The tactic I've used in the past has been to ensure that cynic continues compiling in the face of changes, unless those changes would break the runtime queries. That was the line of reasoning I used for suggesting #[cynic(exhaustive)] - queries would still run as expected and the client code has a fallback so presumably should handle the new variants just fine.

But at the same time, yes - exhaustiveness checking on enums does seem like a thing most users would want...

Not to abdicate my responsibilities. but I might leave this decision up to you. It needs to be configurable one way or the other - but it's your feature so whatever makes the most sense to you.

Would you like me to add the attribute in this PR? I can do that, just wondering if it should be a separate change.

Yeah, this PR is fine unless you particularly want to make another one.

I can also get round to it myself (eventually) if you've not got the time - let me know if so.

[jakubadamw]

@obmarg thank you for your guidance, this makes sense!

I went ahead and added one more commit that adds a non_exhaustive attribute to enums.

The rule is as follows:
enums with the non_exhaustive attribute must have a fallback variant.
If non_exhaustive is present, exhaustiveness is not checked, and the fallback variant is used for all the variants missing from the selection, both that were known from the enum definition, and those which were not.
If non_exhaustive is not present (that is, the enum is considerd exhaustive), any selection on a value of an enum must exhaust all its variants, and the fallback variant is used for any future variants added in the API that weren't present in the schema when the client code was compiled.

What do you think?

[obmarg]

Nice one @jakubadamw - that all seems pretty sensible.

Thanks for doing that.

[obmarg]

Released in v3.4.0