graceful handling of syntax and semantic errors: `apollo-compiler` panics on syntax errors

[EverlastingBugstopper]

if you place characters after a [Type], like [Type]oops, apollo-compiler will panic.

example valid schema:

type Person {
  id: ID!
  name: String
  appearedIn: [Film]
  directed: [Film]
}

type Film {
  id: ID!
  title: String
  actors: [Person]
  director: Person
}

type Query {
  person(id: ID!): Person
  people: [Person]
  film(id: ID!): Film!
  films: [Film]
}

example panic schema:

type Person {
  id: ID!
  name: String
  appearedIn: [Film]s
  directed: [Film]
}

type Film {
  id: ID!
  title: String
  actors: [Person]
  director: Person
}

type Query {
  person(id: ID!): Person
  people: [Person]
  film(id: ID!): Film!
  films: [Film]
}

abbreviated panic

$ rover schema lint --schema ./schema.out
Validating that --schema ./schema.out conforms to the GraphQL specification.
thread 'main' panicked at 'Field must have a type', /home/a/.cargo/git/checkouts/apollo-rs-c2f4fdeb51021351/2008021/crates/apollo-compiler/src/queries/database.rs:963:28
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

panic with full backtrace

$ rover schema lint --schema ./schema.out
Validating that --schema ./schema.out conforms to the GraphQL specification.
thread 'main' panicked at 'Field must have a type', /home/a/.cargo/git/checkouts/apollo-rs-c2f4fdeb51021351/2008021/crates/apollo-compiler/src/queries/database.rs:963:28
stack backtrace:
   0: rust_begin_unwind
             at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/core/src/panicking.rs:143:14
   2: core::panicking::panic_display
             at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/core/src/panicking.rs:73:5
   3: core::panicking::panic_str
             at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/core/src/panicking.rs:56:5
   4: core::option::expect_failed
             at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/core/src/option.rs:1852:5
   5: core::option::Option<T>::expect
             at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/core/src/option.rs:715:21
   6: apollo_compiler::queries::database::field_definition
             at /home/a/.cargo/git/checkouts/apollo-rs-c2f4fdeb51021351/2008021/crates/apollo-compiler/src/queries/database.rs:963:17
   7: core::ops::function::FnMut::call_mut
             at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/core/src/ops/function.rs:150:5
   8: core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &mut F>::call_once
             at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/core/src/ops/function.rs:280:13
   9: core::option::Option<T>::map
             at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/core/src/option.rs:906:29
  10: <core::iter::adapters::map::Map<I,F> as core::iter::traits::iterator::Iterator>::next
             at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/core/src/iter/adapters/map.rs:103:9
  11: alloc::vec::Vec<T,A>::extend_desugared
             at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/alloc/src/vec/mod.rs:2649:35
  12: <alloc::vec::Vec<T,A> as alloc::vec::spec_extend::SpecExtend<T,I>>::spec_extend
             at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/alloc/src/vec/spec_extend.rs:18:9
  13: <alloc::vec::Vec<T> as alloc::vec::spec_from_iter_nested::SpecFromIterNested<T,I>>::from_iter
             at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/alloc/src/vec/spec_from_iter_nested.rs:43:9
  14: <alloc::vec::Vec<T> as alloc::vec::spec_from_iter::SpecFromIter<T,I>>::from_iter
             at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/alloc/src/vec/spec_from_iter.rs:33:9
  15: <alloc::vec::Vec<T> as core::iter::traits::collect::FromIterator<T>>::from_iter
             at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/alloc/src/vec/mod.rs:2552:9
  16: core::iter::traits::iterator::Iterator::collect
             at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/core/src/iter/traits/iterator.rs:1778:9
  17: apollo_compiler::queries::database::fields_definition
             at /home/a/.cargo/git/checkouts/apollo-rs-c2f4fdeb51021351/2008021/crates/apollo-compiler/src/queries/database.rs:949:48
  18: apollo_compiler::queries::database::object_type_definition
             at /home/a/.cargo/git/checkouts/apollo-rs-c2f4fdeb51021351/2008021/crates/apollo-compiler/src/queries/database.rs:709:29
  19: apollo_compiler::queries::database::object_types::{{closure}}
             at /home/a/.cargo/git/checkouts/apollo-rs-c2f4fdeb51021351/2008021/crates/apollo-compiler/src/queries/database.rs:442:22
  20: core::ops::function::impls::<impl core::ops::function::FnMut<A> for &mut F>::call_mut
             at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/core/src/ops/function.rs:269:13
  21: <core::slice::iter::Iter<T> as core::iter::traits::iterator::Iterator>::find_map
             at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/core/src/slice/iter/macros.rs:276:38
  22: <core::iter::adapters::filter_map::FilterMap<I,F> as core::iter::traits::iterator::Iterator>::next
             at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/core/src/iter/adapters/filter_map.rs:61:9
  23: <alloc::vec::Vec<T> as alloc::vec::spec_from_iter_nested::SpecFromIterNested<T,I>>::from_iter
             at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/alloc/src/vec/spec_from_iter_nested.rs:26:32
  24: <alloc::vec::Vec<T> as alloc::vec::spec_from_iter::SpecFromIter<T,I>>::from_iter
             at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/alloc/src/vec/spec_from_iter.rs:33:9
  25: <alloc::vec::Vec<T> as core::iter::traits::collect::FromIterator<T>>::from_iter
             at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/alloc/src/vec/mod.rs:2552:9
  26: core::iter::traits::iterator::Iterator::collect
             at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/core/src/iter/traits/iterator.rs:1778:9
  27: apollo_compiler::queries::database::object_types
             at /home/a/.cargo/git/checkouts/apollo-rs-c2f4fdeb51021351/2008021/crates/apollo-compiler/src/queries/database.rs:437:19
  28: <apollo_compiler::queries::database::ObjectTypesQuery as salsa::plumbing::QueryFunction>::execute
             at /home/a/.cargo/git/checkouts/apollo-rs-c2f4fdeb51021351/2008021/crates/apollo-compiler/src/queries/database.rs:50:8
  29: salsa::derived::slot::Slot<Q,MP>::read_upgrade::{{closure}}
             at /home/a/.cargo/registry/src/github.com-1ecc6299db9ec823/salsa-0.16.1/src/derived/slot.rs:218:13
  30: salsa::runtime::Runtime::execute_query_implementation
             at /home/a/.cargo/registry/src/github.com-1ecc6299db9ec823/salsa-0.16.1/src/runtime.rs:330:21
  31: salsa::derived::slot::Slot<Q,MP>::read_upgrade
             at /home/a/.cargo/registry/src/github.com-1ecc6299db9ec823/salsa-0.16.1/src/derived/slot.rs:215:26
  32: salsa::derived::slot::Slot<Q,MP>::read
             at /home/a/.cargo/registry/src/github.com-1ecc6299db9ec823/salsa-0.16.1/src/derived/slot.rs:148:9
  33: <salsa::derived::DerivedStorage<Q,MP> as salsa::plumbing::QueryStorageOps<Q>>::try_fetch
             at /home/a/.cargo/registry/src/github.com-1ecc6299db9ec823/salsa-0.16.1/src/derived.rs:170:13
  34: salsa::QueryTable<Q>::try_get
             at /home/a/.cargo/registry/src/github.com-1ecc6299db9ec823/salsa-0.16.1/src/lib.rs:494:9
  35: salsa::QueryTable<Q>::get
             at /home/a/.cargo/registry/src/github.com-1ecc6299db9ec823/salsa-0.16.1/src/lib.rs:490:9
  36: <DB as apollo_compiler::queries::database::SourceDatabase>::object_types::__shim
             at /home/a/.cargo/git/checkouts/apollo-rs-c2f4fdeb51021351/2008021/crates/apollo-compiler/src/queries/database.rs:27:1
  37: <DB as apollo_compiler::queries::database::SourceDatabase>::object_types
             at /home/a/.cargo/git/checkouts/apollo-rs-c2f4fdeb51021351/2008021/crates/apollo-compiler/src/queries/database.rs:27:1
  38: apollo_compiler::queries::database::add_object_type_id_to_schema
             at /home/a/.cargo/git/checkouts/apollo-rs-c2f4fdeb51021351/2008021/crates/apollo-compiler/src/queries/database.rs:899:55
  39: apollo_compiler::queries::database::schema
             at /home/a/.cargo/git/checkouts/apollo-rs-c2f4fdeb51021351/2008021/crates/apollo-compiler/src/queries/database.rs:428:21
  40: <apollo_compiler::queries::database::SchemaQuery as salsa::plumbing::QueryFunction>::execute
             at /home/a/.cargo/git/checkouts/apollo-rs-c2f4fdeb51021351/2008021/crates/apollo-compiler/src/queries/database.rs:48:8
  41: salsa::derived::slot::Slot<Q,MP>::read_upgrade::{{closure}}
             at /home/a/.cargo/registry/src/github.com-1ecc6299db9ec823/salsa-0.16.1/src/derived/slot.rs:218:13
  42: salsa::runtime::Runtime::execute_query_implementation
             at /home/a/.cargo/registry/src/github.com-1ecc6299db9ec823/salsa-0.16.1/src/runtime.rs:330:21
  43: salsa::derived::slot::Slot<Q,MP>::read_upgrade
             at /home/a/.cargo/registry/src/github.com-1ecc6299db9ec823/salsa-0.16.1/src/derived/slot.rs:215:26
  44: salsa::derived::slot::Slot<Q,MP>::read
             at /home/a/.cargo/registry/src/github.com-1ecc6299db9ec823/salsa-0.16.1/src/derived/slot.rs:148:9
  45: <salsa::derived::DerivedStorage<Q,MP> as salsa::plumbing::QueryStorageOps<Q>>::try_fetch
             at /home/a/.cargo/registry/src/github.com-1ecc6299db9ec823/salsa-0.16.1/src/derived.rs:170:13
  46: salsa::QueryTable<Q>::try_get
             at /home/a/.cargo/registry/src/github.com-1ecc6299db9ec823/salsa-0.16.1/src/lib.rs:494:9
  47: salsa::QueryTable<Q>::get
             at /home/a/.cargo/registry/src/github.com-1ecc6299db9ec823/salsa-0.16.1/src/lib.rs:490:9
  48: <DB as apollo_compiler::queries::database::SourceDatabase>::schema::__shim
             at /home/a/.cargo/git/checkouts/apollo-rs-c2f4fdeb51021351/2008021/crates/apollo-compiler/src/queries/database.rs:27:1
  49: <DB as apollo_compiler::queries::database::SourceDatabase>::schema
             at /home/a/.cargo/git/checkouts/apollo-rs-c2f4fdeb51021351/2008021/crates/apollo-compiler/src/queries/database.rs:27:1
  50: apollo_compiler::validation::schema::check
             at /home/a/.cargo/git/checkouts/apollo-rs-c2f4fdeb51021351/2008021/crates/apollo-compiler/src/validation/schema.rs:13:8
  51: apollo_compiler::validation::Validator::validate
             at /home/a/.cargo/git/checkouts/apollo-rs-c2f4fdeb51021351/2008021/crates/apollo-compiler/src/validation/mod.rs:40:33
  52: apollo_compiler::ApolloCompiler::validate
             at /home/a/.cargo/git/checkouts/apollo-rs-c2f4fdeb51021351/2008021/crates/apollo-compiler/src/lib.rs:37:9
  53: rover::command::schema::lint::Lint::run
             at ./src/command/schema/lint.rs:28:22
  54: rover::command::schema::Schema::run
             at ./src/command/schema/mod.rs:24:39
  55: rover::cli::Rover::execute_command
             at ./src/cli.rs:189:41
  56: rover::cli::Rover::run
             at ./src/cli.rs:127:34
  57: rover::main::{{closure}}
             at ./src/bin/rover.rs:15:5
  58: rover::main
             at ./src/bin/rover.rs:5:1
  59: core::ops::function::FnOnce::call_once
             at /rustc/7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c/library/core/src/ops/function.rs:227:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace

[SimonSapin]

I was gonna open an issue but it looks the same is this.

Test case: add crates/apollo-compiler/test_data/diagnostics/0050_unnamed_object_type.graphql

type {
  cat: String
}

cargo test:

thread 'tests::compiler_tests' panicked at 'Field must have a name', crates/apollo-compiler/src/database/hir_db.rs:1092:24

In general, any use of Option::expect or similar should probably be replaced by something that emits a diagnostic and continues with some kind of fallback (such as skipping ill-formed definitions in the HIR)

[goto-bus-stop]

Yes! I think I'll be working on this next as I'm already having to write more .expect()s changing some of our From impls to TryFrom, and it feels bad (and it is bad for untrusted user input 😇 ).

I think our first step will be to outright skip things in the HIR. If the HIR can't be generated fully, it should be because there was a parser error, which is already reported.

[lrlna]

ugh yea this is handled really rather terribly atm (i only say this because i am responsible >.<). would be so very good to fix this!!

there are a few overarching considerations when uniting the two types of errors here:

• syntax errors are stored separately from tokens in the ast, when creating the HIR node, we should try to correlate the fact the ast node is missing because of a specific syntax error, or just output the syntax error as part of diagnostics
• how do we provide as much as diagnostics as possible? let's say a name for a fragment definition is missing, and there are also some semantic errors in that fragment definition (e.g. field doesn't exist on the interface it's referring to). do we want to scrap the existence of the whole fragment definition node and therefore not do any validation on it until the syntax error is fixed? or do we try to continue with semantic validation of the parts that exist?