Merge pull request #577 from Shopify/02-09-return_array_of_io_errors_from_indexing

Return array of IO errors from indexing

Author: vinistock

ext/rubydex/graph.c

@@ -9,7 +9,6 @@
 #include "utils.h"
 
 static VALUE cGraph;
-static VALUE eIndexingError;
 
 // Free function for the custom Graph allocator. We always have to call into Rust to free data allocated by it
 static void graph_free(void *ptr) {
@@ -27,35 +26,39 @@ static VALUE rdxr_graph_alloc(VALUE klass) {
     return TypedData_Wrap_Struct(klass, &graph_type, graph);
 }
 
-// Graph#index_all: (Array[String] file_paths) -> nil
-// Raises IndexingError if anything failed during indexing
+// Graph#index_all: (Array[String] file_paths) -> Array[String]
+// Returns an array of IO error messages encountered during indexing
 static VALUE rdxr_graph_index_all(VALUE self, VALUE file_paths) {
     rdxi_check_array_of_strings(file_paths);
 
     // Convert the given file paths into a char** array, so that we can pass to Rust
     size_t length = RARRAY_LEN(file_paths);
     char **converted_file_paths = rdxi_str_array_to_char(file_paths, length);
 
-    // Get the underying graph pointer and then invoke the Rust index all implementation
+    // Get the underlying graph pointer and then invoke the Rust index all implementation
     void *graph;
     TypedData_Get_Struct(self, void *, &graph_type, graph);
-    const char *error_messages = rdx_index_all(graph, (const char **)converted_file_paths, length);
+
+    size_t error_count = 0;
+    const char *const *errors = rdx_index_all(graph, (const char **)converted_file_paths, length, &error_count);
 
     // Free the converted file paths and allow the GC to collect them
     for (size_t i = 0; i < length; i++) {
         free(converted_file_paths[i]);
     }
     free(converted_file_paths);
 
-    // If indexing errors were returned, turn them into a Ruby string, call Rust to free the CString it allocated and
-    // return the Ruby string
-    if (error_messages != NULL) {
-        VALUE error_string = rb_utf8_str_new_cstr(error_messages);
-        free_c_string(error_messages);
-        rb_raise(eIndexingError, "%s", StringValueCStr(error_string));
+    if (errors == NULL) {
+        return rb_ary_new();
     }
 
-    return Qnil;
+    VALUE array = rb_ary_new_capa((long)error_count);
+    for (size_t i = 0; i < error_count; i++) {
+        rb_ary_push(array, rb_utf8_str_new_cstr(errors[i]));
+    }
+
+    free_c_string_array(errors, error_count);
+    return array;
 }
 
 // Size function for the declarations enumerator
@@ -443,9 +446,6 @@ static VALUE rdxr_graph_diagnostics(VALUE self) {
 }
 
 void rdxi_initialize_graph(VALUE mRubydex) {
-    VALUE eRubydexError = rb_const_get(mRubydex, rb_intern("Error"));
-    eIndexingError = rb_define_class_under(mRubydex, "IndexingError", eRubydexError);
-
     cGraph = rb_define_class_under(mRubydex, "Graph", rb_cObject);
     rb_define_alloc_func(cGraph, rdxr_graph_alloc);
     rb_define_method(cGraph, "index_all", rdxr_graph_index_all, 1);

lib/rubydex/graph.rb

@@ -19,7 +19,7 @@ def initialize(workspace_path: Dir.pwd)
     end
 
     # Index all files and dependencies of the workspace that exists in `@workspace_path`
-    #: -> String?
+    #: -> Array[String]
     def index_workspace
       index_all(workspace_paths)
     end

rakelib/index_top_gems.rake

@@ -51,10 +51,10 @@ task index_top_gems: :compile_release do
 
             # Index the gem's files and yield errors back to the main Ractor
             graph = Rubydex::Graph.new
-            error = graph.index_all(Dir.glob("#{gem_dir}/**/*.rb"))
-            next unless error
+            errors = graph.index_all(Dir.glob("#{gem_dir}/**/*.rb"))
+            next if errors.empty?
 
-            @mutex.synchronize { @errors << "#{gem} => #{error}" }
+            @mutex.synchronize { @errors << "#{gem} => #{errors.join(", ")}" }
           end
         end
       end

rust/rubydex-sys/src/graph_api.rs

@@ -115,7 +115,9 @@ pub unsafe extern "C" fn rdx_graph_resolve_constant(
     })
 }
 
-/// Indexes all given file paths in parallel using the provided Graph pointer
+/// Indexes all given file paths in parallel using the provided Graph pointer.
+/// Returns an array of error message strings and writes the count to `out_error_count`.
+/// Returns NULL if there are no errors. Caller must free with `free_c_string_array`.
 ///
 /// # Panics
 ///
@@ -130,34 +132,38 @@ pub unsafe extern "C" fn rdx_index_all(
     pointer: GraphPointer,
     file_paths: *const *const c_char,
     count: usize,
-) -> *const c_char {
+    out_error_count: *mut usize,
+) -> *const *const c_char {
     let file_paths: Vec<String> = unsafe { utils::convert_double_pointer_to_vec(file_paths, count).unwrap() };
-    let (file_paths, errors) = listing::collect_file_paths(file_paths);
-
-    if !errors.is_empty() {
-        let error_messages = errors
-            .iter()
-            .map(std::string::ToString::to_string)
-            .collect::<Vec<_>>()
-            .join("\n");
-
-        return CString::new(error_messages).unwrap().into_raw().cast_const();
-    }
+    let (file_paths, listing_errors) = listing::collect_file_paths(file_paths);
 
     with_mut_graph(pointer, |graph| {
-        let errors = indexing::index_files(graph, file_paths);
+        let indexing_errors = indexing::index_files(graph, file_paths);
 
-        if !errors.is_empty() {
-            let error_messages = errors
-                .iter()
-                .map(std::string::ToString::to_string)
-                .collect::<Vec<_>>()
-                .join("\n");
+        let all_errors: Vec<String> = listing_errors
+            .into_iter()
+            .chain(indexing_errors)
+            .map(|e| e.to_string())
+            .collect();
 
-            return CString::new(error_messages).unwrap().into_raw().cast_const();
+        if all_errors.is_empty() {
+            unsafe { *out_error_count = 0 };
+            return ptr::null();
         }
 
-        ptr::null()
+        let c_strings: Vec<*const c_char> = all_errors
+            .into_iter()
+            .filter_map(|string| {
+                CString::new(string)
+                    .ok()
+                    .map(|c_string| c_string.into_raw().cast_const())
+            })
+            .collect();
+
+        unsafe { *out_error_count = c_strings.len() };
+
+        let boxed = c_strings.into_boxed_slice();
+        Box::into_raw(boxed).cast::<*const c_char>()
     })
 }
 

test/graph_test.rb

@@ -9,7 +9,7 @@ class GraphTest < Minitest::Test
   def test_indexing_empty_context
     with_context do |context|
       graph = Rubydex::Graph.new
-      assert_nil(graph.index_all(context.glob("**/*.rb")))
+      assert_empty(graph.index_all(context.glob("**/*.rb")))
     end
   end
 
@@ -19,19 +19,17 @@ def test_indexing_context_files
       context.write!("bar.rb", "class Bar; end")
 
       graph = Rubydex::Graph.new
-      assert_nil(graph.index_all(context.glob("**/*.rb")))
+      assert_empty(graph.index_all(context.glob("**/*.rb")))
     end
   end
 
   def test_indexing_invalid_file_paths
     graph = Rubydex::Graph.new
 
-    error = assert_raises(Rubydex::IndexingError) do
-      graph.index_all(["not_found.rb"])
-    end
+    errors = graph.index_all(["not_found.rb"])
 
-    assert_kind_of(Rubydex::Error, error)
-    assert_match(/FileError: Path `.*not_found.rb` does not exist/, error.message)
+    assert_equal(1, errors.length)
+    assert_match(/FileError: Path `.*not_found.rb` does not exist/, errors.first)
   end
 
   def test_indexing_with_parse_errors