Path expansion no longer removes trailing slashes (nushell#12662)

This PR changes `nu_path::expand_path_with()` to no longer remove
trailing slashes. It also fixes bugs in the current implementation due
to ineffective tests (Fixes nushell#12602).

crates/nu-path/fuzz/fuzz_targets/path_fuzzer.rs

@@ -1,7 +1,7 @@
 #![no_main]
 
 use libfuzzer_sys::fuzz_target;
-use nu_path::{expand_path_with, expand_tilde, expand_to_real_path, trim_trailing_slash};
+use nu_path::{expand_path_with, expand_tilde, expand_to_real_path};
 
 fuzz_target!(|data: &[u8]| {
     if let Ok(s) = std::str::from_utf8(data) {
@@ -10,9 +10,6 @@ fuzz_target!(|data: &[u8]| {
         // Fuzzing expand_to_real_path function
         let _ = expand_to_real_path(path);
 
-        // Fuzzing trim_trailing_slash function
-        let _ = trim_trailing_slash(s);
-
         // Fuzzing expand_tilde function
         let _ = expand_tilde(path);
 

crates/nu-path/src/assert_path_eq.rs

@@ -0,0 +1,50 @@
+//! Path equality in Rust is defined by comparing their `components()`. However,
+//! `Path::components()` will perform its own normalization, which makes
+//! `assert_eq!` not suitable testing.
+//!
+//! This module provides two macros, `assert_path_eq!` and `assert_path_ne!`,
+//! which converts path to string before comparison. They accept PathBuf, Path,
+//! String, and &str as parameters.
+
+#[macro_export]
+macro_rules! assert_path_eq {
+    ($left:expr, $right:expr $(,)?) => {
+        assert_eq!(
+            AsRef::<Path>::as_ref(&$left).to_str().unwrap(),
+            AsRef::<Path>::as_ref(&$right).to_str().unwrap()
+        )
+    };
+}
+
+#[macro_export]
+macro_rules! assert_path_ne {
+    ($left:expr, $right:expr $(,)?) => {
+        assert_ne!(
+            AsRef::<Path>::as_ref(&$left).to_str().unwrap(),
+            AsRef::<Path>::as_ref(&$right).to_str().unwrap()
+        )
+    };
+}
+
+#[cfg(test)]
+mod test {
+    use std::path::{Path, PathBuf};
+
+    #[test]
+    fn assert_path_eq_works() {
+        assert_path_eq!(PathBuf::from("/foo/bar"), Path::new("/foo/bar"));
+        assert_path_eq!(PathBuf::from("/foo/bar"), String::from("/foo/bar"));
+        assert_path_eq!(PathBuf::from("/foo/bar"), "/foo/bar");
+        assert_path_eq!(Path::new("/foo/bar"), String::from("/foo/bar"));
+        assert_path_eq!(Path::new("/foo/bar"), "/foo/bar");
+        assert_path_eq!(Path::new(r"\foo\bar"), r"\foo\bar");
+
+        assert_path_ne!(PathBuf::from("/foo/bar/."), Path::new("/foo/bar"));
+        assert_path_ne!(PathBuf::from("/foo/bar/."), String::from("/foo/bar"));
+        assert_path_ne!(PathBuf::from("/foo/bar/."), "/foo/bar");
+        assert_path_ne!(Path::new("/foo/./bar"), String::from("/foo/bar"));
+        assert_path_ne!(Path::new("/foo/./bar"), "/foo/bar");
+        assert_path_ne!(Path::new(r"\foo\bar"), r"/foo/bar");
+        assert_path_ne!(Path::new(r"/foo/bar"), r"\foo\bar");
+    }
+}

crates/nu-path/src/components.rs

@@ -0,0 +1,242 @@
+//! A wrapper around `Path::components()` that preserves trailing slashes.
+//!
+//! Trailing slashes are semantically important for us. For example, POSIX says
+//! that path resolution should always follow the final symlink if it has
+//! trailing slashes. Here's a demonstration:
+//!
+//! ```sh
+//! mkdir foo
+//! ln -s foo link
+//!
+//! cp -r link bar      # This copies the symlink, so bar is now a symlink to foo
+//! cp -r link/ baz     # This copies the directory, so baz is now a directory
+//! ```
+//!
+//! However, `Path::components()` normalizes trailing slashes away, and so does
+//! other APIs that uses `Path::components()` under the hood, such as
+//! `Path::parent()`. This is not ideal for path manipulation.
+//!
+//! This module provides a wrapper around `Path::components()` that produces an
+//! empty component when there's a trailing slash.
+//!
+//! You can reconstruct a path with a trailing slash by concatenating the
+//! components returned by this function using `PathBuf::push()` or
+//! `Path::join()`. It works because `PathBuf::push("")` will add a trailing
+//! slash when the original path doesn't have one.
+
+#[cfg(unix)]
+use std::os::unix::ffi::OsStrExt;
+#[cfg(windows)]
+use std::os::windows::ffi::OsStrExt;
+use std::{
+    ffi::OsStr,
+    path::{Component, Path},
+};
+
+/// Like `Path::components()`, but produces an extra empty component at the end
+/// when `path` contains a trailing slash.
+///
+/// Example:
+///
+/// ```
+/// # use std::path::{Path, Component};
+/// # use std::ffi::OsStr;
+/// use nu_path::components;
+///
+/// let path = Path::new("/foo/bar/");
+/// let mut components = components(path);
+///
+/// assert_eq!(components.next(), Some(Component::RootDir));
+/// assert_eq!(components.next(), Some(Component::Normal(OsStr::new("foo"))));
+/// assert_eq!(components.next(), Some(Component::Normal(OsStr::new("bar"))));
+/// assert_eq!(components.next(), Some(Component::Normal(OsStr::new(""))));
+/// assert_eq!(components.next(), None);
+/// ```
+pub fn components(path: &Path) -> impl Iterator<Item = Component> {
+    let mut final_component = Some(Component::Normal(OsStr::new("")));
+    path.components().chain(std::iter::from_fn(move || {
+        if has_trailing_slash(path) {
+            final_component.take()
+        } else {
+            None
+        }
+    }))
+}
+
+#[cfg(windows)]
+fn has_trailing_slash(path: &Path) -> bool {
+    let last = path.as_os_str().encode_wide().last();
+    last == Some(b'\\' as u16) || last == Some(b'/' as u16)
+}
+#[cfg(unix)]
+fn has_trailing_slash(path: &Path) -> bool {
+    let last = path.as_os_str().as_bytes().last();
+    last == Some(&b'/')
+}
+
+#[cfg(test)]
+mod test {
+    //! We'll go through every variant of Component, with or without trailing
+    //! slashes. Then we'll try reconstructing the path on some typical use cases.
+
+    use crate::assert_path_eq;
+    use std::{
+        ffi::OsStr,
+        path::{Component, Path, PathBuf},
+    };
+
+    #[test]
+    fn empty_path() {
+        let path = Path::new("");
+        let mut components = crate::components(path);
+
+        assert_eq!(components.next(), None);
+    }
+
+    #[test]
+    #[cfg(windows)]
+    fn prefix_only() {
+        let path = Path::new("C:");
+        let mut components = crate::components(path);
+
+        assert!(matches!(components.next(), Some(Component::Prefix(_))));
+        assert_eq!(components.next(), None);
+    }
+
+    #[test]
+    #[cfg(windows)]
+    fn prefix_with_trailing_slash() {
+        let path = Path::new("C:\\");
+        let mut components = crate::components(path);
+
+        assert!(matches!(components.next(), Some(Component::Prefix(_))));
+        assert!(matches!(components.next(), Some(Component::RootDir)));
+        assert_eq!(components.next(), Some(Component::Normal(OsStr::new(""))));
+        assert_eq!(components.next(), None);
+    }
+
+    #[test]
+    fn root() {
+        let path = Path::new("/");
+        let mut components = crate::components(path);
+
+        assert!(matches!(components.next(), Some(Component::RootDir)));
+        assert_eq!(components.next(), Some(Component::Normal(OsStr::new(""))));
+        assert_eq!(components.next(), None);
+    }
+
+    #[test]
+    fn cur_dir_only() {
+        let path = Path::new(".");
+        let mut components = crate::components(path);
+
+        assert!(matches!(components.next(), Some(Component::CurDir)));
+        assert_eq!(components.next(), None);
+    }
+
+    #[test]
+    fn cur_dir_with_trailing_slash() {
+        let path = Path::new("./");
+        let mut components = crate::components(path);
+
+        assert!(matches!(components.next(), Some(Component::CurDir)));
+        assert_eq!(components.next(), Some(Component::Normal(OsStr::new(""))));
+        assert_eq!(components.next(), None);
+    }
+
+    #[test]
+    fn parent_dir_only() {
+        let path = Path::new("..");
+        let mut components = crate::components(path);
+
+        assert!(matches!(components.next(), Some(Component::ParentDir)));
+        assert_eq!(components.next(), None);
+    }
+
+    #[test]
+    fn parent_dir_with_trailing_slash() {
+        let path = Path::new("../");
+        let mut components = crate::components(path);
+
+        assert!(matches!(components.next(), Some(Component::ParentDir)));
+        assert_eq!(components.next(), Some(Component::Normal(OsStr::new(""))));
+        assert_eq!(components.next(), None);
+    }
+
+    #[test]
+    fn normal_only() {
+        let path = Path::new("foo");
+        let mut components = crate::components(path);
+
+        assert_eq!(
+            components.next(),
+            Some(Component::Normal(OsStr::new("foo")))
+        );
+        assert_eq!(components.next(), None);
+    }
+
+    #[test]
+    fn normal_with_trailing_slash() {
+        let path = Path::new("foo/");
+        let mut components = crate::components(path);
+
+        assert_eq!(
+            components.next(),
+            Some(Component::Normal(OsStr::new("foo")))
+        );
+        assert_eq!(components.next(), Some(Component::Normal(OsStr::new(""))));
+        assert_eq!(components.next(), None);
+    }
+
+    #[test]
+    #[cfg(not(windows))]
+    fn reconstruct_unix_only() {
+        let path = Path::new("/home/Alice");
+
+        let mut buf = PathBuf::new();
+        for component in crate::components(path) {
+            buf.push(component);
+        }
+
+        assert_path_eq!(path, buf);
+    }
+
+    #[test]
+    #[cfg(not(windows))]
+    fn reconstruct_unix_with_trailing_slash() {
+        let path = Path::new("/home/Alice/");
+
+        let mut buf = PathBuf::new();
+        for component in crate::components(path) {
+            buf.push(component);
+        }
+
+        assert_path_eq!(path, buf);
+    }
+
+    #[test]
+    #[cfg(windows)]
+    fn reconstruct_windows_only() {
+        let path = Path::new("C:\\WINDOWS\\System32");
+
+        let mut buf = PathBuf::new();
+        for component in crate::components(path) {
+            buf.push(component);
+        }
+
+        assert_path_eq!(path, buf);
+    }
+
+    #[test]
+    #[cfg(windows)]
+    fn reconstruct_windows_with_trailing_slash() {
+        let path = Path::new("C:\\WINDOWS\\System32\\");
+
+        let mut buf = PathBuf::new();
+        for component in crate::components(path) {
+            buf.push(component);
+        }
+
+        assert_path_eq!(path, buf);
+    }
+}