RFC: Fix ambiguity with null variable values and default values (#418)

* RFC: Fix ambiguity with null variable values and default values

> This is a **behavioral change** which changes how explicit `null` values interact with variable and argument default values. This also changes a validation rule which makes the rule more strict.

There is currently ambiguity and inconsistency in how `null` values are coerced and resolved as part of variable values, default values, and argument values. This inconsistency and ambiguity can allow for `null` values to appear at non-null arguments, which might result in unforseen null-pointer-errors.

This appears in three distinct but related issues:

**Validation: All Variable Usages are Allowed**

The explicit value `null` may be used as a default value for a variable with a nullable type, however this rule asks to treat a variable's type as non-null if it has a default value. Instead this rule should specifically only treat the variable's type as non-null if the default value is not `null`.

Additionally, the `AreTypesCompatible` algorithm is underspecificied, which could lead to further misinterpretation of this validation rule.

**Coercing Variable Values**

`CoerceVariableValues()` allows the explicit `null` value to be used instead of a default value. This can result in a null value flowing to a non-null argument due to the validation rule mentioned above. Instead a default value must be used even when an explicit `null` value is provided. This is also more consistent with the explanation for validation rule "Variable Default Value Is Allowed"

Also, how to treat an explicit `null` value is currently underspecified. While an input object explains that a `null` value should result in an explicit `null` value at the input object field, there is no similar explaination for typical scalar input types. Instead, `CoerceVariableValues()` should explicitly handle the `null` value to make it clear a `null` is the resulting value in the `coercedValues` Map.

**Coercing Argument Values**

The `CoerceArgumentValues()` algorithm is intentionally similar to `CoerceVariableValues()` and suffers from the same inconsistency. Explicit `null` values should not take precedence over default values, and should also be explicitly handled rather than left to underspecified input scalar coercion.

* Updated based on feedback.

This updates this proposal to be a bit broader in scope however much narrower in breaking behavior changes.

Mirroring the changes in graphql/graphql-js#1274, this update better defines the difference between a "required" and "non-null" argument / input field as a non-null typed argument / input-field with a default value is no longer required. As such the validation rule which prohibited queries from using non-null variables and default values has been removed. This also adds clarity to the input field validation - this rule has existed in the GraphQL.js reference implementation however was found missing within the spec.

This also updates the CoerceVariableValues() and CoerceArgumentValues() algorithms to retain explicit null values overriding a default value (minimizing breaking changes), however critically adding additional protection to CoerceArgumentValues() to explicitly block null values from variables - thus allowing the older pattern of passing a nullable variable into a non-null argument while limiting the problematic case of an explicit null value at runtime.

* One step further towards the idealized "from scratch" proposal, this makes it more explicitly clear that changing the effective type of a variable definition is only relevent when supporting legacy clients and suggests that new services should not use this behavior.

I like that this balances a clear description of how this rule should work for existing services along with a stricter and therefore safer future path for new services.

* Editing AreTypesCompatible() to avoid trailing "Otherwise return false" statements for easier reading. Functionality is equivalent.

* Update "All Variable Usages are Allowed" to remove breaking change.

Also attempts to improve clarity and formatting and adds an example case.

* Make related changes to input object coercion rules

* Final review edits

Author: leebyron

spec/Section 3 -- Type System.md

@@ -1263,25 +1263,36 @@ type of an Object or Interface field.
 **Input Coercion**
 
 The value for an input object should be an input object literal or an unordered
-map supplied by a variable, otherwise an error should be thrown. In either
-case, the input object literal or unordered map should not contain any entries
+map supplied by a variable, otherwise an error must be thrown. In either
+case, the input object literal or unordered map must not contain any entries
 with names not defined by a field of this input object type, otherwise an error
-should be thrown.
+must be thrown.
 
 The result of coercion is an unordered map with an entry for each field both
-defined by the input object type and provided with a value. If the value {null}
-was provided, an entry in the coerced unordered map must exist for that field.
-In other words, there is a semantic difference between the explicitly provided
-value {null} versus having not provided a value.
-
-The value of each entry in the coerced unordered map is the result of input
-coercion of the value provided for that field for the type of the field defined
-by the input object type
-
-Any non-nullable field defined by the input object type which does not have
-a corresponding entry in the original value, or is represented by a variable
-which was not provided a value, or for which the value {null} was provided, an
-error should be thrown.
+defined by the input object type and for which a value exists. The resulting map
+is constructed with the following rules:
+
+* If no value is provided for a defined input object field and that field
+  definition provides a default value, the default value should be used. If no
+  default value is provided and the input object field's type is non-null, an
+  error should be thrown. Otherwise, if the field is not required, then no entry
+  is added to the coerced unordered map.
+
+* If the value {null} was provided for an input object field, and the field's
+  type is not a non-null type, an entry in the coerced unordered map is given
+  the value {null}. In other words, there is a semantic difference between the
+  explicitly provided value {null} versus having not provided a value.
+
+* If a literal value is provided for an input object field, an entry in the
+  coerced unordered map is given the result of coercing that value according
+  to the input coercion rules for the type of that field.
+
+* If a variable is provided for an input object field, the runtime value of that
+  variable must be used. If the runtime value is {null} and the field type
+  is non-null, a field error must be thrown. If no runtime value is provided,
+  the variable definition's default value should be used. If the variable
+  definition does not provide a default value, the input object field
+  definition's default value should be used.
 
 Following are examples of input coercion for an input object type with a
 `String` field `a` and a required (non-null) `Int!` field `b`:

spec/Section 5 -- Validation.md

@@ -676,6 +676,7 @@ type Arguments {
   intArgField(intArg: Int): Int
   nonNullBooleanArgField(nonNullBooleanArg: Boolean!): Boolean!
   booleanListArgField(booleanListArg: [Boolean]!): [Boolean]
+  optionalNonNullBooleanArgField(optionalBooleanArg: Boolean! = false): Boolean!
 }
 
 extend type Query {
@@ -710,25 +711,25 @@ and invalid.
   * {arguments} must be the set containing only {argument}.
 
 
-#### Required Non-Null Arguments
+#### Required Arguments
 
   * For each Field or Directive in the document.
   * Let {arguments} be the arguments provided by the Field or Directive.
   * Let {argumentDefinitions} be the set of argument definitions of that Field or Directive.
-  * For each {definition} in {argumentDefinitions}:
-    * Let {type} be the expected type of {definition}.
-    * If {type} is Non-Null:
-      * Let {argumentName} be the name of {definition}.
+  * For each {argumentDefinition} in {argumentDefinitions}:
+    * Let {type} be the expected type of {argumentDefinition}.
+    * Let {defaultValue} be the default value of {argumentDefinition}.
+    * If {type} is Non-Null and {defaultValue} does not exist:
+      * Let {argumentName} be the name of {argumentDefinition}.
       * Let {argument} be the argument in {arguments} named {argumentName}
       * {argument} must exist.
       * Let {value} be the value of {argument}.
       * {value} must not be the {null} literal.
 
 **Explanatory Text**
 
-Arguments can be required. If the argument type is non-null the argument is
-required and furthermore the explicit value {null} may not be provided.
-Otherwise, the argument is optional.
+Arguments can be required. An argument is required if the argument type is
+non-null and does not have a default value. Otherwise, the argument is optional.
 
 For example the following are valid:
 
@@ -752,19 +753,20 @@ fragment goodBooleanArgDefault on Arguments {
 }
 ```
 
-but this is not valid on a non-null argument.
+but this is not valid on a required argument.
 
 ```graphql counter-example
 fragment missingRequiredArg on Arguments {
   nonNullBooleanArgField
 }
 ```
 
-Providing the explicit value {null} is also not valid.
+Providing the explicit value {null} is also not valid since required arguments
+always have a non-null type.
 
 ```graphql counter-example
 fragment missingRequiredArg on Arguments {
-  notNullBooleanArgField(nonNullBooleanArg: null)
+  nonNullBooleanArgField(nonNullBooleanArg: null)
 }
 ```
 
@@ -1358,6 +1360,31 @@ For example the following query will not pass validation.
 ```
 
 
+### Input Object Required Fields
+
+**Formal Specification**
+
+  * For each Input Object in the document.
+    * Let {fields} be the fields provided by that Input Object.
+    * Let {fieldDefinitions} be the set of input field definitions of that Input Object.
+  * For each {fieldDefinition} in {fieldDefinitions}:
+    * Let {type} be the expected type of {fieldDefinition}.
+    * Let {defaultValue} be the default value of {fieldDefinition}.
+    * If {type} is Non-Null and {defaultValue} does not exist:
+      * Let {fieldName} be the name of {fieldDefinition}.
+      * Let {field} be the input field in {fields} named {fieldName}
+      * {field} must exist.
+      * Let {value} be the value of {field}.
+      * {value} must not be the {null} literal.
+
+**Explanatory Text**
+
+Input object fields may be required. Much like a field may have required
+arguments, an input object may have required fields. An input field is required
+if it has a non-null type and does not have a default value. Otherwise, the
+input object field is optional.
+
+
 ## Directives
 
 
@@ -1494,44 +1521,6 @@ fragment HouseTrainedFragment {
 ```
 
 
-### Variable Default Value Is Allowed
-
-**Formal Specification**
-
-  * For every Variable Definition {variableDefinition} in a document
-    * Let {variableType} be the type of {variableDefinition}
-    * Let {defaultValue} be the default value of {variableDefinition}
-    * If {variableType} is Non-null:
-      * {defaultValue} must be undefined.
-
-**Explanatory Text**
-
-Variables defined by operations are allowed to define default values
-if the type of that variable is not non-null.
-
-For example the following query will pass validation.
-
-```graphql example
-query houseTrainedQuery($atOtherHomes: Boolean = true) {
-  dog {
-    isHousetrained(atOtherHomes: $atOtherHomes)
-  }
-}
-```
-
-However if the variable is defined as non-null, default values
-are unreachable. Therefore queries such as the following fail
-validation
-
-```graphql counter-example
-query houseTrainedQuery($atOtherHomes: Boolean! = true) {
-  dog {
-    isHousetrained(atOtherHomes: $atOtherHomes)
-  }
-}
-```
-
-
 ### Variables Are Input Types
 
 **Formal Specification**
@@ -1833,20 +1822,45 @@ an extraneous variable.
 
 **Formal Specification**
 
-  * For each {operation} in {document}
-  * Let {variableUsages} be all usages transitively included in the {operation}
-  * For each {variableUsage} in {variableUsages}
-    * Let {variableType} be the type of variable definition in the {operation}
-    * Let {argumentType} be the type of the argument the variable is passed to.
-    * Let {hasDefault} be true if the variable definition defines a default.
-    * AreTypesCompatible({argumentType}, {variableType}, {hasDefault}) must be true
-
-  * AreTypesCompatible({argumentType}, {variableType}, {hasDefault}):
-    * If {hasDefault} is true, treat the {variableType} as non-null.
-    * If inner type of {argumentType} and {variableType} are different, return false
-    * If {argumentType} and {variableType} have different list dimensions, return false
-    * If any list level of {variableType} is not non-null, and the corresponding level
-      in {argument} is non-null, the types are not compatible.
+  * For each {operation} in {document}:
+  * Let {variableUsages} be all usages transitively included in the {operation}.
+  * For each {variableUsage} in {variableUsages}:
+    * Let {variableName} be the name of {variableUsage}.
+    * Let {variableDefinition} be the {VariableDefinition} named {variableName}
+      defined within {operation}.
+    * {IsVariableUsageAllowed(variableDefinition, variableUsage)} must be {true}.
+
+IsVariableUsageAllowed(variableDefinition, variableUsage):
+  * Let {variableType} be the expected type of {variableDefinition}.
+  * Let {locationType} be the expected type of the {Argument}, {ObjectField},
+    or {ListValue} entry where {variableUsage} is located.
+  * If {locationType} is a non-null type AND {variableType} is NOT a non-null type:
+    * Let {hasNonNullVariableDefaultValue} be {true} if a default value exists
+      for {variableDefinition} and is not the value {null}.
+    * Let {hasLocationDefaultValue} be {true} if a default value exists for
+      the {Argument} or {ObjectField} where {variableUsage} is located.
+    * If {hasNonNullVariableDefaultValue} is NOT {true} AND
+      {hasLocationDefaultValue} is NOT {true}, return {false}.
+    * Let {nullableLocationType} be the unwrapped nullable type of {locationType}.
+    * Return {AreTypesCompatible(variableType, nullableLocationType)}.
+  * Return {AreTypesCompatible(variableType, locationType)}.
+
+AreTypesCompatible(variableType, locationType):
+  * If {locationType} is a non-null type:
+    * If {variableType} is NOT a non-null type, return {false}.
+    * Let {nullableLocationType} be the unwrapped nullable type of {locationType}.
+    * Let {nullableVariableType} be the unwrapped nullable type of {variableType}.
+    * Return {AreTypesCompatible(nullableVariableType, nullableLocationType)}.
+  * Otherwise, if {variableType} is a non-null type:
+    * Let {nullableVariableType} be the nullable type of {variableType}.
+    * Return {AreTypesCompatible(nullableVariableType, locationType)}.
+  * Otherwise, if {locationType} is a list type:
+    * If {variableType} is NOT a list type, return {false}.
+    * Let {itemLocationType} be the unwrapped item type of {locationType}.
+    * Let {itemVariableType} be the unwrapped item type of {variableType}.
+    * Return {AreTypesCompatible(itemVariableType, itemLocationType)}.
+  * Otherwise, if {variableType} is a list type, return {false}.
+  * Return {true} if {variableType} and {locationType} are identical, otherwise {false}.
 
 **Explanatory Text**
 
@@ -1890,17 +1904,6 @@ query booleanArgQuery($booleanArg: Boolean) {
 }
 ```
 
-A notable exception is when default arguments are provided. They are, in effect,
-treated as non-nulls.
-
-```graphql example
-query booleanArgQueryWithDefault($booleanArg: Boolean = true) {
-  arguments {
-    nonNullBooleanArgField(nonNullBooleanArg: $booleanArg)
-  }
-}
-```
-
 For list types, the same rules around nullability apply to both outer types
 and inner types. A nullable list cannot be passed to a non-null list, and a list
 of nullable values cannot be passed to a list of non-null values.
@@ -1925,5 +1928,36 @@ query listToNonNullList($booleanList: [Boolean]) {
 ```
 
 This would fail validation because a `[T]` cannot be passed to a `[T]!`.
-
 Similarly a `[T]` cannot be passed to a `[T!]`.
+
+**Allowing optional variables when default values exist**
+
+A notable exception to typical variable type compatibility is allowing a
+variable definition with a nullable type to be provided to a non-null location
+as long as either that variable or that location provides a default value.
+
+```graphql example
+query booleanArgQueryWithDefault($booleanArg: Boolean) {
+  arguments {
+    optionalNonNullBooleanArgField(optionalBooleanArg: $booleanArg)
+  }
+}
+```
+
+In the example above, an optional variable is allowed to be used in an non-null argument which provides a default value.
+
+```graphql example
+query booleanArgQueryWithDefault($booleanArg: Boolean = true) {
+  arguments {
+    nonNullBooleanArgField(nonNullBooleanArg: $booleanArg)
+  }
+}
+```
+
+In the example above, a variable provides a default value and can be used in a
+non-null argument. This behavior is explicitly supported for compatibility with
+earlier editions of this specification. GraphQL authoring tools may wish to
+report this is a warning with the suggestion to replace `Boolean` with `Boolean!`.
+
+Note: The value {null} could still be provided to a such a variable at runtime.
+A non-null argument must produce a field error if provided a {null} value.