From bec17478fd08e7defb95cc7a01704b6d4d659b47 Mon Sep 17 00:00:00 2001 From: Rebecca Turner Date: Wed, 12 Jun 2024 17:17:26 -0700 Subject: [PATCH] Fix `:show targets` / `:show paths` parsing logic Previously, if `:show targets` displayed a path, we would attempt to join that path to each of the module search paths and to GHCi's working directory to find the file. In reality, GHCi only checks such paths relative to its working directory. (Indeed, if you change the working directory with `:cd`, GHCi will unload all modules. [1]) Also, the logic for checking these paths was broken, considering that the search paths can be relative to the working directory, but we weren't properly joining these paths. In the future, I'll write some tests that do `--after-startup-ghci ":cd ../"` and see how much of ghciwatch explodes. [1]: https://gitlab.haskell.org/ghc/ghc/-/blob/077cb2e11fa81076e8c9c5f8dd3bdfa99c8aaf8d/compiler/GHC.hs#L1073-L1083 --- src/ghci/parse/show_paths.rs | 46 +++++++++++++++------------------- src/ghci/parse/show_targets.rs | 44 ++++++++++++-------------------- src/normal_path.rs | 45 +++++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 54 deletions(-) diff --git a/src/ghci/parse/show_paths.rs b/src/ghci/parse/show_paths.rs index 49a5abd6..895e00b2 100644 --- a/src/ghci/parse/show_paths.rs +++ b/src/ghci/parse/show_paths.rs @@ -37,14 +37,13 @@ impl ShowPaths { } /// Convert a target (from `:show targets` output) to a module source path. - pub fn target_to_path(&self, target: &str) -> miette::Result<(Utf8PathBuf, TargetKind)> { + pub fn target_to_path(&self, target: &str) -> miette::Result<(NormalPath, TargetKind)> { let target_path = Utf8Path::new(target); if is_haskell_source_file(target_path) { // The target is already a path. - if let Some(path) = self.target_path_to_path(target_path) { - tracing::trace!(%path, %target, "Target is path"); - return Ok((path, TargetKind::Path)); - } + let path = self.cwd.join(target_path); + tracing::trace!(%path, %target, "Target is path"); + return Ok((NormalPath::new(path, &self.cwd)?, TargetKind::Path)); } else { // Else, split by `.` to get path components. let mut path = target.split('.').collect::(); @@ -53,31 +52,26 @@ impl ShowPaths { for haskell_source_extension in HASKELL_SOURCE_EXTENSIONS { path.set_extension(haskell_source_extension); - if let Some(path) = self.target_path_to_path(&path) { - tracing::trace!(%path, %target, "Found path for target"); - return Ok((path, TargetKind::Module)); + for search_path in self.absolute_search_paths() { + let path = search_path.join(&path); + if path.exists() { + tracing::trace!(%path, %target, "Found path for target"); + return Ok((NormalPath::new(path, &self.cwd)?, TargetKind::Module)); + } } } } Err(miette!("Couldn't find source path for {target}")) } - /// Convert a target path like `src/MyLib.hs` to a module source path starting with one of the - /// `search_paths`. - fn target_path_to_path(&self, target: &Utf8Path) -> Option { - for search_path in self.paths() { - let path = search_path.join(target); - if path.exists() { - // Found it! - return Some(path); + fn absolute_search_paths(&self) -> impl Iterator + '_ { + self.search_paths.iter().map(|path| { + if path.is_absolute() { + path.clone() + } else { + self.cwd.join(path) } - } - - None - } - - fn paths(&self) -> impl Iterator { - self.search_paths.iter().chain(std::iter::once(&self.cwd)) + }) } /// Convert a Haskell source path to a module name. @@ -85,7 +79,7 @@ impl ShowPaths { let path = path.with_extension(""); let path_str = path.as_str(); - for search_path in self.paths() { + for search_path in self.absolute_search_paths() { if let Some(suffix) = path_str.strip_prefix(search_path.as_str()) { let module_name = Utf8Path::new(suffix) .components() @@ -238,12 +232,12 @@ mod tests { fn test_path_to_module() { let paths = ShowPaths { cwd: Utf8PathBuf::from("/Users/wiggles/ghciwatch/"), - search_paths: vec![], + search_paths: vec![Utf8PathBuf::from("src")], }; assert_eq!( paths - .path_to_module(Utf8Path::new("/Users/wiggles/ghciwatch/Foo/Bar/Baz.hs")) + .path_to_module(Utf8Path::new("/Users/wiggles/ghciwatch/src/Foo/Bar/Baz.hs")) .unwrap(), "Foo.Bar.Baz" ); diff --git a/src/ghci/parse/show_targets.rs b/src/ghci/parse/show_targets.rs index 8dcc6f2d..2b56f631 100644 --- a/src/ghci/parse/show_targets.rs +++ b/src/ghci/parse/show_targets.rs @@ -1,4 +1,3 @@ -use camino::Utf8PathBuf; use miette::miette; use winnow::combinator::repeat; use winnow::Parser; @@ -6,12 +5,13 @@ use winnow::Parser; use super::lines::until_newline; use super::show_paths::ShowPaths; use super::TargetKind; +use crate::normal_path::NormalPath; /// Parse `:show targets` output into a set of module source paths. pub fn parse_show_targets( search_paths: &ShowPaths, input: &str, -) -> miette::Result> { +) -> miette::Result> { let targets: Vec<_> = repeat(0.., until_newline) .parse(input) .map_err(|err| miette!("{err}"))?; @@ -24,27 +24,31 @@ pub fn parse_show_targets( #[cfg(test)] mod tests { + use crate::normal_path::NormalPath; + use super::*; + use camino::Utf8PathBuf; use indoc::indoc; use pretty_assertions::assert_eq; #[test] fn test_parse_show_targets() { let show_paths = ShowPaths { - cwd: Utf8PathBuf::from("tests/data/simple"), - search_paths: vec![ - Utf8PathBuf::from("tests/data/simple/test"), - Utf8PathBuf::from("tests/data/simple/src"), - ], + cwd: NormalPath::from_cwd("tests/data/simple") + .unwrap() + .absolute() + .to_owned(), + search_paths: vec![Utf8PathBuf::from("test"), Utf8PathBuf::from("src")], }; + let normal_path = |p: &str| NormalPath::new(p, &show_paths.cwd).unwrap(); + assert_eq!( parse_show_targets( &show_paths, indoc!( " src/MyLib.hs - MyLib.hs TestMain MyLib MyModule @@ -53,26 +57,10 @@ mod tests { ) .unwrap(), vec![ - ( - Utf8PathBuf::from("tests/data/simple/src/MyLib.hs"), - TargetKind::Path - ), - ( - Utf8PathBuf::from("tests/data/simple/src/MyLib.hs"), - TargetKind::Path - ), - ( - Utf8PathBuf::from("tests/data/simple/test/TestMain.hs"), - TargetKind::Module - ), - ( - Utf8PathBuf::from("tests/data/simple/src/MyLib.hs"), - TargetKind::Module - ), - ( - Utf8PathBuf::from("tests/data/simple/src/MyModule.hs"), - TargetKind::Module - ), + (normal_path("src/MyLib.hs"), TargetKind::Path), + (normal_path("test/TestMain.hs"), TargetKind::Module), + (normal_path("src/MyLib.hs"), TargetKind::Module), + (normal_path("src/MyModule.hs"), TargetKind::Module), ] ); } diff --git a/src/normal_path.rs b/src/normal_path.rs index 12606d41..26e3cb84 100644 --- a/src/normal_path.rs +++ b/src/normal_path.rs @@ -201,4 +201,49 @@ mod tests { assert_eq!(test_path.into_absolute().as_os_str(), dir.as_os_str()); } + + #[test] + fn test_normalpath_new() { + let base = Utf8Path::new("/Users/wiggles/ghciwatch/tests/data/simple"); + let relative = Utf8Path::new("src/MyLib.hs"); + let path = NormalPath::new(relative, base).unwrap(); + + assert_eq!( + path.absolute(), + Utf8Path::new("/Users/wiggles/ghciwatch/tests/data/simple/src/MyLib.hs") + ); + assert_eq!(path.relative(), Utf8Path::new("src/MyLib.hs")); + } + + #[test] + fn test_normalpath_new_parent() { + let base = Utf8Path::new("/a/b/c"); + let relative = Utf8Path::new("../puppy"); + let path = NormalPath::new(relative, base).unwrap(); + + assert_eq!(path.absolute(), Utf8Path::new("/a/b/puppy")); + assert_eq!(path.relative(), Utf8Path::new("../puppy")); + } + + #[test] + fn test_normalpath_new_unrelated() { + let base = Utf8Path::new("/a/b/c"); + let relative = Utf8Path::new("/d/e/f"); + let path = NormalPath::new(relative, base).unwrap(); + + assert_eq!(path.absolute(), Utf8Path::new("/d/e/f")); + // This is kinda silly; the paths share no components in common, they're both absolute, but + // we don't get an absolute path out of it. + assert_eq!(path.relative(), Utf8Path::new("../../../d/e/f")); + } + + #[test] + fn test_normalpath_new_both_relative() { + let base = Utf8Path::new("a/b/c"); + let relative = Utf8Path::new("d/e/f"); + let path = NormalPath::new(relative, base).unwrap(); + + assert_eq!(path.absolute(), Utf8Path::new("a/b/c/d/e/f")); + assert_eq!(path.relative(), Utf8Path::new("d/e/f")); + } }