fix(compiler): raise diagnostics when using null in non-nullable position (#746)

* chore(compiler): add regression test for #738

* chore(compiler): remove 2 suffix from method names

* fix(compiler): raise a diagnostic for null items in a non-nullable list

* clippy

* test nested list inside object

* fix(compiler): null type is never assignable to a non-null type and always assignable to a nullable type

* chore(compiler): add failing test for providing list to non-list type

* Use name from ExtendedType where possible

* fix(compiler): reject list values provided to non-list types

* port value tests from graphql-js

* only instantiate the test schema once

* Use #[track_caller]

Co-authored-by: Simon Sapin <[email protected]>

* Use `ExtendedType::is_input_type` instead of manual match

* Assert diagnostics output for the graphql-js value type tests

* remove excess whitespace from test schema and queries

Used the runtime `unindent` crate instead of compile-time `indoc` so it doesn't need to be repeated at every call site.

* Remove outdated comment

---------

Co-authored-by: Simon Sapin <[email protected]>

Author: goto-bus-stop

crates/apollo-compiler/Cargo.toml

@@ -39,8 +39,9 @@ criterion = "0.5.1"
 expect-test = "1.4"
 notify = "6.0.0"
 pretty_assertions = "1.3.0"
-serial_test = "2.0.0"
 serde_json = "1.0"
+serial_test = "2.0.0"
+unindent = "0.2.3"
 
 [[bench]]
 name = "multi-source"

crates/apollo-compiler/src/ast/impls.rs

@@ -587,6 +587,23 @@ impl Type {
         Type::List(Box::new(self))
     }
 
+    /// If the type is a list type or a non-null list type, return the item type.
+    ///
+    /// # Example
+    /// ```
+    /// use apollo_compiler::ty;
+    /// // Returns the inner type of the list.
+    /// assert_eq!(ty!([Foo!]).item_type(), &ty!(Foo!));
+    /// // Not a list: returns the input.
+    /// assert_eq!(ty!(Foo!).item_type(), &ty!(Foo!));
+    /// ```
+    pub fn item_type(&self) -> &Self {
+        match self {
+            Type::List(inner) | Type::NonNullList(inner) => inner,
+            ty => ty,
+        }
+    }
+
     /// Returns the inner named type, after unwrapping any non-null or list markers.
     pub fn inner_named_type(&self) -> &NamedType {
         match self {

crates/apollo-compiler/src/diagnostics.rs

@@ -180,7 +180,7 @@ pub(crate) enum DiagnosticData {
         /// The source location where the directive that's being used was defined.
         directive_def: Option<NodeLocation>,
     },
-    #[error("{ty} cannot be represented by a {value} value")]
+    #[error("expected value of type {ty}, found {value}")]
     UnsupportedValueType {
         // input value
         value: String,

crates/apollo-compiler/src/validation/directive.rs

@@ -280,7 +280,7 @@ pub(crate) fn validate_directives<'dir>(
                         diagnostics.push(diag)
                     } else {
                         let type_diags =
-                            super::value::validate_values2(db, &input_value.ty, argument, var_defs);
+                            super::value::validate_values(db, &input_value.ty, argument, var_defs);
 
                         diagnostics.extend(type_diags);
                     }

crates/apollo-compiler/src/validation/field.rs

@@ -54,7 +54,7 @@ pub(crate) fn validate_field(
                 {
                     diagnostics.push(diag)
                 } else {
-                    diagnostics.extend(super::value::validate_values2(
+                    diagnostics.extend(super::value::validate_values(
                         db,
                         &arg_definition.ty,
                         argument,

crates/apollo-compiler/src/validation/value.rs

@@ -36,21 +36,18 @@ fn unsupported_type(
     ))
 }
 
-//for bigint
-/*
-*/
-pub(crate) fn validate_values2(
+pub(crate) fn validate_values(
     db: &dyn ValidationDatabase,
     ty: &Node<ast::Type>,
     argument: &Node<ast::Argument>,
     var_defs: &[Node<ast::VariableDefinition>],
 ) -> Vec<ApolloDiagnostic> {
     let mut diagnostics = vec![];
-    value_of_correct_type2(db, ty, &argument.value, var_defs, &mut diagnostics);
+    value_of_correct_type(db, ty, &argument.value, var_defs, &mut diagnostics);
     diagnostics
 }
 
-pub(crate) fn value_of_correct_type2(
+pub(crate) fn value_of_correct_type(
     db: &dyn ValidationDatabase,
     ty: &Node<ast::Type>,
     arg_value: &Node<ast::Value>,
@@ -72,8 +69,10 @@ pub(crate) fn value_of_correct_type2(
         // When expected as an input type, any string (such as "4") or
         // integer (such as 4 or -4) input value should be coerced to ID
         ast::Value::Int(int) => match &type_definition {
+            // Any value is valid for a custom scalar.
             schema::ExtendedType::Scalar(scalar) if !scalar.is_built_in() => {}
-            schema::ExtendedType::Scalar(_) => match ty.inner_named_type().as_str() {
+            schema::ExtendedType::Scalar(scalar) => match scalar.name.as_str() {
+                // Any integer sequence is valid for an ID.
                 "ID" => {}
                 "Int" => {
                     if int.try_to_i32().is_err() {
@@ -118,8 +117,9 @@ pub(crate) fn value_of_correct_type2(
         // with numeric content, must raise a request error indicating an
         // incorrect type.
         ast::Value::Float(float) => match &type_definition {
+            // Any value is valid for a custom scalar.
             schema::ExtendedType::Scalar(scalar) if !scalar.is_built_in() => {}
-            schema::ExtendedType::Scalar(_) if ty.inner_named_type() == "Float" => {
+            schema::ExtendedType::Scalar(scalar) if scalar.name == "Float" => {
                 if float.try_to_f64().is_err() {
                     diagnostics.push(
                         ApolloDiagnostic::new(
@@ -149,9 +149,7 @@ pub(crate) fn value_of_correct_type2(
                 // booleans.
                 // string, ids and custom scalars are ok, and
                 // don't need a diagnostic.
-                if scalar.is_built_in()
-                    && !matches!(ty.inner_named_type().as_str(), "String" | "ID")
-                {
+                if scalar.is_built_in() && !matches!(scalar.name.as_str(), "String" | "ID") {
                     diagnostics.push(unsupported_type(db, arg_value, ty));
                 }
             }
@@ -162,17 +160,14 @@ pub(crate) fn value_of_correct_type2(
         // indicating an incorrect type.
         ast::Value::Boolean(_) => match &type_definition {
             schema::ExtendedType::Scalar(scalar) => {
-                if scalar.is_built_in() && ty.inner_named_type().as_str() != "Boolean" {
+                if scalar.is_built_in() && scalar.name.as_str() != "Boolean" {
                     diagnostics.push(unsupported_type(db, arg_value, ty));
                 }
             }
             _ => diagnostics.push(unsupported_type(db, arg_value, ty)),
         },
         ast::Value::Null => {
-            if !matches!(
-                type_definition,
-                schema::ExtendedType::Enum(_) | schema::ExtendedType::Scalar(_)
-            ) {
+            if ty.is_non_null() {
                 diagnostics.push(unsupported_type(db, arg_value, ty));
             }
         }
@@ -191,7 +186,7 @@ pub(crate) fn value_of_correct_type2(
                         if var_def.ty.is_non_null() && default_value.is_null() {
                             diagnostics.push(unsupported_type(db, default_value, &var_def.ty))
                         } else {
-                            value_of_correct_type2(
+                            value_of_correct_type(
                                 db,
                                 &var_def.ty,
                                 default_value,
@@ -204,9 +199,6 @@ pub(crate) fn value_of_correct_type2(
             }
             _ => diagnostics.push(unsupported_type(db, arg_value, ty)),
         },
-        // GraphQL has a constant literal to represent enum input values.
-        // GraphQL string literals must not be accepted as an enum input and
-        // instead raise a request error.
         ast::Value::Enum(value) => match &type_definition {
             schema::ExtendedType::Enum(enum_) => {
                 if !enum_.values.contains_key(value) {
@@ -216,12 +208,12 @@ pub(crate) fn value_of_correct_type2(
                             value.location(),
                             DiagnosticData::UndefinedValue {
                                 value: value.to_string(),
-                                definition: ty.inner_named_type().to_string(),
+                                definition: enum_.name.to_string(),
                             },
                         )
                         .label(Label::new(
                             arg_value.location(),
-                            format!("does not exist on `{}` type", ty.inner_named_type()),
+                            format!("does not exist on `{}` type", enum_.name),
                         )),
                     );
                 }
@@ -236,14 +228,23 @@ pub(crate) fn value_of_correct_type2(
         // of size one, where the single item value is the result of input
         // coercion for the list’s item type on the provided value (note
         // this may apply recursively for nested lists).
-        ast::Value::List(li) => match &type_definition {
-            schema::ExtendedType::Scalar(_)
-            | schema::ExtendedType::Enum(_)
-            | schema::ExtendedType::InputObject(_) => li
-                .iter()
-                .for_each(|v| value_of_correct_type2(db, ty, v, var_defs, diagnostics)),
-            _ => diagnostics.push(unsupported_type(db, arg_value, ty)),
-        },
+        ast::Value::List(li) => {
+            let accepts_list = ty.is_list()
+                // A named type can still accept a list if it is a custom scalar.
+                || matches!(type_definition, schema::ExtendedType::Scalar(scalar) if !scalar.is_built_in());
+            if !accepts_list {
+                diagnostics.push(unsupported_type(db, arg_value, ty))
+            } else {
+                let item_type = ty.same_location(ty.item_type().clone());
+                if type_definition.is_input_type() {
+                    for v in li {
+                        value_of_correct_type(db, &item_type, v, var_defs, diagnostics);
+                    }
+                } else {
+                    diagnostics.push(unsupported_type(db, arg_value, &item_type));
+                }
+            }
+        }
         ast::Value::Object(obj) => match &type_definition {
             schema::ExtendedType::Scalar(scalar) if !scalar.is_built_in() => (),
             schema::ExtendedType::InputObject(input_obj) => {
@@ -260,12 +261,12 @@ pub(crate) fn value_of_correct_type2(
                             value.location(),
                             DiagnosticData::UndefinedValue {
                                 value: name.to_string(),
-                                definition: ty.inner_named_type().to_string(),
+                                definition: input_obj.name.to_string(),
                             },
                         )
                         .label(Label::new(
                             value.location(),
-                            format!("does not exist on `{}` type", ty.inner_named_type()),
+                            format!("does not exist on `{}` type", input_obj.name),
                         )),
                     );
                 }
@@ -302,7 +303,7 @@ pub(crate) fn value_of_correct_type2(
                     let used_val = obj.iter().find(|(obj_name, ..)| obj_name == input_name);
 
                     if let Some((_, v)) = used_val {
-                        value_of_correct_type2(db, ty, v, var_defs, diagnostics);
+                        value_of_correct_type(db, ty, v, var_defs, diagnostics);
                     }
                 })
             }