From 1beccf22dafe108247136470a4d2beda30c49b43 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 8 Jul 2026 04:30:28 +0000 Subject: [PATCH 1/3] [libgraphql-core-v1] Hygiene fixes, from_str family, Schema::resolve_span Implements Task 16.6e. Adds the small public APIs the Task 16.5 audit found claimed-but-missing, and clears the stale-doc debt in the validators: - SchemaBuilder::from_str constructs a builder from schema source (the API Disposition ledger named it; only load_str/build_from_str existed). build_from_str now delegates through it. - Source labels for multi-source schemas: new from_str_with_label / load_str_with_label / build_from_str_with_label variants thread an optional label (impl AsRef) into SchemaSourceMap.file_path, so diagnostics from multi-document schemas can name their source. Unlabeled entry points keep their clean signatures; all variants share a private load_str_impl. Task 19's operation builders will copy this shape. - Schema::resolve_span(Span) -> Option per AD18: indexes the schema's source maps by the span's source_map_id (None when out of range), rustdoc states the schema-originated-span domain per AD18's cross-artifact rule. - Stale rustdoc fixed: validators/mod.rs claimed build() was still todo!(); union_type_validator carried an already-completed TODO. - Dead #[allow(dead_code)] mutation_type_name() accessor removed from SchemaBuilder (build() reads the field directly). - New schema_def_tests.rs covers the previously-untested typed query API: typed lookups, typed iterators, types_implementing, root-op accessors, and resolve_span (hand-computed positions on a multi-line schema; out-of-range -> None; builtin -> 0:0). - Zeroed 9 pre-existing cargo-doc warnings so the doc gate holds. 17 tests + 2 running doctests added. All workspace tests, clippy -Dwarnings, and cargo doc pass clean. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01TipQtpvuLuHjHmRsVEgwJw --- crates/libgraphql-core-v1/src/error_note.rs | 4 +- .../src/schema/schema_builder.rs | 143 ++++++- .../src/schema/schema_def.rs | 46 +++ .../src/schema/tests/mod.rs | 1 + .../src/schema/tests/schema_builder_tests.rs | 126 +++++++ .../src/schema/tests/schema_def_tests.rs | 355 ++++++++++++++++++ .../src/schema_source_map.rs | 3 +- crates/libgraphql-core-v1/src/span.rs | 4 +- .../src/types/field_definition.rs | 4 +- .../libgraphql-core-v1/src/validators/mod.rs | 13 +- .../src/validators/union_type_validator.rs | 8 +- libgraphql-core-v1-plan.md | 56 ++- 12 files changed, 716 insertions(+), 47 deletions(-) create mode 100644 crates/libgraphql-core-v1/src/schema/tests/schema_def_tests.rs diff --git a/crates/libgraphql-core-v1/src/error_note.rs b/crates/libgraphql-core-v1/src/error_note.rs index 681b07b..35d1add 100644 --- a/crates/libgraphql-core-v1/src/error_note.rs +++ b/crates/libgraphql-core-v1/src/error_note.rs @@ -10,9 +10,9 @@ use crate::span::Span; /// defined, or where a conflicting field exists) /// /// This is the schema-layer analogue of -/// [`libgraphql_parser::GraphQLErrorNote`](libgraphql_parser::GraphQLErrorNote) +/// [`libgraphql_parser::GraphQLErrorNote`] /// in the parser crate. The key difference is that spans here -/// are [`libgraphql_core::Span`]s (deferred byte-offset + source map ID) rather +/// are [`Span`]s (deferred byte-offset + source map ID) rather /// than pre-resolved `SourceSpan`s, since schema errors may /// reference locations across multiple source files loaded at /// different times. diff --git a/crates/libgraphql-core-v1/src/schema/schema_builder.rs b/crates/libgraphql-core-v1/src/schema/schema_builder.rs index 92d3529..8d996c6 100644 --- a/crates/libgraphql-core-v1/src/schema/schema_builder.rs +++ b/crates/libgraphql-core-v1/src/schema/schema_builder.rs @@ -52,6 +52,8 @@ use indexmap::IndexMap; use libgraphql_parser::ast; use libgraphql_parser::ByteSpan; use libgraphql_parser::GraphQLErrorNoteKind; +use std::path::Path; +use std::path::PathBuf; /// Accumulates GraphQL type definitions, directive definitions, /// and schema metadata, then validates and produces an immutable @@ -409,9 +411,108 @@ impl SchemaBuilder { /// errors are collected into the returned `Err` variant /// with their original parser spans translated to our /// [`Span`] type. + /// + /// The registered source map carries no source label; when + /// loading multiple sources, prefer + /// [`load_str_with_label()`](Self::load_str_with_label) so + /// diagnostics can identify which source a location refers + /// to. pub fn load_str( &mut self, source: &str, + ) -> Result<&mut Self, Vec> { + self.load_str_impl(source, /* label = */ None) + } + + /// Like [`load_str()`](Self::load_str), but labels the + /// registered [`SchemaSourceMap`] with `label` (typically + /// the path the source text was read from). + /// + /// The label is stored as the source map's + /// [`file_path()`](SchemaSourceMap::file_path) and is + /// surfaced by diagnostics that resolve spans from this + /// source, which is especially useful when a schema is + /// assembled from multiple source strings. + /// + /// # Example + /// + /// ```rust + /// # use libgraphql_core_v1 as libgraphql_core; + /// use libgraphql_core::schema::SchemaBuilder; + /// use std::path::Path; + /// + /// let mut builder = SchemaBuilder::new(); + /// builder.load_str_with_label( + /// "type Query { hello: String }", + /// "schemas/main.graphql", + /// ).unwrap(); + /// let schema = builder.build().unwrap(); + /// + /// assert_eq!( + /// schema.source_maps()[1].file_path(), + /// Some(Path::new("schemas/main.graphql")), + /// ); + /// ``` + pub fn load_str_with_label( + &mut self, + source: &str, + label: impl AsRef, + ) -> Result<&mut Self, Vec> { + self.load_str_impl(source, Some(label.as_ref().to_path_buf())) + } + + /// Convenience: creates a new `SchemaBuilder` and loads + /// `source` into it in one step. + /// + /// Equivalent to [`SchemaBuilder::new()`](Self::new) + /// followed by [`load_str()`](Self::load_str). Additional + /// sources can still be loaded into the returned builder + /// before calling [`build()`](Self::build). + /// + /// # Example + /// + /// ```rust + /// # use libgraphql_core_v1 as libgraphql_core; + /// use libgraphql_core::schema::SchemaBuilder; + /// + /// let builder = SchemaBuilder::from_str( + /// "type Query { hello: String }", + /// ).unwrap(); + /// let schema = builder.build().unwrap(); + /// + /// assert!(schema.object_type("Query").is_some()); + /// ``` + // An inherent `from_str` is intentional (mirroring the other + // string entry points and the v0 API) rather than a + // `std::str::FromStr` impl, which would force callers to + // import the trait for a builder-construction convenience. + #[allow(clippy::should_implement_trait)] + pub fn from_str(source: &str) -> Result> { + let mut builder = Self::new(); + builder.load_str(source)?; + Ok(builder) + } + + /// Like [`from_str()`](Self::from_str), but labels the + /// registered [`SchemaSourceMap`] with `label` (see + /// [`load_str_with_label()`](Self::load_str_with_label)). + pub fn from_str_with_label( + source: &str, + label: impl AsRef, + ) -> Result> { + let mut builder = Self::new(); + builder.load_str_with_label(source, label)?; + Ok(builder) + } + + /// Shared implementation for the `load_str` family: + /// registers a [`SchemaSourceMap`] (labeled with `label`, + /// if provided) for `source`, parses it, and loads all + /// definitions into this builder. + fn load_str_impl( + &mut self, + source: &str, + label: Option, ) -> Result<&mut Self, Vec> { let parse_result = libgraphql_parser::parse_schema(source); @@ -434,7 +535,7 @@ impl SchemaBuilder { }, }; self.source_maps.push( - SchemaSourceMap::from_source(source, None), + SchemaSourceMap::from_source(source, label), ); // Report parse-level errors with proper spans @@ -791,11 +892,11 @@ impl SchemaBuilder { /// 3. **Empty type checks** -- rejects object/interface types /// with no fields, unions with no members, and enums with /// no values. - /// 4. **Type-system validators** -- runs - /// [`ObjectOrInterfaceTypeValidator`], - /// [`UnionTypeValidator`], - /// [`InputObjectTypeValidator`], and - /// [`validate_directive_definitions`] to enforce + /// 4. **Type-system validators** -- runs the (internal) + /// `ObjectOrInterfaceTypeValidator`, + /// `UnionTypeValidator`, + /// `InputObjectTypeValidator`, and + /// `validate_directive_definitions` passes to enforce /// cross-type reference rules. /// /// See [Schema](https://spec.graphql.org/September2025/#sec-Schema). @@ -1024,9 +1125,24 @@ impl SchemaBuilder { pub fn build_from_str( source: &str, ) -> Result { - let mut sb = Self::new(); - sb.load_str(source).map_err(SchemaErrors::new)?; - sb.build() + Self::from_str(source) + .map_err(SchemaErrors::new) + .and_then(Self::build) + } + + /// Like [`build_from_str()`](Self::build_from_str), but + /// labels the registered [`SchemaSourceMap`] with `label` + /// (see [`load_str_with_label()`](Self::load_str_with_label)). + // TODO: SchemaErrors wraps Vec which is + // large. Consider boxing once error strategy is finalized. + #[allow(clippy::result_large_err)] + pub fn build_from_str_with_label( + source: &str, + label: impl AsRef, + ) -> Result { + Self::from_str_with_label(source, label) + .map_err(SchemaErrors::new) + .and_then(Self::build) } // --------------------------------------------------------- @@ -1107,15 +1223,6 @@ impl SchemaBuilder { pub(crate) fn errors(&self) -> &[SchemaBuildError] { &self.errors } - - /// Returns the mutation root type name binding (for test - /// inspection). - // TODO: Remove #[allow(dead_code)] once mutation root type - // tests are added or build() consumes this field. - #[allow(dead_code)] - pub(crate) fn mutation_type_name(&self) -> Option<&(TypeName, Span)> { - self.mutation_type_name.as_ref() - } } // --------------------------------------------------------- diff --git a/crates/libgraphql-core-v1/src/schema/schema_def.rs b/crates/libgraphql-core-v1/src/schema/schema_def.rs index 120f044..53b7a62 100644 --- a/crates/libgraphql-core-v1/src/schema/schema_def.rs +++ b/crates/libgraphql-core-v1/src/schema/schema_def.rs @@ -1,6 +1,8 @@ use crate::names::DirectiveName; use crate::names::TypeName; +use crate::schema_source_map::LineCol; use crate::schema_source_map::SchemaSourceMap; +use crate::span::Span; use crate::types::DirectiveDefinition; use crate::types::EnumType; use crate::types::GraphQLType; @@ -265,4 +267,48 @@ impl Schema { pub fn source_maps(&self) -> &[SchemaSourceMap] { &self.source_maps } + + // --------------------------------------------------------- + // Span resolution + // --------------------------------------------------------- + + /// Resolves the start of a **schema-originated** [`Span`] to + /// a 0-based [`LineCol`] position using this schema's stored + /// source maps. + /// + /// # Accepted span domain + /// + /// This method only accepts spans that originated from this + /// `Schema` — i.e. spans found on its types, fields, + /// directive definitions, and the errors produced while + /// building it. A [`Span`]'s `source_map_id` is an index + /// that is only meaningful relative to the artifact that + /// produced it, so passing a span from any *other* artifact + /// (an operation, an executable document, or a different + /// `Schema`) yields `None` or a plausible-but-wrong + /// location. + /// + /// # Return value + /// + /// - `Some(LineCol)` for spans whose `source_map_id` is in + /// range for this schema. Spans on built-in definitions + /// (e.g. the `Boolean` scalar or `@skip` directive, which + /// have no user-authored source) resolve against the + /// synthetic built-in source map to line `0`, column `0`. + /// - `None` when the span's `source_map_id` is out of range + /// for this schema. + /// + /// Because `Schema` does not retain source text, the + /// returned [`LineCol`]'s `col_utf8` always equals its + /// `col_linestart_byte_offset` (exact for ASCII source; a + /// byte-based approximation when the line contains + /// multi-byte characters). + pub fn resolve_span(&self, span: Span) -> Option { + let source_map = self.source_maps + .get(span.source_map_id.0 as usize)?; + Some(source_map.resolve_offset( + span.byte_span.start, + /* source = */ None, + )) + } } diff --git a/crates/libgraphql-core-v1/src/schema/tests/mod.rs b/crates/libgraphql-core-v1/src/schema/tests/mod.rs index ba78e92..a587b12 100644 --- a/crates/libgraphql-core-v1/src/schema/tests/mod.rs +++ b/crates/libgraphql-core-v1/src/schema/tests/mod.rs @@ -1,5 +1,6 @@ mod schema_build_error_tests; mod schema_builder_extension_tests; mod schema_builder_tests; +mod schema_def_tests; mod schema_errors_tests; mod type_validation_error_tests; diff --git a/crates/libgraphql-core-v1/src/schema/tests/schema_builder_tests.rs b/crates/libgraphql-core-v1/src/schema/tests/schema_builder_tests.rs index efbdff8..bec75af 100644 --- a/crates/libgraphql-core-v1/src/schema/tests/schema_builder_tests.rs +++ b/crates/libgraphql-core-v1/src/schema/tests/schema_builder_tests.rs @@ -4,11 +4,13 @@ use crate::operation_kind::OperationKind; use crate::schema::SchemaBuildErrorKind; use crate::schema::SchemaBuilder; use crate::schema::TypeValidationErrorKind; +use crate::span::SourceMapId; use crate::span::Span; use crate::type_builders::ObjectTypeBuilder; use crate::types::DeprecationState; use crate::types::GraphQLTypeKind; use crate::types::ScalarKind; +use std::path::Path; // Verifies that SchemaBuilder::new() pre-seeds the five built-in // scalar types: Boolean, Float, ID, Int, String. @@ -1461,3 +1463,127 @@ fn all_build_level_errors_carry_spec_notes() { ); } } + +// Verifies the `SchemaBuilder::from_str()` round trip: a schema +// string is loaded into a freshly constructed builder in one +// step, additional definitions can still be loaded into the +// returned builder, and `build()` produces a schema containing +// definitions from both sources. +// +// Written by Claude Code, reviewed by a human. +#[test] +fn from_str_round_trip_builds_schema() { + let mut builder = SchemaBuilder::from_str( + "type Query { user: User }", + ).expect("from_str should succeed on a valid schema string"); + builder.load_str("type User { id: ID! }") + .expect("load_str on a from_str-built builder should succeed"); + let schema = builder.build().expect("schema should build"); + + assert!(schema.object_type("Query").is_some()); + assert!(schema.object_type("User").is_some()); + // Builtin map (id 0) + one map per loaded source. + assert_eq!(schema.source_maps().len(), 3); +} + +// Verifies that `SchemaBuilder::from_str()` surfaces parse +// errors: an unparsable schema string produces `Err` with at +// least one `ParseError`, matching `load_str()` semantics. +// +// Written by Claude Code, reviewed by a human. +#[test] +fn from_str_parse_error_returns_errors() { + let Err(errors) = SchemaBuilder::from_str("type Query {{{{") else { + panic!("from_str should fail on unparsable source"); + }; + assert!(!errors.is_empty()); + assert!(errors.iter().any(|e| { + matches!(e.kind(), SchemaBuildErrorKind::ParseError { .. }) + })); +} + +// Verifies that a source label supplied to +// `load_str_with_label()` is threaded through `build()` into +// the resulting schema: the labeled `SchemaSourceMap` carries +// the label as its `file_path()`, and spans on types loaded +// from each source resolve against the correct (labeled) +// source map. +// +// Written by Claude Code, reviewed by a human. +#[test] +fn load_str_with_label_threads_label_into_schema_source_maps() { + let mut builder = SchemaBuilder::new(); + builder.load_str_with_label( + "type Query { user: User }", + "schemas/query.graphql", + ).expect("first labeled source should load"); + builder.load_str_with_label( + "type User {\n id: ID!\n}", + "schemas/user.graphql", + ).expect("second labeled source should load"); + let schema = builder.build().expect("schema should build"); + + // Builtin map (id 0) is unlabeled; each loaded source's map + // carries its label. + let source_maps = schema.source_maps(); + assert_eq!(source_maps.len(), 3); + assert_eq!(source_maps[0].file_path(), None); + assert_eq!( + source_maps[1].file_path(), + Some(Path::new("schemas/query.graphql")), + ); + assert_eq!( + source_maps[2].file_path(), + Some(Path::new("schemas/user.graphql")), + ); + + // Spans on types from each source are stamped with the id + // of that source's (labeled) map... + let query_span = schema.object_type("Query").unwrap().span(); + let user_span = schema.object_type("User").unwrap().span(); + assert_eq!(query_span.source_map_id, SourceMapId(1)); + assert_eq!(user_span.source_map_id, SourceMapId(2)); + + // ...and resolve to positions within their own source text: + // `type User` sits on line 0 of the second document even + // though the first document also has a line 0. + let user_line_col = schema.resolve_span(user_span) + .expect("User span should resolve"); + assert_eq!(user_line_col.line, 0); +} + +// Verifies that the label-taking one-step conveniences +// (`from_str_with_label()` and `build_from_str_with_label()`) +// both land the label in the built schema's source map, and +// that the label-less entry points leave `file_path()` unset. +// +// Written by Claude Code, reviewed by a human. +#[test] +fn from_str_and_build_from_str_label_variants_set_label() { + let source = "type Query { hello: String }"; + + let schema = SchemaBuilder::from_str_with_label( + source, + "main.graphql", + ).expect("from_str_with_label should succeed") + .build() + .expect("schema should build"); + assert_eq!( + schema.source_maps()[1].file_path(), + Some(Path::new("main.graphql")), + ); + + let schema = SchemaBuilder::build_from_str_with_label( + source, + "main.graphql", + ).expect("build_from_str_with_label should succeed"); + assert_eq!( + schema.source_maps()[1].file_path(), + Some(Path::new("main.graphql")), + ); + + // Label-less entry points register an unlabeled source map. + let schema = SchemaBuilder::build_from_str(source) + .expect("build_from_str should succeed"); + assert_eq!(schema.source_maps()[1].file_path(), None); +} diff --git a/crates/libgraphql-core-v1/src/schema/tests/schema_def_tests.rs b/crates/libgraphql-core-v1/src/schema/tests/schema_def_tests.rs new file mode 100644 index 0000000..330a35f --- /dev/null +++ b/crates/libgraphql-core-v1/src/schema/tests/schema_def_tests.rs @@ -0,0 +1,355 @@ +use crate::schema::Schema; +use crate::schema::SchemaBuilder; +use crate::span::SourceMapId; +use crate::span::Span; +use libgraphql_parser::ByteSpan; + +/// A multi-kind schema exercising every typed lookup category: +/// objects (including all three root operation types), +/// interfaces (including an interface implementing another +/// interface), an enum, a union, an input object, and a custom +/// scalar. +const KITCHEN_SINK_SCHEMA: &str = "\ +schema { + query: TheQuery + mutation: TheMutation + subscription: TheSubscription +} + +type TheQuery { user: User } +type TheMutation { addUser(name: String!): User } +type TheSubscription { userAdded: User } + +interface Node { id: ID! } +interface Timestamped implements Node { id: ID! createdAt: String } + +type User implements Node { id: ID! name: String } +type Post implements Node & Timestamped { + id: ID! + createdAt: String + title: String +} + +enum Color { RED GREEN BLUE } +union SearchResult = User | Post +input UserFilter { nameContains: String } +scalar DateTime +"; + +fn build_kitchen_sink_schema() -> Schema { + SchemaBuilder::build_from_str(KITCHEN_SINK_SCHEMA) + .expect("kitchen-sink schema should build") +} + +// Verifies that each typed lookup (`object_type`, +// `interface_type`, `enum_type`, `union_type`, +// `input_object_type`, `scalar_type`) returns the named type +// when the name refers to a type of the matching kind -- +// including built-in scalars for `scalar_type`. +// +// See https://spec.graphql.org/September2025/#sec-Types +// +// Written by Claude Code, reviewed by a human. +#[test] +fn typed_lookups_return_kind_matched_types() { + let schema = build_kitchen_sink_schema(); + + let user = schema.object_type("User") + .expect("object_type(User) should be Some"); + assert_eq!(user.name().as_str(), "User"); + + let node = schema.interface_type("Node") + .expect("interface_type(Node) should be Some"); + assert_eq!(node.name().as_str(), "Node"); + + let color = schema.enum_type("Color") + .expect("enum_type(Color) should be Some"); + assert_eq!(color.name().as_str(), "Color"); + + let search_result = schema.union_type("SearchResult") + .expect("union_type(SearchResult) should be Some"); + assert_eq!(search_result.name().as_str(), "SearchResult"); + + let user_filter = schema.input_object_type("UserFilter") + .expect("input_object_type(UserFilter) should be Some"); + assert_eq!(user_filter.name().as_str(), "UserFilter"); + + let date_time = schema.scalar_type("DateTime") + .expect("scalar_type(DateTime) should be Some"); + assert_eq!(date_time.name().as_str(), "DateTime"); + + // Built-in scalars are reachable through the typed lookup + // too. + let string_scalar = schema.scalar_type("String") + .expect("scalar_type(String) should be Some"); + assert!(string_scalar.is_builtin()); +} + +// Verifies that the typed lookups return `None` both for names +// that are not defined at all and for names that are defined +// but refer to a type of a different kind (e.g. asking for +// `object_type("Node")` when `Node` is an interface). +// +// See https://spec.graphql.org/September2025/#sec-Types +// +// Written by Claude Code, reviewed by a human. +#[test] +fn typed_lookups_return_none_for_missing_or_mismatched() { + let schema = build_kitchen_sink_schema(); + + // Undefined name -> None from every lookup. + assert!(schema.get_type("Missing").is_none()); + assert!(schema.object_type("Missing").is_none()); + assert!(schema.interface_type("Missing").is_none()); + assert!(schema.enum_type("Missing").is_none()); + assert!(schema.union_type("Missing").is_none()); + assert!(schema.input_object_type("Missing").is_none()); + assert!(schema.scalar_type("Missing").is_none()); + + // Defined name, wrong kind -> None. + assert!(schema.object_type("Node").is_none()); + assert!(schema.interface_type("User").is_none()); + assert!(schema.enum_type("SearchResult").is_none()); + assert!(schema.union_type("Color").is_none()); + assert!(schema.input_object_type("DateTime").is_none()); + assert!(schema.scalar_type("UserFilter").is_none()); +} + +// Verifies that the typed iterators (`object_types`, +// `interface_types`, `enum_types`) yield exactly the types of +// each respective kind that are defined in the schema. +// +// See https://spec.graphql.org/September2025/#sec-Types +// +// Written by Claude Code, reviewed by a human. +#[test] +fn typed_iterators_yield_all_types_of_each_kind() { + let schema = build_kitchen_sink_schema(); + + let mut object_names: Vec<&str> = schema.object_types() + .map(|t| t.name().as_str()) + .collect(); + object_names.sort_unstable(); + assert_eq!( + object_names, + vec!["Post", "TheMutation", "TheQuery", "TheSubscription", "User"], + ); + + let mut interface_names: Vec<&str> = schema.interface_types() + .map(|t| t.name().as_str()) + .collect(); + interface_names.sort_unstable(); + assert_eq!(interface_names, vec!["Node", "Timestamped"]); + + let enum_names: Vec<&str> = schema.enum_types() + .map(|t| t.name().as_str()) + .collect(); + assert_eq!(enum_names, vec!["Color"]); +} + +// Verifies that `types_implementing()` returns every type -- +// both object types AND interface types -- that declares it +// implements the given interface, and nothing else. +// +// See https://spec.graphql.org/September2025/#IsValidImplementation() +// +// Written by Claude Code, reviewed by a human. +#[test] +fn types_implementing_returns_objects_and_interfaces() { + let schema = build_kitchen_sink_schema(); + + let mut node_impls: Vec<&str> = schema.types_implementing("Node") + .iter() + .map(|t| t.name().as_str()) + .collect(); + node_impls.sort_unstable(); + assert_eq!(node_impls, vec!["Post", "Timestamped", "User"]); + + let timestamped_impls: Vec<&str> = + schema.types_implementing("Timestamped") + .iter() + .map(|t| t.name().as_str()) + .collect(); + assert_eq!(timestamped_impls, vec!["Post"]); +} + +// Verifies that `types_implementing()` returns an empty list +// for an interface name that no type implements (including a +// name that is not defined in the schema at all). +// +// See https://spec.graphql.org/September2025/#IsValidImplementation() +// +// Written by Claude Code, reviewed by a human. +#[test] +fn types_implementing_unknown_interface_returns_empty() { + let schema = build_kitchen_sink_schema(); + assert!(schema.types_implementing("Missing").is_empty()); + // `User` is a defined type but not an interface anything + // implements. + assert!(schema.types_implementing("User").is_empty()); +} + +// Verifies the root operation accessors (`query_type`, +// `mutation_type`, `subscription_type` and their `*_type_name` +// variants) when all three root operation types are explicitly +// bound via a `schema { ... }` definition. +// +// See https://spec.graphql.org/September2025/#sec-Root-Operation-Types +// +// Written by Claude Code, reviewed by a human. +#[test] +fn root_operation_type_accessors() { + let schema = build_kitchen_sink_schema(); + + assert_eq!(schema.query_type_name().as_str(), "TheQuery"); + let query_type = schema.query_type() + .expect("query_type() should be Some"); + assert_eq!(query_type.name().as_str(), "TheQuery"); + + assert_eq!( + schema.mutation_type_name().map(|n| n.as_str()), + Some("TheMutation"), + ); + let mutation_type = schema.mutation_type() + .expect("mutation_type() should be Some"); + assert_eq!(mutation_type.name().as_str(), "TheMutation"); + + assert_eq!( + schema.subscription_type_name().map(|n| n.as_str()), + Some("TheSubscription"), + ); + let subscription_type = schema.subscription_type() + .expect("subscription_type() should be Some"); + assert_eq!(subscription_type.name().as_str(), "TheSubscription"); +} + +// Verifies root operation accessor behavior for a schema with +// no explicit `schema { ... }` definition: the query root +// defaults to the type named `Query`, while the mutation and +// subscription accessors (and their `*_type_name` variants) +// return `None`. +// +// See https://spec.graphql.org/September2025/#sec-Root-Operation-Types +// +// Written by Claude Code, reviewed by a human. +#[test] +fn root_operation_defaults_when_not_declared() { + let schema = SchemaBuilder::build_from_str( + "type Query { hello: String }", + ).expect("schema should build"); + + assert_eq!(schema.query_type_name().as_str(), "Query"); + assert!(schema.query_type().is_some()); + assert!(schema.mutation_type_name().is_none()); + assert!(schema.mutation_type().is_none()); + assert!(schema.subscription_type_name().is_none()); + assert!(schema.subscription_type().is_none()); +} + +// Verifies that `Schema::resolve_span()` resolves an in-range +// span to the correct 0-based line and column on a multi-line +// schema. The span is constructed manually at a hand-computed +// byte offset so the expected `LineCol` is exact: +// +// ```text +// type Query {\n <- line 0, bytes 0..=12 +// hello: String\n <- line 1 starts at byte 13 +// }\n <- `hello` starts at byte 15 (col 2) +// ``` +// +// Written by Claude Code, reviewed by a human. +#[test] +fn resolve_span_in_range_returns_correct_line_col() { + let source = "type Query {\n hello: String\n}\n"; + let schema = SchemaBuilder::build_from_str(source) + .expect("schema should build"); + + // User sources start at source-map id 1 (id 0 is reserved + // for built-in definitions). + let span = Span::new(ByteSpan::new(15, 20), SourceMapId(1)); + let line_col = schema.resolve_span(span) + .expect("in-range span should resolve"); + assert_eq!(line_col.line, 1); + assert_eq!(line_col.col_linestart_byte_offset, 2); + assert_eq!(line_col.col_utf8, 2); +} + +// Verifies that resolving the spans stored on schema types +// yields the line each type definition appears on in the +// original multi-line source text (expected lines are located +// independently via `str::lines()`). +// +// Written by Claude Code, reviewed by a human. +#[test] +fn resolve_span_on_type_spans_matches_definition_lines() { + let schema = build_kitchen_sink_schema(); + + for (type_name, line_marker) in [ + ("User", "type User implements Node"), + ("Node", "interface Node"), + ("Color", "enum Color"), + ("DateTime", "scalar DateTime"), + ] { + let expected_line = KITCHEN_SINK_SCHEMA.lines() + .position(|line| line.starts_with(line_marker)) + .unwrap_or_else(|| { + panic!("marker `{line_marker}` not found in source") + }) as u32; + let graphql_type = schema.get_type(type_name) + .unwrap_or_else(|| panic!("type `{type_name}` not found")); + let line_col = schema.resolve_span(graphql_type.span()) + .expect("schema-originated span should resolve"); + assert_eq!( + line_col.line, expected_line, + "span of `{type_name}` should resolve to its definition line", + ); + } +} + +// Verifies that `Schema::resolve_span()` returns `None` for a +// span whose `SourceMapId` is out of range for this schema +// (e.g. a span that originated from a different artifact with +// more source maps). +// +// Written by Claude Code, reviewed by a human. +#[test] +fn resolve_span_out_of_range_source_map_id_returns_none() { + let schema = SchemaBuilder::build_from_str( + "type Query { hello: String }", + ).expect("schema should build"); + + // Only ids 0 (builtin) and 1 (the loaded source) exist. + assert_eq!(schema.source_maps().len(), 2); + let span = Span::new(ByteSpan::new(0, 1), SourceMapId(2)); + assert!(schema.resolve_span(span).is_none()); + let far_span = Span::new(ByteSpan::new(0, 1), SourceMapId(999)); + assert!(schema.resolve_span(far_span).is_none()); +} + +// Verifies built-in span behavior: spans on built-in +// definitions (which carry the reserved source-map id 0 and no +// user-authored source) resolve against the synthetic built-in +// source map to line 0, column 0 -- they never resolve to user +// source text. +// +// Written by Claude Code, reviewed by a human. +#[test] +fn resolve_span_builtin_resolves_to_line_zero() { + let schema = SchemaBuilder::build_from_str( + "type Query { hello: String }", + ).expect("schema should build"); + + let builtin_line_col = schema.resolve_span(Span::builtin()) + .expect("builtin span should resolve"); + assert_eq!(builtin_line_col.line, 0); + assert_eq!(builtin_line_col.col_linestart_byte_offset, 0); + assert_eq!(builtin_line_col.col_utf8, 0); + + // A real built-in definition's span behaves the same way. + let boolean_scalar = schema.scalar_type("Boolean") + .expect("built-in Boolean scalar should exist"); + let line_col = schema.resolve_span(boolean_scalar.span()) + .expect("built-in definition span should resolve"); + assert_eq!(line_col.line, 0); + assert_eq!(line_col.col_linestart_byte_offset, 0); +} diff --git a/crates/libgraphql-core-v1/src/schema_source_map.rs b/crates/libgraphql-core-v1/src/schema_source_map.rs index 2b25f21..aa07d48 100644 --- a/crates/libgraphql-core-v1/src/schema_source_map.rs +++ b/crates/libgraphql-core-v1/src/schema_source_map.rs @@ -8,7 +8,8 @@ use std::path::PathBuf; /// /// The parser's [`SourceMap<'src>`](libgraphql_parser::SourceMap) /// borrows source text via `'src` and does not implement -/// `serde::Serialize`. A [`Schema`] must be `'static` and +/// `serde::Serialize`. A [`Schema`](crate::schema::Schema) must +/// be `'static` and /// serde-serializable (the `libgraphql-macros` crate embeds /// schemas as binary at compile time). `SchemaSourceMap` stores /// just the line-start byte offsets and optional file path — diff --git a/crates/libgraphql-core-v1/src/span.rs b/crates/libgraphql-core-v1/src/span.rs index 65947fb..33f4597 100644 --- a/crates/libgraphql-core-v1/src/span.rs +++ b/crates/libgraphql-core-v1/src/span.rs @@ -22,8 +22,8 @@ pub const BUILTIN_SOURCE_MAP_ID: SourceMapId = SourceMapId(0); /// corresponding [`SchemaSourceMap`](crate::SchemaSourceMap) stored /// on the [`Schema`](crate::schema::Schema). /// -/// See [`ByteSpan`](libgraphql_parser::ByteSpan) for the -/// underlying byte-offset representation. +/// See [`ByteSpan`] for the underlying byte-offset +/// representation. #[derive(Copy, Clone, Debug, Eq, PartialEq)] #[derive(serde::Deserialize, serde::Serialize)] pub struct Span { diff --git a/crates/libgraphql-core-v1/src/types/field_definition.rs b/crates/libgraphql-core-v1/src/types/field_definition.rs index 305f294..3be04f8 100644 --- a/crates/libgraphql-core-v1/src/types/field_definition.rs +++ b/crates/libgraphql-core-v1/src/types/field_definition.rs @@ -13,8 +13,8 @@ use indexmap::IndexMap; /// /// This is a *schema-level* field definition — the shape and type /// of a field as declared in the schema. For a *selected* field -/// within an operation, see -/// [`FieldSelection`](crate::operation::FieldSelection). +/// within an operation, see `FieldSelection` (part of the +/// operation types, planned in Task 18). /// /// See [Field Definitions](https://spec.graphql.org/September2025/#FieldsDefinition). #[derive(Clone, Debug, PartialEq)] diff --git a/crates/libgraphql-core-v1/src/validators/mod.rs b/crates/libgraphql-core-v1/src/validators/mod.rs index 6810cd2..78c0ea8 100644 --- a/crates/libgraphql-core-v1/src/validators/mod.rs +++ b/crates/libgraphql-core-v1/src/validators/mod.rs @@ -1,9 +1,10 @@ -/// Type-system validators for cross-type validation. -/// -/// These validators are designed to be called from -/// `SchemaBuilder::build()` (Task 16) to enforce the GraphQL -/// specification's type-system rules. They are not yet wired -/// into the build pipeline — `build()` is currently `todo!()`. +//! Type-system validators for cross-type validation. +//! +//! These validators are invoked from +//! [`SchemaBuilder::build()`](crate::schema::SchemaBuilder::build) +//! to enforce the GraphQL specification's type-system rules +//! across the full set of registered types. + mod directive_definition_validator; mod edit_distance; mod input_object_type_validator; diff --git a/crates/libgraphql-core-v1/src/validators/union_type_validator.rs b/crates/libgraphql-core-v1/src/validators/union_type_validator.rs index 9c695bd..8fab83c 100644 --- a/crates/libgraphql-core-v1/src/validators/union_type_validator.rs +++ b/crates/libgraphql-core-v1/src/validators/union_type_validator.rs @@ -14,11 +14,9 @@ use indexmap::IndexMap; /// [Union Members](https://spec.graphql.org/September2025/#sel-HAHdfFDABABlG3ib). /// /// Note: the empty-union check (`EmptyUnionType`) is a build-level -/// error handled by `SchemaBuildErrorKind`; this validator only -/// covers member-exists and member-is-object checks. -// TODO(Task 16): Wire this validator into SchemaBuilder::build(). -// Also add EmptyUnionType check in build() since it's a -// SchemaBuildErrorKind (not a TypeValidationErrorKind). +/// error handled by `SchemaBuildErrorKind` in +/// `SchemaBuilder::build()`; this validator only covers +/// member-exists and member-is-object checks. pub(crate) struct UnionTypeValidator<'a> { errors: Vec, type_: &'a UnionType, diff --git a/libgraphql-core-v1-plan.md b/libgraphql-core-v1-plan.md index bfabfd6..80e428a 100644 --- a/libgraphql-core-v1-plan.md +++ b/libgraphql-core-v1-plan.md @@ -327,9 +327,10 @@ deliberately dropped-and-documented** — nothing disappears silently. Decision: **strings-only conveniences** (all string entry points restored; all file-I/O entry points dropped deliberately — callers do their own I/O). -**Preserved/added (strings-only parity):** `SchemaBuilder::{load_str, build_from_str}` -(exist) and `SchemaBuilder::from_str` (does NOT exist yet — Task 16.6e; signature takes -an optional source label per the source-label item there); `OperationBuilder::from_str` +**Preserved/added (strings-only parity):** `SchemaBuilder::{from_str, load_str, +build_from_str}` (all exist as of Task 16.6e, each with a `_with_label` variant +taking `label: impl AsRef` in place of v0's `Option<&Path>` param — see the +16.6e completion notes); `OperationBuilder::from_str` + `build()` (19c); `Query/Mutation/SubscriptionOperationBuilder::{from_str, build_from_str}` delegates (19c); `ExecutableDocumentBuilder::{from_str, build_from_str}` (19d); @@ -3977,24 +3978,57 @@ fixed source — a future error kind not triggered by that schema escapes it, so error kinds must extend the kitchen-sink alongside their own tests. **16.6e — Hygiene + small missing APIs:** -- [ ] Fix stale rustdoc: `validators/mod.rs` ("build() is currently todo!()" — false), +- [x] Fix stale rustdoc: `validators/mod.rs` ("build() is currently todo!()" — false), `union_type_validator.rs:19` (TODO already done) -- [ ] Remove or use the dead `mutation_type_name()` accessor on `SchemaBuilder` +- [x] Remove or use the dead `mutation_type_name()` accessor on `SchemaBuilder` (`schema_builder.rs` — currently kept alive via `#[allow(dead_code)]`; the `Schema` accessor of the same name in `schema_def.rs` is fine) -- [ ] Implement `SchemaBuilder::from_str` (claimed by the API Disposition ledger but +- [x] Implement `SchemaBuilder::from_str` (claimed by the API Disposition ledger but does not exist — only `load_str`/`build_from_str` do). While here, add an optional source-label parameter to the string entry points (v0's `load_str` took `Option<&Path>`; v1 currently hardcodes `from_source(source, None)`, so multi-source schemas get label-less diagnostics). Operation builders copy this signature in Task 19 — land it first. -- [ ] Implement `Schema::resolve_span(span) -> Option` (AD18 lists it as +- [x] Implement `Schema::resolve_span(span) -> Option` (AD18 lists it as public API but nothing in the crate implements it; delegate to `SchemaSourceMap::resolve_offset`, returning `None` for out-of-range `SourceMapId`s) -- [ ] Add dedicated `schema_def` test file covering the typed query API +- [x] Add dedicated `schema_def` test file covering the typed query API (`types_implementing()`, typed lookups/iterators, root-op accessors) -- [ ] Commit: `[libgraphql-core-v1] Schema hygiene fixes + schema_def tests` +- [x] Commit: `[libgraphql-core-v1] Schema hygiene fixes + schema_def tests` + +**Completion Notes (16.6e):** +Label-API design: kept unlabeled entry points unchanged + added `_with_label` +variants (breaking `load_str` to take `Option<&Path>` rejected — Option-noise at +every unlabeled call site; separate variants keep the common single-source path +clean, and Task 19 operation builders copy this shape). Signatures: +`from_str(source: &str) -> Result>` (inherent, w/ +`#[allow(clippy::should_implement_trait)]` — a `FromStr` impl would force a trait +import for a builder convenience); `from_str_with_label(source, label: impl +AsRef)`; `load_str_with_label(&mut self, source, label)`; +`build_from_str_with_label(source, label)`. All four `load_str` family fns share +private `load_str_impl(source, Option)`; `build_from_str[_with_label]` +delegates via `from_str*().map_err(SchemaErrors::new).and_then(build)`. Label +lands in `SchemaSourceMap.file_path` (asserted via `schema.source_maps()`); +doctests added for `from_str` + `load_str_with_label`. +`Schema::resolve_span(span: Span) -> Option`: indexes +`self.source_maps[span.source_map_id]` (`None` if out of range), delegates to +`SchemaSourceMap::resolve_offset(span.byte_span.start, None)` — no retained +source text so `col_utf8 == col_linestart_byte_offset`; rustdoc states the +schema-originated-spans-only domain per AD18. Dead `SchemaBuilder:: +mutation_type_name()` accessor removed outright (nothing should use it; `build()` +reads the field directly). New `schema/tests/schema_def_tests.rs` (11 tests): +typed lookups incl. kind-mismatch/missing → `None`, typed iterators, +`types_implementing` (object + interface implementors, empty cases), root-op +accessors (explicit `schema {}` bindings + default-`Query`/`None` behavior), +`resolve_span` (hand-computed in-range LineCol, per-type definition lines, +out-of-range id → `None`, builtin spans → 0:0). 4 tests added to +`schema_builder_tests.rs`: `from_str` round-trip + parse-error, label threading +across two labeled sources (file_path + span source_map_id + cross-doc line +resolution), `*_with_label` one-step variants + unlabeled `file_path == None`. +Also zeroed pre-existing `cargo doc` warnings (broken/redundant intra-doc links +in `error_note.rs`, `field_definition.rs`, `schema_source_map.rs`, `span.rs`, +`build()` links to private validators). **16.6f — SDL directive-application validation + assorted schema rules:** The largest coverage hole found by the 16.5 review fan-out: nothing validates @@ -4418,8 +4452,8 @@ The structs above cover all operation types. `FragmentSpread` is the only operat (holds `Cow<'fragreg, FragmentRegistry<'schema>>` + its own source maps; requires `Clone` derives across the fragment/selection types per AD19) - [ ] Implement `resolve_span()` accessors per AD18 (`FragmentRegistry`, - `ExecutableDocument`, `Operation` — plus `Schema::resolve_span`, which does NOT - exist yet; see Task 16.6e) + `ExecutableDocument`, `Operation` — `Schema::resolve_span` already exists, + implemented in Task 16.6e) - [ ] Test per AD18's seeding rule: `Span::dummy()` (id 0) never resolves to user source text on any artifact - [ ] Write basic construction/accessor tests, including a doctest proving the natural From 42a7f6aff47e6a2782f82b7a230c32778d44883c Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 8 Jul 2026 04:31:34 +0000 Subject: [PATCH 2/3] [libgraphql-core-v1] 16.6e review follow-up: document from_str error-type rationale Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01TipQtpvuLuHjHmRsVEgwJw --- crates/libgraphql-core-v1/src/schema/schema_builder.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/crates/libgraphql-core-v1/src/schema/schema_builder.rs b/crates/libgraphql-core-v1/src/schema/schema_builder.rs index 8d996c6..016a39d 100644 --- a/crates/libgraphql-core-v1/src/schema/schema_builder.rs +++ b/crates/libgraphql-core-v1/src/schema/schema_builder.rs @@ -469,6 +469,11 @@ impl SchemaBuilder { /// sources can still be loaded into the returned builder /// before calling [`build()`](Self::build). /// + /// The error type mirrors `load_str`'s + /// (`Vec`, the load-phase shape) rather than + /// [`build()`](Self::build)'s `SchemaErrors` — `from_str` is a + /// load-phase API; only `build*` methods return `SchemaErrors`. + /// /// # Example /// /// ```rust From b9f10fc9244e3458f21b7e7acb90c3b954853d1c Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 9 Jul 2026 03:45:00 +0000 Subject: [PATCH 3/3] [libgraphql-core-v1] Drop internal task ID from FieldDefinition rustdoc Per PR review: public rustdoc should not reference internal roadmap items ("planned in Task 18") -- reworded to point at the operation-layer FieldSelection concept without embedding plan-internal task IDs. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01TipQtpvuLuHjHmRsVEgwJw --- crates/libgraphql-core-v1/src/types/field_definition.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/libgraphql-core-v1/src/types/field_definition.rs b/crates/libgraphql-core-v1/src/types/field_definition.rs index 3be04f8..b01275f 100644 --- a/crates/libgraphql-core-v1/src/types/field_definition.rs +++ b/crates/libgraphql-core-v1/src/types/field_definition.rs @@ -12,9 +12,9 @@ use indexmap::IndexMap; /// [`InterfaceType`](crate::types::InterfaceType). /// /// This is a *schema-level* field definition — the shape and type -/// of a field as declared in the schema. For a *selected* field -/// within an operation, see `FieldSelection` (part of the -/// operation types, planned in Task 18). +/// of a field as declared in the schema. A field *selected* +/// within an operation is a distinct concept, represented by the +/// operation layer's `FieldSelection` type. /// /// See [Field Definitions](https://spec.graphql.org/September2025/#FieldsDefinition). #[derive(Clone, Debug, PartialEq)]