API refactor to make it harder to ignore errors

Fixes #709

See changelog

crates/apollo-compiler/CHANGELOG.md

@@ -17,25 +17,106 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 ## Maintenance
 ## Documentation-->
 
-# [1.0.0-beta.7](https://crates.io/crates/apollo-compiler/1.0.0-beta.7) - 2023-11-17
+# [x.x.x] (unreleased) - 2023-mm-dd
 
 ## BREAKING
 
-- **API refactor to make it harder to ignore errors - [SimonSapin], [pull/FIXME]:**
-  - Move items from the crate top-level to `apollo_compiler::validation`
+- **API refactor to make it harder to ignore errors - [SimonSapin], 
+  [pull/752] fixing [issue/709]:**
+  - `ast::Document`, `Schema`, and `ExecutableDocument` not longer contain potential errors
+    that users need to check separately.
+  - Instead, various constructors and methods now return a `Result`,
+    with the `Err` case containing both both errors and a maybe-incomplete value.
+  - Change `validate` methods of `Schema` and `ExecutableDocument` to take ownership of `self`.
+    On success they return the schema or document (unmodified) wrapped in a `Valid<_>` marker type,
+    which is **immutable**.
+  - Change `ExecutableDocument` to require a `&Valid<Schema>` instead of `&Schema`,
+    forcing callers to either run validation or opt out explicitly with `Valid::assert_valid`.
+  - Make `parse_mixed` and `to_mixed` validate both the schema and document.
+    Rename them with a `_validate` suffix.
+  - Corresponding changes to all of the above in `Parser` method signatures
+  - Remove `ast::Document::check_parse_errors`:
+    parse errors are now encoded in the return value of `parse`.
+  - Remove `ast::Document::to_schema_builder`. Use `SchemaBuilder::add_ast` instead.
+  - Move items from the crate top-level to `apollo_compiler::validation`:
     * `Diagnostic`
     * `DiagnosticList`
     * `FileId`
     * `NodeLocation`
-  - Move items from the crate top-level to `apollo_compiler::execution`
+  - Move items from the crate top-level to `apollo_compiler::execution`:
     * `GraphQLError`
     * `GraphQLLocation`
   - Remove warning-level and advice-level diagnostics. See [issue/751].
 
+  Highlight of signature changes:
+
+  ```diff
+  +struct Valid<T>(T); // Implements `Deref` and `AsRef` but not `DerefMut` or `AsMut`
+  +
+  +struct WithErrors<T> {
+  +    partial: T, // Errors may cause components to be missing
+  +    errors: DiagnosticList,
+  +}
+
+  -pub fn parse_mixed(…) -> (Schema, ExecutableDocument)
+  +pub fn parse_mixed_validate(…)
+  +    -> Result<(Valid<Schema>, Valid<ExecutableDocument>), DiagnosticList>
+
+   impl ast::Document {
+  -    pub fn parse(…) -> Self
+  +    pub fn parse(…) -> Result<Self, WithErrors<Self>>
+
+  -    pub fn to_schema(&self) -> Schema
+  +    pub fn to_schema(&self) -> Result<Schema, WithErrors<Schema>>
+
+  -    pub fn to_executable(&self) -> ExecutableDocument
+  +    pub fn to_executable(&self) -> Result<ExecutableDocument, WithErrors<ExecutableDocument>>
+
+  -    pub fn to_mixed(&self) -> (Schema, ExecutableDocument)
+  +    pub fn to_mixed_validate(
+  +        &self,
+  +    ) -> Result<(Valid<Schema>, Valid<ExecutableDocument>), DiagnosticList>
+   }
+
+   impl Schema {
+  -    pub fn parse(…) -> Self
+  -    pub fn validate(&self) -> Result<DiagnosticList, DiagnosticList>
+
+  +    pub fn parse_and_validate(…) -> Result<Valid<Self>, WithErrors<Self>>
+  +    pub fn parse(…) -> Result<Self, WithErrors<Self>>
+  +    pub fn validate(self) -> Result<Valid<Self>, WithErrors<Self>>
+   }
+
+   impl SchemaBuilder {
+  -    pub fn build(self) -> Schema
+  +    pub fn build(self) -> Result<Schema, WithErrors<Schema>>
+   }
+
+   impl ExecutableDocument {
+  -    pub fn parse(schema: &Schema, …) -> Self
+  -    pub fn validate(&self, schema: &Schema) -> Result<(), DiagnosticList>
+
+  +    pub fn parse_and_validate(schema: &Valid<Schema>, …) -> Result<Valid<Self>, WithErrors<Self>>
+  +    pub fn parse(schema: &Valid<Schema>, …) -> Result<Self, WithErrors<Self>>
+  +    pub fn validate(self, schema: &Valid<Schema>) -> Result<Valid<Self>, WithErrors<Self>>
+   }
+  ```
+
+## Features
+
+- **Add `parse_and_validate` constructors for `Schema` and `ExecutableDocument` - [SimonSapin], 
+  [pull/752]:**
+  when mutating isn’t needed after parsing,
+  this returns an immutable `Valid<_>` value in one step.
+
 [SimonSapin]: https://github.com/SimonSapin
+[issue/709]: https://github.com/apollographql/apollo-rs/issues/709
 [issue/751]: https://github.com/apollographql/apollo-rs/issues/751
+[pull/752]: https://github.com/apollographql/apollo-rs/pull/752
 
 
+# [1.0.0-beta.7](https://crates.io/crates/apollo-compiler/1.0.0-beta.7) - 2023-11-17
+
 ## Features
 
 - **Helper features for `Name` and `Type` - [SimonSapin], [pull/739]:**

crates/apollo-compiler/README.md

@@ -83,11 +83,9 @@ let input = r#"
   }
 "#;
 
-let schema = Schema::parse(input, "document.graphql");
-
 /// In case of validation errors, the panic message will be nicely formatted
 /// to point at relevant parts of the source file(s)
-schema.validate().unwrap();
+let schema = Schema::parse_and_validate(input, "document.graphql").unwrap();
 ```
 
 ### Examples
@@ -121,11 +119,9 @@ fn main() {
     }
     "#;
 
-    let schema = Schema::parse(schema_input, "schema.graphql");
-    let document = ExecutableDocument::parse(&schema, query_input, "query.graphql");
-
-    schema.validate().unwrap();
-    document.validate(&schema).unwrap();
+    let schema = Schema::parse_and_validate(schema_input, "schema.graphql").unwrap();
+    let document = ExecutableDocument::parse_and_validate(&schema, query_input, "query.graphql")
+        .unwrap();
 
     let op = document.get_operation(Some("getUser")).expect("getUser query does not exist");
     let fragment_in_op = op.selection_set.selections.iter().filter_map(|sel| match sel {
@@ -184,11 +180,9 @@ fn main() {
     }
     "#;
 
-    let schema = Schema::parse(schema_input, "schema.graphql");
-    let document = ExecutableDocument::parse(&schema, query_input, "query.graphql");
-
-    schema.validate().unwrap();
-    document.validate(&schema).unwrap();
+    let schema = Schema::parse_and_validate(schema_input, "schema.graphql").unwrap();
+    let document = ExecutableDocument::parse_and_validate(&schema, query_input, "query.graphql")
+        .unwrap();
 
     let get_product_op = document
         .get_operation(Some("getProduct"))
@@ -271,12 +265,7 @@ type Cat implements Pet {
 union CatOrDog = Cat | Dog
 "#;
 
-let (schema, executable) = apollo_compiler::parse_mixed(input, "document.graphql");
-
-if let Err(diagnostics) = schema.validate() {
-    println!("{diagnostics}")
-}
-if let Err(diagnostics) = executable.validate(&schema) {
+if let Err(diagnostics) = apollo_compiler::parse_mixed_validate(input, "document.graphql") {
     println!("{diagnostics}")
 }
 ```

crates/apollo-compiler/benches/multi_source.rs

@@ -2,17 +2,16 @@ use apollo_compiler::ExecutableDocument;
 use apollo_compiler::Schema;
 use criterion::*;
 
-fn compile(schema: &str, query: &str) -> (Schema, ExecutableDocument) {
-    let schema = Schema::parse(schema, "schema.graphql");
-    let doc = ExecutableDocument::parse(&schema, query, "query.graphql");
-    black_box((schema, doc))
+fn compile(schema: &str, query: &str) {
+    let schema = Schema::parse_and_validate(schema, "schema.graphql").unwrap();
+    let doc = ExecutableDocument::parse(&schema, query, "query.graphql").unwrap();
+    black_box((schema, doc));
 }
 
 fn compile_and_validate(schema: &str, query: &str) {
-    let schema = Schema::parse(schema, "schema.graphql");
-    let doc = ExecutableDocument::parse(&schema, query, "query.graphql");
-    let _ = black_box(schema.validate());
-    let _ = black_box(doc.validate(&schema));
+    let schema = Schema::parse_and_validate(schema, "schema.graphql").unwrap();
+    let doc = ExecutableDocument::parse_and_validate(&schema, query, "query.graphql").unwrap();
+    black_box((schema, doc));
 }
 
 fn bench_simple_query_compiler(c: &mut Criterion) {

crates/apollo-compiler/examples/extract_directives_used_by_query.rs

@@ -52,7 +52,7 @@ fn main() {
           }
         "#;
 
-    let schema = Schema::parse(schema_src, "not-used-here.graphql");
+    let schema = Schema::parse_and_validate(schema_src, "not-used-here.graphql").unwrap();
 
     let query_src0 = r#"query {
           directivesQuery {
@@ -64,7 +64,9 @@ fn main() {
           }
         }
         "#;
-    let query0 = ExecutableDocument::parse(&schema, query_src0, "not-used-here.graphql");
+    let query0 =
+        ExecutableDocument::parse_and_validate(&schema, query_src0, "not-used-here.graphql")
+            .unwrap();
 
     let directives = get_directives_used_in_query(&query0);
     assert_eq!(directives.len(), 4);
@@ -75,7 +77,7 @@ fn main() {
           }
         }
         "#;
-    let query1 = ExecutableDocument::parse(&schema, query_src1, "not-used-here.graphql");
+    let query1 = ExecutableDocument::parse(&schema, query_src1, "not-used-here.graphql").unwrap();
 
     let directives = get_directives_used_in_query(&query1);
     assert_eq!(directives.len(), 2);
@@ -86,7 +88,7 @@ fn main() {
           }
         }
         "#;
-    let query2 = ExecutableDocument::parse(&schema, query_src2, "not-used-here.graphql");
+    let query2 = ExecutableDocument::parse(&schema, query_src2, "not-used-here.graphql").unwrap();
 
     let directives = get_directives_used_in_query(&query2);
     assert_eq!(directives.len(), 0);

crates/apollo-compiler/examples/file_watcher.rs

@@ -7,6 +7,7 @@ use std::{
 };
 
 use anyhow::{anyhow, Result};
+use apollo_compiler::validation::Valid;
 use apollo_compiler::ExecutableDocument;
 use apollo_compiler::Schema;
 use notify::{Config, EventKind, PollWatcher, RecursiveMode, Watcher};
@@ -18,7 +19,7 @@ fn main() -> Result<()> {
 }
 
 pub struct FileWatcher {
-    manifest: HashMap<PathBuf, (Schema, ExecutableDocument)>,
+    manifest: HashMap<PathBuf, (Valid<Schema>, Valid<ExecutableDocument>)>,
 }
 
 #[allow(clippy::new_without_default)]
@@ -39,10 +40,6 @@ impl FileWatcher {
             self.add_document(proposed_document, src_path)?;
         }
 
-        for entry in self.manifest.values() {
-            self.validate(entry);
-        }
-
         self.watch_broadcast(dir.as_ref())
     }
 
@@ -72,8 +69,7 @@ impl FileWatcher {
                                 match fs::read_to_string(&path) {
                                     Ok(contents) => {
                                         println!("changes detected in {}", path.display());
-                                        let path = self.add_document(contents, path)?;
-                                        self.validate(&self.manifest[&path]);
+                                        self.add_document(contents, path)?;
                                     }
                                     Err(e) => {
                                         println!(
@@ -112,19 +108,10 @@ impl FileWatcher {
         src_path: PathBuf,
     ) -> Result<PathBuf, anyhow::Error> {
         let full_path = fs::canonicalize(&src_path)?;
-        let doc = apollo_compiler::parse_mixed(proposed_document, src_path);
+        let doc = apollo_compiler::parse_mixed_validate(proposed_document, src_path).unwrap();
         self.manifest.insert(full_path.clone(), doc);
         Ok(full_path)
     }
-
-    fn validate(&self, (schema, executable): &(Schema, ExecutableDocument)) {
-        if let Err(err) = schema.validate() {
-            println!("{err}")
-        }
-        if let Err(err) = executable.validate(schema) {
-            println!("{err}")
-        }
-    }
 }
 
 fn get_schema_and_maybe_path(entry: DirEntry) -> Result<(String, PathBuf)> {

crates/apollo-compiler/examples/hello_world.rs

@@ -7,7 +7,7 @@ fn compile_query() -> Option<Node<executable::Fragment>> {
     let file = Path::new("crates/apollo-compiler/examples/query_with_errors.graphql");
     let src = fs::read_to_string(file).expect("Could not read schema file.");
 
-    let (_, document) = apollo_compiler::parse_mixed(src, file);
+    let (_, document) = apollo_compiler::parse_mixed_validate(src, file).unwrap();
     let operation_names: Vec<_> = document
         .named_operations
         .keys()

crates/apollo-compiler/examples/multi_source_validation.rs

@@ -1,4 +1,4 @@
-use apollo_compiler::parse_mixed;
+use apollo_compiler::parse_mixed_validate;
 use apollo_compiler::ExecutableDocument;
 use apollo_compiler::Schema;
 use std::fs;
@@ -19,9 +19,7 @@ fn compile_from_dir() -> io::Result<()> {
         for entry in fs::read_dir(dir)? {
             let entry = entry?;
             let src = fs::read_to_string(entry.path()).expect("Could not read document file.");
-            let (schema, executable) = parse_mixed(&src, entry.path());
-            schema.validate().unwrap();
-            executable.validate(&schema).unwrap();
+            let (_schema, _executable) = parse_mixed_validate(&src, entry.path()).unwrap();
         }
     }
     Ok(())
@@ -41,16 +39,14 @@ fn compile_schema_and_query_files() -> io::Result<()> {
     let schema = Schema::builder()
         .parse(src, schema)
         .parse(src_ext, schema_ext)
-        .build();
-    schema.validate().unwrap();
+        .build()
+        .unwrap();
+    let schema = schema.validate().unwrap();
 
     // get_dog_name is a query-only file
     let query = Path::new("crates/apollo-compiler/examples/documents/get_dog_name.graphql");
     let src = fs::read_to_string(query).expect("Could not read query file.");
-    let doc = ExecutableDocument::parse(&schema, src, query);
-
-    doc.validate(&schema).unwrap();
-
+    let _doc = ExecutableDocument::parse_and_validate(&schema, src, query).unwrap();
     Ok(())
 }
 
@@ -126,10 +122,8 @@ query getDogName {
   }
 }
     "#;
-    let schema = Schema::parse(schema, "schema.graphl");
-    schema.validate().unwrap();
-    let doc = ExecutableDocument::parse(&schema, query, "query.graphql");
-    doc.validate(&schema).unwrap();
+    let schema = Schema::parse_and_validate(schema, "schema.graphl").unwrap();
+    let _doc = ExecutableDocument::parse_and_validate(&schema, query, "query.graphql").unwrap();
 
     Ok(())
 }

crates/apollo-compiler/examples/rename.rs

@@ -2,17 +2,17 @@
 
 use apollo_compiler::name;
 use apollo_compiler::schema::ExtendedType;
+use apollo_compiler::validation::Valid;
 use apollo_compiler::Schema;
 
 #[cfg(not(test))]
 fn main() {
     print!("{}", renamed())
 }
 
-fn renamed() -> Schema {
+fn renamed() -> Valid<Schema> {
     let input = "type Query { field: Int }";
-    let mut schema = Schema::parse(input, "schema.graphql");
-    schema.validate().unwrap();
+    let mut schema = Schema::parse(input, "schema.graphql").unwrap();
 
     // 1. Remove the definition from the `types` map, using its old name as a key
     let mut type_def = schema.types.remove("Query").unwrap();
@@ -38,8 +38,7 @@ fn renamed() -> Schema {
         .unwrap()
         .name = new_name;
 
-    schema.validate().unwrap();
-    schema
+    schema.validate().unwrap()
 }
 
 #[test]