Repository: kythe/kythe
Pull request #4974 (2021-06-17 19:27:08)

feat(rust_indexer): Get source file contents from the FileProvider
Previously, the indexer binary would extract an entire kzip to the file system, and the library would read those source files from the file system. This PR makes the indexer library request source files from a FileProvider instead.

wcalandro: @jaysachs The linter was complaining about how some of the rust code was formatted. I can try to isolate that the files it was explicitly complaining about and only include those in the PR

Patch:
diff --git a/kythe/rust/indexer/src/bin/main.rs b/kythe/rust/indexer/src/bin/main.rs
index 13b223afd..ba672dde1 100644
--- a/kythe/rust/indexer/src/bin/main.rs
+++ b/kythe/rust/indexer/src/bin/main.rs
@@ -11,9 +11,6 @@
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
-#[macro_use]
-extern crate anyhow;
-
 extern crate kythe_rust_indexer;
 use kythe_rust_indexer::{indexer::KytheIndexer, providers::*, writer::CodedOutputStreamWriter};
 
@@ -60,66 +57,48 @@ fn main() -> Result<()> {
         let temp_path = PathBuf::new().join(temp_dir.path());
 
         // Extract all of the files from the kzip into the temporary directory
-        extract_from_kzip(&unit, &temp_path, &mut kzip_provider)?;
+        extract_analysis_from_kzip(&unit, &temp_path, &mut kzip_provider)?;
 
         // Index the CompilationUnit
-        indexer.index_cu(&unit, &temp_path)?;
+        indexer.index_cu(&unit, &temp_path, &mut kzip_provider)?;
     }
     Ok(())
 }
 
-/// Takes files from a kzip loaded into `provider` and extracts them to
-/// `temp_path` using the file names and digests in the CompilationUnit
-pub fn extract_from_kzip(
+/// Takes analysis files from a kzip loaded into `provider` and extracts them
+/// to `temp_path` using the file names and digests in the CompilationUnit
+pub fn extract_analysis_from_kzip(
     c_unit: &CompilationUnit,
     temp_path: &Path,
     provider: &mut dyn FileProvider,
 ) -> Result<()> {
     for required_input in c_unit.get_required_input() {
-        // Get path where we will copy the file to
         let input_path = required_input.get_info().get_path();
-
-        // The only JSON files in the kzip are the save_analyses, so we put them in the
-        // analysis folder.
         let input_path_buf = PathBuf::new().join(input_path);
-        let output_path = match input_path_buf.extension() {
-            Some(os_str) => match os_str.to_str() {
-                Some("json") => temp_path
-                    .join("analysis")
-                    .join(input_path_buf.file_name().unwrap()),
-                _ => temp_path.join(input_path),
-            },
-            _ => temp_path.join(input_path),
-        };
 
-        // Create directories for the output path
-        let parent = output_path
-            .parent()
-            .ok_or_else(|| anyhow!("Failed to get parent for path: {:?}", output_path))?;
-        std::fs::create_dir_all(parent).with_context(|| {
-            format!(
-                "Failed to create temporary directories for path: {}",
-                parent.display()
-            )
-        })?;
+        // save_analysis files are JSON files
+        if let Some(os_str) = input_path_buf.extension() {
+            if let Some("json") = os_str.to_str() {
+                let digest = required_input.get_info().get_digest();
+                let file_contents = provider.contents(digest).with_context(|| {
+                    format!(
+                        "Failed to get contents of file \"{}\" with digest \"{}\"",
+                        input_path, digest
+                    )
+                })?;
 
-        // Copy the file contents to the output path in the temporary directory
-        let digest = required_input.get_info().get_digest();
-        let file_contents = provider.contents(digest).with_context(|| {
-            format!(
-                "Failed to get contents of file \"{}\" with digest \"{}\"",
-                input_path, digest
-            )
-        })?;
-        let mut output_file = File::create(&output_path).context("Failed to create file")?;
-        output_file.write_all(&file_contents).with_context(|| {
-            format!(
-                "Failed to copy contents of \"{}\" with digest \"{}\" to \"{}\"",
-                input_path,
-                digest,
-                output_path.display()
-            )
-        })?;
+                let output_path = temp_path.join(input_path_buf.file_name().unwrap());
+                let mut output_file = File::create(&output_path).context("Failed to create file")?;
+                output_file.write_all(&file_contents).with_context(|| {
+                    format!(
+                        "Failed to copy contents of \"{}\" with digest \"{}\" to \"{}\"",
+                        input_path,
+                        digest,
+                        output_path.display()
+                    )
+                })?;
+            }
+        }
     }
     Ok(())
 }
diff --git a/kythe/rust/indexer/src/error.rs b/kythe/rust/indexer/src/error.rs
index 0e861cf07..9aa994192 100644
--- a/kythe/rust/indexer/src/error.rs
+++ b/kythe/rust/indexer/src/error.rs
@@ -18,8 +18,8 @@ quick_error! {
     #[derive(Debug)]
     pub enum KytheError {
         /// The FileProvider failed to find the file
-        FileNotFoundError {
-            display("The requested file could not be found")
+        FileNotFoundError(name: String) {
+            display("The requested file could not be found: {}", name)
         }
         /// The FileProvider failed to read the file
         FileReadError(err: std::io::Error) {
diff --git a/kythe/rust/indexer/src/indexer/analyzers.rs b/kythe/rust/indexer/src/indexer/analyzers.rs
index ec192be7b..01922c92e 100644
--- a/kythe/rust/indexer/src/indexer/analyzers.rs
+++ b/kythe/rust/indexer/src/indexer/analyzers.rs
@@ -13,6 +13,7 @@
 // limitations under the License.
 
 use crate::error::KytheError;
+use crate::providers::FileProvider;
 use crate::writer::KytheWriter;
 
 use super::entries::EntryEmitter;
@@ -23,8 +24,6 @@ use rls_analysis::Crate;
 use rls_data::{Def, DefKind};
 use std::collections::HashMap;
 use std::ffi::OsStr;
-use std::fs;
-use std::path::Path;
 use storage_rust_proto::*;
 
 /// A data structure to analyze and index CompilationUnit protobufs
@@ -35,19 +34,21 @@ pub struct UnitAnalyzer<'a> {
     unit_storage_vname: VName,
     // The emitter used to  write generated nodes and edges
     emitter: EntryEmitter<'a>,
-    // The root directory for the source files
-    root_dir: &'a Path,
-    // A map between a file name and it's Kythe VName
+    // A map between a file name and its Kythe VName
     file_vnames: HashMap<String, VName>,
+    // A map between a file name and its sha256 digest
+    file_digests: HashMap<String, String>,
     // An index for computing byte offsets in files based on line and column number
     offset_index: OffsetIndex,
+    // A file provider
+    provider: &'a mut dyn FileProvider,
 }
 
 /// A data structure to analyze and index individual crates
 pub struct CrateAnalyzer<'a, 'b> {
     // The emitter used to  write generated nodes and edges
     emitter: &'b mut EntryEmitter<'a>,
-    // A map between a file name and it's Kythe VName
+    // A map between a file name and its Kythe VName
     file_vnames: &'b HashMap<String, VName>,
     // The current CompilationUnit's VName
     unit_vname: &'b VName,
@@ -72,7 +73,7 @@ pub struct CrateAnalyzer<'a, 'b> {
 }
 
 /// A data struct to keep track of method implementations. Used in a HashMap to
-/// map a method definition Id to it's struct and corresponding trait.
+/// map a method definition Id to its struct and corresponding trait.
 pub struct MethodImpl {
     // The struct definition Id the method is being implemented on
     pub struct_target: rls_data::Id,
@@ -87,16 +88,18 @@ impl<'a> UnitAnalyzer<'a> {
     pub fn new(
         unit: &'a CompilationUnit,
         writer: &'a mut dyn KytheWriter,
-        root_dir: &'a Path,
+        provider: &'a mut dyn FileProvider,
     ) -> Self {
         // Create a HashMap between the file path and the VName which we can retrieve
-        // later to emit nodes
+        // later to emit nodes and create a HashMap between a file path and its digest
         let mut file_vnames = HashMap::new();
+        let mut file_digests = HashMap::new();
         for required_input in unit.get_required_input() {
             let analysis_vname = required_input.get_v_name();
             let storage_vname: VName = analysis_to_storage_vname(&analysis_vname);
             let path = storage_vname.get_path().to_owned();
-            file_vnames.insert(path, storage_vname);
+            file_vnames.insert(path.clone(), storage_vname);
+            file_digests.insert(path.clone(), required_input.get_info().get_digest().to_string());
         }
 
         let unit_storage_vname: VName = analysis_to_storage_vname(&unit.get_v_name());
@@ -104,9 +107,10 @@ impl<'a> UnitAnalyzer<'a> {
             unit,
             unit_storage_vname,
             emitter: EntryEmitter::new(writer),
-            root_dir,
             file_vnames,
+            file_digests,
             offset_index: OffsetIndex::default(),
+            provider
         }
     }
 
@@ -127,7 +131,16 @@ impl<'a> UnitAnalyzer<'a> {
 
             // Read the file contents and set it on the fact
             // Returns a FileReadError if we can't read the file
-            let file_contents = fs::read_to_string(self.root_dir.join(Path::new(&source_file)))?;
+            let file_contents: String;
+            if let Some(file_digest) = self.file_digests.get(&source_file.to_string()) {
+                let file_bytes = self.provider.contents(file_digest)?;
+                file_contents = String::from_utf8(file_bytes)
+                    .map_err(|_| KytheError::IndexerError(
+                        format!("Failed to read file {} as UTF8 string", source_file.to_string())
+                    ))?;
+            } else {
+                return Err(KytheError::FileNotFoundError(source_file.to_string()));
+            }
 
             // Add the file to the OffsetIndex
             self.offset_index.add_file(&source_file, &file_contents);
diff --git a/kythe/rust/indexer/src/indexer/mod.rs b/kythe/rust/indexer/src/indexer/mod.rs
index 9974a14e1..cd9b06ce8 100644
--- a/kythe/rust/indexer/src/indexer/mod.rs
+++ b/kythe/rust/indexer/src/indexer/mod.rs
@@ -18,6 +18,7 @@ pub mod offset;
 pub mod save_analysis;
 
 use crate::error::KytheError;
+use crate::providers::FileProvider;
 use crate::writer::KytheWriter;
 
 use analysis_rust_proto::*;
@@ -35,16 +36,16 @@ impl<'a> KytheIndexer<'a> {
         Self { writer }
     }
 
-    /// Accepts a CompilationUnit and the root directory of source files and
+    /// Accepts a CompilationUnit and the directory for analysis files and
     /// indexes the CompilationUnit
-    pub fn index_cu(&mut self, unit: &CompilationUnit, root_dir: &Path) -> Result<(), KytheError> {
-        let mut generator = UnitAnalyzer::new(unit, self.writer, root_dir);
+    pub fn index_cu(&mut self, unit: &CompilationUnit, analysis_dir: &Path, provider: &mut dyn FileProvider) -> Result<(), KytheError> {
+        let mut generator = UnitAnalyzer::new(unit, self.writer, provider);
 
         // First, create file nodes for all of the source files in the CompilationUnit
         generator.handle_files()?;
 
         // Then, index all of the crates from the save_analysis
-        let analyzed_crates = save_analysis::load_analysis(&root_dir.join("analysis"));
+        let analyzed_crates = save_analysis::load_analysis(&analysis_dir);
         for krate in analyzed_crates {
             generator.index_crate(krate)?;
         }
diff --git a/kythe/rust/indexer/src/providers.rs b/kythe/rust/indexer/src/providers.rs
index 54cdaffe0..feedf24c8 100644
--- a/kythe/rust/indexer/src/providers.rs
+++ b/kythe/rust/indexer/src/providers.rs
@@ -132,7 +132,7 @@ impl FileProvider for KzipFileProvider {
         let file = self
             .zip_archive
             .by_name(&name)
-            .map_err(|_| KytheError::FileNotFoundError)?;
+            .map_err(|_| KytheError::FileNotFoundError(name))?;
         let mut reader = BufReader::new(file);
         let mut file_contents: Vec<u8> = Vec::new();
         reader.read_to_end(&mut file_contents)?;


Test patch:
diff --git a/kythe/rust/indexer/tests/providers.rs b/kythe/rust/indexer/tests/providers.rs
index 762ccd391..9ce9c9c91 100644
--- a/kythe/rust/indexer/tests/providers.rs
+++ b/kythe/rust/indexer/tests/providers.rs
@@ -63,7 +63,7 @@ fn test_kzip_provider() {
     assert!(invalid_contents.is_err(), "Expected Err while reading contents for non-existent file, but received file contents: {:?}", invalid_contents.unwrap());
     let contents_error = invalid_contents.err().unwrap();
     match contents_error {
-        KytheError::FileNotFoundError => {}
+        KytheError::FileNotFoundError(_) => {}
         _ => panic!(
             "Unexpected error while getting contents of nonexistent file: {}",
             contents_error