Repository: apollographql/apollo-rs
Pull request #861 (2024-05-24 21:22:52)

Convert SourceLocation to Range<GraphQLLocation>, not just starting line/column
Needed for diagnostics in https://github.com/apollographql/router/pull/5247 .

Seems like maybe we want to have a single struct somewhere instead which returns start and end to save a bit of extra work in calling both methods? But not sure how to align that with the need of `GraphQLLocation` to be the spec's execution error location. 

goto-bus-stop: Maybe instead of `_start` and `_end`, we could have a method that returns a `(GraphQLLocation, GraphQLLocation)` tuple? This would address the double-lookup and I think it would be extremely rare to *only* need the end but not the start.

The methods could be called `get_line_column` and `get_line_column_range` for getting the start only or the start+end range respectively.
SimonSapin: +1 for `get_line_column` and `get_line_column_range`. The latter could even return `std::ops::Range<GraphQLLocation>` (or `Option` of)
trevor-scheer: Having the range available would be a nice convenience for the language server. The type that's most interesting for my use case is the [`lsp_types::Range`](https://docs.rs/lsp-types/latest/lsp_types/struct.Range.html) which says 0 indexed with an exclusive end, so that sounds compatible with the suggested `Range<GraphQLLocation>` approach. I am indifferent to tuple or struct, I think in this case the ordering of the tuple is very clear. A struct would match the `lsp_types::Range` type more closely, but in either case I will be mapping the type.
dylan-apollo: I've reworked this PR to a completely new proposal that uses _new_ structs for representing both line/col, decoupled from the GraphQL error representation.

However, given that we're going to need an exclusive end & zero-indexing for LSP (and therefore composition), this may need further tweaking to get everything just right, maybe with `Range<LineColumn>` instead of the new dedicated struct. Interested to hear any ideas on how to best handle exclusive ends.
dylan-apollo: Also, a quick prototype of these changes in `router`: https://github.com/apollographql/router/pull/5263

Patch:
diff --git a/crates/apollo-compiler/src/diagnostic.rs b/crates/apollo-compiler/src/diagnostic.rs
index 66a2a66..417794a 100644
--- a/crates/apollo-compiler/src/diagnostic.rs
+++ b/crates/apollo-compiler/src/diagnostic.rs
@@ -325,9 +325,9 @@ impl ariadne::Cache<FileId> for Cache<'_> {
 impl<T: ToCliReport> std::error::Error for Diagnostic<'_, T> {}
 
 impl<T: ToCliReport> Diagnostic<'_, T> {
-    /// Get the line and column number where this diagnostic was raised.
-    pub fn get_line_column(&self) -> Option<GraphQLLocation> {
-        GraphQLLocation::from_node(self.sources, self.error.location())
+    /// Get the line and column numbers where this diagnostic spans.
+    pub fn line_column_range(&self) -> Option<Range<GraphQLLocation>> {
+        self.error.location()?.line_column_range(self.sources)
     }
 
     /// Get a [`serde`]-serializable version of the current diagnostic. The shape is compatible
diff --git a/crates/apollo-compiler/src/execution/response.rs b/crates/apollo-compiler/src/execution/response.rs
index 6d06525..5f861bc 100644
--- a/crates/apollo-compiler/src/execution/response.rs
+++ b/crates/apollo-compiler/src/execution/response.rs
@@ -74,7 +74,7 @@ pub struct GraphQLError {
     pub extensions: JsonMap,
 }
 
-/// A source location (line and column numbers) for a [`GraphQLError`].
+/// A source location (1-indexed line and column numbers) within a GraphQL document.
 #[derive(Debug, Clone, Copy, Hash, PartialEq, Eq, Serialize, Deserialize)]
 #[serde(deny_unknown_fields)]
 pub struct GraphQLLocation {
@@ -141,8 +141,9 @@ impl GraphQLError {
     ) -> Self {
         Self {
             message: message.into(),
-            locations: GraphQLLocation::from_node(sources, location)
+            locations: location
                 .into_iter()
+                .filter_map(|location| location.line_column(sources))
                 .collect(),
             path: Default::default(),
             extensions: Default::default(),
@@ -150,20 +151,6 @@ impl GraphQLError {
     }
 }
 
-impl GraphQLLocation {
-    /// Convert a `NodeLocation` to a line and column number
-    pub fn from_node(sources: &SourceMap, location: Option<NodeLocation>) -> Option<Self> {
-        let loc = location?;
-        let source = sources.get(&loc.file_id)?;
-        source
-            .get_line_column(loc.offset())
-            .map(|(line, column)| GraphQLLocation {
-                line: line + 1,
-                column: column + 1,
-            })
-    }
-}
-
 impl ResponseData {
     /// For serde `skip_serializing_if`
     fn is_absent(&self) -> bool {
diff --git a/crates/apollo-compiler/src/name.rs b/crates/apollo-compiler/src/name.rs
index 651150f..a277eca 100644
--- a/crates/apollo-compiler/src/name.rs
+++ b/crates/apollo-compiler/src/name.rs
@@ -12,6 +12,7 @@ use std::fmt;
 use std::marker::PhantomData;
 use std::mem::size_of;
 use std::mem::ManuallyDrop;
+use std::ops::Range;
 use std::ptr::NonNull;
 use std::sync::Arc;
 
@@ -185,8 +186,8 @@ impl Name {
     }
 
     /// If this string contains a location, convert it to line and column numbers
-    pub fn line_column(&self, sources: &SourceMap) -> Option<GraphQLLocation> {
-        GraphQLLocation::from_node(sources, self.location())
+    pub fn line_column_range(&self, sources: &SourceMap) -> Option<Range<GraphQLLocation>> {
+        self.location()?.line_column_range(sources)
     }
 
     #[allow(clippy::len_without_is_empty)] // GraphQL Name is never empty
diff --git a/crates/apollo-compiler/src/node.rs b/crates/apollo-compiler/src/node.rs
index e54a00d..8208478 100644
--- a/crates/apollo-compiler/src/node.rs
+++ b/crates/apollo-compiler/src/node.rs
@@ -8,6 +8,7 @@ use std::fmt;
 use std::hash::Hash;
 use std::hash::Hasher;
 use std::num::NonZeroU64;
+use std::ops::Range;
 use std::sync::atomic;
 use std::sync::atomic::AtomicU64;
 use triomphe::HeaderSlice;
@@ -107,9 +108,9 @@ impl<T: ?Sized> Node<T> {
         self.location().map(|l| l.file_id()) == Some(FileId::BUILT_IN)
     }
 
-    /// If this node contains a location, convert it to line and column numbers
-    pub fn line_column(&self, sources: &SourceMap) -> Option<GraphQLLocation> {
-        GraphQLLocation::from_node(sources, self.location())
+    /// If this node contains a location, convert it to the line and column numbers.
+    pub fn line_column_range(&self, sources: &SourceMap) -> Option<Range<GraphQLLocation>> {
+        self.location()?.line_column_range(sources)
     }
 
     /// Returns the given `node` at the same location as `self` (e.g. for a type conversion).
@@ -277,6 +278,8 @@ impl NodeLocation {
     }
 
     /// Returns the offset from the start of the file to the end of the range, in UTF-8 bytes
+    ///
+    /// The range is exclusive, so this offset is one past the end of the range.
     pub fn end_offset(&self) -> usize {
         self.text_range.end().into()
     }
@@ -303,6 +306,21 @@ impl NodeLocation {
             }
         }
     }
+
+    /// The line and column numbers of [`Self::offset`]
+    pub fn line_column(&self, sources: &SourceMap) -> Option<GraphQLLocation> {
+        let source = sources.get(&self.file_id)?;
+        source.get_line_column(self.offset())
+    }
+
+    /// The line and column numbers of the range from [`Self::offset`] to [`Self::end_offset`]
+    /// inclusive.
+    pub fn line_column_range(&self, sources: &SourceMap) -> Option<Range<GraphQLLocation>> {
+        let source = sources.get(&self.file_id)?;
+        let start = source.get_line_column(self.offset())?;
+        let end = source.get_line_column(self.end_offset())?;
+        Some(Range { start, end })
+    }
 }
 
 impl fmt::Debug for NodeLocation {
diff --git a/crates/apollo-compiler/src/parser.rs b/crates/apollo-compiler/src/parser.rs
index 14bf108..fe4314f 100644
--- a/crates/apollo-compiler/src/parser.rs
+++ b/crates/apollo-compiler/src/parser.rs
@@ -2,6 +2,7 @@ use crate::ast;
 use crate::ast::from_cst::Convert;
 use crate::ast::Document;
 use crate::executable;
+use crate::execution::GraphQLLocation;
 use crate::schema::SchemaBuilder;
 use crate::validation::Details;
 use crate::validation::DiagnosticList;
@@ -362,9 +363,12 @@ impl SourceFile {
         })
     }
 
-    pub fn get_line_column(&self, index: usize) -> Option<(usize, usize)> {
-        let (_, line, column) = self.ariadne().get_byte_line(index)?;
-        Some((line, column))
+    pub(crate) fn get_line_column(&self, index: usize) -> Option<GraphQLLocation> {
+        let (_, zero_indexed_line, zero_indexed_column) = self.ariadne().get_byte_line(index)?;
+        Some(GraphQLLocation {
+            line: zero_indexed_line + 1,
+            column: zero_indexed_column + 1,
+        })
     }
 }
 


Test patch:
diff --git a/crates/apollo-compiler/tests/validation/mod.rs b/crates/apollo-compiler/tests/validation/mod.rs
index c34bc27..b6d7ffe 100644
--- a/crates/apollo-compiler/tests/validation/mod.rs
+++ b/crates/apollo-compiler/tests/validation/mod.rs
@@ -10,6 +10,7 @@ use apollo_compiler::ast;
 use apollo_compiler::execution::GraphQLLocation;
 use apollo_compiler::ExecutableDocument;
 use apollo_compiler::Schema;
+use std::ops::Range;
 
 #[test]
 fn executable_and_type_system_definitions() {
@@ -70,8 +71,11 @@ query {
 
     diagnostics.iter().for_each(|diag| {
         assert_eq!(
-            diag.get_line_column(),
-            Some(GraphQLLocation { line: 2, column: 1 })
+            diag.line_column_range(),
+            Some(Range {
+                start: GraphQLLocation { line: 2, column: 1 },
+                end: GraphQLLocation { line: 4, column: 2 }
+            })
         );
         json.assert_eq(&serde_json::to_string_pretty(&diag.to_json()).unwrap());
     });
@@ -151,8 +155,14 @@ fragment q on TestObject {
     );
     diagnostics.iter().for_each(|diag| {
         assert_eq!(
-            diag.get_line_column(),
-            Some(GraphQLLocation { line: 8, column: 1 })
+            diag.line_column_range(),
+            Some(Range {
+                start: GraphQLLocation { line: 8, column: 1 },
+                end: GraphQLLocation {
+                    line: 10,
+                    column: 2
+                }
+            })
         );
         json.assert_eq(&serde_json::to_string_pretty(&diag.to_json()).unwrap());
     });
@@ -190,10 +200,16 @@ fn validation_without_type_system() {
     );
     diagnostics.iter().for_each(|diag| {
         assert_eq!(
-            diag.get_line_column(),
-            Some(GraphQLLocation {
-                line: 2,
-                column: 13
+            diag.line_column_range(),
+            Some(Range {
+                start: GraphQLLocation {
+                    line: 2,
+                    column: 13,
+                },
+                end: GraphQLLocation {
+                    line: 2,
+                    column: 37,
+                }
             })
         );
         json.assert_eq(&serde_json::to_string_pretty(&diag.to_json()).unwrap());
@@ -226,10 +242,16 @@ fn validation_without_type_system() {
     );
     diagnostics.iter().for_each(|diag| {
         assert_eq!(
-            diag.get_line_column(),
-            Some(GraphQLLocation {
-                line: 3,
-                column: 22
+            diag.line_column_range(),
+            Some(Range {
+                start: GraphQLLocation {
+                    line: 3,
+                    column: 22,
+                },
+                end: GraphQLLocation {
+                    line: 3,
+                    column: 23,
+                }
             })
         );
         json.assert_eq(&serde_json::to_string_pretty(&diag.to_json()).unwrap());
@@ -254,8 +276,11 @@ fn validation_without_type_system() {
     );
     diagnostics.iter().for_each(|diag| {
         assert_eq!(
-            diag.get_line_column(),
-            Some(GraphQLLocation { line: 1, column: 3 })
+            diag.line_column_range(),
+            Some(Range {
+                start: GraphQLLocation { line: 1, column: 3 },
+                end: GraphQLLocation { line: 1, column: 7 },
+            })
         );
         json.assert_eq(&serde_json::to_string_pretty(&diag.to_json()).unwrap());
     });