Refactor API of VfsPath

crates/ide/src/base.rs

@@ -2,7 +2,7 @@ use rowan::{TextRange, TextSize};
 use salsa::Durability;
 use std::collections::HashMap;
 use std::fmt;
-use std::path::Path;
+use std::path::{Component, Path};
 use std::sync::Arc;
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
@@ -11,71 +11,85 @@ pub struct FileId(pub u32);
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
 pub struct SourceRootId(pub u32);
 
-/// An absolute path in format `(/.+)*`
-/// Currently, it represent an absolute filesytem path.
-#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+/// An absolute Unix-like path in the virtual filesystem.
+///
+/// It must be in form `(/[^/]+)+` and every segment must be non-empty and not `.` or `..`.
+/// The root represented by an empty path.
+#[derive(Default, Debug, Clone, PartialEq, Eq, Hash)]
 pub struct VfsPath(String);
 
 impl VfsPath {
     pub fn root() -> Self {
-        Self(String::new())
+        Self::default()
     }
 
-    pub fn from_path(p: &Path) -> Option<Self> {
-        Self::new(p.to_str()?)
+    /// Construct a new absolute path in Unix-like format.
+    pub fn new(s: impl Into<String>) -> Result<Self, ParseVfsPathError> {
+        let s = s.into();
+        if s == "/" {
+            return Ok(Self::root());
+        }
+        if !s.starts_with('/') || s[1..].split('/').any(|seg| matches!(seg, "" | "." | "..")) {
+            return Err(ParseVfsPathError);
+        }
+        Ok(Self(s))
     }
 
-    pub fn new(s: impl Into<String>) -> Option<Self> {
-        let mut s: String = s.into();
-        if s.is_empty() || s == "/" {
-            return Some(Self::root());
+    pub fn from_path(path: &Path) -> Result<Self, ParseVfsPathError> {
+        let mut ret = Self::root();
+        for comp in path.components() {
+            match comp {
+                Component::RootDir => {}
+                Component::Normal(seg) => {
+                    ret.push_segment(seg.to_str().ok_or(ParseVfsPathError)?);
                 }
-        if s.ends_with('/') || s.as_bytes().windows(2).any(|w| w == b"//") {
-            return None;
+                _ => return Err(ParseVfsPathError),
             }
-        if !s.starts_with('/') {
-            s.insert(0, '/');
         }
-        Some(Self(s))
+        Ok(ret)
     }
 
-    pub fn push(&mut self, relative: &Self) {
+    /// Assume another VfsPath as relative and append it to this one.
+    pub fn append(&mut self, relative: &Self) {
         self.0.push_str(&relative.0);
     }
 
-    pub fn push_segment(&mut self, segment: &str) -> Option<()> {
-        if !segment.contains('/') {
+    /// Push a path segment at the end.
+    /// Panic if it is empty or contains `/`.
+    pub fn push_segment(&mut self, segment: &str) {
+        assert!(!segment.is_empty() && !segment.contains('/'));
         self.0 += "/";
         self.0 += segment;
-            Some(())
-        } else {
-            None
-        }
     }
 
+    /// Pop the last segment from the end.
+    /// Returns `None` if it is already the root.
     pub fn pop(&mut self) -> Option<()> {
         self.0.truncate(self.0.rsplit_once('/')?.0.len());
         Some(())
     }
 
+    /// Get the path in Unix-like form.
     pub fn as_str(&self) -> &str {
+        if !self.0.is_empty() {
             &self.0
+        } else {
+            "/"
+        }
     }
 }
 
-impl TryFrom<String> for VfsPath {
-    type Error = ();
-    fn try_from(s: String) -> Result<Self, Self::Error> {
-        Self::new(s).ok_or(())
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
+#[non_exhaustive]
+pub struct ParseVfsPathError;
+
+impl fmt::Display for ParseVfsPathError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.write_str("Invalid VfsPath")
     }
 }
 
-impl<'a> TryFrom<&'a str> for VfsPath {
-    type Error = ();
-    fn try_from(s: &'a str) -> Result<Self, Self::Error> {
-        Self::new(s).ok_or(())
-    }
-}
+impl std::error::Error for ParseVfsPathError {}
 
 /// A set of [`VfsPath`]s identified by [`FileId`]s.
 #[derive(Default, Clone, PartialEq, Eq)]

crates/ide/src/def/path.rs

@@ -28,7 +28,7 @@ impl Path {
         for _ in 0..(data.supers.saturating_add(1)) {
             vpath.pop()?;
         }
-        vpath.push(&data.relative);
+        vpath.append(&data.relative);
         Some(vpath)
     }
 
@@ -58,7 +58,7 @@ impl PathData {
             .filter(|&seg| !seg.is_empty() && seg != ".")
         {
             if seg != ".." {
-                relative.push_segment(seg).expect("Checked by lexer");
+                relative.push_segment(seg);
             // Extra ".." has no effect for absolute path.
             } else if relative.pop().is_none() && anchor != PathAnchor::Absolute {
                 supers = supers.saturating_add(1);
@@ -91,12 +91,12 @@ mod tests {
         for anchor in [PathAnchor::Relative(FileId(0)), PathAnchor::Home, PathAnchor::Search("foo".into())] {
             let norm = |s| PathData::normalize(anchor.clone(), s);
             let path = |supers, p: &str| PathData { anchor: anchor.clone(), supers, relative: VfsPath::new(p).unwrap() };
-            assert_eq!(norm(""), path(0, ""));
-            assert_eq!(norm("./."), path(0, ""));
-            assert_eq!(norm("./.."), path(1, ""));
-            assert_eq!(norm("../."), path(1, ""));
-            assert_eq!(norm("foo/./bar/../.baz"), path(0, "foo/.baz"));
-            assert_eq!(norm("../../foo"), path(2, "foo"));
+            assert_eq!(norm(""), path(0, "/"));
+            assert_eq!(norm("./."), path(0, "/"));
+            assert_eq!(norm("./.."), path(1, "/"));
+            assert_eq!(norm("../."), path(1, "/"));
+            assert_eq!(norm("foo/./bar/../.baz"), path(0, "/foo/.baz"));
+            assert_eq!(norm("../../foo"), path(2, "/foo"));
         }
     }
 
@@ -106,11 +106,11 @@ mod tests {
         let anchor = PathAnchor::Absolute;
         let norm = |s| PathData::normalize(anchor.clone(), s);
             let path = |p: &str| PathData { anchor: anchor.clone(), supers: 0, relative: VfsPath::new(p).unwrap() };
-        assert_eq!(norm("/"), path(""));
-        assert_eq!(norm("./."), path(""));
-        assert_eq!(norm("./.."), path(""));
-        assert_eq!(norm("../."), path(""));
-        assert_eq!(norm("foo/./bar/../.baz"), path("foo/.baz"));
-        assert_eq!(norm("../../foo"), path("foo"));
+        assert_eq!(norm("/"), path("/"));
+        assert_eq!(norm("./."), path("/"));
+        assert_eq!(norm("./.."), path("/"));
+        assert_eq!(norm("../."), path("/"));
+        assert_eq!(norm("foo/./bar/../.baz"), path("/foo/.baz"));
+        assert_eq!(norm("../../foo"), path("/foo"));
     }
 }

crates/ide/src/ide/goto_definition.rs

@@ -221,7 +221,7 @@ mod tests {
 
     #[test]
     fn path() {
-        check("1 + $0./.", expect!["file://"]);
+        check("1 + $0./.", expect!["file:///"]);
         check(
             "
 #- /default.nix

crates/ide/src/tests.rs

@@ -37,7 +37,7 @@ impl TestDB {
             change.change_file(file, text.to_owned().into());
         }
         let entry = file_set
-            .file_for_path(&"/default.nix".try_into().unwrap())
+            .file_for_path(&VfsPath::new("/default.nix").unwrap())
             .context("Missing entry file")?;
         change.set_roots(vec![SourceRoot::new_local(file_set, Some(entry))]);
         change.apply(&mut db);
@@ -101,7 +101,7 @@ impl Fixture {
             if let Some(path) = line.strip_prefix("#- ") {
                 ensure!(!missing_header, "Missing path header at the first line");
 
-                let path = VfsPath::new(path).context("Invalid path")?;
+                let path = VfsPath::new(path)?;
                 if let Some(prev_path) = cur_path.replace(path) {
                     this.insert_file(prev_path, mem::take(&mut cur_text))?;
                     cur_file.0 += 1;
@@ -109,7 +109,7 @@ impl Fixture {
             } else {
                 if cur_path.is_none() {
                     missing_header = true;
-                    cur_path = Some("/default.nix".try_into().unwrap());
+                    cur_path = Some(VfsPath::new("/default.nix").unwrap());
                 }
 
                 let mut iter = line.chars().peekable();