Avoid constructing paths using string literals

To avoid the chance of an issue involving a case-sensitive filesystem
and hardcoded string literals in loot-condition-interpreter.

However, I'm pretty sure this was a waste of time, because the library
input strings come from condition strings that do not necessarily
reflect the casing of filenames and paths in the filesystem that the
conditions are evaluated against, and that's much more likely to cause
issues.

Author: Ortham

benches/eval.rs

@@ -23,14 +23,47 @@ fn generate_plugin_versions() -> Vec<(String, String)> {
 
 fn criterion_benchmark(c: &mut Criterion) {
     c.bench_function("Expression.eval() file(path)", |b| {
-        let state = State::new(GameType::Oblivion, ".".into());
-        let expression = Expression::from_str("file(\"Cargo.toml\")").unwrap();
+        let tmp_dir = tempfile::tempdir().unwrap();
+        let data_path = tmp_dir.path().join("Data");
+        std::fs::create_dir(&data_path).unwrap();
+        let mut state = State::new(GameType::Oblivion, data_path.clone());
+
+        for entry in std::fs::read_dir("tests/testing-plugins/Oblivion/Data").unwrap() {
+            let entry = entry.unwrap();
+            std::fs::copy(entry.path(), data_path.join(entry.file_name())).unwrap();
+        }
+
+        state.clear_condition_cache().unwrap();
+
+        let expression = Expression::from_str("file(\"Blank.esp\")").unwrap();
 
         b.iter(|| {
             assert!(expression.eval(&state).unwrap());
         });
     });
 
+    c.bench_function("Expression.eval() file(path) with missing plugin", |b| {
+        let tmp_dir = tempfile::tempdir().unwrap();
+        let data_path = tmp_dir.path().join("Data");
+        std::fs::create_dir(&data_path).unwrap();
+        let mut state = State::new(GameType::Oblivion, data_path.clone());
+
+        for entry in std::fs::read_dir("tests/testing-plugins/Oblivion/Data").unwrap() {
+            let entry = entry.unwrap();
+            let mut ghosted = entry.file_name();
+            ghosted.push(".ghost");
+            std::fs::copy(entry.path(), data_path.join(ghosted)).unwrap();
+        }
+
+        state.clear_condition_cache().unwrap();
+
+        let expression = Expression::from_str("file(\"plugin.esp\")").unwrap();
+
+        b.iter(|| {
+            assert!(!expression.eval(&state).unwrap());
+        });
+    });
+
     c.bench_function("Expression.eval() file(regex)", |b| {
         let state = State::new(GameType::Oblivion, ".".into());
         let expression = Expression::from_str("file(\"Cargo.*\")").unwrap();

src/function/eval.rs

@@ -316,6 +316,8 @@ impl Function {
 mod tests {
     use super::*;
 
+    use std::collections::HashMap;
+    use std::ffi::OsString;
     use std::fs::{copy, create_dir, remove_file};
     use std::path::PathBuf;
     use std::sync::RwLock;
@@ -376,6 +378,7 @@ mod tests {
                 .map(|(p, v)| (p.to_lowercase(), v.to_string()))
                 .collect(),
             condition_cache: RwLock::default(),
+            ghosted_plugins: HashMap::default(),
         }
     }
 
@@ -407,7 +410,8 @@ mod tests {
     fn function_file_path_eval_should_return_true_if_given_a_plugin_that_is_ghosted() {
         let tmp_dir = tempdir().unwrap();
         let data_path = tmp_dir.path().join("Data");
-        let state = state(data_path);
+        let mut state = state(&data_path);
+        state.ghosted_plugins.insert(data_path.clone(), vec![OsString::from("blank.esp.ghost")]);
 
         copy(
             Path::new("tests/testing-plugins/Oblivion/Data/Blank.esp"),
@@ -818,11 +822,12 @@ mod tests {
     fn function_checksum_eval_should_support_checking_the_crc_of_a_ghosted_plugin() {
         let tmp_dir = tempdir().unwrap();
         let data_path = tmp_dir.path().join("Data");
-        let state = state(data_path);
+        let mut state = state(&data_path);
+        state.ghosted_plugins.insert(data_path.clone(), vec![OsString::from("blank.esm.ghost")]);
 
         copy(
             Path::new("tests/testing-plugins/Oblivion/Data/Blank.esm"),
-            &state.data_path.join("Blank.esm.ghost"),
+            &state.data_path.join("blank.esm.ghost"),
         )
         .unwrap();
 

src/function/mod.rs

@@ -8,7 +8,7 @@ use unicase::eq;
 
 pub mod eval;
 pub mod parse;
-mod path;
+pub mod path;
 mod version;
 
 #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]

src/function/path.rs

@@ -1,5 +1,6 @@
 use std::{
-    ffi::OsStr,
+    collections::HashMap,
+    ffi::{OsStr, OsString},
     path::{Path, PathBuf},
 };
 
@@ -21,25 +22,24 @@ fn has_unghosted_plugin_file_extension(game_type: GameType, path: &Path) -> bool
     }
 }
 
-pub fn has_plugin_file_extension(game_type: GameType, path: &Path) -> bool {
+pub fn has_ghosted_plugin_file_extension(game_type: GameType, path: &Path) -> bool {
     match path.extension() {
         Some(ext) if ext.eq_ignore_ascii_case(GHOST_EXTENSION) => path
             .file_stem()
             .map(|s| has_unghosted_plugin_file_extension(game_type, Path::new(s)))
             .unwrap_or(false),
-        Some(ext) => is_unghosted_plugin_file_extension(game_type, ext),
         _ => false,
     }
 }
 
-fn add_ghost_extension(path: PathBuf) -> PathBuf {
+pub fn has_plugin_file_extension(game_type: GameType, path: &Path) -> bool {
     match path.extension() {
-        Some(e) => {
-            let mut new_extension = e.to_os_string();
-            new_extension.push(GHOST_EXTENSION_WITH_PERIOD);
-            path.with_extension(&new_extension)
-        }
-        None => path.with_extension(GHOST_EXTENSION),
+        Some(ext) if ext.eq_ignore_ascii_case(GHOST_EXTENSION) => path
+            .file_stem()
+            .map(|s| has_unghosted_plugin_file_extension(game_type, Path::new(s)))
+            .unwrap_or(false),
+        Some(ext) => is_unghosted_plugin_file_extension(game_type, ext),
+        _ => false,
     }
 }
 
@@ -61,6 +61,36 @@ pub fn normalise_file_name(game_type: GameType, name: &OsStr) -> &OsStr {
     name
 }
 
+fn get_ghosted_filename(path: &Path) -> Option<OsString> {
+    let mut filename = path.file_name()?.to_os_string();
+    filename.push(GHOST_EXTENSION_WITH_PERIOD);
+    Some(filename)
+}
+
+fn add_ghost_extension(
+    path: &Path,
+    ghosted_plugins: &HashMap<PathBuf, Vec<OsString>>,
+) -> Option<PathBuf> {
+    // Can't just append a .ghost extension as the filesystem may be case-sensitive and the ghosted
+    // file may have a .GHOST extension (for example). Instead loop through the other files in the
+    // same parent directory and look for one that's unicode-case-insensitively-equal.
+    let expected_filename = get_ghosted_filename(&path)?;
+    let expected_filename = expected_filename.to_str()?;
+    let parent_path = path.parent()?;
+
+    let ghosted_plugins = ghosted_plugins.get(&parent_path.to_path_buf())?;
+
+    for ghosted_plugin in ghosted_plugins {
+        let ghosted_plugin_str = ghosted_plugin.to_str()?;
+
+        if unicase::eq(expected_filename, ghosted_plugin_str) {
+            return Some(parent_path.join(ghosted_plugin));
+        }
+    }
+
+    None
+}
+
 pub fn resolve_path(state: &State, path: &Path) -> PathBuf {
     // First check external data paths, as files there may override files in the main data path.
     for data_path in &state.additional_data_paths {
@@ -71,7 +101,9 @@ pub fn resolve_path(state: &State, path: &Path) -> PathBuf {
         }
 
         if has_unghosted_plugin_file_extension(state.game_type, &path) {
-            path = add_ghost_extension(path);
+            if let Some(ghosted_path) = add_ghost_extension(&path, &state.ghosted_plugins) {
+                path = ghosted_path
+            }
         }
 
         if path.exists() {
@@ -83,7 +115,7 @@ pub fn resolve_path(state: &State, path: &Path) -> PathBuf {
     let path = state.data_path.join(path);
 
     if !path.exists() && has_unghosted_plugin_file_extension(state.game_type, &path) {
-        add_ghost_extension(path)
+        add_ghost_extension(&path, &state.ghosted_plugins).unwrap_or(path)
     } else {
         path
     }
@@ -395,15 +427,52 @@ mod tests {
     }
 
     #[test]
-    fn add_ghost_extension_should_add_dot_ghost_to_an_existing_extension() {
-        let path = add_ghost_extension("plugin.esp".into());
-        assert_eq!(PathBuf::from("plugin.esp.ghost"), path);
+    fn add_ghost_extension_should_return_none_if_the_given_parent_path_is_not_in_hashmap() {
+        let path = Path::new("subdir/plugin.esp");
+        let result = add_ghost_extension(path, &HashMap::new());
+
+        assert!(result.is_none());
     }
 
     #[test]
-    fn add_ghost_extension_should_add_dot_ghost_to_an_a_path_with_no_extension() {
-        let path = add_ghost_extension("plugin".into());
-        assert_eq!(PathBuf::from("plugin.ghost"), path);
+    fn add_ghost_extension_should_return_none_if_the_given_parent_path_has_no_ghosted_plugins() {
+        let path = Path::new("subdir/plugin.esp");
+        let mut map = HashMap::new();
+        map.insert(PathBuf::from("subdir"), Vec::new());
+
+        let result = add_ghost_extension(path, &map);
+
+        assert!(result.is_none());
+    }
+
+    #[test]
+    fn add_ghost_extension_should_return_none_if_the_given_parent_path_has_no_matching_ghosted_plugins(
+    ) {
+        let path = Path::new("subdir/plugin.esp");
+        let mut map = HashMap::new();
+        map.insert(
+            PathBuf::from("subdir"),
+            vec![OsString::from("plugin.esm.ghost")],
+        );
+        let result = add_ghost_extension(path, &map);
+
+        assert!(result.is_none());
+    }
+
+    #[test]
+    fn add_ghost_extension_should_return_some_if_the_given_parent_path_has_a_case_insensitively_equal_ghosted_plugin(
+    ) {
+        let path = Path::new("subdir/plugin.esp");
+        let ghosted_plugin = "Plugin.ESp.GHoST";
+        let mut map = HashMap::new();
+        map.insert(
+            PathBuf::from("subdir"),
+            vec![OsString::from(ghosted_plugin)],
+        );
+        let result = add_ghost_extension(path, &map);
+
+        assert!(result.is_some());
+        assert_eq!(Path::new("subdir").join(ghosted_plugin), result.unwrap());
     }
 
     #[test]
@@ -436,7 +505,9 @@ mod tests {
     fn resolve_path_should_return_the_given_data_relative_path_plus_a_ghost_extension_if_the_plugin_path_does_not_exist(
     ) {
         let data_path = PathBuf::from(".");
-        let state = State::new(GameType::Skyrim, data_path.clone());
+        let mut state = State::new(GameType::Skyrim, data_path.clone());
+        state.ghosted_plugins.insert(data_path.clone(), vec![OsString::from("plugin.esp.ghost")]);
+
         let input_path = Path::new("plugin.esp");
         let resolved_path = resolve_path(&state, input_path);
 

src/lib.rs

@@ -2,9 +2,10 @@ mod error;
 mod function;
 
 use std::collections::{HashMap, HashSet};
+use std::ffi::OsString;
 use std::fmt;
 use std::ops::DerefMut;
-use std::path::PathBuf;
+use std::path::{Path, PathBuf};
 use std::str;
 use std::sync::{PoisonError, RwLock, RwLockWriteGuard};
 
@@ -18,6 +19,7 @@ use nom::IResult;
 
 use error::ParsingError;
 pub use error::{Error, ParsingErrorKind};
+use function::path::has_ghosted_plugin_file_extension;
 use function::Function;
 
 type ParsingResult<'a, T> = IResult<&'a str, T, ParsingError<&'a str>>;
@@ -62,10 +64,47 @@ pub struct State {
     plugin_versions: HashMap<String, String>,
     /// Conditions that have already been evaluated, and their results.
     condition_cache: RwLock<HashMap<Function, bool>>,
+    /// Ghosted plugin filenames that have been found in data paths, organised by data path.
+    ghosted_plugins: HashMap<PathBuf, Vec<OsString>>,
+}
+
+fn find_ghosted_plugins(game_type: GameType, data_path: &Path) -> Vec<OsString> {
+    std::fs::read_dir(data_path)
+        .into_iter()
+        .flatten()
+        .flat_map(|e| e.ok())
+        .filter(|e| e.metadata().map(|m| m.is_file()).unwrap_or(false))
+        .map(|e| e.file_name())
+        .filter(|p| has_ghosted_plugin_file_extension(game_type, Path::new(p)))
+        .collect()
+}
+
+fn build_ghosted_plugins_cache(
+    game_type: GameType,
+    data_path: &Path,
+    additional_data_paths: &[PathBuf],
+) -> HashMap<PathBuf, Vec<OsString>> {
+    let mut map: HashMap<PathBuf, Vec<OsString>> = HashMap::new();
+
+    map.insert(
+        data_path.to_path_buf(),
+        find_ghosted_plugins(game_type, data_path),
+    );
+
+    for additional_data_path in additional_data_paths {
+        map.insert(
+            additional_data_path.clone(),
+            find_ghosted_plugins(game_type, &additional_data_path),
+        );
+    }
+
+    map
 }
 
 impl State {
     pub fn new(game_type: GameType, data_path: PathBuf) -> Self {
+        let ghosted_plugins = build_ghosted_plugins_cache(game_type, &data_path, &[]);
+
         State {
             game_type,
             data_path,
@@ -74,6 +113,7 @@ impl State {
             crc_cache: RwLock::default(),
             plugin_versions: HashMap::default(),
             condition_cache: RwLock::default(),
+            ghosted_plugins,
         }
     }
 
@@ -123,11 +163,23 @@ impl State {
     pub fn clear_condition_cache(
         &mut self,
     ) -> Result<(), PoisonError<RwLockWriteGuard<HashMap<Function, bool>>>> {
+        self.refresh_ghosted_plugins();
+
         self.condition_cache.write().map(|mut c| c.clear())
     }
 
     pub fn set_additional_data_paths(&mut self, additional_data_paths: Vec<PathBuf>) {
         self.additional_data_paths = additional_data_paths;
+
+        self.refresh_ghosted_plugins()
+    }
+
+    fn refresh_ghosted_plugins(&mut self) {
+        self.ghosted_plugins = build_ghosted_plugins_cache(
+            self.game_type,
+            &self.data_path,
+            &self.additional_data_paths,
+        );
     }
 }
 
@@ -297,6 +349,7 @@ mod tests {
             crc_cache: RwLock::default(),
             plugin_versions: HashMap::default(),
             condition_cache: RwLock::default(),
+            ghosted_plugins: HashMap::default(),
         }
     }