Repository: apollographql/apollo-rs (rust)

Issue:
validation: name uniqueness should apply across different definition types
For example:

```graphql
type SomeIdentifier {}
scalar SomeIdentifier
input SomeIdentifier {}
```
is not valid, but currently allowed(?)

Pull request description:
Check name uniqueness across different types
Fixes #386

This replaces the multiple cases of name uniqueness validations by one single implementation, so that all type definitions (`object`/`interface`/`scalar`/`enum`/`union`/`input`) can share one scope. Fragments, directives, and operations are each in their own scope, but they can be validated by the same implementation.

![image](https://user-images.githubusercontent.com/1006268/218083428-ff0c270b-4a0f-4f9f-82cc-125aa4e51c7b.png)

Todo:
- [x] add a `ok` test with duplicate, but valid names in different scopes

Patch:
diff --git a/crates/apollo-compiler/src/diagnostics.rs b/crates/apollo-compiler/src/diagnostics.rs
index 95617aa..a458ae7 100644
--- a/crates/apollo-compiler/src/diagnostics.rs
+++ b/crates/apollo-compiler/src/diagnostics.rs
@@ -37,6 +37,16 @@ impl DiagnosticLocation {
     }
 }
 
+impl From<(FileId, rowan::TextRange)> for DiagnosticLocation {
+    fn from((file_id, range): (FileId, rowan::TextRange)) -> Self {
+        Self {
+            file_id,
+            offset: range.start().into(),
+            length: range.len().into(),
+        }
+    }
+}
+
 impl From<(FileId, usize, usize)> for DiagnosticLocation {
     fn from((file_id, offset, length): (FileId, usize, usize)) -> Self {
         Self {
diff --git a/crates/apollo-compiler/src/validation/directive.rs b/crates/apollo-compiler/src/validation/directive.rs
index 21074ee..fc9e9ab 100644
--- a/crates/apollo-compiler/src/validation/directive.rs
+++ b/crates/apollo-compiler/src/validation/directive.rs
@@ -2,55 +2,12 @@ use std::collections::HashSet;
 
 use crate::{
     diagnostics::{ApolloDiagnostic, DiagnosticData, Label},
-    hir,
-    validation::ast_type_definitions,
-    ValidationDatabase,
+    hir, ValidationDatabase,
 };
-use apollo_parser::ast;
 
 pub fn validate_directive_definitions(db: &dyn ValidationDatabase) -> Vec<ApolloDiagnostic> {
     let mut diagnostics = Vec::new();
 
-    // Directive definitions must have unique names.
-    //
-    // Return a Unique Definition error in case of a duplicate name.
-    let hir = db.directive_definitions();
-    for (file_id, ast_def) in ast_type_definitions::<ast::DirectiveDefinition>(db) {
-        if let Some(name) = ast_def.name() {
-            let name = &*name.text();
-            let hir_def = &hir[name];
-            if let Some(original_definition) = hir_def.loc() {
-                let redefined_definition = (file_id, &ast_def).into();
-                if original_definition == redefined_definition {
-                    // The HIR node was built from this AST node. This is fine.
-                } else {
-                    diagnostics.push(
-                        ApolloDiagnostic::new(
-                            db,
-                            original_definition.into(),
-                            DiagnosticData::UniqueDefinition {
-                                ty: "directive",
-                                name: name.to_owned(),
-                                original_definition: original_definition.into(),
-                                redefined_definition: redefined_definition.into(),
-                            },
-                        )
-                        .help(format!(
-                            "`{name}` must only be defined once in this document."
-                        ))
-                        .labels([
-                            Label::new(
-                                original_definition,
-                                format!("previous definition of `{name}` here"),
-                            ),
-                            Label::new(redefined_definition, format!("`{name}` redefined here")),
-                        ]),
-                    );
-                }
-            }
-        }
-    }
-
     // A directive definition must not contain the use of a directive which
     // references itself directly.
     //
diff --git a/crates/apollo-compiler/src/validation/input_object.rs b/crates/apollo-compiler/src/validation/input_object.rs
index 6e8623a..4ecc37c 100644
--- a/crates/apollo-compiler/src/validation/input_object.rs
+++ b/crates/apollo-compiler/src/validation/input_object.rs
@@ -2,11 +2,8 @@ use std::{collections::HashMap, sync::Arc};
 
 use crate::{
     diagnostics::{ApolloDiagnostic, DiagnosticData, Label},
-    hir,
-    validation::ast_type_definitions,
-    ValidationDatabase,
+    hir, ValidationDatabase,
 };
-use apollo_parser::ast;
 
 pub fn validate_input_object_definitions(db: &dyn ValidationDatabase) -> Vec<ApolloDiagnostic> {
     let mut diagnostics = Vec::new();
@@ -27,44 +24,6 @@ pub fn validate_input_object_definition(
         input_obj.directives().to_vec(),
         hir::DirectiveLocation::InputObject,
     );
-    // Input Object Definitions must have unique names.
-    //
-    // Return a Unique Definition error in case of a duplicate name.
-    let hir = db.input_objects();
-    for (file_id, ast_def) in ast_type_definitions::<ast::InputObjectTypeDefinition>(db) {
-        if let Some(name) = ast_def.name() {
-            let name = &*name.text();
-            let hir_def = &hir[name];
-            let original_definition = hir_def.loc();
-            let redefined_definition = (file_id, &ast_def).into();
-            if original_definition == redefined_definition {
-                // The HIR node was built from this AST node. This is fine.
-            } else {
-                diagnostics.push(
-                    ApolloDiagnostic::new(
-                        db,
-                        original_definition.into(),
-                        DiagnosticData::UniqueDefinition {
-                            ty: "input object",
-                            name: name.to_owned(),
-                            original_definition: original_definition.into(),
-                            redefined_definition: redefined_definition.into(),
-                        },
-                    )
-                    .help(format!(
-                        "`{name}` must only be defined once in this document."
-                    ))
-                    .labels([
-                        Label::new(
-                            original_definition,
-                            format!("previous definition of `{name}` here"),
-                        ),
-                        Label::new(redefined_definition, format!("`{name}` redefined here")),
-                    ]),
-                );
-            }
-        }
-    }
 
     // Fields in an Input Object Definition must be unique
     //
diff --git a/crates/apollo-compiler/src/validation/interface.rs b/crates/apollo-compiler/src/validation/interface.rs
index d5e2fbe..5d679d8 100644
--- a/crates/apollo-compiler/src/validation/interface.rs
+++ b/crates/apollo-compiler/src/validation/interface.rs
@@ -3,10 +3,9 @@ use std::{collections::HashSet, sync::Arc};
 use crate::{
     diagnostics::{ApolloDiagnostic, DiagnosticData, Label},
     hir::{self, ImplementsInterface},
-    validation::{ast_type_definitions, ValidationSet},
+    validation::ValidationSet,
     ValidationDatabase,
 };
-use apollo_parser::ast;
 
 pub fn validate_interface_definitions(db: &dyn ValidationDatabase) -> Vec<ApolloDiagnostic> {
     let mut diagnostics = Vec::new();
@@ -28,48 +27,6 @@ pub fn validate_interface_definition(
 ) -> Vec<ApolloDiagnostic> {
     let mut diagnostics = Vec::new();
 
-    // Interface definitions must have unique names.
-    //
-    // Return a Unique Definition error in case of a duplicate name.
-    let hir = db.interfaces();
-    for (file_id, ast_def) in ast_type_definitions::<ast::InterfaceTypeDefinition>(db) {
-        if let Some(name) = ast_def.name() {
-            let name = &*name.text();
-            let hir_def = &hir[name];
-            let original_definition = hir_def.name_src().loc().unwrap_or_else(|| hir_def.loc());
-            let redefined_definition = ast_def
-                .name()
-                .map(|name| (file_id, &name).into())
-                .unwrap_or_else(|| (file_id, &ast_def).into());
-            if original_definition == redefined_definition {
-                // The HIR node was built from this AST node. This is fine.
-            } else {
-                diagnostics.push(
-                    ApolloDiagnostic::new(
-                        db,
-                        redefined_definition.into(),
-                        DiagnosticData::UniqueDefinition {
-                            ty: "interface",
-                            name: name.into(),
-                            original_definition: original_definition.into(),
-                            redefined_definition: redefined_definition.into(),
-                        },
-                    )
-                    .labels([
-                        Label::new(
-                            original_definition,
-                            format!("previous definition of `{name}` here"),
-                        ),
-                        Label::new(redefined_definition, format!("`{name}` redefined here")),
-                    ])
-                    .help(format!(
-                        "`{name}` must only be defined once in this document."
-                    )),
-                );
-            }
-        }
-    }
-
     // Interface must not implement itself.
     //
     // Return Recursive Definition error.
diff --git a/crates/apollo-compiler/src/validation/mod.rs b/crates/apollo-compiler/src/validation/mod.rs
index 5f3c7f8..31e2be3 100644
--- a/crates/apollo-compiler/src/validation/mod.rs
+++ b/crates/apollo-compiler/src/validation/mod.rs
@@ -17,8 +17,7 @@ mod variable;
 
 pub use validation_db::{ValidationDatabase, ValidationStorage};
 
-use crate::{hir::HirNodeLocation, FileId};
-use apollo_parser::ast::AstNode;
+use crate::hir::HirNodeLocation;
 
 #[derive(Debug, Eq)]
 struct ValidationSet {
@@ -37,24 +36,3 @@ impl PartialEq for ValidationSet {
         self.name == other.name
     }
 }
-
-/// Finds top-level AST nodes of a given type in type definition files.
-///
-/// This ignores pre-computed inputs.
-pub(crate) fn ast_type_definitions<'db, AstType>(
-    db: &'db dyn ValidationDatabase,
-) -> impl Iterator<Item = (FileId, AstType)> + 'db
-where
-    AstType: 'db + AstNode,
-{
-    db.type_definition_files()
-        .into_iter()
-        .flat_map(move |file_id| {
-            db.ast(file_id)
-                .document()
-                .syntax()
-                .children()
-                .filter_map(AstNode::cast)
-                .map(move |def| (file_id, def))
-        })
-}
diff --git a/crates/apollo-compiler/src/validation/object.rs b/crates/apollo-compiler/src/validation/object.rs
index 95a9c85..67cee15 100644
--- a/crates/apollo-compiler/src/validation/object.rs
+++ b/crates/apollo-compiler/src/validation/object.rs
@@ -3,10 +3,9 @@ use std::{collections::HashSet, sync::Arc};
 use crate::{
     diagnostics::{ApolloDiagnostic, DiagnosticData, Label},
     hir,
-    validation::{ast_type_definitions, ValidationSet},
+    validation::ValidationSet,
     ValidationDatabase,
 };
-use apollo_parser::ast;
 
 pub fn validate_object_type_definitions(db: &dyn ValidationDatabase) -> Vec<ApolloDiagnostic> {
     let mut diagnostics = Vec::new();
@@ -29,44 +28,6 @@ pub fn validate_object_type_definition(
         db.validate_directives(object.directives().to_vec(), hir::DirectiveLocation::Object),
     );
 
-    // Object Type definitions must have unique names.
-    //
-    // Return a Unique Definition error in case of a duplicate name.
-    let hir = db.object_types();
-    for (file_id, ast_def) in ast_type_definitions::<ast::ObjectTypeDefinition>(db) {
-        if let Some(name) = ast_def.name() {
-            let name = &*name.text();
-            let original_definition = hir[name].loc();
-            let redefined_definition = (file_id, &ast_def).into();
-            if original_definition == redefined_definition {
-                // The HIR node was built from this AST node. This is fine.
-            } else {
-                diagnostics.push(
-                    ApolloDiagnostic::new(
-                        db,
-                        redefined_definition.into(),
-                        DiagnosticData::UniqueDefinition {
-                            ty: "root operation type definition",
-                            name: name.to_string(),
-                            original_definition: original_definition.into(),
-                            redefined_definition: redefined_definition.into(),
-                        },
-                    )
-                    .labels([
-                        Label::new(
-                            original_definition,
-                            format!("previous definition of `{name}` here"),
-                        ),
-                        Label::new(redefined_definition, format!("`{name}` redefined here")),
-                    ])
-                    .help(format!(
-                        "`{name}` must only be defined once in this document."
-                    )),
-                );
-            }
-        }
-    }
-
     // Object Type field validations.
     diagnostics.extend(db.validate_field_definitions(object.fields_definition().to_vec()));
 
diff --git a/crates/apollo-compiler/src/validation/operation.rs b/crates/apollo-compiler/src/validation/operation.rs
index bcb783b..840baaa 100644
--- a/crates/apollo-compiler/src/validation/operation.rs
+++ b/crates/apollo-compiler/src/validation/operation.rs
@@ -1,4 +1,4 @@
-use std::{collections::HashMap, sync::Arc};
+use std::sync::Arc;
 
 use crate::{
     diagnostics::{ApolloDiagnostic, DiagnosticData, Label},
@@ -56,49 +56,6 @@ pub fn validate_operation_definitions(
         diagnostics.extend(missing_ident);
     }
 
-    // Operation definitions must have unique names.
-    //
-    // Return a Unique Operation Definition error in case of a duplicate name.
-    let mut seen: HashMap<&str, &hir::OperationDefinition> = HashMap::new();
-    for op in operations.iter() {
-        if let Some(name) = op.name() {
-            if let Some(prev_def) = seen.get(&name) {
-                let original_definition = prev_def
-                    .name_src()
-                    .and_then(|name| name.loc())
-                    .unwrap_or_else(|| prev_def.loc());
-                let redefined_definition = op
-                    .name_src()
-                    .and_then(|name| name.loc())
-                    .unwrap_or_else(|| op.loc());
-                diagnostics.push(
-                    ApolloDiagnostic::new(
-                        db,
-                        redefined_definition.into(),
-                        DiagnosticData::UniqueDefinition {
-                            ty: "operation",
-                            name: name.into(),
-                            original_definition: original_definition.into(),
-                            redefined_definition: redefined_definition.into(),
-                        },
-                    )
-                    .labels([
-                        Label::new(
-                            original_definition,
-                            format!("previous definition of `{name}` here"),
-                        ),
-                        Label::new(redefined_definition, format!("`{name}` redefined here")),
-                    ])
-                    .help(format!(
-                        "`{name}` must only be defined once in this document."
-                    )),
-                );
-            } else {
-                seen.insert(name, op);
-            }
-        }
-    }
-
     diagnostics
 }
 
diff --git a/crates/apollo-compiler/src/validation/validation_db.rs b/crates/apollo-compiler/src/validation/validation_db.rs
index 7c2dc09..29c8d3e 100644
--- a/crates/apollo-compiler/src/validation/validation_db.rs
+++ b/crates/apollo-compiler/src/validation/validation_db.rs
@@ -1,8 +1,9 @@
+use std::collections::{hash_map::Entry, HashMap};
 use std::sync::Arc;
 
 use crate::{
     database::db::Upcast,
-    diagnostics::ApolloDiagnostic,
+    diagnostics::{ApolloDiagnostic, DiagnosticData, Label},
     hir::*,
     validation::{
         argument, directive, enum_, fragment, input_object, interface, object, operation, scalar,
@@ -10,6 +11,8 @@ use crate::{
     },
     AstDatabase, FileId, HirDatabase, InputDatabase,
 };
+use apollo_parser::ast;
+use apollo_parser::ast::AstNode;
 
 use super::field;
 
@@ -24,9 +27,11 @@ pub trait ValidationDatabase:
     /// the compiler.
     fn validate_type_system(&self) -> Vec<ApolloDiagnostic>;
 
-    /// Validate the corresonding executable document.
+    /// Validate an executable document.
     fn validate_executable(&self, file_id: FileId) -> Vec<ApolloDiagnostic>;
 
+    fn validate_name_uniqueness(&self) -> Vec<ApolloDiagnostic>;
+
     #[salsa::invoke(schema::validate_schema_definition)]
     fn validate_schema_definition(&self, def: Arc<SchemaDefinition>) -> Vec<ApolloDiagnostic>;
 
@@ -180,9 +185,105 @@ pub fn validate(db: &dyn ValidationDatabase) -> Vec<ApolloDiagnostic> {
     diagnostics
 }
 
+fn validate_name_uniqueness(db: &dyn ValidationDatabase) -> Vec<ApolloDiagnostic> {
+    let mut diagnostics = Vec::new();
+
+    // Different node types use different namespaces.
+    let mut fragment_scope = HashMap::<String, (FileId, ast::Name)>::new();
+    let mut operation_scope = HashMap::new();
+    let mut directive_scope = HashMap::new();
+    let mut type_scope = HashMap::new();
+
+    let all_types = db
+        .type_definition_files()
+        .into_iter()
+        .flat_map(move |file_id| {
+            db.ast(file_id)
+                .document()
+                .syntax()
+                .children()
+                .filter_map(ast::Definition::cast)
+                // Extension names are allowed to be duplicates,
+                // and schema definitions don't have names.
+                .filter(|def| {
+                    !def.is_extension_definition()
+                        && !matches!(def, ast::Definition::SchemaDefinition(_))
+                })
+                .map(move |def| (file_id, def))
+        });
+
+    for (file_id, ast_def) in all_types {
+        let ty_ = match ast_def {
+            ast::Definition::OperationDefinition(_) => "operation",
+            ast::Definition::FragmentDefinition(_) => "fragment",
+            ast::Definition::DirectiveDefinition(_) => "directive",
+            ast::Definition::ScalarTypeDefinition(_)
+            | ast::Definition::ObjectTypeDefinition(_)
+            | ast::Definition::InterfaceTypeDefinition(_)
+            | ast::Definition::UnionTypeDefinition(_)
+            | ast::Definition::EnumTypeDefinition(_)
+            | ast::Definition::InputObjectTypeDefinition(_) => "type",
+            ast::Definition::SchemaDefinition(_)
+            | ast::Definition::SchemaExtension(_)
+            | ast::Definition::ScalarTypeExtension(_)
+            | ast::Definition::ObjectTypeExtension(_)
+            | ast::Definition::InterfaceTypeExtension(_)
+            | ast::Definition::UnionTypeExtension(_)
+            | ast::Definition::EnumTypeExtension(_)
+            | ast::Definition::InputObjectTypeExtension(_) => unreachable!(),
+        };
+        let scope = match ast_def {
+            ast::Definition::OperationDefinition(_) => &mut operation_scope,
+            ast::Definition::FragmentDefinition(_) => &mut fragment_scope,
+            ast::Definition::DirectiveDefinition(_) => &mut directive_scope,
+            _ => &mut type_scope,
+        };
+
+        if let Some(name_node) = ast_def.name() {
+            let name = &*name_node.text();
+            match scope.entry(name.to_string()) {
+                Entry::Occupied(entry) => {
+                    let (original_file_id, original) = entry.get();
+                    let original_definition = (*original_file_id, original.syntax().text_range());
+                    let redefined_definition = (file_id, name_node.syntax().text_range());
+                    diagnostics.push(
+                        ApolloDiagnostic::new(
+                            db,
+                            redefined_definition.into(),
+                            DiagnosticData::UniqueDefinition {
+                                ty: ty_,
+                                name: name.to_string(),
+                                original_definition: original_definition.into(),
+                                redefined_definition: redefined_definition.into(),
+                            },
+                        )
+                        .labels([
+                            Label::new(
+                                original_definition,
+                                format!("previous definition of `{name}` here"),
+                            ),
+                            Label::new(redefined_definition, format!("`{name}` redefined here")),
+                        ])
+                        .help(format!(
+                            "`{name}` must only be defined once in this document."
+                        )),
+                    );
+                }
+                Entry::Vacant(entry) => {
+                    entry.insert((file_id, name_node));
+                }
+            }
+        }
+    }
+
+    diagnostics
+}
+
 pub fn validate_type_system(db: &dyn ValidationDatabase) -> Vec<ApolloDiagnostic> {
     let mut diagnostics = Vec::new();
 
+    diagnostics.extend(db.validate_name_uniqueness());
+
     diagnostics.extend(db.validate_schema_definition(db.type_system_definitions().schema.clone()));
 
     diagnostics.extend(db.validate_scalar_definitions());
diff --git a/crates/apollo-parser/src/ast/node_ext.rs b/crates/apollo-parser/src/ast/node_ext.rs
index da0882f..1253409 100644
--- a/crates/apollo-parser/src/ast/node_ext.rs
+++ b/crates/apollo-parser/src/ast/node_ext.rs
@@ -78,6 +78,45 @@ impl ast::DirectiveLocation {
     }
 }
 
+impl ast::Definition {
+    /// Return the name of this definition, if any. Schema definitions are unnamed and always
+    /// return `None`.
+    pub fn name(&self) -> Option<ast::Name> {
+        match self {
+            Self::OperationDefinition(it) => it.name(),
+            Self::FragmentDefinition(it) => it.fragment_name()?.name(),
+            Self::DirectiveDefinition(it) => it.name(),
+            Self::SchemaDefinition(_) => None,
+            Self::ScalarTypeDefinition(it) => it.name(),
+            Self::ObjectTypeDefinition(it) => it.name(),
+            Self::InterfaceTypeDefinition(it) => it.name(),
+            Self::UnionTypeDefinition(it) => it.name(),
+            Self::EnumTypeDefinition(it) => it.name(),
+            Self::InputObjectTypeDefinition(it) => it.name(),
+            Self::SchemaExtension(_) => None,
+            Self::ScalarTypeExtension(it) => it.name(),
+            Self::ObjectTypeExtension(it) => it.name(),
+            Self::InterfaceTypeExtension(it) => it.name(),
+            Self::UnionTypeExtension(it) => it.name(),
+            Self::EnumTypeExtension(it) => it.name(),
+            Self::InputObjectTypeExtension(it) => it.name(),
+        }
+    }
+
+    pub fn is_extension_definition(&self) -> bool {
+        matches!(
+            self,
+            Self::SchemaExtension(_)
+                | Self::ScalarTypeExtension(_)
+                | Self::ObjectTypeExtension(_)
+                | Self::InterfaceTypeExtension(_)
+                | Self::UnionTypeExtension(_)
+                | Self::EnumTypeExtension(_)
+                | Self::InputObjectTypeExtension(_)
+        )
+    }
+}
+
 impl From<ast::StringValue> for String {
     fn from(val: ast::StringValue) -> Self {
         Self::from(&val)


Test patch:
diff --git a/crates/apollo-compiler/test_data/diagnostics/0022_duplicate_interface_definitions.txt b/crates/apollo-compiler/test_data/diagnostics/0022_duplicate_interface_definitions.txt
index 76b5a5c..89f76a0 100644
--- a/crates/apollo-compiler/test_data/diagnostics/0022_duplicate_interface_definitions.txt
+++ b/crates/apollo-compiler/test_data/diagnostics/0022_duplicate_interface_definitions.txt
@@ -36,7 +36,7 @@
             "`NamedEntity` must only be defined once in this document.",
         ),
         data: UniqueDefinition {
-            ty: "interface",
+            ty: "type",
             name: "NamedEntity",
             original_definition: DiagnosticLocation {
                 file_id: FileId {
diff --git a/crates/apollo-compiler/test_data/diagnostics/0029_directive_definitions_with_duplicate_names.txt b/crates/apollo-compiler/test_data/diagnostics/0029_directive_definitions_with_duplicate_names.txt
index c2ba520..b964594 100644
--- a/crates/apollo-compiler/test_data/diagnostics/0029_directive_definitions_with_duplicate_names.txt
+++ b/crates/apollo-compiler/test_data/diagnostics/0029_directive_definitions_with_duplicate_names.txt
@@ -7,8 +7,8 @@
             file_id: FileId {
                 id: 29,
             },
-            offset: 80,
-            length: 24,
+            offset: 116,
+            length: 3,
         },
         labels: [
             Label {
@@ -16,8 +16,8 @@
                     file_id: FileId {
                         id: 29,
                     },
-                    offset: 80,
-                    length: 24,
+                    offset: 91,
+                    length: 3,
                 },
                 text: "previous definition of `foo` here",
             },
@@ -26,8 +26,8 @@
                     file_id: FileId {
                         id: 29,
                     },
-                    offset: 105,
-                    length: 24,
+                    offset: 116,
+                    length: 3,
                 },
                 text: "`foo` redefined here",
             },
@@ -42,15 +42,15 @@
                 file_id: FileId {
                     id: 29,
                 },
-                offset: 80,
-                length: 24,
+                offset: 91,
+                length: 3,
             },
             redefined_definition: DiagnosticLocation {
                 file_id: FileId {
                     id: 29,
                 },
-                offset: 105,
-                length: 24,
+                offset: 116,
+                length: 3,
             },
         },
     },
diff --git a/crates/apollo-compiler/test_data/diagnostics/0030_schema_with_duplicate_input_objects.txt b/crates/apollo-compiler/test_data/diagnostics/0030_schema_with_duplicate_input_objects.txt
index 9bb0b67..2d5ebb7 100644
--- a/crates/apollo-compiler/test_data/diagnostics/0030_schema_with_duplicate_input_objects.txt
+++ b/crates/apollo-compiler/test_data/diagnostics/0030_schema_with_duplicate_input_objects.txt
@@ -7,8 +7,8 @@
             file_id: FileId {
                 id: 30,
             },
-            offset: 115,
-            length: 39,
+            offset: 162,
+            length: 7,
         },
         labels: [
             Label {
@@ -16,8 +16,8 @@
                     file_id: FileId {
                         id: 30,
                     },
-                    offset: 115,
-                    length: 39,
+                    offset: 121,
+                    length: 7,
                 },
                 text: "previous definition of `Point2D` here",
             },
@@ -26,8 +26,8 @@
                     file_id: FileId {
                         id: 30,
                     },
-                    offset: 156,
-                    length: 39,
+                    offset: 162,
+                    length: 7,
                 },
                 text: "`Point2D` redefined here",
             },
@@ -36,21 +36,21 @@
             "`Point2D` must only be defined once in this document.",
         ),
         data: UniqueDefinition {
-            ty: "input object",
+            ty: "type",
             name: "Point2D",
             original_definition: DiagnosticLocation {
                 file_id: FileId {
                     id: 30,
                 },
-                offset: 115,
-                length: 39,
+                offset: 121,
+                length: 7,
             },
             redefined_definition: DiagnosticLocation {
                 file_id: FileId {
                     id: 30,
                 },
-                offset: 156,
-                length: 39,
+                offset: 162,
+                length: 7,
             },
         },
     },
diff --git a/crates/apollo-compiler/test_data/diagnostics/0032_duplicate_object_type_definition.txt b/crates/apollo-compiler/test_data/diagnostics/0032_duplicate_object_type_definition.txt
index db4b95d..7f97302 100644
--- a/crates/apollo-compiler/test_data/diagnostics/0032_duplicate_object_type_definition.txt
+++ b/crates/apollo-compiler/test_data/diagnostics/0032_duplicate_object_type_definition.txt
@@ -7,8 +7,8 @@
             file_id: FileId {
                 id: 32,
             },
-            offset: 31,
-            length: 29,
+            offset: 36,
+            length: 5,
         },
         labels: [
             Label {
@@ -16,8 +16,8 @@
                     file_id: FileId {
                         id: 32,
                     },
-                    offset: 0,
-                    length: 29,
+                    offset: 5,
+                    length: 5,
                 },
                 text: "previous definition of `Query` here",
             },
@@ -26,8 +26,8 @@
                     file_id: FileId {
                         id: 32,
                     },
-                    offset: 31,
-                    length: 29,
+                    offset: 36,
+                    length: 5,
                 },
                 text: "`Query` redefined here",
             },
@@ -36,21 +36,21 @@
             "`Query` must only be defined once in this document.",
         ),
         data: UniqueDefinition {
-            ty: "root operation type definition",
+            ty: "type",
             name: "Query",
             original_definition: DiagnosticLocation {
                 file_id: FileId {
                     id: 32,
                 },
-                offset: 0,
-                length: 29,
+                offset: 5,
+                length: 5,
             },
             redefined_definition: DiagnosticLocation {
                 file_id: FileId {
                     id: 32,
                 },
-                offset: 31,
-                length: 29,
+                offset: 36,
+                length: 5,
             },
         },
     },
diff --git a/crates/apollo-compiler/test_data/diagnostics/0056_variables_are_input_types.graphql b/crates/apollo-compiler/test_data/diagnostics/0056_variables_are_input_types.graphql
index 7b23ccf..2c54857 100644
--- a/crates/apollo-compiler/test_data/diagnostics/0056_variables_are_input_types.graphql
+++ b/crates/apollo-compiler/test_data/diagnostics/0056_variables_are_input_types.graphql
@@ -20,7 +20,7 @@ query takesCatOrDog($catOrDog: CatOrDog) {
   }
 }
 
-query takesCatOrDog($catOrDog: Dragon) {
+query takesCatOrDog2($catOrDog: Dragon) {
   dog {
     isHouseTrained(atOtherHomes: $catOrDog)
   }
@@ -83,4 +83,4 @@ type Cat implements Pet {
 
 union CatOrDog = Cat | Dog
 union DogOrHuman = Dog | Human
-union HumanOrAlien = Human | Alien
\ No newline at end of file
+union HumanOrAlien = Human | Alien
diff --git a/crates/apollo-compiler/test_data/diagnostics/0056_variables_are_input_types.txt b/crates/apollo-compiler/test_data/diagnostics/0056_variables_are_input_types.txt
index 20b80d6..356c595 100644
--- a/crates/apollo-compiler/test_data/diagnostics/0056_variables_are_input_types.txt
+++ b/crates/apollo-compiler/test_data/diagnostics/0056_variables_are_input_types.txt
@@ -131,7 +131,7 @@
             file_id: FileId {
                 id: 56,
             },
-            offset: 368,
+            offset: 369,
             length: 17,
         },
         labels: [
@@ -140,7 +140,7 @@
                     file_id: FileId {
                         id: 56,
                     },
-                    offset: 368,
+                    offset: 369,
                     length: 17,
                 },
                 text: "not found in the type system",
@@ -151,59 +151,4 @@
             name: "Dragon",
         },
     },
-    ApolloDiagnostic {
-        cache: {
-            56: "0056_variables_are_input_types.graphql",
-        },
-        location: DiagnosticLocation {
-            file_id: FileId {
-                id: 56,
-            },
-            offset: 354,
-            length: 13,
-        },
-        labels: [
-            Label {
-                location: DiagnosticLocation {
-                    file_id: FileId {
-                        id: 56,
-                    },
-                    offset: 252,
-                    length: 13,
-                },
-                text: "previous definition of `takesCatOrDog` here",
-            },
-            Label {
-                location: DiagnosticLocation {
-                    file_id: FileId {
-                        id: 56,
-                    },
-                    offset: 354,
-                    length: 13,
-                },
-                text: "`takesCatOrDog` redefined here",
-            },
-        ],
-        help: Some(
-            "`takesCatOrDog` must only be defined once in this document.",
-        ),
-        data: UniqueDefinition {
-            ty: "operation",
-            name: "takesCatOrDog",
-            original_definition: DiagnosticLocation {
-                file_id: FileId {
-                    id: 56,
-                },
-                offset: 252,
-                length: 13,
-            },
-            redefined_definition: DiagnosticLocation {
-                file_id: FileId {
-                    id: 56,
-                },
-                offset: 354,
-                length: 13,
-            },
-        },
-    },
 ]
diff --git a/crates/apollo-compiler/test_data/diagnostics/0057_duplicate_type_names.graphql b/crates/apollo-compiler/test_data/diagnostics/0057_duplicate_type_names.graphql
new file mode 100644
index 0000000..bfa6f86
--- /dev/null
+++ b/crates/apollo-compiler/test_data/diagnostics/0057_duplicate_type_names.graphql
@@ -0,0 +1,19 @@
+interface World {
+  a: Int
+}
+type World implements World {
+  a: Int
+}
+
+# OK
+scalar X @specifiedBy(url: "https://apollographql.com")
+# not OK
+union X @X = World
+# not OK
+enum X { Y, Z }
+
+type X {
+  x: X
+}
+
+directive @X(ok: Boolean) on UNION
diff --git a/crates/apollo-compiler/test_data/diagnostics/0057_duplicate_type_names.txt b/crates/apollo-compiler/test_data/diagnostics/0057_duplicate_type_names.txt
new file mode 100644
index 0000000..ddd3e5b
--- /dev/null
+++ b/crates/apollo-compiler/test_data/diagnostics/0057_duplicate_type_names.txt
@@ -0,0 +1,222 @@
+[
+    ApolloDiagnostic {
+        cache: {
+            57: "0057_duplicate_type_names.graphql",
+        },
+        location: DiagnosticLocation {
+            file_id: FileId {
+                id: 57,
+            },
+            offset: 34,
+            length: 5,
+        },
+        labels: [
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 57,
+                    },
+                    offset: 10,
+                    length: 5,
+                },
+                text: "previous definition of `World` here",
+            },
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 57,
+                    },
+                    offset: 34,
+                    length: 5,
+                },
+                text: "`World` redefined here",
+            },
+        ],
+        help: Some(
+            "`World` must only be defined once in this document.",
+        ),
+        data: UniqueDefinition {
+            ty: "type",
+            name: "World",
+            original_definition: DiagnosticLocation {
+                file_id: FileId {
+                    id: 57,
+                },
+                offset: 10,
+                length: 5,
+            },
+            redefined_definition: DiagnosticLocation {
+                file_id: FileId {
+                    id: 57,
+                },
+                offset: 34,
+                length: 5,
+            },
+        },
+    },
+    ApolloDiagnostic {
+        cache: {
+            57: "0057_duplicate_type_names.graphql",
+        },
+        location: DiagnosticLocation {
+            file_id: FileId {
+                id: 57,
+            },
+            offset: 147,
+            length: 1,
+        },
+        labels: [
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 57,
+                    },
+                    offset: 83,
+                    length: 1,
+                },
+                text: "previous definition of `X` here",
+            },
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 57,
+                    },
+                    offset: 147,
+                    length: 1,
+                },
+                text: "`X` redefined here",
+            },
+        ],
+        help: Some(
+            "`X` must only be defined once in this document.",
+        ),
+        data: UniqueDefinition {
+            ty: "type",
+            name: "X",
+            original_definition: DiagnosticLocation {
+                file_id: FileId {
+                    id: 57,
+                },
+                offset: 83,
+                length: 1,
+            },
+            redefined_definition: DiagnosticLocation {
+                file_id: FileId {
+                    id: 57,
+                },
+                offset: 147,
+                length: 1,
+            },
+        },
+    },
+    ApolloDiagnostic {
+        cache: {
+            57: "0057_duplicate_type_names.graphql",
+        },
+        location: DiagnosticLocation {
+            file_id: FileId {
+                id: 57,
+            },
+            offset: 174,
+            length: 1,
+        },
+        labels: [
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 57,
+                    },
+                    offset: 83,
+                    length: 1,
+                },
+                text: "previous definition of `X` here",
+            },
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 57,
+                    },
+                    offset: 174,
+                    length: 1,
+                },
+                text: "`X` redefined here",
+            },
+        ],
+        help: Some(
+            "`X` must only be defined once in this document.",
+        ),
+        data: UniqueDefinition {
+            ty: "type",
+            name: "X",
+            original_definition: DiagnosticLocation {
+                file_id: FileId {
+                    id: 57,
+                },
+                offset: 83,
+                length: 1,
+            },
+            redefined_definition: DiagnosticLocation {
+                file_id: FileId {
+                    id: 57,
+                },
+                offset: 174,
+                length: 1,
+            },
+        },
+    },
+    ApolloDiagnostic {
+        cache: {
+            57: "0057_duplicate_type_names.graphql",
+        },
+        location: DiagnosticLocation {
+            file_id: FileId {
+                id: 57,
+            },
+            offset: 191,
+            length: 1,
+        },
+        labels: [
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 57,
+                    },
+                    offset: 83,
+                    length: 1,
+                },
+                text: "previous definition of `X` here",
+            },
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 57,
+                    },
+                    offset: 191,
+                    length: 1,
+                },
+                text: "`X` redefined here",
+            },
+        ],
+        help: Some(
+            "`X` must only be defined once in this document.",
+        ),
+        data: UniqueDefinition {
+            ty: "type",
+            name: "X",
+            original_definition: DiagnosticLocation {
+                file_id: FileId {
+                    id: 57,
+                },
+                offset: 83,
+                length: 1,
+            },
+            redefined_definition: DiagnosticLocation {
+                file_id: FileId {
+                    id: 57,
+                },
+                offset: 191,
+                length: 1,
+            },
+        },
+    },
+]
diff --git a/crates/apollo-compiler/test_data/diagnostics/0058_fragment_definitions_with_duplicate_names.graphql b/crates/apollo-compiler/test_data/diagnostics/0058_fragment_definitions_with_duplicate_names.graphql
new file mode 100644
index 0000000..c67fc6c
--- /dev/null
+++ b/crates/apollo-compiler/test_data/diagnostics/0058_fragment_definitions_with_duplicate_names.graphql
@@ -0,0 +1,25 @@
+# https://github.com/apollographql/apollo-rs/issues/457
+type Query {
+  pet: Pet
+}
+type Pet {
+  name: String!
+  owner: String
+}
+
+query getPet {
+  pet {
+    name
+    ... petFragment
+  }
+}
+
+fragment petFragment on Pet {
+  name
+  owner
+}
+
+fragment petFragment on Pet {
+  name
+  owner
+}
diff --git a/crates/apollo-compiler/test_data/diagnostics/0058_fragment_definitions_with_duplicate_names.txt b/crates/apollo-compiler/test_data/diagnostics/0058_fragment_definitions_with_duplicate_names.txt
new file mode 100644
index 0000000..1b9d0bf
--- /dev/null
+++ b/crates/apollo-compiler/test_data/diagnostics/0058_fragment_definitions_with_duplicate_names.txt
@@ -0,0 +1,57 @@
+[
+    ApolloDiagnostic {
+        cache: {
+            58: "0058_fragment_definitions_with_duplicate_names.graphql",
+        },
+        location: DiagnosticLocation {
+            file_id: FileId {
+                id: 58,
+            },
+            offset: 244,
+            length: 11,
+        },
+        labels: [
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 58,
+                    },
+                    offset: 196,
+                    length: 11,
+                },
+                text: "previous definition of `petFragment` here",
+            },
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 58,
+                    },
+                    offset: 244,
+                    length: 11,
+                },
+                text: "`petFragment` redefined here",
+            },
+        ],
+        help: Some(
+            "`petFragment` must only be defined once in this document.",
+        ),
+        data: UniqueDefinition {
+            ty: "fragment",
+            name: "petFragment",
+            original_definition: DiagnosticLocation {
+                file_id: FileId {
+                    id: 58,
+                },
+                offset: 196,
+                length: 11,
+            },
+            redefined_definition: DiagnosticLocation {
+                file_id: FileId {
+                    id: 58,
+                },
+                offset: 244,
+                length: 11,
+            },
+        },
+    },
+]
diff --git a/crates/apollo-compiler/test_data/ok/0018_non_clashing_names.graphql b/crates/apollo-compiler/test_data/ok/0018_non_clashing_names.graphql
new file mode 100644
index 0000000..1731225
--- /dev/null
+++ b/crates/apollo-compiler/test_data/ok/0018_non_clashing_names.graphql
@@ -0,0 +1,19 @@
+# Types, directives, fragments, and operations each have their own namespace.
+type A @A {
+  a: Int
+}
+type Query {
+  a: A
+}
+
+directive @A on OBJECT
+
+fragment A on A {
+  a
+}
+
+query A {
+  a {
+    ...A
+  }
+}
diff --git a/crates/apollo-compiler/test_data/ok/0018_non_clashing_names.txt b/crates/apollo-compiler/test_data/ok/0018_non_clashing_names.txt
new file mode 100644
index 0000000..628821c
--- /dev/null
+++ b/crates/apollo-compiler/test_data/ok/0018_non_clashing_names.txt
@@ -0,0 +1,113 @@
+- DOCUMENT@0..204
+    - COMMENT@0..77 "# Types, directives, fragments, and operations each have their own namespace."
+    - WHITESPACE@77..78 "\n"
+    - OBJECT_TYPE_DEFINITION@78..100
+        - type_KW@78..82 "type"
+        - WHITESPACE@82..83 " "
+        - NAME@83..84
+            - IDENT@83..84 "A"
+        - WHITESPACE@84..85 " "
+        - DIRECTIVES@85..87
+            - DIRECTIVE@85..87
+                - AT@85..86 "@"
+                - NAME@86..87
+                    - IDENT@86..87 "A"
+        - WHITESPACE@87..88 " "
+        - FIELDS_DEFINITION@88..100
+            - L_CURLY@88..89 "{"
+            - WHITESPACE@89..92 "\n  "
+            - FIELD_DEFINITION@92..98
+                - NAME@92..93
+                    - IDENT@92..93 "a"
+                - COLON@93..94 ":"
+                - WHITESPACE@94..95 " "
+                - NAMED_TYPE@95..98
+                    - NAME@95..98
+                        - IDENT@95..98 "Int"
+            - WHITESPACE@98..99 "\n"
+            - R_CURLY@99..100 "}"
+    - WHITESPACE@100..101 "\n"
+    - OBJECT_TYPE_DEFINITION@101..122
+        - type_KW@101..105 "type"
+        - WHITESPACE@105..106 " "
+        - NAME@106..111
+            - IDENT@106..111 "Query"
+        - WHITESPACE@111..112 " "
+        - FIELDS_DEFINITION@112..122
+            - L_CURLY@112..113 "{"
+            - WHITESPACE@113..116 "\n  "
+            - FIELD_DEFINITION@116..120
+                - NAME@116..117
+                    - IDENT@116..117 "a"
+                - COLON@117..118 ":"
+                - WHITESPACE@118..119 " "
+                - NAMED_TYPE@119..120
+                    - NAME@119..120
+                        - IDENT@119..120 "A"
+            - WHITESPACE@120..121 "\n"
+            - R_CURLY@121..122 "}"
+    - WHITESPACE@122..124 "\n\n"
+    - DIRECTIVE_DEFINITION@124..146
+        - directive_KW@124..133 "directive"
+        - WHITESPACE@133..134 " "
+        - AT@134..135 "@"
+        - NAME@135..136
+            - IDENT@135..136 "A"
+        - WHITESPACE@136..137 " "
+        - on_KW@137..139 "on"
+        - WHITESPACE@139..140 " "
+        - DIRECTIVE_LOCATIONS@140..146
+            - DIRECTIVE_LOCATION@140..146
+                - OBJECT_KW@140..146 "OBJECT"
+    - WHITESPACE@146..148 "\n\n"
+    - FRAGMENT_DEFINITION@148..171
+        - fragment_KW@148..156 "fragment"
+        - WHITESPACE@156..157 " "
+        - FRAGMENT_NAME@157..158
+            - NAME@157..158
+                - IDENT@157..158 "A"
+        - WHITESPACE@158..159 " "
+        - TYPE_CONDITION@159..163
+            - on_KW@159..161 "on"
+            - WHITESPACE@161..162 " "
+            - NAMED_TYPE@162..163
+                - NAME@162..163
+                    - IDENT@162..163 "A"
+        - WHITESPACE@163..164 " "
+        - SELECTION_SET@164..171
+            - L_CURLY@164..165 "{"
+            - WHITESPACE@165..168 "\n  "
+            - FIELD@168..169
+                - NAME@168..169
+                    - IDENT@168..169 "a"
+            - WHITESPACE@169..170 "\n"
+            - R_CURLY@170..171 "}"
+    - WHITESPACE@171..173 "\n\n"
+    - OPERATION_DEFINITION@173..203
+        - OPERATION_TYPE@173..178
+            - query_KW@173..178 "query"
+        - WHITESPACE@178..179 " "
+        - NAME@179..180
+            - IDENT@179..180 "A"
+        - WHITESPACE@180..181 " "
+        - SELECTION_SET@181..203
+            - L_CURLY@181..182 "{"
+            - WHITESPACE@182..185 "\n  "
+            - FIELD@185..201
+                - NAME@185..186
+                    - IDENT@185..186 "a"
+                - WHITESPACE@186..187 " "
+                - SELECTION_SET@187..201
+                    - L_CURLY@187..188 "{"
+                    - WHITESPACE@188..193 "\n    "
+                    - FRAGMENT_SPREAD@193..197
+                        - SPREAD@193..196 "..."
+                        - FRAGMENT_NAME@196..197
+                            - NAME@196..197
+                                - IDENT@196..197 "A"
+                    - WHITESPACE@197..200 "\n  "
+                    - R_CURLY@200..201 "}"
+            - WHITESPACE@201..202 "\n"
+            - R_CURLY@202..203 "}"
+    - WHITESPACE@203..204 "\n"
+recursion limit: 4096, high: 3