From c85f2537d308d44961565255a2ca8c20f45fd5d2 Mon Sep 17 00:00:00 2001 From: Simon Hausmann Date: Thu, 16 Jul 2020 11:15:16 +0200 Subject: [PATCH] Fix signature of function to load types from a directory Taking &mut self makes more sense when wanting to populate from a list. Also we should return a list of diagnostics for each file that we loaded or an error if we couldn't load the file (for example due to permission problems, etc.). This way the caller can choose to show diagnostics, ignore them, etc. --- sixtyfps_compiler/Cargo.toml | 1 + sixtyfps_compiler/diagnostics.rs | 18 +-------- sixtyfps_compiler/lib.rs | 7 ++++ sixtyfps_compiler/typeregister.rs | 67 +++++++++++++++++++------------ 4 files changed, 50 insertions(+), 43 deletions(-) diff --git a/sixtyfps_compiler/Cargo.toml b/sixtyfps_compiler/Cargo.toml index 7a2d10c24..264cb074a 100644 --- a/sixtyfps_compiler/Cargo.toml +++ b/sixtyfps_compiler/Cargo.toml @@ -29,6 +29,7 @@ codemap = { version = "0.1", optional = true } quote = { version = "1.0", optional = true } proc-macro2 = { version = "1.0.17", optional = true } lyon = { version = "0.15.8", features = ["svg"] } +thiserror = "1" [dev-dependencies] diff --git a/sixtyfps_compiler/diagnostics.rs b/sixtyfps_compiler/diagnostics.rs index 68313c07f..106af0db9 100644 --- a/sixtyfps_compiler/diagnostics.rs +++ b/sixtyfps_compiler/diagnostics.rs @@ -30,7 +30,7 @@ pub struct CompilerDiagnostic { pub span: Span, } -#[derive(Default, Debug, Clone)] +#[derive(Default, Debug, Clone, PartialEq)] pub struct Diagnostics { pub inner: Vec, pub current_path: std::path::PathBuf, @@ -133,22 +133,6 @@ impl Diagnostics { self } - pub fn check_errors(self) -> std::io::Result { - if !self.has_error() { - return Ok(self); - } - #[cfg(feature = "display-diagnostics")] - return Err(std::io::Error::new(std::io::ErrorKind::Other, self.diagnostics_as_string())); - #[cfg(not(feature = "display-diagnostics"))] - return Err(std::io::Error::new( - std::io::ErrorKind::Other, - format!( - "Error compiling {} but diagnostics were disabled in the compiler", - self.current_path.to_string_lossy() - ), - )); - } - #[cfg(feature = "proc_macro_span")] /// Will convert the diagnostics that only have offsets to the actual span pub fn map_offsets_to_span(&mut self, span_map: &[crate::parser::Token]) { diff --git a/sixtyfps_compiler/lib.rs b/sixtyfps_compiler/lib.rs index 3b913057c..3e5e136e3 100644 --- a/sixtyfps_compiler/lib.rs +++ b/sixtyfps_compiler/lib.rs @@ -41,6 +41,13 @@ mod passes { pub mod unique_id; } +#[derive(thiserror::Error, Debug)] +#[error("Error loading {path} from disk: {source}")] +pub struct FileLoadError { + path: std::path::PathBuf, + source: std::io::Error, +} + #[derive(Default)] /// CompilationConfiguration allows configuring different aspects of the compiler. pub struct CompilerConfiguration { diff --git a/sixtyfps_compiler/typeregister.rs b/sixtyfps_compiler/typeregister.rs index c8dcf665b..f1a556c92 100644 --- a/sixtyfps_compiler/typeregister.rs +++ b/sixtyfps_compiler/typeregister.rs @@ -1,3 +1,5 @@ +use crate::diagnostics::Diagnostics; +use crate::FileLoadError; use std::collections::{BTreeMap, HashMap, HashSet}; use std::{fmt::Display, rc::Rc}; @@ -434,39 +436,49 @@ impl TypeRegister { self.types.insert(comp.id.clone(), Type::Component(comp)); } + /// Loads the .60 file and adds it to the type registry. An error is returned if there were I/O problems, + /// otherwise the diagnostics collected during the parsing are returned. pub fn add_type_from_source>( &mut self, path: P, - ) -> std::io::Result<()> { + ) -> std::io::Result { let (syntax_node, diag) = crate::parser::parse_file(&path)?; - let diag = diag.check_errors()?; - // For the time being .60 files added to a type registry cannot depend on other .60 files. let (doc, diag) = crate::compile_syntax_node(syntax_node, diag); - diag.check_errors()?; - self.add(doc.root_component); - Ok(()) - } - - pub fn new_from_library>( - directory: P, - parent_registry: &Rc, - ) -> std::io::Result> { - let mut result = - TypeRegister { parent_registry: Some(parent_registry.clone()), ..Default::default() }; - - for entry in std::fs::read_dir(directory)? { - let entry = entry?; - let path = entry.path(); - if path.is_file() && path.extension().unwrap_or_default() == std::ffi::OsStr::new("60") - { - result.add_type_from_source(path)?; - } + if !doc.root_component.id.is_empty() { + self.add(doc.root_component); } - Ok(Rc::new(result)) + Ok(diag) + } + + /// Adds all .60 files from the specified directory to the type registry. For each file a result is + /// included in the returned vector that either contains the diagnostics encountered during the parsing + /// (if any) or an I/O error if it occured. If there was a problem reading the directory, then an I/O error + /// is returned. + pub fn add_from_directory>( + &mut self, + directory: P, + ) -> std::io::Result>> { + Ok(std::fs::read_dir(directory)? + .filter_map(Result::ok) + .filter_map(|entry| { + let path = entry.path(); + if path.is_file() + && path.extension().unwrap_or_default() == std::ffi::OsStr::new("60") + { + Some(path) + } else { + None + } + }) + .map(|path| { + self.add_type_from_source(&path) + .map_err(|ioerr| FileLoadError { path, source: ioerr }) + }) + .collect()) } pub fn property_animation_type_for_property(&self, property_type: Type) -> Type { @@ -503,11 +515,14 @@ fn test_registry_from_library() { let test_source_path: std::path::PathBuf = [env!("CARGO_MANIFEST_DIR"), "tests"].iter().collect(); - let result = TypeRegister::new_from_library(test_source_path, &global_types); + let mut local_types = TypeRegister::new(&global_types); + let result = local_types.add_from_directory(test_source_path); assert!(result.is_ok()); - - let local_types = result.unwrap(); + let file_load_status_list = result.unwrap(); + assert_eq!(file_load_status_list.len(), 1); + assert!(file_load_status_list[0].is_ok()); + assert!(!file_load_status_list[0].as_ref().unwrap().has_error()); assert_ne!(local_types.lookup("PublicType"), Type::Invalid); assert_eq!(local_types.lookup("HiddenInternalType"), Type::Invalid);