From a81009607a304f5eadc96d5d1abf02fe9e3060a7 Mon Sep 17 00:00:00 2001 From: Martin Nordholts Date: Tue, 27 Jul 2021 09:43:51 +0200 Subject: [PATCH] HighlightingAssets: Make .syntaxes() and syntax_for_file_name() failable Or rather, introduce new versions of these methods and deprecate the old ones. This is preparation to enable robust and user-friendly support for lazy-loading. With lazy-loading, we don't know if the SyntaxSet is valid until after we try to use it, so wherever we try to use it, we need to return a Result. See discussion about panics in #1747. --- CHANGELOG.md | 1 + src/assets.rs | 116 ++++++++++++++++++++++--------- src/bin/bat/main.rs | 7 +- src/pretty_printer.rs | 4 +- src/printer.rs | 4 +- tests/no_duplicate_extensions.rs | 2 +- 6 files changed, 95 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 79481d87..a600f997 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ ## `bat` as a library +- Deprecate `HighlightingAssets::syntaxes()` and `HighlightingAssets::syntax_for_file_name()`. Use `HighlightingAssets::get_syntaxes()` and `HighlightingAssets::get_syntax_for_file_name()` instead. They return a `Result` which is needed for upcoming lazy-loading work to improve startup performance. See #1747 and #1755 (@Enselic) diff --git a/src/assets.rs b/src/assets.rs index 1615fcac..5bdd2651 100644 --- a/src/assets.rs +++ b/src/assets.rs @@ -128,7 +128,7 @@ impl HighlightingAssets { let _ = fs::create_dir_all(target_dir); asset_to_cache(&self.theme_set, &target_dir.join("themes.bin"), "theme set")?; asset_to_cache( - self.get_syntax_set(), + self.get_syntax_set()?, &target_dir.join("syntaxes.bin"), "syntax set", )?; @@ -147,31 +147,51 @@ impl HighlightingAssets { self.fallback_theme = Some(theme); } - pub(crate) fn get_syntax_set(&self) -> &SyntaxSet { - &self.syntax_set + pub(crate) fn get_syntax_set(&self) -> Result<&SyntaxSet> { + Ok(&self.syntax_set) } + /// Use [Self::get_syntaxes] instead + #[deprecated] pub fn syntaxes(&self) -> &[SyntaxReference] { - self.get_syntax_set().syntaxes() + self.get_syntax_set() + .expect(".syntaxes() is deprecated, use .get_syntaxes() instead") + .syntaxes() + } + + pub fn get_syntaxes(&self) -> Result<&[SyntaxReference]> { + Ok(self.get_syntax_set()?.syntaxes()) } pub fn themes(&self) -> impl Iterator { self.theme_set.themes.keys().map(|s| s.as_ref()) } + /// Use [Self::get_syntax_for_file_name] instead + #[deprecated] pub fn syntax_for_file_name( &self, file_name: impl AsRef, mapping: &SyntaxMapping, ) -> Option<&SyntaxReference> { + self.get_syntax_for_file_name(file_name, mapping).expect( + ".syntax_for_file_name() is deprecated, use .get_syntax_for_file_name() instead", + ) + } + + pub fn get_syntax_for_file_name( + &self, + file_name: impl AsRef, + mapping: &SyntaxMapping, + ) -> Result> { let file_name = file_name.as_ref(); - match mapping.get_syntax_for(file_name) { + Ok(match mapping.get_syntax_for(file_name) { Some(MappingTarget::MapToUnknown) => None, Some(MappingTarget::MapTo(syntax_name)) => { - self.get_syntax_set().find_syntax_by_name(syntax_name) + self.get_syntax_set()?.find_syntax_by_name(syntax_name) } - None => self.get_extension_syntax(file_name.as_os_str()), - } + None => self.get_extension_syntax(file_name.as_os_str())?, + }) } pub(crate) fn get_theme(&self, theme: &str) -> &Theme { @@ -197,11 +217,11 @@ impl HighlightingAssets { mapping: &SyntaxMapping, ) -> Result<&SyntaxReference> { if let Some(language) = language { - self.get_syntax_set() + self.get_syntax_set()? .find_syntax_by_token(language) .ok_or_else(|| ErrorKind::UnknownSyntax(language.to_owned()).into()) } else { - let line_syntax = self.get_first_line_syntax(&mut input.reader); + let line_syntax = self.get_first_line_syntax(&mut input.reader)?; // Get the path of the file: // If this was set by the metadata, that will take priority. @@ -230,13 +250,13 @@ impl HighlightingAssets { }), Some(MappingTarget::MapTo(syntax_name)) => self - .get_syntax_set() + .get_syntax_set()? .find_syntax_by_name(syntax_name) .ok_or_else(|| ErrorKind::UnknownSyntax(syntax_name.to_owned()).into()), None => { let file_name = path.file_name().unwrap_or_default(); - self.get_extension_syntax(file_name) + self.get_extension_syntax(file_name)? .or(line_syntax) .ok_or_else(|| { ErrorKind::UndetectedSyntax(path.to_string_lossy().into()).into() @@ -250,26 +270,34 @@ impl HighlightingAssets { } } - fn get_extension_syntax(&self, file_name: &OsStr) -> Option<&SyntaxReference> { - self.find_syntax_by_file_name(file_name).or_else(|| { - self.find_syntax_by_file_name_extension(file_name) - .or_else(|| self.get_extension_syntax_with_stripped_suffix(file_name)) - }) + fn get_extension_syntax(&self, file_name: &OsStr) -> Result> { + let mut syntax = self.find_syntax_by_file_name(file_name)?; + if syntax.is_none() { + syntax = self.find_syntax_by_file_name_extension(file_name)?; + } + if syntax.is_none() { + syntax = self.get_extension_syntax_with_stripped_suffix(file_name)?; + } + Ok(syntax) } - fn find_syntax_by_file_name(&self, file_name: &OsStr) -> Option<&SyntaxReference> { - self.get_syntax_set() - .find_syntax_by_extension(file_name.to_str().unwrap_or_default()) + fn find_syntax_by_file_name(&self, file_name: &OsStr) -> Result> { + Ok(self + .get_syntax_set()? + .find_syntax_by_extension(file_name.to_str().unwrap_or_default())) } - fn find_syntax_by_file_name_extension(&self, file_name: &OsStr) -> Option<&SyntaxReference> { + fn find_syntax_by_file_name_extension( + &self, + file_name: &OsStr, + ) -> Result> { let file_path = Path::new(file_name); - self.get_syntax_set().find_syntax_by_extension( + Ok(self.get_syntax_set()?.find_syntax_by_extension( file_path .extension() .and_then(|x| x.to_str()) .unwrap_or_default(), - ) + )) } /// If we find an ignored suffix on the file name, e.g. '~', we strip it and @@ -277,22 +305,25 @@ impl HighlightingAssets { fn get_extension_syntax_with_stripped_suffix( &self, file_name: &OsStr, - ) -> Option<&SyntaxReference> { + ) -> Result> { let file_path = Path::new(file_name); + let mut syntax = None; if let Some(file_str) = file_path.to_str() { for suffix in IGNORED_SUFFIXES.iter() { if let Some(stripped_filename) = file_str.strip_suffix(suffix) { - return self.get_extension_syntax(OsStr::new(stripped_filename)); + syntax = self.get_extension_syntax(OsStr::new(stripped_filename))?; + break; } } } - None + Ok(syntax) } - fn get_first_line_syntax(&self, reader: &mut InputReader) -> Option<&SyntaxReference> { - String::from_utf8(reader.first_line.clone()) + fn get_first_line_syntax(&self, reader: &mut InputReader) -> Result> { + let syntax_set = self.get_syntax_set()?; + Ok(String::from_utf8(reader.first_line.clone()) .ok() - .and_then(|l| self.get_syntax_set().find_syntax_by_first_line(&l)) + .and_then(|l| syntax_set.find_syntax_by_first_line(&l))) } } @@ -364,7 +395,12 @@ mod tests { self.assets .get_syntax(None, &mut opened_input, &self.syntax_mapping) - .unwrap_or_else(|_| self.assets.get_syntax_set().find_syntax_plain_text()) + .unwrap_or_else(|_| { + self.assets + .get_syntax_set() + .expect("this is mod tests") + .find_syntax_plain_text() + }) .name .clone() } @@ -378,7 +414,12 @@ mod tests { self.assets .get_syntax(None, &mut opened_input, &self.syntax_mapping) - .unwrap_or_else(|_| self.assets.get_syntax_set().find_syntax_plain_text()) + .unwrap_or_else(|_| { + self.assets + .get_syntax_set() + .expect("this is mod tests") + .find_syntax_plain_text() + }) .name .clone() } @@ -402,7 +443,12 @@ mod tests { self.assets .get_syntax(None, &mut opened_input, &self.syntax_mapping) - .unwrap_or_else(|_| self.assets.get_syntax_set().find_syntax_plain_text()) + .unwrap_or_else(|_| { + self.assets + .get_syntax_set() + .expect("this is mod tests") + .find_syntax_plain_text() + }) .name .clone() } @@ -560,7 +606,11 @@ mod tests { assert_eq!( test.assets .get_syntax(None, &mut opened_input, &test.syntax_mapping) - .unwrap_or_else(|_| test.assets.get_syntax_set().find_syntax_plain_text()) + .unwrap_or_else(|_| test + .assets + .get_syntax_set() + .expect("this is mod tests") + .find_syntax_plain_text()) .name, "SSH Config" ); diff --git a/src/bin/bat/main.rs b/src/bin/bat/main.rs index fb407b90..ae4b30e9 100644 --- a/src/bin/bat/main.rs +++ b/src/bin/bat/main.rs @@ -82,7 +82,7 @@ pub fn get_languages(config: &Config) -> Result { let assets = assets_from_cache_or_binary()?; let mut languages = assets - .syntaxes() + .get_syntaxes()? .iter() .filter(|syntax| !syntax.hidden && !syntax.file_extensions.is_empty()) .cloned() @@ -101,7 +101,10 @@ pub fn get_languages(config: &Config) -> Result { true } else { let test_file = Path::new("test").with_extension(extension); - match assets.syntax_for_file_name(test_file, &config.syntax_mapping) { + let syntax = assets + .get_syntax_for_file_name(test_file, &config.syntax_mapping) + .unwrap(); // safe since .get_syntaxes() above worked + match syntax { Some(syntax) => syntax.name == lang_name, None => false, } diff --git a/src/pretty_printer.rs b/src/pretty_printer.rs index f72122ee..d537b890 100644 --- a/src/pretty_printer.rs +++ b/src/pretty_printer.rs @@ -235,7 +235,9 @@ impl<'a> PrettyPrinter<'a> { } pub fn syntaxes(&self) -> impl Iterator { - self.assets.syntaxes().iter() + // We always use assets from the binary, which are guaranteed to always + // be valid, so get_syntaxes() can never fail here + self.assets.get_syntaxes().unwrap().iter() } /// Pretty-print all specified inputs. This method will "use" all stored inputs. diff --git a/src/printer.rs b/src/printer.rs index b186c9c2..22d7c22d 100644 --- a/src/printer.rs +++ b/src/printer.rs @@ -174,7 +174,7 @@ impl<'a> InteractivePrinter<'a> { let syntax = match assets.get_syntax(config.language, input, &config.syntax_mapping) { Ok(syntax) => syntax, Err(Error(ErrorKind::UndetectedSyntax(_), _)) => { - assets.get_syntax_set().find_syntax_plain_text() + assets.get_syntax_set()?.find_syntax_plain_text() } Err(e) => return Err(e), }; @@ -192,7 +192,7 @@ impl<'a> InteractivePrinter<'a> { #[cfg(feature = "git")] line_changes, highlighter, - syntax_set: assets.get_syntax_set(), + syntax_set: assets.get_syntax_set()?, background_color_highlight, }) } diff --git a/tests/no_duplicate_extensions.rs b/tests/no_duplicate_extensions.rs index b2be5e4e..c28a9c6a 100644 --- a/tests/no_duplicate_extensions.rs +++ b/tests/no_duplicate_extensions.rs @@ -26,7 +26,7 @@ fn no_duplicate_extensions() { let mut extensions = HashSet::new(); - for syntax in assets.syntaxes() { + for syntax in assets.get_syntaxes().expect("this is a #[test]") { for extension in &syntax.file_extensions { assert!( KNOWN_EXCEPTIONS.contains(&extension.as_str()) || extensions.insert(extension),