Remove ariadne byte/char mapping (#857)

* Add a test with multi-byte characters

* Remove MappedSource

* Tweak usage and docs for MappedSpan

* chglg

* spelling

crates/apollo-compiler/CHANGELOG.md

@@ -17,6 +17,20 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 ## Maintenance
 ## Documentation-->
 
+# [x.x.x] (unreleased) - 2024-mm-dd
+
+## Maintenance
+
+- **Remove ariadne byte/char mapping - [goto-bus-stop] in [pull/855]**
+  Generating JSON or CLI reports for apollo-compiler diagnostics used a translation layer
+  between byte offsets and character offsets, which cost some computation and memory
+  proportional to the size of the source text. The latest version of [ariadne] allows us to
+  remove this translation.
+
+[goto-bus-stop]: https://github.com/goto-bus-stop
+[pull/855]: https://github.com/apollographql/apollo-rs/pull/855
+[ariadne]: https://github.com/zesterer/ariadne
+
 # [1.0.0-beta.16](https://crates.io/crates/apollo-compiler/1.0.0-beta.16) - 2024-04-12
 
 > This release has no user-facing changes.

crates/apollo-compiler/Cargo.toml

@@ -19,7 +19,7 @@ autotests = false # Most tests/*.rs files are modules of tests/main.rs
 
 [dependencies]
 apollo-parser = { path = "../apollo-parser", version = "0.7.7" }
-ariadne = { version = "0.4.0", features = ["auto-color"] }
+ariadne = { version = "0.4.1", features = ["auto-color"] }
 indexmap = "2.0.0"
 rowan = "0.15.5"
 serde = { version = "1.0", features = ["derive"] }

crates/apollo-compiler/src/diagnostic.rs

@@ -110,7 +110,7 @@ where
 pub struct CliReport<'s> {
     sources: &'s SourceMap,
     colors: ColorGenerator,
-    report: ariadne::ReportBuilder<'static, MappedSpan>,
+    report: ariadne::ReportBuilder<'static, AriadneSpan>,
 }
 
 /// Indicate when to use ANSI colors for printing.
@@ -164,14 +164,14 @@ impl<T: ToCliReport> ToCliReport for &T {
     }
 }
 
-type MappedSpan = (FileId, Range<usize>);
+/// An ariadne span type. We avoid implementing `ariadne::Span` for `NodeLocation`
+/// so ariadne doesn't leak into the public API of apollo-compiler.
+type AriadneSpan = (FileId, Range<usize>);
 
-/// Translate a byte-offset location into a char-offset location for use with ariadne.
-fn map_span(sources: &SourceMap, location: NodeLocation) -> Option<MappedSpan> {
-    let source = sources.get(&location.file_id)?;
-    let mapped_source = source.mapped_source();
-    let start = mapped_source.map_index(location.offset());
-    let end = mapped_source.map_index(location.end_offset());
+/// Translate a NodeLocation into an ariadne span type.
+fn to_span(location: NodeLocation) -> Option<AriadneSpan> {
+    let start = location.offset();
+    let end = location.end_offset();
     Some((location.file_id, start..end))
 }
 
@@ -203,7 +203,7 @@ impl<'s> CliReport<'s> {
         color: Color,
     ) -> Self {
         let (file_id, range) = main_location
-            .and_then(|location| map_span(sources, location))
+            .and_then(to_span)
             .unwrap_or((FileId::NONE, 0..0));
         let report = ariadne::Report::build(ReportKind::Error, file_id, range.start);
         let enable_color = match color {
@@ -212,7 +212,9 @@ impl<'s> CliReport<'s> {
             // only if stderr is a terminal.
             Color::StderrIsTerminal => true,
         };
-        let config = ariadne::Config::default().with_color(enable_color);
+        let config = ariadne::Config::default()
+            .with_index_type(ariadne::IndexType::Byte)
+            .with_color(enable_color);
         Self {
             sources,
             colors: ColorGenerator::new(),
@@ -238,9 +240,9 @@ impl<'s> CliReport<'s> {
 
     /// Add a label at a given location. If the location is `None`, the message is discarded.
     pub fn with_label_opt(&mut self, location: Option<NodeLocation>, message: impl ToString) {
-        if let Some(mapped_span) = location.and_then(|location| map_span(self.sources, location)) {
+        if let Some(span) = location.and_then(to_span) {
             self.report.add_label(
-                ariadne::Label::new(mapped_span)
+                ariadne::Label::new(span)
                     .with_message(message)
                     .with_color(self.colors.next()),
             );

crates/apollo-compiler/src/parser.rs

@@ -31,18 +31,11 @@ pub struct Parser {
 pub struct SourceFile {
     pub(crate) path: PathBuf,
     pub(crate) source_text: String,
-    pub(crate) source: OnceLock<MappedSource>,
+    pub(crate) source: OnceLock<ariadne::Source>,
 }
 
 pub type SourceMap = Arc<IndexMap<FileId, Arc<SourceFile>>>;
 
-/// Translate byte offsets to ariadne's char offsets.
-#[derive(Debug, Clone, PartialEq, Eq)]
-pub(crate) struct MappedSource {
-    ariadne: ariadne::Source,
-    map: Vec<u32>,
-}
-
 /// Parse a schema and executable document from the given source text
 /// containing a mixture of type system definitions and executable definitions.
 /// and validate them.
@@ -350,30 +343,6 @@ impl Parser {
     }
 }
 
-impl MappedSource {
-    fn new(input: &str) -> Self {
-        // FIXME This string copy is not ideal, but changing to a reference counted string affects
-        // public API
-        let ariadne = ariadne::Source::from(input.to_string());
-
-        let mut map = vec![0; input.len() + 1];
-        let mut char_index = 0;
-        for (byte_index, _) in input.char_indices() {
-            map[byte_index] = char_index;
-            char_index += 1;
-        }
-
-        // Support 1 past the end of the string, for use in exclusive ranges.
-        map[input.len()] = char_index;
-
-        Self { ariadne, map }
-    }
-
-    pub(crate) fn map_index(&self, byte_index: usize) -> usize {
-        self.map[byte_index] as usize
-    }
-}
-
 impl SourceFile {
     /// The filesystem path (or arbitrary string) used in diagnostics
     /// to identify this source file to users.
@@ -386,17 +355,15 @@ impl SourceFile {
     }
 
     pub(crate) fn ariadne(&self) -> &ariadne::Source {
-        &self.mapped_source().ariadne
-    }
-
-    pub(crate) fn mapped_source(&self) -> &MappedSource {
-        self.source
-            .get_or_init(|| MappedSource::new(&self.source_text))
+        self.source.get_or_init(|| {
+            // FIXME This string copy is not ideal, but changing to a reference counted string affects
+            // public API
+            ariadne::Source::from(self.source_text.clone())
+        })
     }
 
     pub fn get_line_column(&self, index: usize) -> Option<(usize, usize)> {
-        let char_index = self.mapped_source().map_index(index);
-        let (_, line, column) = self.ariadne().get_offset_line(char_index)?;
+        let (_, line, column) = self.ariadne().get_byte_line(index)?;
         Some((line, column))
     }
 }

crates/apollo-compiler/tests/validation/mod.rs

@@ -275,3 +275,54 @@ fn validate_variable_usage_without_type_system() {
     let doc = ast::Document::parse(input, "query.graphql").unwrap();
     doc.validate_standalone_executable().unwrap()
 }
+
+#[test]
+fn json_location_with_multibyte() {
+    let input_type_system = r#"
+type Query {
+    obj: TestObject
+}
+
+type TestObject {
+    name: String
+}
+"#;
+
+    let input_executable = r#"
+{
+    # กรุงเทพมหานคร อมรรัตนโกสินทร์ มหินทรายุธยา มหาดิลกภพ นพรัตนราชธานีบูรีรมย์ อุดมราชนิเวศน์มหาสถาน อมรพิมานอวตารสถิต สักกะทัตติยวิษณุกรรมประสิทธิ์
+    obj { ...q }
+    # City of angels, great city of immortals, magnificent city of the nine gems, seat of the king, city of royal palaces, home of gods incarnate, erected by Vishvakarman at Indra's behest.
+}
+"#;
+
+    let schema = Schema::parse_and_validate(input_type_system, "schema.graphql").unwrap();
+    let err = ExecutableDocument::parse_and_validate(&schema, input_executable, "query.graphql")
+        .expect_err("should have a validation error");
+
+    let actual = err.to_string();
+    let expected = expect_test::expect![[r#"
+        Error: cannot find fragment `q` in this document
+           ╭─[query.graphql:4:11]
+           │
+         4 │     obj { ...q }
+           │           ──┬─  
+           │             ╰─── fragment `q` is not defined
+        ───╯
+    "#]];
+    expected.assert_eq(&actual);
+
+    let first_error = err.errors.iter().next().unwrap();
+    let actual = serde_json::to_string_pretty(&first_error.to_json()).unwrap();
+    let expected = expect_test::expect![[r#"
+        {
+          "message": "cannot find fragment `q` in this document",
+          "locations": [
+            {
+              "line": 4,
+              "column": 11
+            }
+          ]
+        }"#]];
+    expected.assert_eq(&actual);
+}