diff --git a/crates/apollo-compiler/src/validation/fragment.rs b/crates/apollo-compiler/src/validation/fragment.rs index c92e3d9d4..d1b4b032c 100644 --- a/crates/apollo-compiler/src/validation/fragment.rs +++ b/crates/apollo-compiler/src/validation/fragment.rs @@ -298,17 +298,19 @@ pub(crate) fn validate_fragment_cycles( // synthetic trees. let named_fragments = db.ast_named_fragments(file_id); + /// If a fragment spread is recursive, returns a vec containing the spread that refers back to + /// the original fragment, and a trace of each fragment spread back to the original fragment. fn detect_fragment_cycles( named_fragments: &HashMap>, selection_set: &[ast::Selection], visited: &mut RecursionGuard<'_>, - ) -> Result<(), ast::Selection> { + ) -> Result<(), Vec>> { for selection in selection_set { match selection { ast::Selection::FragmentSpread(spread) => { if visited.contains(&spread.fragment_name) { if visited.first() == Some(&spread.fragment_name) { - return Err(selection.clone()); + return Err(vec![spread.clone()]); } continue; } @@ -318,13 +320,19 @@ pub(crate) fn validate_fragment_cycles( named_fragments, &fragment.selection_set, &mut visited.push(&fragment.name), - )?; + ) + .map_err(|mut trace| { + trace.push(spread.clone()); + trace + })?; } } ast::Selection::InlineFragment(inline) => { detect_fragment_cycles(named_fragments, &inline.selection_set, visited)?; } - _ => {} + ast::Selection::Field(field) => { + detect_fragment_cycles(named_fragments, &field.selection_set, visited)?; + } } } @@ -338,17 +346,38 @@ pub(crate) fn validate_fragment_cycles( { let head_location = NodeLocation::recompose(def.location(), def.name.location()); - diagnostics.push( - ApolloDiagnostic::new( - db, - def.location(), - DiagnosticData::RecursiveFragmentDefinition { - name: def.name.to_string(), - }, - ) - .label(Label::new(head_location, "recursive fragment definition")) - .label(Label::new(cycle.location(), "refers to itself here")), - ); + let mut diagnostic = ApolloDiagnostic::new( + db, + def.location(), + DiagnosticData::RecursiveFragmentDefinition { + name: def.name.to_string(), + }, + ) + .label(Label::new(head_location, "recursive fragment definition")); + + if let Some((cyclical_spread, path)) = cycle.split_first() { + let mut prev_name = &def.name; + for spread in path.iter().rev() { + diagnostic = diagnostic.label(Label::new( + spread.location(), + format!( + "`{}` references `{}` here...", + prev_name, spread.fragment_name + ), + )); + prev_name = &spread.fragment_name; + } + + diagnostic = diagnostic.label(Label::new( + cyclical_spread.location(), + format!( + "`{}` circularly references `{}` here", + prev_name, cyclical_spread.fragment_name + ), + )); + } + + diagnostics.push(diagnostic); } diagnostics diff --git a/crates/apollo-compiler/test_data/diagnostics/0092_recursive_fragment_spread.graphql b/crates/apollo-compiler/test_data/diagnostics/0092_recursive_fragment_spread.graphql index 57bb3a9f2..08733dc32 100644 --- a/crates/apollo-compiler/test_data/diagnostics/0092_recursive_fragment_spread.graphql +++ b/crates/apollo-compiler/test_data/diagnostics/0092_recursive_fragment_spread.graphql @@ -9,7 +9,16 @@ type Query { # Query { human { ...fragA } + __schema { types { ...cycle1 } } } fragment fragA on Human { name, ...fragB } fragment fragB on Human { name, ...fragC } fragment fragC on Human { name, ...fragA } + +# Indirect cycle +fragment cycle1 on __Type { kind, ...cycle2 } + +fragment cycle2 on __Type { + kind + ofType { ...cycle1 } +} diff --git a/crates/apollo-compiler/test_data/diagnostics/0092_recursive_fragment_spread.txt b/crates/apollo-compiler/test_data/diagnostics/0092_recursive_fragment_spread.txt index 680c43af8..ab0b539a0 100644 --- a/crates/apollo-compiler/test_data/diagnostics/0092_recursive_fragment_spread.txt +++ b/crates/apollo-compiler/test_data/diagnostics/0092_recursive_fragment_spread.txt @@ -1,12 +1,29 @@ Error: `fragA` fragment cannot reference itself - ╭─[0092_recursive_fragment_spread.graphql:13:1] + ╭─[0092_recursive_fragment_spread.graphql:14:1] │ - 13 │ fragment fragA on Human { name, ...fragB } - │ ───────┬────── - │ ╰──────── recursive fragment definition - │ - 15 │ fragment fragC on Human { name, ...fragA } + 14 │ fragment fragA on Human { name, ...fragB } + │ ───────┬────── ────┬─── + │ ╰────────────────────────────────── recursive fragment definition + │ │ + │ ╰───── `fragA` references `fragB` here... + 15 │ fragment fragB on Human { name, ...fragC } + │ ────┬─── + │ ╰───── `fragB` references `fragC` here... + 16 │ fragment fragC on Human { name, ...fragA } │ ────┬─── - │ ╰───── refers to itself here + │ ╰───── `fragC` circularly references `fragA` here +────╯ +Error: `cycle1` fragment cannot reference itself + ╭─[0092_recursive_fragment_spread.graphql:19:1] + │ + 19 │ fragment cycle1 on __Type { kind, ...cycle2 } + │ ───────┬─────── ────┬──── + │ ╰───────────────────────────────────── recursive fragment definition + │ │ + │ ╰────── `cycle1` references `cycle2` here... + │ + 23 │ ofType { ...cycle1 } + │ ────┬──── + │ ╰────── `cycle2` circularly references `cycle1` here ────╯ diff --git a/crates/apollo-compiler/test_data/serializer/diagnostics/0092_recursive_fragment_spread.graphql b/crates/apollo-compiler/test_data/serializer/diagnostics/0092_recursive_fragment_spread.graphql index 4a50fd36d..4280aa7ba 100644 --- a/crates/apollo-compiler/test_data/serializer/diagnostics/0092_recursive_fragment_spread.graphql +++ b/crates/apollo-compiler/test_data/serializer/diagnostics/0092_recursive_fragment_spread.graphql @@ -10,6 +10,11 @@ query { human { ...fragA } + __schema { + types { + ...cycle1 + } + } } fragment fragA on Human { @@ -26,3 +31,15 @@ fragment fragC on Human { name ...fragA } + +fragment cycle1 on __Type { + kind + ...cycle2 +} + +fragment cycle2 on __Type { + kind + ofType { + ...cycle1 + } +}