represent parameterised grammar (const values)

[lrlna]

We don't correctly diagnose incorrectly parameterised values on the parser side of things. For example, Directive in a VariableDefinition must not have a Variable as its Argument's Value. spec. We should produce this diagnostic in the parser.

Perhaps the most straightforward way to do this is to have two versions of the the Value enum - one with Variable and the other one without, and then propagate that up to Arguments, Argument, Directives and Directive.

Would be useful to have that enum variant type representation in rust here, but alas not possible in the language yet :/

[SimonSapin]

Would it be worth making const-ness a type parameter?

enum Value<C: Constness> {
    Null,
    Boolean(bool),
    // …
    Variable(C::Variable),
}

trait Constness {
    type Variable;
}

struct NotConst;
impl Constness for NotConst {
    type Variable = Name;
}

struct Const;
impl ConstNess for Const {
    type Variable = Never;
}
enum Never {}

Or without the indirection:

enum MaybeConstValue<Variable> {
    Null,
    Boolean(bool),
    // …
    Variable(Variable),
}

type Value = MaybeConstValue<Name>;
type ConstValue = MaybeConstValue<Never>;
enum Never {}

[goto-bus-stop]

With the Never pattern you don't need to match on ConstValue::Variable(_) right?

The constness has to propagate through a few levels (Directives -> Arguments -> Value), and users will have to write functions that are generic over constness. So I think having a clearly named Constness trait that they can use as a bound is a good idea.

Maybe an ergonomic consideration: if you have a function that operates on a not-const value you can't put a const value in unless you make it generic over constness or use a conversion function.

[SimonSapin]

I tried it, with Never you’d need a match arm like this:

ConstValue::Variable(never) => match never {}

… which type-checks for any expected type. "All" of the zero arms of the inner match for each variant of Never have a value of the expected type.

It’s only with the unstable ! type that the Variable match arm could be skipped entirely.

[SimonSapin]

To avoid zero-variant-enum weirdness, another possibility is separate Value and ConstValue enums. Then Argument, Directive, etc can be generic over the value type.

[goto-bus-stop]

Both have tradeoffs, you can't generically match on two unrelated enum types, but the incantation you have to do for the never branch is also not great. Maybe the former isn't such a problem if you can easily convert ConstValue to Value and just use that whenever you want to handle both kinds of values.

[SimonSapin]

#777 fixed the parser to emit errors for variable in const values. I’ve opened #852 to do the same in validation.

This leaves for this issue the possibility of encoding value constness in the Rust type system, which is not a bug fix but a design decision. Based on the experience that two separate DirectiveList types are inconvenient, I’m inclined to not have two entirely separate Value and ConstValue enums.

This leaves the possibility of a generic enum, but I’m not sure it’s worth the complexity. The status quo is not so bad:

match value_expected_to_be_const {
    Value::Variable(_) => unreachable!()
    // Handle other cases
}

With #852, if the value is from a Valid<_> document we can be reasonably confident that code like the above won’t panic.

[SimonSapin]

With the Never pattern you don't need to match on ConstValue::Variable(_) right?

You don’t anymore! (Though Router is stuck on an older toolchain right now) https://blog.rust-lang.org/2024/10/17/Rust-1.82.0.html#omitting-empty-types-in-pattern-matching

[goto-bus-stop]

that does help!

i'm not opposed to using a generic enum, but i'm also pretty happy with the status quo (+ #852).

[SimonSapin]

#777 and #925 ensure that variable values used in const contexts are treated as errors during parsing and validation. Making them further unrepresentable in the Rust data structure would be possible but probably not worth the API complexity, so we’ve decided to stick with the current representation for apollo-compiler 1.0: Value is a single Rust enum that always includes a Variable variant, even where using that variant would be invalid.